Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[macos] expose runWithEngine constructor in FlutterViewController (#7… #36410

Merged
cbracken merged 2 commits into from
Oct 14, 2022
Merged

[macos] expose runWithEngine constructor in FlutterViewController (#7… #36410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b13d4c7
[macos] expose runWithEngine constructor in FlutterViewController (#7…
sergiy-sc Sep 1, 2022
deeae0a
[macos] Added an assert when FlutterEngine already bound to a controller
sergiy-sc Sep 30, 2022
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
13 changes: 12 additions & 1 deletion shell/platform/darwin/macos/framework/Headers/FlutterViewController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,18 @@ FLUTTER_DARWIN_EXPORT
bundle:(nullable NSBundle*)nibBundleOrNil
NS_DESIGNATED_INITIALIZER;
- (nonnull instancetype)initWithCoder:(nonnull NSCoder*)nibNameOrNil NS_DESIGNATED_INITIALIZER;

/**
* Initializes this FlutterViewController with the specified `FlutterEngine`.
*
* The initialized viewcontroller will attach itself to the engine as part of this process.
*
* @param engine The `FlutterEngine` instance to attach to. Cannot be nil.
* @param nibName The NIB name to initialize this controller with.
* @param nibBundle The NIB bundle.
*/
- (nonnull instancetype)initWithEngine:(nonnull FlutterEngine*)engine
nibName:(nullable NSString*)nibName
bundle:(nullable NSBundle*)nibBundle NS_DESIGNATED_INITIALIZER;
/**
* Invoked by the engine right before the engine is restarted.
*
Expand Down
19 changes: 11 additions & 8 deletions shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,18 +329,21 @@ - (instancetype)initWithEngine:(nonnull FlutterEngine*)engine
nibName:(nullable NSString*)nibName
bundle:(nullable NSBundle*)nibBundle {
NSAssert(engine != nil, @"Engine is required");
NSAssert(engine.viewController == nil,
@"The supplied FlutterEngine is already used with FlutterViewController "
"instance. One instance of the FlutterEngine can only be attached to one "
"FlutterViewController at a time. Set FlutterEngine.viewController "
"to nil before attaching it to another FlutterViewController.");

self = [super initWithNibName:nibName bundle:nibBundle];
if (self) {
if (engine.viewController) {
NSLog(@"The supplied FlutterEngine %@ is already used with FlutterViewController "
"instance %@. One instance of the FlutterEngine can only be attached to one "
"FlutterViewController at a time. Set FlutterEngine.viewController "
"to nil before attaching it to another FlutterViewController.",
[engine description], [engine.viewController description]);
}
_engine = engine;
CommonInit(self);
[engine setViewController:self];
Copy link
Contributor

@dkwingsmt dkwingsmt Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Bypassing the initialization logic by using .viewController = instead of setViewController seems very hacky. I'm surprised that viewController is not readonly (I suggest we make it so).

It feels to me that this PR complexes the relationship between FVC and FlutterEngine, which can be seen from the three initializeKeyboard calls throughout the file. It's hard to track how many ways are there we expect to use FVC and whether a line is executed by each, as well as how to add a new line to the initialization.

Copy link
Member

@jmagman jmagman Sep 30, 2022

Choose a reason for hiding this comment

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

Commenting only about

Bypassing the initialization logic by using .viewController = instead of setViewController seems very hacky.

That's just Objective-C, they are completely identical under the covers. The dot notation .viewController = is a more idiomatic way to set a property.

Copy link
Contributor Author

@sergiy-sc sergiy-sc Sep 30, 2022

Choose a reason for hiding this comment

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

It feels to me that this PR complexes the relationship

It does. When the existing engine is passed, it can be running or not. So that increases the complexity of the initializer and the patch adds basically one if which loads FlutterView and initializes the keyboard if it's running. So to me the added complexity doesn't look excessive.

It's hard to track how many ways are there we expect to use FVC and whether a line is executed by each, as well as how to add a new line to the initialization.

There is CommonInit and all initializers use it. initWithEngine handles the case when the engine is running and I could extract that into the method, but I couldn't come up with a good name and it doesn't clarify anything in that context.

Copy link
Contributor Author

@sergiy-sc sergiy-sc Sep 30, 2022

Choose a reason for hiding this comment

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

three initializeKeyboard calls

To me, the story with the keyboard is rather complicated. There are quite a few cases to handle, and I'm not sure controller is the best place for that. There is a TODO comment in initializeKeyboard which I guess, could solve some of the problems. But fixing that TODO goes way beyond my capabilities/knowledge of the engine's internals.

  1. Engine is not running, then FVC is created (displaying FVC will start the engine)
  2. Engine is running, then FVC is created
  3. Engine is not running, then FVC is created, and the engine is started by the user, and later FVC is shown (not handled by this patch, but viewWillAppear perhaps should check if the keyboard was correctly configured before)

One solution could be that the engine would fire an event when it's started, and FVC would subscribe to that. This can simplify keyboard initialization for cases 1. and 3.

if (engine.running) {
[self loadView];
engine.viewController = self;
[self initializeKeyboard];
}
}

return self;
Expand Down
26 changes: 26 additions & 0 deletions shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ - (bool)testFlagsChangedEventsArePropagatedIfNotHandled;
- (bool)testKeyboardIsRestartedOnEngineRestart;
- (bool)testTrackpadGesturesAreSentToFramework;
- (bool)testViewWillAppearCalledMultipleTimes;
- (bool)testFlutterViewIsConfigured;

+ (void)respondFalseForSendEvent:(const FlutterKeyEvent&)event
callback:(nullable FlutterKeyEventCallback)callback
Expand Down Expand Up @@ -145,6 +146,10 @@ + (void)respondFalseForSendEvent:(const FlutterKeyEvent&)event
ASSERT_TRUE([[FlutterViewControllerTestObjC alloc] testViewWillAppearCalledMultipleTimes]);
}

TEST(FlutterViewControllerTest, testFlutterViewIsConfigured) {
ASSERT_TRUE([[FlutterViewControllerTestObjC alloc] testFlutterViewIsConfigured]);
}

} // namespace flutter::testing

@implementation FlutterViewControllerTestObjC
Expand Down Expand Up @@ -242,6 +247,27 @@ - (bool)testKeyEventsArePropagatedIfNotHandled {
return true;
}

- (bool)testFlutterViewIsConfigured {
id engineMock = OCMClassMock([FlutterEngine class]);

id renderer_ = [[FlutterMetalRenderer alloc] initWithFlutterEngine:engineMock];
OCMStub([engineMock renderer]).andReturn(renderer_);

FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engineMock
nibName:@""
bundle:nil];
[viewController loadView];

@try {
// Make sure "renderer" was called during "loadView", which means "flutterView" is created
OCMVerify([engineMock renderer]);
} @catch (...) {
return false;
}
Comment on lines +264 to +266
Copy link
Member

@jmagman jmagman Sep 26, 2022

Choose a reason for hiding this comment

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

(note for @cbracken not @dubik) I know this pattern was copied from the other tests in this file but why are these tests returning bool (it's not even BOOL...) instead of failing in place? For example this @catch hides the details of any exception instead of letting XCTest fail the test with the actual thing that happened.

Copy link
Member

@cbracken cbracken Oct 7, 2022

Choose a reason for hiding this comment

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

That's an excellent question that'll involve some archaeology. We should leave this for now and I'll follow-up with a cleanup PR once I figure out what the history of this was.


return true;
}

- (bool)testFlagsChangedEventsArePropagatedIfNotHandled {
id engineMock = OCMClassMock([FlutterEngine class]);
id binaryMessengerMock = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
Expand Down
13 changes: 0 additions & 13 deletions shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,6 @@
*/
@property(nonatomic, readonly, nonnull) FlutterTextInputPlugin* textInputPlugin;

/**
* Initializes this FlutterViewController with the specified `FlutterEngine`.
*
* The initialized viewcontroller will attach itself to the engine as part of this process.
*
* @param engine The `FlutterEngine` instance to attach to. Cannot be nil.
* @param nibName The NIB name to initialize this controller with.
* @param nibBundle The NIB bundle.
*/
- (nonnull instancetype)initWithEngine:(nonnull FlutterEngine*)engine
nibName:(nullable NSString*)nibName
bundle:(nullable NSBundle*)nibBundle NS_DESIGNATED_INITIALIZER;

/**
* Returns YES if provided event is being currently redispatched by keyboard manager.
*/
Expand Down