Dynamically add certain AppDelegate methods. #8843

cyanglaz · 2019-05-03T23:48:05Z

Reason
We implemented all the possible AppDelegate methods in FlutterAppDelegate. They are used for plugins that needs to listen to certain life cycle messages.

2 of those methods requires users to set corresponding keys in info.plist in their app. (didReceiveRemoteNotification and performFetchWithCompletionHandle). For FirebaseMessaging implemented didReceiveRemoteNotification)
For people who doesn't use plugins that uses either of these 2 methods, they don't need to add the corresponding keys in the info.plist. And If they don't, they get a warning email from Apple when submitting their app.

Current solution for our user
As for now, the workaround for our users is that for every APP, regardless what plugins they use.

What does this PR do
This PR dynamically adding the implementation of these 2 methods based on if plugins registered implemented any of these methods.
(Update: Using respondsToSelector/forwardingTargetForSelector)

Thoughs
Using method swizzling in OBJC is usually not encouraged since it can have all sorts of issues. We might want to find a better solution if we can. We also want to analyze the trade off between "adding this fix" and "ask our users to always add the keys in the info.plist"
(Update: Not using method swizzling anymore, now using respondsToSelector/forwardingTargetForSelector for a solution. Thanks to @jamesderlin)

Testing
If we have to merge this in, I think before merge this in, we want to set up engine to have objc tests and have strong test case for this implementation. The engine unit testing for iOS is tracked here flutter/flutter#31288
We also need to add E2E test in the plugins that uses either of the 2 methods.

CC @collinjackson @cbracken

dnfield · 2019-05-04T14:45:40Z

shell/platform/darwin/ios/framework/Headers/FlutterPluginAppLifeCycleDelegate.h

+/**
+ * Get all registered plugins;
+ */
+- (NSPointerArray*)allPluginsDelegates;


Should this be a readonly property?

I thought NSPointerArray was immutable.

jamesderlin · 2019-05-04T15:04:43Z

Did you verify that if an app does want to receive notifications that enabling them works? IIRC, one issue I ran into in my earlier attempt to address (#6086) this was that the OS caches whether the app responds to application:didReceiveRemoteNotification:fetchCompletionHandler: (and I could not find a way to refresh it).

cyanglaz · 2019-05-04T20:37:48Z

Did you verify that if an app does want to receive notifications that enabling them works? IIRC, one issue I ran into in my earlier attempt to address (#6086) this was that the OS caches whether the app responds to application:didReceiveRemoteNotification:fetchCompletionHandler: (and I could not find a way to refresh it).

I haven't verity it with a real push notification yet. I have been having trouble to set it up with our flutter app id. I only tested with manually calling the methods.
I do need to test with the actual notification but I think they should perform the same. As long as the methods were added before the UIApplication received the notification.

cyanglaz · 2019-05-06T18:52:50Z

I have updated this PR to use respondsToSelector/forwardingTargetForSelector to handle the dynamism. Thanks to @jamesderlin
Now we don't use the nasty class_addMethod to handle the problem anymore.
There was a PR doing similar things previously #6259

collinjackson

this looks like an improvement over the current situation. a few nits about comments

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

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

cyanglaz · 2019-05-08T18:02:52Z

@chinmaygarde Could you also take a look at this?

cbracken

Seems reasonable as a restricted subset of the previous approach. @chinmaygarde may have further thoughts.

cbracken · 2019-05-13T18:08:10Z

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

+            @"application:performFetchWithCompletionHandler:], but you still need to add \"fetch\" "
+            @"to the list of your supported UIBackgroundModes in your Info.plist.");
+    }
+  }


This feels potentially high-maintenance. If Apple ever changes the SDK to add more of these, we won't know. That said, these aren't grouped into a protocol of any sort that we could check against so I'm not sure there's much we can do about it.

No actual action here, unless you can think of some way to mitigate that.

chinmaygarde

lgtm with nits.

