Add capability to add AppDelegate as UNUserNotificationCenterDelegate

[bparrishMines]

It looks like we added callbacks for UNUserNotificationCenterDelegate in this PR, but I was unable to find where we set it as the delegate.

The UNUserNotificationCenter is a shared object, so there can only be one UNUserNotificationCenterDelegate. The problem we have now is if two plugins (e.g. firebase_messaging and flutter_local_notifications) need the app to receive notifications, they would both set themselves as this delegate and one of the plugins wouldn't receive notifications.

This PR tries to create a shared delegate that all plugins can listen to. More detail is in this issue flutter/flutter#22099

Related issues:
flutter/flutter#22099

[dnfield]

This would need to be guarded, it's iOS 10+.

I thought @cyanglaz made this stuff happen dynamically?

[bparrishMines]

I don't believe this would cause a crash on iOS < 10. The documentation for Firebase Messaging gives instructions on how to add it to your app. Even despite supporting 8+. I also tested this on a 9.0 iOS simulator and the app still compiled and installed on device.

[bparrishMines]

Also, @cyanglaz and I talked offline. He was able to dynamically add methods, but not implementing protocols.

[dnfield]

UNUserNotificationCenterDelegate wasn't added until IOS10. We'll have to guard this, otherwise warnings will get displayed and people using -Werror will fail.

[bparrishMines]

I added an if statement, but I'm confident I didn't implement it correctly. How would I go about doing this?

[jmagman]

You can just wrap the macro around the one protocol you need to add instead of duplicating the entire class. Also, use __IPHONE_10_0 instead of 100000.

@interface FlutterAppDelegate : UIResponder <UIApplicationDelegate,
                                              FlutterPluginRegistry,
                                              FlutterAppLifeCycleProvider,
                                              #if __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_10_0
                                              UNUserNotificationCenterDelegate
                                              #endif
                                              >
@property(strong, nonatomic) UIWindow* window;
@end

[cbracken]

@bparrishMines @dnfield is this PR still WIP?

[dnfield]

I don't think you need this anymore - have you tested compilation on something targetting iOS 9?

[jmagman]

The API_AVAILABLE in the header is enough to give the warning. Are you trying to guard against people ignoring the warning and calling it anyway with a nil center and response?

[jmagman]

I had some individual comments, but it looks like the intention is for FlutterAppDelegate and FlutterPlugins to each pass along handle UNUserNotificationCenter notifications. So can this be accomplished by conforming FlutterApplicationLifeCycleDelegate to UNUserNotificationCenterDelegate, and not touching the FlutterAppDelegate class or FlutterPlugin protocol? Then you wouldn't need any availability checks anywhere because UNUserNotificationCenterDelegate is already decorated.

So:
FlutterPlugin.h:

@protocol FlutterApplicationLifeCycleDelegate
#if __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_10_4
<UNUserNotificationCenterDelegate>
#endif

FlutterAppDelegate.mm:

- (void)userNotificationCenter:(UNUserNotificationCenter*)center
    didReceiveNotificationResponse:(UNNotificationResponse*)response
             withCompletionHandler:(void (^)(void))completionHandler {
    [_lifeCycleDelegate userNotificationCenter:center
                didReceiveNotificationResponse:response
                         withCompletionHandler:completionHandler];
  }
}

FlutterPluginAppLifeCycleDelegate.mm:

- (void)userNotificationCenter:(UNUserNotificationCenter*)center
    didReceiveNotificationResponse:(UNNotificationResponse*)response
             withCompletionHandler:(void (^)(void))completionHandler {
    for (id<FlutterPlugin> plugin in _delegates) {
        [plugin userNotificationCenter:center didReceiveNotificationResponse:response withCompletionHandler:completionHandler];
    }
}

[jmagman]

You can just wrap the macro around the one protocol you need to add instead of duplicating the entire class. Also, use __IPHONE_10_0 instead of 100000.

@interface FlutterAppDelegate : UIResponder <UIApplicationDelegate,
                                              FlutterPluginRegistry,
                                              FlutterAppLifeCycleProvider,
                                              #if __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_10_0
                                              UNUserNotificationCenterDelegate
                                              #endif
                                              >
@property(strong, nonatomic) UIWindow* window;
@end

[jmagman]

This also should have a API_AVAILABLE(ios(10));

But is this thin convenience method necessary? It seems like it would be better for callers to explicitly set the delegate if that's what they want instead of implying there's some additional magic going on. It also hides that calling this will stomp any previous delegates set on the notification center.

[bparrishMines]

The UNUserNotificationCenter is a shared object, so there can only be one UNUserNotificationCenterDelegate. The problem we have now is if two plugins (e.g. firebase_messaging and flutter_local_notifications) need the app to receive notifications, they would both set this delegate and one of the plugins wouldn't receive notifications.

This PR tries to create a shared delegate that all plugins can listen to. More detail is in this issue flutter/flutter#22099

[jmagman]

You don't need the API_AVAILABLE in the .mm.

[jmagman]

The API_AVAILABLE in the header is enough to give the warning. Are you trying to guard against people ignoring the warning and calling it anyway with a nil center and response?

[jmagman]

You don't need this if you add availability to registerAsUserNotificationCenterDelegate.

[jmagman]

Same here, no API_AVAILABLE in the implementation, and you don't need the @availabile check because the header is decorated with the API availability.

[jmagman]

It looks like this was renamed to _delegates in 273f4cf#diff-71f628ed3f0d51c80d58e053985f8a81R22? Does this code compile?

[jmagman]

You don't need this nil check because [nil respondsToSelector:_cmd] is always NO. It's better to just call selectors on nil instead of nil-checking everywhere since the message sends are already optimized to handle nil.

[chinmaygarde]

This is causing LUCI failures on ToT on all iOS jobs.

[bparrishMines]

@chinmaygarde I can revert this to find out what went wrong.

[chinmaygarde]

I am also trying to reproduce to see if we can just roll forward.

[dnfield]

Errors indicate something isn't guarded properly for <iOS 10.0...

[bparrishMines]

It looks like the line if (@available(iOS 10.0, *)) is required in FlutterPluginAppLifeCycleDelegate.mm since the UNUserNotificationCenter is 10+

[jmagman]

Looks like the -[userNotificationCenter:didReceiveNotificationResponse:withCompletionHandler:] implementation needs the __IPHONE_OS_VERSION_MAX_ALLOWED check (not API_AVAILABLE)

[chinmaygarde]

#12084 fixes the tree.

[magicleon94]

Hey is this PR still in in progress? Will it be merged any soon?

[bparrishMines]

@magicleon94 This PR is already merged. Addition of this feature to firebase_messaging is in firebase/flutterfire#121 if that is what you were looking for.