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

[macOS] Consolidate view management #52254

Merged
dkwingsmt merged 7 commits into from
Apr 19, 2024
Merged

[macOS] Consolidate view management #52254

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f783266
Impl
dkwingsmt Apr 19, 2024
26f6b0f
Remove useless code
dkwingsmt Apr 19, 2024
a778212
Better doc
dkwingsmt Apr 19, 2024
850a468
Remove extra changes
dkwingsmt Apr 19, 2024
84ba335
Better doc
dkwingsmt Apr 19, 2024
6f459e7
Remove extra space
dkwingsmt Apr 19, 2024
6e810f1
Fix some docs
dkwingsmt 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
53 changes: 24 additions & 29 deletions shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,6 @@ @implementation FlutterEngine {

FlutterThreadSynchronizer* _threadSynchronizer;

// The next available view ID.
int _nextviewIdentifier;

// Whether the application is currently the active application.
BOOL _active;

Expand Down Expand Up @@ -515,8 +512,6 @@ - (instancetype)initWithName:(NSString*)labelPrefix
_binaryMessenger = [[FlutterBinaryMessengerRelay alloc] initWithParent:self];
_isResponseValid = [[NSMutableArray alloc] initWithCapacity:1];
[_isResponseValid addObject:@YES];
// kFlutterImplicitViewId is reserved for the implicit view.
_nextviewIdentifier = kFlutterImplicitViewId + 1;

_embedderAPI.struct_size = sizeof(FlutterEngineProcTable);
FlutterEngineGetProcAddresses(&_embedderAPI);
Expand Down Expand Up @@ -736,15 +731,21 @@ - (void)loadAOTData:(NSString*)assetsDir {
- (void)registerViewController:(FlutterViewController*)controller
forIdentifier:(FlutterViewIdentifier)viewIdentifier {
NSAssert(controller != nil, @"The controller must not be nil.");
NSAssert(![controller attached],
@"The incoming view controller is already attached to an engine.");
NSAssert(controller.engine == nil,
@"The FlutterViewController is unexpectedly attached to "
@"engine %@ before initialization.",
controller.engine);
NSAssert([_viewControllers objectForKey:@(viewIdentifier)] == nil,
@"The requested view ID is occupied.");
[_viewControllers setObject:controller forKey:@(viewIdentifier)];
[controller setUpWithEngine:self
viewIdentifier:viewIdentifier
threadSynchronizer:_threadSynchronizer];
NSAssert(controller.viewIdentifier == viewIdentifier, @"Failed to assign view ID.");
[_viewControllers setObject:controller forKey:@(viewIdentifier)];
NSAssert(controller.attached && controller.engine == self,

@loic-sharma loic-sharma Apr 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we separate the "attached to an unexpected engine" to a separate assertion? Currently, the assertion message you'll get for this scenario would be confusing.

@dkwingsmt dkwingsmt Apr 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I combined them initially because the "suggestion" part is too long. I now moved the "suggestion" part to in-code comment and only left the symptom to the assertion. Let me know if they still feel off.

@"The FlutterViewController unexpectedly stays unattached after being added. "

@loic-sharma loic-sharma Apr 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assertion message makes sense when you're reading this code, but it might be a little confusing if you see this in the command line without any other context.

Perhaps consider expanding the message to explain this is attaching/adding to the engine. Consider something like:

The FlutterViewController should switch to the attached mode after it is added to a FlutterEngine.

Same comment for the other similar assertions

@"In unit tests, this is likely because either the FlutterViewController or "
@"the FlutterEngine is mocked. Please subclass these classes instead.");

if (controller.viewLoaded) {
[self viewControllerViewDidLoad:controller];
Expand Down Expand Up @@ -779,11 +780,17 @@ - (void)viewControllerViewDidLoad:(FlutterViewController*)viewController {
}

- (void)deregisterViewControllerForIdentifier:(FlutterViewIdentifier)viewIdentifier {
FlutterViewController* oldController = [self viewControllerForIdentifier:viewIdentifier];
if (oldController != nil) {
[oldController detachFromEngine];
[_viewControllers removeObjectForKey:@(viewIdentifier)];
FlutterViewController* controller = [self viewControllerForIdentifier:viewIdentifier];
// The controller might be nil, because the engine only stores a weak ref, and
// this method might have been called from the controller's dealloc.

@loic-sharma loic-sharma Apr 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider something like:

Suggested change
// The controller might be nil, because the engine only stores a weak ref, and
// this method might have been called from the controller's dealloc.
// The controller can be nil. The engine stores only a weak ref, this method could have been called from the controller's dealloc.

I don't feel strongly, feel free to keep as is

if (controller != nil) {
[controller detachFromEngine];
NSAssert(!controller.attached,
@"The FlutterViewController unexpectedly stays attached after being removed. "
@"In unit tests, this is likely because either the FlutterViewController or "
@"the FlutterEngine is mocked. Please subclass these classes instead.");
}
[_viewControllers removeObjectForKey:@(viewIdentifier)];
@synchronized(_vsyncWaiters) {
[_vsyncWaiters removeObjectForKey:@(viewIdentifier)];
}
Expand Down Expand Up @@ -877,26 +884,14 @@ - (FlutterCompositor*)createFlutterCompositor {
#pragma mark - Framework-internal methods

- (void)addViewController:(FlutterViewController*)controller {
NSAssert(controller.engine == nil,
@"The FlutterViewController is unexpectedly attached to "
@"engine %@ before initialization.",
controller.engine);
[self registerViewController:controller forIdentifier:kFlutterImplicitViewId];
NSAssert(controller.attached,
@"The FlutterViewController unexpectedly stays unattached after being added. "
@"In unit tests, this is likely because either the FlutterViewController or "
@"the FlutterEngine is mocked. Please subclass these classes instead.");
NSAssert(controller.engine == self,
@"The FlutterViewController #%lld has unexpected engine %@ after being added, "
@"instead of %@. "
@"In unit tests, this is likely because either the FlutterViewController or "
@"the FlutterEngine is mocked. Please subclass these classes instead.",
controller.viewIdentifier, controller.engine, self);
// FlutterEngine can only handle the implicit view for now. Adding more views
// throws an assertion.
NSAssert(self.viewController == nil,
@"The engine already has a view controller for the implicit view.");
self.viewController = controller;
}

- (void)removeViewController:(nonnull FlutterViewController*)viewController {
NSAssert([viewController attached] && viewController.engine == self,
@"The given view controller is not associated with this engine.");
[self deregisterViewControllerForIdentifier:viewController.viewIdentifier];
[self shutDownIfNeeded];
}
Expand Down
10 changes: 4 additions & 6 deletions shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,10 @@ typedef NS_ENUM(NSInteger, FlutterAppExitResponse) {
/**
* Attach a view controller to the engine as its default controller.
*
* Practically, since FlutterEngine can only be attached with one controller,
* the given controller, if successfully attached, will always have the default
* view ID kFlutterImplicitViewId.
* Since FlutterEngine can only handle the implicit view for now, the given
* controller will always be assigned to the implicit view, if there isn't an
* implicit view yet. If the engine already has an implicit view, this call
* throws an assertion.
*
* The engine holds a weak reference to the attached view controller.
*
Expand All @@ -146,9 +147,6 @@ typedef NS_ENUM(NSInteger, FlutterAppExitResponse) {
*
* If the view controller is not associated with this engine, this call throws an
* assertion.
*
* Practically, since FlutterEngine can only be attached with one controller for
* now, the given controller must be the current view controller.
*/
- (void)removeViewController:(FlutterViewController*)viewController;

Expand Down