chinmaygarde · 2019-05-21T00:21:42Z

shell/platform/darwin/ios/framework/Headers/FlutterPluginAppLifeCycleDelegate.h

+/**
+ * Check whether the selector should be handled dynamically.
+ */
+- (BOOL)isSelectorAddedDynamically:(SEL)selector;


Does this have to be in the public header?

chinmaygarde · 2019-05-21T00:22:46Z

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

+- (void)logCapabilityConfigurationWarningIfNeeded:(SEL)selector {
+  NSArray* backgroundModesArray =
+      [[NSBundle mainBundle] objectForInfoDictionaryKey:kUIBackgroundMode];
+  NSSet* backgroundModesSet = [[NSSet alloc] initWithArray:backgroundModesArray];


Nit: Just autorelease this instead of adding the release later. Just in case we refactor this to add an early return or something.

…ine#8843)

* 49b6de8 Dynamically add certain iOS AppDelegate methods. (flutter/engine#8843) * 75da3d4 Roll src/third_party/dart fee615c5a5..39ec9fc4f3 (46 commits) * ba6cc8c Fix type mismatches in C++ standard codec (flutter/engine#9112) * 37b31ca Roll src/third_party/skia 69aaee0ff927..f62e575bab08 (11 commits) (flutter/engine#9142)

May revert when flutter/engine#8843 is landed

* 49b6de8 Dynamically add certain iOS AppDelegate methods. (flutter/engine#8843) * 75da3d4 Roll src/third_party/dart fee615c5a5..39ec9fc4f3 (46 commits) * ba6cc8c Fix type mismatches in C++ standard codec (flutter/engine#9112) * 37b31ca Roll src/third_party/skia 69aaee0ff927..f62e575bab08 (11 commits) (flutter/engine#9142)

ronaldmaymone · 2020-04-28T18:37:54Z

Issue:
flutter/flutter#9984

Reason
We implemented all the possible AppDelegate methods in FlutterAppDelegate. They are used for plugins that needs to listen to certain life cycle messages.

2 of those methods requires users to set corresponding keys in info.plist in their app. (didReceiveRemoteNotification and performFetchWithCompletionHandle). For FirebaseMessaging implemented didReceiveRemoteNotification)
For people who doesn't use plugins that uses either of these 2 methods, they don't need to add the corresponding keys in the info.plist. And If they don't, they get a warning email from Apple when submitting their app.

Current solution for our user
As for now, the workaround for our users is that for every APP, regardless what plugins they use.

What does this PR do
This PR dynamically adding the implementation of these 2 methods based on if plugins registered implemented any of these methods.
(Update: Using respondsToSelector/forwardingTargetForSelector)

Thoughs
Using method swizzling in OBJC is usually not encouraged since it can have all sorts of issues. We might want to find a better solution if we can. We also want to analyze the trade off between "adding this fix" and "ask our users to always add the keys in the info.plist"
(Update: Not using method swizzling anymore, now using respondsToSelector/forwardingTargetForSelector for a solution. Thanks to @jamesderlin)

Testing
If we have to merge this in, I think before merge this in, we want to set up engine to have objc tests and have strong test case for this implementation. The engine unit testing for iOS is tracked here flutter/flutter#31288
We also need to add E2E test in the plugins that uses either of the 2 methods.

CC @collinjackson @cbracken

Sorry for commenting here but how did you implement the didReceiveRemoteNotification on FlutterAppDelegate? Has this merge changed the way you implemented?

swizzle certain delegate methods

5bccbba

googlebot added the cla: yes label May 3, 2019

adding flt_dcheck for class_addmethod

d5fa729

dnfield reviewed May 4, 2019

View reviewed changes

Chris Yang added 3 commits May 6, 2019 11:49

using forwarding target for selector to handle the dynamicism

0381f1e

remove unnecessary include and commented out code

8c475bf

cleanup

b25ac5a

collinjackson approved these changes May 6, 2019

View reviewed changes

review fixes

68d0c8d

cyanglaz requested a review from chinmaygarde May 8, 2019 18:02

cbracken approved these changes May 13, 2019

View reviewed changes

chinmaygarde approved these changes May 21, 2019

View reviewed changes

Chris Yang added 4 commits May 28, 2019 12:53

Merge branch 'master' into appdelegate_method_swizzle

a811e58

review fixes

8d42d8e

review fixes

81b3311

formatting

34f7a56

chinmaygarde approved these changes May 28, 2019

View reviewed changes

Chris Yang and others added 3 commits May 28, 2019 16:03

fix license

2c3a7b2

fix licenses to make the ci happy

207b5c9

make the ci happy

2764bd5

cyanglaz merged commit 49b6de8 into flutter:master May 30, 2019

cyanglaz deleted the appdelegate_method_swizzle branch May 30, 2019 16:06

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 30, 2019

49b6de8 Dynamically add certain iOS AppDelegate methods. (flutter/eng…

021ce34

…ine#8843)

engine-flutter-autoroll mentioned this pull request May 30, 2019

Roll engine 58eff77ef2f8..75da3d42d56a (2 commits) flutter/flutter#33594

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 30, 2019

49b6de8 Dynamically add certain iOS AppDelegate methods. (flutter/eng…

a7c99bf

…ine#8843)

engine-flutter-autoroll mentioned this pull request May 30, 2019

Roll engine 58eff77ef2f8..ba6cc8c93c24 (3 commits) flutter/flutter#33604

Closed

engine-flutter-autoroll mentioned this pull request May 30, 2019

Roll engine 58eff77ef2f8..37b31ca70523 (4 commits) flutter/flutter#33606

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 30, 2019

49b6de8 Dynamically add certain iOS AppDelegate methods. (flutter/eng…

9f5f163

…ine#8843)

cyanglaz mentioned this pull request Jun 3, 2019

Plugins could avoid warnings by installing UIApplicationDelegate methods dynamically flutter/flutter#9984

Open

daohoangson added a commit to daohoangson/flutter-tinhte_demo that referenced this pull request Jun 6, 2019

Add missing UIBackgroundModes.

bc665ab

May revert when flutter/engine#8843 is landed

huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019

Dynamically add certain iOS AppDelegate methods. (flutter#8843)

b430f25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Dynamically add certain AppDelegate methods. #8843

Dynamically add certain AppDelegate methods. #8843

Uh oh!

cyanglaz commented May 3, 2019 •

edited

Loading

Uh oh!

dnfield May 4, 2019

Uh oh!

cyanglaz May 4, 2019 •

edited

Loading

Uh oh!

jamesderlin commented May 4, 2019

Uh oh!

cyanglaz commented May 4, 2019

Uh oh!

cyanglaz commented May 6, 2019

Uh oh!

collinjackson left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cyanglaz commented May 8, 2019

Uh oh!

cbracken left a comment

Uh oh!

cbracken May 13, 2019

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde May 21, 2019

Uh oh!

chinmaygarde May 21, 2019

Uh oh!

ronaldmaymone commented Apr 28, 2020 •

edited

Loading

Uh oh!

Uh oh!

Dynamically add certain AppDelegate methods. #8843

Dynamically add certain AppDelegate methods. #8843

Uh oh!

Conversation

cyanglaz commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield May 4, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz May 4, 2019 • 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!

jamesderlin commented May 4, 2019

Uh oh!

cyanglaz commented May 4, 2019

Uh oh!

cyanglaz commented May 6, 2019

Uh oh!

collinjackson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cyanglaz commented May 8, 2019

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken May 13, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 21, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 21, 2019

Choose a reason for hiding this comment

Uh oh!

ronaldmaymone commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cyanglaz commented May 3, 2019 •

edited

Loading

cyanglaz May 4, 2019 •

edited

Loading

ronaldmaymone commented Apr 28, 2020 •

edited

Loading