Add code to support the new notification system starting with iOS 10 #2207
Conversation
|
That's great. Thanks a lot @fridtjof. |
… using iOS 10+ features easier
There was a problem hiding this comment.
Thank you for your contribution, we are close to integrate UserNotifications framework in Riot thanks to you.
You are not obliged to fix all comments especially factorization ones. There are here for the record.
FYI we also have to make some changes on our side before using UserNotifications framework:
Indeed in settings the notification toggle value match -[MXKAccount isPushKitNotificationActive] value. This method rely on -[MXKAccountManager isPushAvailable] that check deprecated local notification API (UIUserNotificationSettings) to know if notifcation are enabled. Now we have to use -[UNUserNotificationCenter getNotificationSettingsWithCompletionHandler:] for this. The problem is that this method is asynchronous, this means that we have to make some rework before using UserNotifications framework and what you've done in this PR.
We also have to check periodically for notification settings changes as they could changed at any time. For example in applicationDidBecomeActive:.
In all cases thank you for your work we will mention you in changelogs when you're PR will be merged, if you are agree.
| UNNotification *notification = response.notification; | ||
| UNNotificationContent *content = notification.request.content; | ||
| if ([[response actionIdentifier] isEqualToString:@"inline-reply"]) { | ||
| UNTextInputNotificationResponse *textResponse = (UNTextInputNotificationResponse *) response; |
There was a problem hiding this comment.
Note:
Could be interesting to factorize inline reply code between the two APIs with a method signature like this for example:
- (void)handleNotificationInlineReplyForRoomId:(NSString*)roomId withResponseText:(NSString*)responseText success:(void(^)(NSString *eventId))success failure:(void(^)(NSError *error))failure.
|
I forgot to push one slight refactor (8cb6b86) - this duplicates code for now, but is necessary in preparation for using the new notification features imo. I can roll it back and reintroduce it later though, if you (understandably) don't want the code duplication. |
|
I addressed most of the reviews, as it's time for bed now. On each review, I linked the corresponding commit for easy referencing. |
Nothing to add, you did a good job all issues has been addressed :) Thank you for your work, I will approve your PR and you will be mentioned in changelog when it will be merged |
WARNING: I couldn't test any of this, because I can't build this app for anything except the simulator (I don't have a paid dev membership - necessary for some capabilities the app has, and also I don't have (for obvious reasons) the entitlements that would allow me to easily receive push notifications from the "official" New Vector push server, which is needed to test any notification related behaviour).
Therefore, I'm kindly asking for someone from the team to test my changes on a device with iOS 10 or higher. Feedback is much appreciated :)
This is the first step in supporting newer notifications features. By using the UserNotifications framework, we will be able to implement new notification features, like titles, grouping and more (see #2208)
Signed-off-by: Fridtjof Mund 2780577+fridtjof@users.noreply.github.com