Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

jmagman · 2024-04-18T20:35:06Z

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate FlutterUndoManagerPlugin from MRC to ARC.

Refactor so the plugin and its tests don't need to understand the details of FlutterViewController or FlutterEngine (its delegate). This decouples the plugin, and means it doesn't depend on any MRC classes.
Change the delegate so conforming only requires the objects the undo plugin actually needs.

Part of flutter/flutter#137801.

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

jmagman · 2024-04-18T20:38:08Z

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

-  id<FlutterUndoManagerDelegate> _undoManagerDelegate;
-}
+@interface FlutterUndoManagerPlugin ()
+@property(nonatomic, weak, readonly) id<FlutterUndoManagerDelegate> undoManagerDelegate;


From below:

// `_undoManagerDelegate` is a weak reference because it should retain FlutterUndoManagerPlugin.

jmagman · 2024-04-18T20:44:28Z

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

+                                        : FlutterUndoRedoDirectionRedo;
+                                [target registerUndoWithDirection:newDirection];
+                                // Invoke method on delegate.
+                                [target.undoManagerDelegate flutterUndoManagerPlugin:target


Changed _undoManagerDelegate to target.undoManagerDelegate and passed target into flutterUndoManagerPlugin so self isn't retained in this block.

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

stuartmorgan

Nice! Some minor comments, but LGTM.

stuartmorgan · 2024-04-19T10:54:49Z

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

@@ -19,8 +21,14 @@ typedef NS_ENUM(NSInteger, FlutterUndoRedoDirection) {
 @class FlutterUndoManagerPlugin;

 @protocol FlutterUndoManagerDelegate <NSObject>
+
+@property(nonatomic, weak) UIResponder* viewController;


Could you add declaration comments to these explaining their role in the delegation? (Per style guide, all declarations should be commented. I know the engine is awful about this, but it would be good to break the cycle in new code.)

Documenting how this delegate works led me to rewrite it.

The -flutterUndoManagerPlugin:handleUndoWithDirection: engine implementation doesn't use the undoManagerPlugin, so there's no need to pass it back through.

I realized we were exposing the view controller just to get the undo manager, and exposing the text input plugin just to get the active text input view, which was a message chain code smell. I changed the delegate to just require the objects the undo manager plugin actually needs, and updated FlutterEngine's implementation of this protocol. This also allowed me to reduce the mocking in the tests.

stuartmorgan · 2024-04-19T10:56:09Z

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

-  id<FlutterUndoManagerDelegate> _undoManagerDelegate;
-}
+@interface FlutterUndoManagerPlugin ()
+@property(nonatomic, weak, readonly) id<FlutterUndoManagerDelegate> undoManagerDelegate;


This should also have a declaration comment (maybe a short summary of what kinds of things are delegated).

stuartmorgan · 2024-04-19T10:59:02Z

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

@@ -34,7 +31,6 @@ - (instancetype)initWithDelegate:(id<FlutterUndoManagerDelegate>)undoManagerDele

 - (void)dealloc {
  [self resetUndoManager];


Optional since it's pre-existing: it would be good to inline the chain of calls (which is still only one line) to use _undoManagerDelegate directly instead of calling a self method in dealloc in violation of style guide/best practices.

Done, and my refactor of the delegate reduced the chaining. It still calls self since that's what being passed into -removeAllActionsWithTarget but at least it isn't calling its own properties.

[_undoManagerDelegate.undoManager removeAllActionsWithTarget:self];

stuartmorgan · 2024-04-19T10:59:44Z

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

-                                                              handleUndoWithDirection:direction];
-                                     }];
-  [[self undoManager] endUndoGrouping];
+  NSUndoManager* undoManager = [self undoManager];


Optional nit: self.undoManager

Right now it's a method, but I can add the readonly property. I was trying to limit the number of changes to this file, but I've touched everything at this point.

Scratch that. I since removed the -undoManager method and changed the callsites to self.undoManagerDelegate.undoManager.

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

