Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

Merged
merged 4 commits into from
Apr 22, 2024
Merged

Refactor and migrate FlutterUndoManagerPlugin to ARC #52234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
231e00e
Refactor and migrate FlutterUndoManagerPlugin to ARC
jmagman Apr 18, 2024
1dad11d
Rewrite delegate and tests
jmagman Apr 19, 2024
f491d7f
More import cleanup
jmagman Apr 19, 2024
98725b4
Remove undoManager
jmagman Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions shell/platform/darwin/ios/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ source_set("flutter_framework_source_arc") {
"framework/Source/FlutterTextureRegistryRelay.mm",
"framework/Source/FlutterUIPressProxy.h",
"framework/Source/FlutterUIPressProxy.mm",
"framework/Source/FlutterUndoManagerDelegate.h",
"framework/Source/FlutterUndoManagerPlugin.h",
"framework/Source/FlutterUndoManagerPlugin.mm",
"framework/Source/KeyCodeMap.g.mm",
"framework/Source/KeyCodeMap_Internal.h",
"framework/Source/UIViewController+FlutterScreenAndSceneIfLoaded.h",
Expand Down Expand Up @@ -157,9 +160,6 @@ source_set("flutter_framework_source") {
"framework/Source/FlutterPluginAppLifeCycleDelegate.mm",
"framework/Source/FlutterSemanticsScrollView.h",
"framework/Source/FlutterSemanticsScrollView.mm",
"framework/Source/FlutterUndoManagerDelegate.h",
"framework/Source/FlutterUndoManagerPlugin.h",
"framework/Source/FlutterUndoManagerPlugin.mm",
"framework/Source/FlutterView.h",
"framework/Source/FlutterView.mm",
"framework/Source/FlutterViewController.mm",
Expand Down
2 changes: 0 additions & 2 deletions shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,6 @@ - (void)setViewController:(FlutterViewController*)viewController {
[self maybeSetupPlatformViewChannels];
[self updateDisplays];
_textInputPlugin.get().viewController = viewController;
_undoManagerPlugin.get().viewController = viewController;

if (viewController) {
__block FlutterEngine* blockSelf = self;
Expand Down Expand Up @@ -465,7 +464,6 @@ - (void)setFlutterViewControllerWillDeallocObserver:(id<NSObject>)observer {
- (void)notifyViewControllerDeallocated {
[[self lifecycleChannel] sendMessage:@"AppLifecycleState.detached"];
_textInputPlugin.get().viewController = nil;
_undoManagerPlugin.get().viewController = nil;
if (!_allowHeadlessExecution) {
[self destroyContext];
} else if (_shell) {
Expand Down
8 changes: 8 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterUndoManagerDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

#import <Foundation/Foundation.h>

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h"

NS_ASSUME_NONNULL_BEGIN

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

@protocol FlutterUndoManagerDelegate <NSObject>

@property(nonatomic, weak) UIResponder* viewController;
jmagman marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documenting how this delegate works led me to rewrite it.

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

  2. 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.


- (FlutterTextInputPlugin*)textInputPlugin;
jmagman marked this conversation as resolved.
Show resolved Hide resolved

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

@end
NS_ASSUME_NONNULL_END

Expand Down
4 changes: 0 additions & 4 deletions shell/platform/darwin/ios/framework/Source/FlutterUndoManagerPlugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,11 @@

#import <UIKit/UIKit.h>

#import "flutter/fml/memory/weak_ptr.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
#import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterViewController.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterUndoManagerDelegate.h"

@interface FlutterUndoManagerPlugin : NSObject

@property(nonatomic, assign) FlutterViewController* viewController;

- (instancetype)init NS_UNAVAILABLE;
+ (instancetype)new NS_UNAVAILABLE;

Expand Down
79 changes: 38 additions & 41 deletions shell/platform/darwin/ios/framework/Source/FlutterUndoManagerPlugin.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,8 @@
// found in the LICENSE file.

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterUndoManagerPlugin.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h"

#import <Foundation/Foundation.h>
#import <UIKit/UIKit.h>

#include "flutter/fml/logging.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h"

#pragma mark - UndoManager channel method names.
static NSString* const kSetUndoStateMethod = @"UndoManager.setUndoState";
Expand All @@ -17,15 +13,16 @@
static NSString* const kCanUndo = @"canUndo";
static NSString* const kCanRedo = @"canRedo";

@implementation FlutterUndoManagerPlugin {
id<FlutterUndoManagerDelegate> _undoManagerDelegate;
}
@interface FlutterUndoManagerPlugin ()
@property(nonatomic, weak, readonly) id<FlutterUndoManagerDelegate> undoManagerDelegate;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From below:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@end

@implementation FlutterUndoManagerPlugin

- (instancetype)initWithDelegate:(id<FlutterUndoManagerDelegate>)undoManagerDelegate {
self = [super init];

if (self) {
// `_undoManagerDelegate` is a weak reference because it should retain FlutterUndoManagerPlugin.
_undoManagerDelegate = undoManagerDelegate;
}

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

- (void)dealloc {
[self resetUndoManager];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jmagman jmagman Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

[super dealloc];
}

- (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
Expand All @@ -49,45 +45,47 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
}

- (NSUndoManager*)undoManager {
return _viewController.undoManager;
return self.undoManagerDelegate.viewController.undoManager;
}

- (void)resetUndoManager API_AVAILABLE(ios(9.0)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[[self undoManager] removeAllActionsWithTarget:self];
}

- (void)registerUndoWithDirection:(FlutterUndoRedoDirection)direction API_AVAILABLE(ios(9.0)) {
[[self undoManager] beginUndoGrouping];
[[self undoManager] registerUndoWithTarget:self
handler:^(id target) {
// Register undo with opposite direction.
FlutterUndoRedoDirection newDirection =
(direction == FlutterUndoRedoDirectionRedo)
? FlutterUndoRedoDirectionUndo
: FlutterUndoRedoDirectionRedo;
[target registerUndoWithDirection:newDirection];
// Invoke method on delegate.
[_undoManagerDelegate flutterUndoManagerPlugin:self
handleUndoWithDirection:direction];
}];
[[self undoManager] endUndoGrouping];
NSUndoManager* undoManager = [self undoManager];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nit: self.undoManager

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[undoManager beginUndoGrouping];
[undoManager registerUndoWithTarget:self
handler:^(FlutterUndoManagerPlugin* target) {
// Register undo with opposite direction.
FlutterUndoRedoDirection newDirection =
(direction == FlutterUndoRedoDirectionRedo)
? FlutterUndoRedoDirectionUndo
: FlutterUndoRedoDirectionRedo;
[target registerUndoWithDirection:newDirection];
// Invoke method on delegate.
[target.undoManagerDelegate flutterUndoManagerPlugin:target
Copy link
Member Author

@jmagman jmagman Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

handleUndoWithDirection:direction];
}];
[undoManager endUndoGrouping];
}

- (void)registerRedo API_AVAILABLE(ios(9.0)) {
[[self undoManager] beginUndoGrouping];
[[self undoManager]
registerUndoWithTarget:self
handler:^(id target) {
// Register undo with opposite direction.
[target registerUndoWithDirection:FlutterUndoRedoDirectionRedo];
}];
[[self undoManager] endUndoGrouping];
[[self undoManager] undo];
NSUndoManager* undoManager = [self undoManager];
jmagman marked this conversation as resolved.
Show resolved Hide resolved
[undoManager beginUndoGrouping];
[undoManager registerUndoWithTarget:self
handler:^(id target) {
// Register undo with opposite direction.
[target registerUndoWithDirection:FlutterUndoRedoDirectionRedo];
}];
[undoManager endUndoGrouping];
[undoManager undo];
}

- (void)setUndoState:(NSDictionary*)dictionary API_AVAILABLE(ios(9.0)) {
BOOL groupsByEvent = [self undoManager].groupsByEvent;
[self undoManager].groupsByEvent = NO;
NSUndoManager* undoManager = [self undoManager];
jmagman marked this conversation as resolved.
Show resolved Hide resolved
BOOL groupsByEvent = undoManager.groupsByEvent;
undoManager.groupsByEvent = NO;
BOOL canUndo = [dictionary[kCanUndo] boolValue];
BOOL canRedo = [dictionary[kCanRedo] boolValue];

Expand All @@ -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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is textInputView not declared as a property?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (textInputView != nil) {
// This is needed to notify the iPadOS keyboard that it needs to update the
// state of the UIBarButtons. Otherwise, the state changes to NSUndoManager
// will not show up until the next keystroke (or other trigger).
UITextInputAssistantItem* assistantItem =
_viewController.engine.textInputPlugin.textInputView.inputAssistantItem;
UITextInputAssistantItem* assistantItem = textInputView.inputAssistantItem;
assistantItem.leadingBarButtonGroups = assistantItem.leadingBarButtonGroups;
}
[self undoManager].groupsByEvent = groupsByEvent;
undoManager.groupsByEvent = groupsByEvent;
}

@end
55 changes: 24 additions & 31 deletions shell/platform/darwin/ios/framework/Source/FlutterUndoManagerPluginTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,51 +8,48 @@
#import <XCTest/XCTest.h>

#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"
#import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterEngine.h"
#import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterViewController.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h"

FLUTTER_ASSERT_ARC

@interface FlutterEngine ()
- (nonnull FlutterUndoManagerPlugin*)undoManagerPlugin;
- (nonnull FlutterTextInputPlugin*)textInputPlugin;
@interface FlutterUndoManagerDelegateForTest : NSObject <FlutterUndoManagerDelegate>
@property(nonatomic, weak) UIResponder* viewController;
@property(nonatomic) FlutterTextInputPlugin* textInputPlugin;
@end

@interface FlutterUndoManagerPluginForTest : FlutterUndoManagerPlugin
@property(nonatomic, assign) NSUndoManager* undoManager;
@end
@implementation FlutterUndoManagerDelegateForTest

@implementation FlutterUndoManagerPluginForTest {
- (void)flutterUndoManagerPlugin:(FlutterUndoManagerPlugin*)undoManagerPlugin
handleUndoWithDirection:(FlutterUndoRedoDirection)direction {
}
@end

@interface FlutterUndoManagerPluginTest : XCTestCase
@property(nonatomic, strong) id engine;
@property(nonatomic, strong) FlutterUndoManagerPluginForTest* undoManagerPlugin;
@property(nonatomic, strong) FlutterViewController* viewController;
@property(nonatomic, strong) NSUndoManager* undoManager;
@property(nonatomic) id undoManagerDelegate;
jmagman marked this conversation as resolved.
Show resolved Hide resolved
@property(nonatomic) FlutterUndoManagerPlugin* undoManagerPlugin;
@property(nonatomic) UIResponder* viewController;
@property(nonatomic) NSUndoManager* undoManager;
@end

@implementation FlutterUndoManagerPluginTest {
}
@implementation FlutterUndoManagerPluginTest

- (void)setUp {
[super setUp];
self.engine = OCMClassMock([FlutterEngine class]);

self.undoManagerPlugin = [[FlutterUndoManagerPluginForTest alloc] initWithDelegate:self.engine];
self.undoManagerDelegate = OCMClassMock([FlutterUndoManagerDelegateForTest class]);

self.viewController = [[FlutterViewController alloc] init];
self.undoManagerPlugin.viewController = self.viewController;
self.undoManagerPlugin =
[[FlutterUndoManagerPlugin alloc] initWithDelegate:self.undoManagerDelegate];
jmagman marked this conversation as resolved.
Show resolved Hide resolved

self.undoManager = OCMClassMock([NSUndoManager class]);
self.undoManagerPlugin.undoManager = self.undoManager;

self.viewController = OCMClassMock([UIResponder class]);
OCMStub([self.viewController undoManager]).andReturn(self.undoManager);
OCMStub([self.undoManagerDelegate viewController]).andReturn(self.viewController);
}

- (void)tearDown {
[self.undoManager removeAllActionsWithTarget:self.undoManagerPlugin];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.engine = nil;
self.undoManagerDelegate = nil;
self.viewController = nil;
self.undoManager = nil;
[super tearDown];
Expand All @@ -75,14 +72,14 @@ - (void)testSetUndoState {
removeAllActionsCount++;
});
__block int delegateUndoCount = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OCMStub([self.engine flutterUndoManagerPlugin:[OCMArg any]
handleUndoWithDirection:FlutterUndoRedoDirectionUndo])
OCMStub([self.undoManagerDelegate flutterUndoManagerPlugin:[OCMArg any]
handleUndoWithDirection:FlutterUndoRedoDirectionUndo])
.andDo(^(NSInvocation* invocation) {
delegateUndoCount++;
});
__block int delegateRedoCount = 0;
OCMStub([self.engine flutterUndoManagerPlugin:[OCMArg any]
handleUndoWithDirection:FlutterUndoRedoDirectionRedo])
OCMStub([self.undoManagerDelegate flutterUndoManagerPlugin:[OCMArg any]
handleUndoWithDirection:FlutterUndoRedoDirectionRedo])
.andDo(^(NSInvocation* invocation) {
delegateRedoCount++;
});
Expand Down Expand Up @@ -143,14 +140,10 @@ - (void)testSetUndoState {

- (void)testSetUndoStateDoesInteractWithInputDelegate {
// Regression test for https://github.com/flutter/flutter/issues/133424
FlutterViewController* viewController = OCMPartialMock(self.viewController);
self.undoManagerPlugin.viewController = self.viewController;

FlutterTextInputPlugin* textInputPlugin = OCMClassMock([FlutterTextInputPlugin class]);
FlutterTextInputView* textInputView = OCMClassMock([FlutterTextInputView class]);

OCMStub([viewController engine]).andReturn(self.engine);
OCMStub([self.engine textInputPlugin]).andReturn(textInputPlugin);
OCMStub([self.undoManagerDelegate textInputPlugin]).andReturn(textInputPlugin);
OCMStub([textInputPlugin textInputView]).andReturn(textInputView);

FlutterMethodCall* setUndoStateCall =
Expand Down