[macOS] Consolidate view management (#52254)
This PR improves view management logic of the macOS `FlutterEngine`
class.
* View operation assertions are now centralized in
`registerViewController:` and `deregisterViewControllerForIdentifier:`.
* `addViewController` now directly calls `.viewController =` on implicit
views, so that it matches its verbatim description.
* The doc for `addViewController` correctly reflects the fact that it
doesn't support multiple views yet.
Additionally, a useless (for now) member variable is removed.
## 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 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].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm b/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
index 9070fbf..b7dd26a 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
+++ b/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
@@ -456,9 +456,6 @@
FlutterThreadSynchronizer* _threadSynchronizer;
- // The next available view ID.
- int _nextviewIdentifier;
-
// Whether the application is currently the active application.
BOOL _active;
@@ -515,8 +512,6 @@
_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);
@@ -736,15 +731,25 @@
- (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)];
+ // Verify that the controller's property are updated accordingly. Failing the
+ // assertions is likely because either the FlutterViewController or the
+ // FlutterEngine is mocked. Please subclass these classes instead.
+ NSAssert(controller.attached, @"The FlutterViewController should switch to the attached mode "
+ @"after it is added to a FlutterEngine.");
+ NSAssert(controller.engine == self,
+ @"The FlutterViewController was added to %@, but its engine unexpectedly became %@.",
+ self, controller.engine);
if (controller.viewLoaded) {
[self viewControllerViewDidLoad:controller];
@@ -779,11 +784,17 @@
}
- (void)deregisterViewControllerForIdentifier:(FlutterViewIdentifier)viewIdentifier {
- FlutterViewController* oldController = [self viewControllerForIdentifier:viewIdentifier];
- if (oldController != nil) {
- [oldController detachFromEngine];
- [_viewControllers removeObjectForKey:@(viewIdentifier)];
+ FlutterViewController* controller = [self viewControllerForIdentifier:viewIdentifier];
+ // The controller can be nil. The engine stores only a weak ref, and this
+ // method could have been called from the controller's dealloc.
+ 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)];
}
@@ -877,26 +888,14 @@
#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];
}
diff --git a/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h b/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h
index 86b79f3..2d63ee2 100644
--- a/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h
+++ b/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h
@@ -125,9 +125,10 @@
/**
* 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.
*
@@ -146,9 +147,6 @@
*
* 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;
