-
Notifications
You must be signed in to change notification settings - Fork 497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for in-app notifications. #6341
Conversation
@@ -347,7 +347,7 @@ - (void)userNotificationCenter:(UNUserNotificationCenter *)center willPresentNot | |||
{ | |||
completionHandler(UNNotificationPresentationOptionBadge | |||
| UNNotificationPresentationOptionSound | |||
| UNNotificationPresentationOptionAlert); | |||
| UNNotificationPresentationOptionBanner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Alert option was deprecated in iOS 14. I'm not sure what this means for the Constants.userInfoKeyPresentNotificationOnForeground
check: one change could be to include it in notification centre too if that is set, but that didn't seem worthwhile to me.
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/WQGm4c |
Hi there, thanks for working on this. Here is my repsonse to the questions:
Additionaly, I would imagine that the notification banner will have the avatar of the user and not element's logo? @daniellekirkwood let me know if you disagree with any of this. |
Super thanks for the feedback.
Yep, it will only work if the user allows notifications, so leaving it as enabled by default with give this exact behaviour.
Sadly this is the system notification banner so it is shown as is. However! There is an issue (#4710) and an oldish community PR (#4819) to enable this style on iOS 15, so there is potential this will happen separately at some stage. |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great action. One thing I am not clear on is the check to not show notifications for the currently open room. Where is this implemented?
@@ -2656,100 +2618,6 @@ - (UIViewController*)presentedViewController | |||
|
|||
#pragma mark - Matrix Accounts handling | |||
|
|||
- (void)enableInAppNotificationsForAccount:(MXKAccount*)account |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice chunk of code gone 🎉
There's a check on LegacyAppDelegate.visibleRoomId which is set/cleared by RoomViewController when the room appears/disappears. |
Ah indeed, thanks |
The old in app notifications UI from the app delegate was an alert which was rather obtrusive. However
PushNotificationService
includes a way for a notification to request native in-app presentation (and makes sure to only do so if the room is not currently visible). This PR adds a setting that is checked in the service alongside the notification content and removes the old UI.Could do with direction from Product on whether
Fixes #1108.