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

Add capability to add AppDelegate as UNUserNotificationCenterDelegate #9864

Merged
bparrishMines merged 28 commits into from
Sep 9, 2019
Merged

Add capability to add AppDelegate as UNUserNotificationCenterDelegate #9864

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d48d4a2
Add other UNUserNotificationDelegate method and register as delegate
bparrishMines Jul 16, 2019
a42883a
Add UNUserNotificationDelegate to receive delegate calls
bparrishMines Jul 17, 2019
a5a3ac3
format FlutterAppDelegate.h
bparrishMines Jul 17, 2019
151293c
format FlutterPlugin.h
bparrishMines Jul 17, 2019
ccc9b64
format FormatAppDelegate.mm
bparrishMines Jul 17, 2019
e6f6b57
format FlutterPluginAppLifeCycleDelegate.mm
bparrishMines Jul 17, 2019
6af5edd
Formatting
bparrishMines Jul 19, 2019
5e1c0c8
Merge branch 'master' into notifications
bparrishMines Jul 19, 2019
bd8ea05
Ad if statement in FlutterAppDelegate
bparrishMines Aug 12, 2019
e33de17
Use __IPHONE_OS_VERSION_MAX_ALLOWED instead
bparrishMines Aug 12, 2019
e2b0ab8
Guard only a single line
bparrishMines Aug 13, 2019
abd3bc6
Merge branch 'master' of github.com:flutter/engine into notifications
bparrishMines Aug 13, 2019
c6c2eea
Keep availability checks in headers
bparrishMines Aug 13, 2019
5618d26
Remove check of UNUserNotificationCenter
bparrishMines Aug 13, 2019
7d82637
Move UNUserNotificationCenterDelegate to FlutterApplicationLifeCycleD…
bparrishMines Aug 13, 2019
204d8b1
Make FlutterAppLifeCycleProvider conform to UNUserNotificationCenterD…
bparrishMines Aug 20, 2019
168c674
Add single quotes
bparrishMines Aug 20, 2019
8e0def6
Formatting
bparrishMines Aug 20, 2019
2501158
Remove delegate methods and use protocol in FlutterPlugin.h
bparrishMines Aug 22, 2019
8875d35
Update FlutterPluginAppLifeCycleDelegate.h
bparrishMines Aug 22, 2019
108155f
Merge branch 'master' of github.com:flutter/engine into notifications
bparrishMines Aug 22, 2019
fb05b3a
Formatting
bparrishMines Aug 22, 2019
4ab5307
If Statement formatting
bparrishMines Aug 22, 2019
d4415f4
Formatting
bparrishMines Aug 29, 2019
1002702
Add respondsToSelector to optional methods
bparrishMines Sep 5, 2019
e9f5d00
Merge branch 'notifications' of github.com:bparrishMines/engine into …
bparrishMines Sep 5, 2019
3e299e4
Square brackets
bparrishMines Sep 5, 2019
5a84f3d
Merge branch 'master' of github.com:flutter/engine into notifications
bparrishMines Sep 5, 2019
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
6 changes: 4 additions & 2 deletions shell/platform/darwin/ios/framework/Headers/FlutterAppDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
* code as necessary from FlutterAppDelegate.mm.
*/
FLUTTER_EXPORT
@interface FlutterAppDelegate
: UIResponder <UIApplicationDelegate, FlutterPluginRegistry, FlutterAppLifeCycleProvider>
@interface FlutterAppDelegate : UIResponder <UIApplicationDelegate,
FlutterPluginRegistry,
FlutterAppLifeCycleProvider,
UNUserNotificationCenterDelegate>

@dnfield dnfield Jul 19, 2019

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 would need to be guarded, it's iOS 10+.

I thought @cyanglaz made this stuff happen dynamically?

@bparrishMines bparrishMines Jul 23, 2019

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 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 bparrishMines Jul 23, 2019

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.

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

@dnfield dnfield Aug 5, 2019

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.

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

@bparrishMines bparrishMines Aug 12, 2019

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 added an if statement, but I'm confident I didn't implement it correctly. How would I go about doing this?

@jmagman jmagman Aug 12, 2019

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.

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


@property(strong, nonatomic) UIWindow* window;

Expand Down
14 changes: 14 additions & 0 deletions shell/platform/darwin/ios/framework/Headers/FlutterPlugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,13 @@ NS_ASSUME_NONNULL_BEGIN
(void (^)(UNNotificationPresentationOptions options))completionHandler
API_AVAILABLE(ios(10));

/**
* Calls all plugins registered for `UNUserNotificationCenterDelegate` callbacks.
*/
- (void)userNotificationCenter:(UNUserNotificationCenter*)center
Comment thread
dnfield marked this conversation as resolved.
Outdated

@jmagman jmagman Aug 13, 2019

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.

Now you don't need this because its conforming to UNUserNotificationCenterDelegate so you get it for free.

@bparrishMines bparrishMines Aug 20, 2019

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.

The FlutterAppLifeCycleProvider now conforms to UNUserNotificationCenterDelegate. So FlutterPlugin should require this now, right?

@jmagman jmagman Aug 20, 2019

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.

FlutterAppDelegate is calling

[_lifeCycleDelegate userNotificationCenter:center
              didReceiveNotificationResponse:response
                       withCompletionHandler:completionHandler];