stuartmorgan · 2024-04-19T11:01:15Z

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

@@ -99,16 +97,15 @@ - (void)setUndoState:(NSDictionary*)dictionary API_AVAILABLE(ios(9.0)) {
  if (canRedo) {
    [self registerRedo];
  }
-
-  if (_viewController.engine.textInputPlugin.textInputView != nil) {
+  UIView<UITextInput>* textInputView = [self.undoManagerDelegate.textInputPlugin textInputView];


Is textInputView not declared as a property?

No:

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

Line 55 in 1363b52

- (UIView<UITextInput>*)textInputView;

At the moment I don't want to tackle changing that in a public header.

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

jmagman · 2024-04-19T18:53:28Z

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

@@ -1189,12 +1187,19 @@ - (void)flutterTextInputView:(FlutterTextInputView*)textInputView

 #pragma mark - Undo Manager Delegate

- (void)flutterUndoManagerPlugin:(FlutterUndoManagerPlugin*)undoManagerPlugin
-         handleUndoWithDirection:(FlutterUndoRedoDirection)direction {
+- (void)handleUndoWithDirection:(FlutterUndoRedoDirection)direction {


undoManagerPlugin was unused.

jmagman · 2024-04-19T18:54:04Z

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

+- (UIView<UITextInput>*)activeTextInputView {
+  return [[self textInputPlugin] textInputView];
+}
+
+- (NSUndoManager*)undoManager {
+  return self.viewController.undoManager;
+}


Two new properties to conform to the delegate, to reduce how much the plugin needs to understand this ownership model.

jmagman · 2024-04-19T18:59:15Z

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


- (void)tearDown {
-  [self.undoManager removeAllActionsWithTarget:self.undoManagerPlugin];


This was only class mocked, I don't think this would do anything.

jmagman · 2024-04-19T18:59:43Z

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

@@ -74,18 +97,6 @@ - (void)testSetUndoState {
      .andDo(^(NSInvocation* invocation) {
        removeAllActionsCount++;
      });
-  __block int delegateUndoCount = 0;


This is now tracked in the FakeFlutterUndoManagerDelegate test object, which is not mocked anywhere.

jmagman · 2024-04-19T19:01:21Z

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

-                                                              handleUndoWithDirection:direction];
-                                     }];
-  [[self undoManager] endUndoGrouping];
+  NSUndoManager* undoManager = [self undoManager];


Scratch that. I since removed the -undoManager method and changed the callsites to self.undoManagerDelegate.undoManager.

jmagman · 2024-04-19T19:04:40Z

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

-  return _viewController.undoManager;
-}
-
- (void)resetUndoManager API_AVAILABLE(ios(9.0)) {


Removed API_AVAILABLE(ios(9.0)) since the min is iOS 12. Must have missed that in a cleanup.

stuartmorgan

Nice!

…er/engine#52234)

…147190) flutter/engine@a4b71f0...33c828f 2024-04-22 skia-flutter-autoroll@skia.org Roll Skia from eb29b46236e6 to 3b32e3280b67 (1 revision) (flutter/engine#52298) 2024-04-22 magder@google.com Break dependency cycle of FlutterViewController <-> FlutterPlatformView (flutter/engine#52271) 2024-04-22 chris@bracken.jp [et] Lookup output filesystem path, not label (flutter/engine#52248) 2024-04-22 skia-flutter-autoroll@skia.org Roll Skia from 975859a96f8f to eb29b46236e6 (9 revisions) (flutter/engine#52297) 2024-04-22 1961493+harryterkelsen@users.noreply.github.com [canvaskit] Add configuration for maximum canvases (flutter/engine#51735) 2024-04-22 120297255+PurplePolyhedron@users.noreply.github.com Fix link in BlendMode.saturation (flutter/engine#52156) 2024-04-22 magder@google.com Refactor and migrate FlutterUndoManagerPlugin to ARC (flutter/engine#52234) 2024-04-22 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from Rr9lFiKCPhMXDGa89... to L533ubzhjWwW7jxbc... (flutter/engine#52293) 2024-04-22 skia-flutter-autoroll@skia.org Roll Dart SDK from 0a83dd7e61b1 to 0ed66a4d77cb (1 revision) (flutter/engine#52294) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from Rr9lFiKCPhMX to L533ubzhjWwW If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jmagman self-assigned this Apr 18, 2024

github-actions bot added the platform-ios label Apr 18, 2024

Refactor and migrate FlutterUndoManagerPlugin to ARC

231e00e

jmagman force-pushed the undo-manager-arc branch from 3e1d857 to 231e00e Compare April 18, 2024 20:42

jmagman commented Apr 18, 2024

View reviewed changes

jmagman marked this pull request as ready for review April 18, 2024 21:37

jmagman requested review from stuartmorgan and LongCatIsLooong April 18, 2024 21:37

LongCatIsLooong approved these changes Apr 18, 2024

View reviewed changes

stuartmorgan approved these changes Apr 19, 2024

View reviewed changes

jmagman added 3 commits April 19, 2024 11:52

Rewrite delegate and tests

1dad11d

More import cleanup

f491d7f

Remove undoManager

98725b4

jmagman commented Apr 19, 2024

View reviewed changes

stuartmorgan approved these changes Apr 20, 2024

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 22, 2024

auto-submit bot merged commit caf81dd into flutter:main Apr 22, 2024
29 checks passed

jmagman deleted the undo-manager-arc branch April 22, 2024 17:19

engine-flutter-autoroll mentioned this pull request Apr 22, 2024

Roll Flutter Engine from a4b71f02a1c7 to bb4c58dcf718 (8 revisions) flutter/flutter#147188

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 22, 2024

caf81dd96 Refactor and migrate FlutterUndoManagerPlugin to ARC (flutt…

87e667f

…er/engine#52234)

engine-flutter-autoroll mentioned this pull request Apr 22, 2024

Roll Flutter Engine from a4b71f02a1c7 to 33c828fb3ff5 (9 revisions) flutter/flutter#147190

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 22, 2024

caf81dd96 Refactor and migrate FlutterUndoManagerPlugin to ARC (flutt…

9556fea

…er/engine#52234)

jmagman mentioned this pull request Jun 21, 2024

FlutterUndoManagerPlugin is not released causes the crash (when not initializing FlutterEngine in global scope) flutter/flutter#150408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

jmagman commented Apr 18, 2024 •

edited

Loading

jmagman Apr 18, 2024

jmagman Apr 18, 2024 •

edited

Loading

stuartmorgan left a comment

stuartmorgan Apr 19, 2024

jmagman Apr 19, 2024

stuartmorgan Apr 19, 2024

stuartmorgan Apr 19, 2024

jmagman Apr 19, 2024 •

edited

Loading

stuartmorgan Apr 19, 2024

jmagman Apr 19, 2024

jmagman Apr 19, 2024

stuartmorgan Apr 19, 2024

jmagman Apr 19, 2024

jmagman Apr 19, 2024

jmagman Apr 19, 2024

jmagman Apr 19, 2024

jmagman Apr 19, 2024

jmagman Apr 19, 2024

jmagman Apr 19, 2024

stuartmorgan left a comment

		@@ -34,7 +31,6 @@ - (instancetype)initWithDelegate:(id<FlutterUndoManagerDelegate>)undoManagerDele

		- (void)dealloc {
		[self resetUndoManager];


		- (void)tearDown {
		[self.undoManager removeAllActionsWithTarget:self.undoManagerPlugin];

Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

Conversation

jmagman commented Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

jmagman Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan 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

jmagman Apr 19, 2024 • edited Loading

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

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

stuartmorgan left a comment

Choose a reason for hiding this comment

jmagman commented Apr 18, 2024 •

edited

Loading

jmagman Apr 18, 2024 •

edited

Loading

jmagman Apr 19, 2024 •

edited

Loading