Make `FlutterAppDelegate` respond to life cycle events dynamically #6086

jamesderlin · 2018-08-25T00:52:52Z

Make FlutterAppDelegate respond to various application life cycle
events (e.g. applicationDidEnterBackground:,
applicationWillEnterForeground:, etc.) dynamically by forwarding
them to the FlutterPluginAppLifeCycleDelegate only if a registered
plugin wants it. This avoids unnecessary runtime warnings (e.g.
issue #9984).

FlutterPluginAppLifeCycleDelegate now maintains an internal table
mapping selectors to corresponding plugins, and it no longer iterates
over all plugins for each event (at the cost of using additional
space).

Note that empirical testing seems to indicate that iOS caches which
parts of the UIApplicationDelegate protocol the application
implements. This means that client applications now must call
-[FlutterAppDelegate addApplicationLifeCycleDelegate] early in
application initialization (e.g. in the -init method) for plugins
to receive life cycle events.

This change also greatly reduces the amount of code that should be
copied and pasted from FlutterAppDelegate if client applications
cannot directly inherit from it. Alternatively, it provides an
example for how client applications can use FlutterAppDelegate via
composition rather than inheritance (issue #20709).

Make `FlutterAppDelegate` respond to various application life cycle events (e.g. `applicationDidEnterBackground:`, `applicationWillEnterForeground:`, etc.) dynamically by forwarding them to the `FlutterPluginAppLifeCycleDelegate` only if a registered plugin wants it. This avoids unnecessary runtime warnings (e.g. issue #9984). `FlutterPluginAppLifeCycleDelegate` now maintains an internal table mapping selectors to corresponding plugins, and it no longer iterates over all plugins for each event (at the cost of using additional space). Note that empirical testing seems to indicate that iOS caches which parts of the `UIApplicationDelegate` protocol the application implements. This means that client applications now must call `-[FlutterAppDelegate addApplicationLifeCycleDelegate]` early in application initialization (e.g. in the `-init` method) for plugins to receive life cycle events. This change also greatly reduces the amount of code that should be copied and pasted from `FlutterAppDelegate` if client applications cannot directly inherit from it. Alternatively, it provides an example for how client applications can use `FlutterAppDelegate` via composition rather than inheritance (issue #20709).

jamesderlin · 2018-08-25T01:02:05Z

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

+        p.reset([[NSPointerArray weakObjectsPointerArray] retain]);
+      }
+
+      [p.get() addPointer:(__bridge void*)delegate];


Note that I currently don't bother doing anything to compact these NSPointerArrays if plugins are destroyed. I'm not familiar with plugin usage so don't know what to design for. Are plugins expected to be created and destroyed a lot? If so, then I think it'd be better to add a mechanism for plugins to unregister themselves. Of course, if plugins are expected to be created and destroyed during the application's lifetime, then that's probably incompatible with this change's requirement of registering plugins during application startup.

The lifespan of Flutter plugins should probably have been tied to that of the FlutterViewController with which they register and set up Dart/iOS communication through platform channels. In general, such Flutter views can be created and destroyed at will during the lifecycle of the app.

However, for Flutter-only apps created using flutter create foo, the life span of the Flutter view is the same as that of the application---which is why we ended up in the current situation where plugins register once and have support for responding to all app delegate calls.

For add2app scenarios in particular, this coupling of the app lifecycle with the Flutter view lifecycle via plugins will have to be broken, I think. A first step was taken in #5173 which made it possible to delay plugin registration until the first Flutter view is created. That doesn't necessarily disqualify the present PR, but indicates that it is a step in a larger refactoring to fix an early design error.

Hmm, ok. Another possibility would be to add another mechanism to let clients apps specify which lifecycle events they want without having to implement handlers that forward to the FlutterPluginAppLifeCycleDelegate themselves. OTOH, I'm a bit wary of adding multiple ways to do similar things.

And, of course, another possibility is to won't-fix #9984 and continue to live with the runtime warnings.

@jamesderlin, as you know, I don't have a lot of familiarity with the specifics of iOS, but I definitely agree with the sentiment that plugins should exist within the lifespan of their associated Flutter view or views. It may be worth grabbing a variety of plugin examples and discussing what impact that might have. From an architecture and maintenance perspective, it may very well create issues that Flutter plugins need to be elevated to global app state when Flutter is being used, for example, as merely a list item in a feed.

mravn-google · 2018-08-25T11:44:39Z

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

    didFinishLaunchingWithOptions:(NSDictionary*)launchOptions {
-  for (id<FlutterPlugin> plugin in [_pluginDelegates allObjects]) {
-    if (!plugin) {
+  auto it = _selectorTable.find(_cmd);


[nit] There appears to be two or three minor variations of a common idiom for finding and forwarding to relevant plugins. Any chance one could consolidate that and reduce duplication?

Possibly. I really don't like the code duplication either and have been thinking of ways to reduce it. Definitely should be able to do it with preprocessor macros, but maybe it's also doable with C++ templates or possibly by switching to some form of iterator pattern. However, tackling that as part of this change would make it larger and less readable, so I was planning on attempting that separately.

mravn-google · 2018-08-25T12:17:09Z

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

+        p.reset([[NSPointerArray weakObjectsPointerArray] retain]);
+      }
+
+      [p.get() addPointer:(__bridge void*)delegate];


The lifespan of Flutter plugins should probably have been tied to that of the FlutterViewController with which they register and set up Dart/iOS communication through platform channels. In general, such Flutter views can be created and destroyed at will during the lifecycle of the app.

However, for Flutter-only apps created using flutter create foo, the life span of the Flutter view is the same as that of the application---which is why we ended up in the current situation where plugins register once and have support for responding to all app delegate calls.

For add2app scenarios in particular, this coupling of the app lifecycle with the Flutter view lifecycle via plugins will have to be broken, I think. A first step was taken in #5173 which made it possible to delay plugin registration until the first Flutter view is created. That doesn't necessarily disqualify the present PR, but indicates that it is a step in a larger refactoring to fix an early design error.

sigurdm · 2018-08-25T12:25:28Z

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

            options:(NSDictionary<UIApplicationOpenURLOptionsKey, id>*)options {
-  for (id<FlutterPlugin> plugin in _pluginDelegates) {
-    if (!plugin) {
+  auto it = _selectorTable.find(_cmd);


Perhaps you could extract a helper method along the lines of:

- (NSArray<FlutterObject>)supportsSelector:(SEL)selector { auto it = _selectorTable.find(_cmd); if (it == _selectorTable.end()) { return nil; } return [it->second.get() allObjects]; }

sigurdm · 2018-08-25T12:30:10Z

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

  NSPointerArray* _pluginDelegates;
+
+  // Maps selectors to an array of weak references to plugins that respond to those selectors.
+  std::map<SEL, fml::scoped_nsobject<NSPointerArray>> _selectorTable;


I think this name sounds wrong. It talks about selectors not plugins. Not sure I have a better one.

cbracken · 2018-08-29T18:32:58Z

Please hold off on merging until @chinmaygarde and I have had a look. We had a couple concerns about a selector table based approach; particularly, we'd like to think through scenarios where the delegate is changed at runtime, etc.

jamesderlin · 2018-08-29T18:42:03Z

No problem. I was going to wait at least a week before submitting this anyway since I wanted more time to collect feedback.

…

On Wed, Aug 29, 2018 at 11:33 AM Chris Bracken ***@***.***> wrote: Please hold off on merging until @chinmaygarde <https://github.com/chinmaygarde> and I have had a look. We had a couple concerns about a selector table based approach; particularly, we'd like to think through scenarios where the delegate is changed at runtime, etc. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6086 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQlfSqOOMUpDmWQKK2smSLXE_WR6WG0Aks5uVt5egaJpZM4WMK6T> .

jamesderlin · 2018-09-05T23:15:04Z

I think I'm going to retract this (mostly). I don't think I'm comfortable with forcing a change to plugin lifecycles. I think I'd still like to make FlutterAppDelegate automatically forward selectors to the FlutterPluginAppLifeCycleDelegate though as a separate pull request.

jamesderlin requested review from cbracken, mravn-google and sigurdm August 25, 2018 00:52

googlebot added the cla: yes label Aug 25, 2018

jamesderlin commented Aug 25, 2018

View reviewed changes

mravn-google approved these changes Aug 25, 2018

View reviewed changes

sigurdm approved these changes Aug 28, 2018

View reviewed changes

cbracken assigned chinmaygarde and unassigned chinmaygarde Aug 29, 2018

cbracken requested a review from chinmaygarde August 29, 2018 18:33

jamesderlin closed this Sep 5, 2018

jamesderlin mentioned this pull request Sep 14, 2018

Make FlutterAppDelegate automatically forward to the FlutterPluginAppLifeCycleDelegate #6259

Closed

jamesderlin mentioned this pull request May 4, 2019

Dynamically add certain AppDelegate methods. #8843

Merged

fzyzcjy added a commit to fzyzcjy/engine that referenced this pull request Oct 9, 2022

add timeline events for flutter#6086

488bb36

fzyzcjy added a commit to fzyzcjy/engine that referenced this pull request Oct 9, 2022

add a flow flutter#6086

022b024

fzyzcjy added a commit to fzyzcjy/engine that referenced this pull request Oct 9, 2022

add a flow flutter#6086

1867db9

fzyzcjy added a commit to fzyzcjy/engine that referenced this pull request Oct 9, 2022

minor change flutter#6086

170886f

Make FlutterAppDelegate respond to life cycle events dynamically #6086

Make FlutterAppDelegate respond to life cycle events dynamically #6086

Uh oh!

Conversation

jamesderlin commented Aug 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesderlin Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken commented Aug 29, 2018

Uh oh!

jamesderlin commented Aug 29, 2018 via email

Uh oh!

jamesderlin commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Make `FlutterAppDelegate` respond to life cycle events dynamically #6086

Make `FlutterAppDelegate` respond to life cycle events dynamically #6086

jamesderlin Aug 27, 2018 •

edited

Loading