and _lifeCycleDelegate is a FlutterPluginAppLifeCycleDelegate, so that's the protocol that needs the method either by declaring it @optional or by conforming to UNUserNotificationCenterDelegate.

Instead of explicitly declaring -userNotificationCenter:didReceiveNotificationResponse:withCompletionHandler API_AVAILABLE(ios(10)) in the headers anywhere I would just have those protocols and/or classes conform to UNUserNotificationCenterDelegate so you just get whatever methods the SDK thinks you should (unlikely example: they add a new required method to that protocol. Now you get nice compilation errors that you are missing required implementation of those methods).

Up to you though!

didReceiveNotificationResponse:(UNNotificationResponse*)response
withCompletionHandler:(void (^)(void))completionHandler API_AVAILABLE(ios(10));

/**
* Called if this has been registered for `UIApplicationDelegate` callbacks.
*
Expand Down Expand Up @@ -375,6 +382,13 @@ typedef void (*FlutterPluginRegistrantCallback)(NSObject<FlutterPluginRegistry>*
*/
@protocol FlutterAppLifeCycleProvider
- (void)addApplicationLifeCycleDelegate:(NSObject<FlutterApplicationLifeCycleDelegate>*)delegate;

/**
* Implement this in the `UIAppDelegate` of your app to enable Flutter plugins to receive
* calls as `UNUserNotificationCenterDelegate` when they are added to
* `addApplicationLifeCycleDelegate`.
*/
- (void)registerAsUserNotificationCenterDelegate;

@jmagman jmagman Aug 12, 2019

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 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 bparrishMines Aug 13, 2019

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.

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

@end

NS_ASSUME_NONNULL_END;
Expand Down
7 changes: 7 additions & 0 deletions shell/platform/darwin/ios/framework/Headers/FlutterPluginAppLifeCycleDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ FLUTTER_EXPORT
(void (^)(UNNotificationPresentationOptions options))completionHandler
API_AVAILABLE(ios(10));

/**
* Calls all plugins registered for `UNUserNotificationCenterDelegate` callbacks.
*/
- (void)userNotificationCenter:(UNUserNotificationCenter*)center
didReceiveNotificationResponse:(UNNotificationResponse*)response
withCompletionHandler:(void (^)(void))completionHandler API_AVAILABLE(ios(10));

/**
* Calls all plugins registered for `UIApplicationDelegate` callbacks in order of registration until
* some plugin handles the request.
Expand Down
21 changes: 21 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterAppDelegate.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,19 @@ - (void)userNotificationCenter:(UNUserNotificationCenter*)center
}
}

/**
* Calls all plugins registered for `UNUserNotificationCenterDelegate` callbacks.
*/
- (void)userNotificationCenter:(UNUserNotificationCenter*)center
didReceiveNotificationResponse:(UNNotificationResponse*)response
withCompletionHandler:(void (^)(void))completionHandler API_AVAILABLE(ios(10)) {

@jmagman jmagman Aug 12, 2019

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.

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

if (@available(iOS 10.0, *)) {

@dnfield dnfield Aug 12, 2019

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.

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

@jmagman jmagman Aug 12, 2019

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.

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?

[_lifeCycleDelegate userNotificationCenter:center
didReceiveNotificationResponse:response
withCompletionHandler:completionHandler];
}
}

- (BOOL)application:(UIApplication*)application
openURL:(NSURL*)url
options:(NSDictionary<UIApplicationOpenURLOptionsKey, id>*)options {
Expand Down Expand Up @@ -177,6 +190,14 @@ - (void)addApplicationLifeCycleDelegate:(NSObject<FlutterPlugin>*)delegate {
[_lifeCycleDelegate addDelegate:delegate];
}

- (void)registerAsUserNotificationCenterDelegate {
if ([UNUserNotificationCenter class] != nil) {

@jmagman jmagman Aug 12, 2019

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.

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

// iOS 10 or later
// For iOS 10 display notification (sent via APNS)
[UNUserNotificationCenter currentNotificationCenter].delegate = self;
}
}

#pragma mark - UIApplicationDelegate method dynamic implementation

- (BOOL)respondsToSelector:(SEL)selector {
Expand Down
17 changes: 17 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterPluginAppLifeCycleDelegate.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,23 @@ - (void)userNotificationCenter:(UNUserNotificationCenter*)center
}
}

- (void)userNotificationCenter:(UNUserNotificationCenter*)center
didReceiveNotificationResponse:(UNNotificationResponse*)response
withCompletionHandler:(void (^)(void))completionHandler API_AVAILABLE(ios(10)) {
if (@available(iOS 10.0, *)) {

@jmagman jmagman Aug 12, 2019

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.

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.

for (id<FlutterPlugin> plugin in _pluginDelegates) {

@jmagman jmagman Aug 12, 2019

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.

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

if (!plugin) {

@jmagman jmagman Aug 12, 2019

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.

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.

continue;
}
if ([plugin respondsToSelector:_cmd]) {
[plugin userNotificationCenter:center
didReceiveNotificationResponse:response
withCompletionHandler:completionHandler];
}
}
}
}

- (BOOL)application:(UIApplication*)application
openURL:(NSURL*)url
options:(NSDictionary<UIApplicationOpenURLOptionsKey, id>*)options {
Expand Down