Migrate iOS text input plugin to use ARC #38179

LongCatIsLooong · 2022-12-10T08:31:51Z

Extracted from #38044

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

hellohuanlin · 2022-12-12T18:24:42Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

 @property(nonatomic, readonly) id<FlutterTextInputDelegate> textInputDelegate;
 @property(nonatomic, readonly) UIView* hostView;
 @end

 @interface FlutterTextInputView ()
+@property(nonatomic, readonly) FlutterTextInputPlugin* textInputPlugin;


should this be weak?

hellohuanlin · 2022-12-12T18:25:39Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

@@ -725,6 +722,7 @@ - (void)setEditableTransform:(NSArray*)matrix;
 @end

 @implementation FlutterTextInputView {
+  __weak FlutterTextInputPlugin* _textInputPlugin;


duplicate ivar? (already in property)

nits: prefer using property.

hellohuanlin · 2022-12-12T18:29:39Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

-// The active view will be decommissioned and removed from its superview too, if
-// includeActiveView is YES.
+// The active view will be removed from its superview too, if includeActiveView is
+// YES.


nit: merge lines

hellohuanlin

Overall LGTM. Just a question on decommissioned in case of behavior change by accident.

hellohuanlin · 2022-12-14T22:30:49Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h

@@ -30,8 +29,8 @@ typedef NS_ENUM(NSInteger, FlutterScribbleInteractionStatus) {
 @interface FlutterTextInputPlugin
    : NSObject <FlutterKeySecondaryResponder, UIIndirectScribbleInteractionDelegate>

-@property(nonatomic, assign) UIViewController* viewController;
-@property(nonatomic, assign) id<FlutterIndirectScribbleDelegate> indirectScribbleDelegate;
+@property(nonatomic, weak) UIViewController* viewController;


Does flutter view controller retains the FlutterTextInputPlugin (opposite direction)?

Yes FlutterViewController seems to retain an engine instance: fml::scoped_nsobject<FlutterEngine> _engine

hellohuanlin · 2022-12-14T22:38:20Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

@@ -2454,7 +2394,6 @@ - (void)cleanUpViewHierarchy:(BOOL)includeActiveView
        if (clearText) {
          [inputView replaceRangeLocal:NSMakeRange(0, inputView.text.length) withText:@""];
        }
-        [inputView decommission];


Do we need to nil out the delegate here? The original code is strange because the _decommissioned ivar doesn't seem to be used anywhere except for an NSAssert, but it could be a bug)

I think the original issue was that a FlutterInputView can be retained by UIKit even if it's no longer in the view hierarchy, so it can outlive the engine instance, and when that happens the reference to the engine becomes a dangling pointer. With ARC I think textInputDelegate will be set to nil once the engine is deallocated.

hellohuanlin · 2022-12-14T22:46:53Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

 @end

 @implementation FlutterTimerProxy

 + (instancetype)proxyWithTarget:(FlutterTextInputPlugin*)target {
-  FlutterTimerProxy* proxy = [[self new] autorelease];
+  FlutterTimerProxy* proxy = [self new];


nit: prefer alloc/init

cyanglaz

LGTM

cyanglaz · 2022-12-15T18:24:51Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

@@ -725,6 +722,7 @@ - (void)setEditableTransform:(NSArray*)matrix;
 @end

 @implementation FlutterTextInputView {
+  __weak FlutterTextInputPlugin* _textInputPlugin;


nits: prefer using property.

cyanglaz · 2022-12-15T18:27:32Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

@@ -2090,8 +2045,7 @@ @interface FlutterTextInputPlugin ()

 @implementation FlutterTextInputPlugin {
  NSTimer* _enableFlutterTextInputViewAccessibilityTimer;
-  std::unique_ptr<fml::WeakPtrFactory<FlutterTextInputPlugin>> _weakFactory;
-  id<FlutterTextInputDelegate> _textInputDelegate;
+  __weak id<FlutterTextInputDelegate> _textInputDelegate;


nits: prefer property

jmagman · 2022-12-15T18:50:21Z

../../flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm:2044:1: error: property has a previous declaration
@property(nonatomic, readonly, weak) id<FlutterTextInputDelegate> textInputDelegate;
^
../../flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm:705:61: note: property declared here
@property(nonatomic, readonly) id<FlutterTextInputDelegate> textInputDelegate;
                                                            ^
../../flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm:2056:5: error: use of undeclared identifier '_textInputDelegate'; did you mean 'textInputDelegate'?
    _textInputDelegate = textInputDelegate;
    ^~~~~~~~~~~~~~~~~~
    textInputDelegate
../../flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm:2051:64: note: 'textInputDelegate' declared here
- (instancetype)initWithDelegate:(id<FlutterTextInputDelegate>)textInputDelegate {
                                                               ^
../../flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm:2056:24: error: explicitly assigning value of variable of type '__strong id<FlutterTextInputDelegate>' to itself [-Werror,-Wself-assign]
    _textInputDelegate = textInputDelegate;
    ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~
../../flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm:2081:10: error: use of undeclared identifier '_textInputDelegate'
  return _textInputDelegate;
         ^

)

…117242) * e28b26e1d [linux] Allow overriding asset, ICU data path (flutter/engine#38296) * 35bdb8bfc Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319) * 97e246cb5 Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320) * d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179) * c9e9fa5a9 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323) * a7ec07f13 [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324) * 61e95bacb Remove doc reference to the compute method (flutter/engine#38246) * 353d6949e make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325) * 47a358c5e Started using FlutterEngineGroups by default on Android (flutter/engine#37822) * 7d8e10652 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210) * 8915b81d4 Update buildroot to b2ab6e1. (flutter/engine#38329) * 4fe620643 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (#38320)" (flutter/engine#38331) * 2ff490c1e Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333) * 22251857f Add missing include to FlutterThreadSynchronizer (flutter/engine#38337) * 467cfd7ef Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339) * 010f4ee7a Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340) * 773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#38327) * 70439f606 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341) * bc1647f0d Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342) * cac228aff Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344) * 13ae6eb75 Revert "Started using FlutterEngineGroups by default on Android (#37822)" (flutter/engine#38351) * ed8063861 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352) * dcafebf44 Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343) * 6f6158580 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348) * 0c00bc0a9 Remove 30fps cap from playgrounds (flutter/engine#38347) * 38340bb57 [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346) * 3a6b3f986 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357) * 81b453535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358) * d91e20879 Port touch-based tests from embedder integration test (flutter/engine#38234)

* migrate iOS text input plugin to ARC * review * review * review

…lutter#117242) * e28b26e1d [linux] Allow overriding asset, ICU data path (flutter/engine#38296) * 35bdb8bfc Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319) * 97e246cb5 Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320) * d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179) * c9e9fa5a9 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323) * a7ec07f13 [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324) * 61e95bacb Remove doc reference to the compute method (flutter/engine#38246) * 353d6949e make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325) * 47a358c5e Started using FlutterEngineGroups by default on Android (flutter/engine#37822) * 7d8e10652 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210) * 8915b81d4 Update buildroot to b2ab6e1. (flutter/engine#38329) * 4fe620643 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter#38320)" (flutter/engine#38331) * 2ff490c1e Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333) * 22251857f Add missing include to FlutterThreadSynchronizer (flutter/engine#38337) * 467cfd7ef Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339) * 010f4ee7a Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340) * 773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#38327) * 70439f606 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341) * bc1647f0d Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342) * cac228aff Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344) * 13ae6eb75 Revert "Started using FlutterEngineGroups by default on Android (flutter#37822)" (flutter/engine#38351) * ed8063861 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352) * dcafebf44 Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343) * 6f6158580 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348) * 0c00bc0a9 Remove 30fps cap from playgrounds (flutter/engine#38347) * 38340bb57 [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346) * 3a6b3f986 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357) * 81b453535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358) * d91e20879 Port touch-based tests from embedder integration test (flutter/engine#38234)

flutter-dashboard bot added the platform-ios label Dec 10, 2022

LongCatIsLooong force-pushed the ios-text-input-arc2 branch 2 times, most recently from 11571ce to 441a66f Compare December 12, 2022 05:35

migrate iOS text input plugin to ARC

9d99dfd

LongCatIsLooong force-pushed the ios-text-input-arc2 branch from 441a66f to 9d99dfd Compare December 12, 2022 05:42

Merge remote-tracking branch 'upstream/master' into ios-text-input-arc2

c38d947

LongCatIsLooong mentioned this pull request Dec 12, 2022

[iOS text input] move FlutterTextInputView implementation to a different file #38044

Closed

8 tasks

hellohuanlin reviewed Dec 12, 2022

View reviewed changes

review

164bb3a

LongCatIsLooong marked this pull request as ready for review December 14, 2022 21:48

LongCatIsLooong requested a review from hellohuanlin December 14, 2022 21:48

hellohuanlin approved these changes Dec 14, 2022

View reviewed changes

review

2bc2661

cyanglaz approved these changes Dec 15, 2022

View reviewed changes

jmagman approved these changes Dec 15, 2022

View reviewed changes

LongCatIsLooong force-pushed the ios-text-input-arc2 branch from 74ce8f8 to 5f48b23 Compare December 15, 2022 18:57

review

0853631

LongCatIsLooong force-pushed the ios-text-input-arc2 branch from 5f48b23 to 0853631 Compare December 15, 2022 19:21

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 15, 2022

auto-submit bot merged commit d9580a5 into flutter:main Dec 15, 2022

engine-flutter-autoroll mentioned this pull request Dec 15, 2022

Roll Flutter Engine from 29196519c124 to d9580a5e70c9 (4 revisions) flutter/flutter#117166

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

64c3e51

)

LongCatIsLooong deleted the ios-text-input-arc2 branch December 15, 2022 20:48

engine-flutter-autoroll mentioned this pull request Dec 15, 2022

Roll Flutter Engine from 29196519c124 to c9e9fa5a99a5 (5 revisions) flutter/flutter#117169

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

f1cd0ee

)

engine-flutter-autoroll mentioned this pull request Dec 15, 2022

Roll Flutter Engine from 29196519c124 to a7ec07f130b9 (6 revisions) flutter/flutter#117171

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

949bfaf

)

engine-flutter-autoroll mentioned this pull request Dec 15, 2022

Roll Flutter Engine from 29196519c124 to 61e95bacb674 (7 revisions) flutter/flutter#117174

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

deba7d7

)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

ac96215

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 70439f606bbe (18 revisions) flutter/flutter#117197

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

cdc935a

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to bc1647f0d2b2 (19 revisions) flutter/flutter#117199

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

836761e

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to bc1647f0d2b2 (19 revisions) flutter/flutter#117204

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

6cb973c

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to bc1647f0d2b2 (19 revisions) flutter/flutter#117205

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

be6a35a

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to ed806386167c (22 revisions) flutter/flutter#117218

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

b566312

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to dcafebf4445c (23 revisions) flutter/flutter#117219

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

327df17

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 6f6158580a1f (24 revisions) flutter/flutter#117223

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

543875e

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 38340bb5734a (26 revisions) flutter/flutter#117225

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

9e49f6c

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 3a6b3f9869c4 (27 revisions) flutter/flutter#117230

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

c959289

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 81b453535eb8 (28 revisions) flutter/flutter#117232

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

b61908e

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to d91e20879a67 (29 revisions) flutter/flutter#117236

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

1c63035

)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to d91e20879a67 (29 revisions) flutter/flutter#117242

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179

a004bb3

)

loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Jan 3, 2023

Migrate iOS text input plugin to use ARC (flutter#38179)

f387f13

* migrate iOS text input plugin to ARC * review * review * review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate iOS text input plugin to use ARC #38179

Migrate iOS text input plugin to use ARC #38179

LongCatIsLooong commented Dec 10, 2022

hellohuanlin Dec 12, 2022

hellohuanlin Dec 12, 2022

cyanglaz Dec 15, 2022

hellohuanlin Dec 12, 2022

hellohuanlin left a comment

hellohuanlin Dec 14, 2022

LongCatIsLooong Dec 15, 2022

hellohuanlin Dec 14, 2022

LongCatIsLooong Dec 15, 2022

hellohuanlin Dec 14, 2022

cyanglaz left a comment

cyanglaz Dec 15, 2022

cyanglaz Dec 15, 2022

jmagman commented Dec 15, 2022

Migrate iOS text input plugin to use ARC #38179

Migrate iOS text input plugin to use ARC #38179

Conversation

LongCatIsLooong commented Dec 10, 2022

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman commented Dec 15, 2022