-
Notifications
You must be signed in to change notification settings - Fork 308
notif ios: Handle opening of conversation on tap; take 2 #1379
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
base: main
Are you sure you want to change the base?
Conversation
463b9ee
to
8478226
Compare
0142dbd
to
89df63b
Compare
89df63b
to
4f3224a
Compare
80a34eb
to
9c07740
Compare
Ah this has gathered a conflict in lib/widgets/app.dart; could you resolve it please? (I see you did a few days ago, but looks like it's happened again; thanks. 🙂) |
3b50218
to
2178fe6
Compare
(Rebased to main, Thanks!) |
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.
Thanks! There's a lot here, and I haven't gotten around to it all today. But here are some comments from an initial review.
lib/widgets/app.dart
Outdated
GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context); | ||
return child!; | ||
}, | ||
return DeferrredBuilderWidget( |
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.
nit: check spelling (here and in commit message)
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.
(also spelling of _ZulipAppState.initState
in the commit message)
lib/widgets/store.dart
Outdated
/// Provides access to the app's data. | ||
/// | ||
/// There should be one of this widget, near the root of the tree. | ||
/// | ||
/// See also: | ||
/// * [GlobalStoreWidget.of], to get access to the data. | ||
/// * [PerAccountStoreWidget], for the user's data associated with a | ||
/// particular Zulip account. | ||
class GlobalStoreWidget extends StatefulWidget { | ||
// This is separate from [GlobalStoreWidget] only because we need | ||
// a [StatefulWidget] to get hold of the store, and an [InheritedWidget] to | ||
// provide it to descendants, and one widget can't be both of those. | ||
class GlobalStoreWidget extends InheritedNotifier<GlobalStore> { |
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.
Did you mean to move this implementation comment here and delete the dartdoc? The implementation comment doesn't make sense in this context—saying GlobalStoreWidget
is "separate from" GlobalStoreWidget
.
test/widgets/content_test.dart
Outdated
child: PerAccountStoreWidget(accountId: eg.selfAccount.id, | ||
child: RealmContentNetworkImage(src)))); | ||
await tester.pumpWidget(DeferrredBuilderWidget( | ||
future: ZulipBinding.instance.getGlobalStoreUniquely(), |
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.
We normally do this as testBinding.getGlobalStoreUniquely
, right?
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.
Also, for these tests that aren't specifically about GlobalStoreWidget
, it would be simpler to use TestZulipApp
instead, I think.
test/widgets/store_test.dart
Outdated
store: store, | ||
child: PerAccountStoreWidget( | ||
accountId: accountId, | ||
child: MyWidgetWithMixin(key: widgetWithMixinKey))); |
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.
Maybe we can use TestZulipApp
for this test?
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.
Used TestZulipApp
in tests where possible, but kept this unchanged because I was getting the same extraneous dep changes mentioned in the above TODO.
lib/notifications/open.dart
Outdated
case TargetPlatform.android: | ||
case TargetPlatform.fuchsia: | ||
case TargetPlatform.linux: | ||
case TargetPlatform.macOS: | ||
case TargetPlatform.windows: | ||
// Do nothing; we don't offer notifications on these platforms. | ||
break; |
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 comment is wrong about Android; we do offer notifications on Android.
A lot of the code added in this commit is implemented just for iOS, but named/documented as though it's cross-platform, because it doesn't say it's just for iOS.
I don't know if we plan to align the implementation with the names/docs or vice versa. The answer might be in the later commits (I haven't read them yet), but it would be helpful to comment on this in the commit message, I think.
lib/widgets/app.dart
Outdated
List<Route<dynamic>> _handleGenerateInitialRoutesIos(_) { | ||
// The `_ZulipAppState.context` lacks the required ancestors. Instead | ||
// we use the Navigator which should be available when this callback is | ||
// called and it's context should have the required ancestors. |
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.
nit: "its"
test/notifications/open_test.dart
Outdated
await tester.pump(); | ||
takeStartingRoutes(); | ||
matchesNavigation(check(pushedRoutes).single, account, message); | ||
debugDefaultTargetPlatformOverride = null; |
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.
Hmm, awkward to have this teardown line so far away from its corresponding setup line.
How about instead passing variant: const TargetPlatformVariant({TargetPlatform.iOS}))
to testWidgets
?
final route = _routeForNotification(context, payload); | ||
if (route == null) return; // TODO(log) | ||
|
||
// TODO(nav): Better interact with existing nav stack on notif open |
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.
Maybe we should have an iOS counterpart to
for this? Or make that a both-platforms issue? That issue says:
(The iOS counterpart is covered by #1147, for navigating at all when a notification is tapped.)
but it seems reasonable to postpone this part of it; we'd just want to keep track of it.
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.
After this PR is merged the potential fix for that issue would work on both platforms, because this PR consolidates the notification routing implementation on both iOS and Android.
pigeon/notifications.dart
Outdated
@EventChannelApi() | ||
abstract class NotificationHostEvents { | ||
/// An event stream that emits a notification payload when | ||
/// app encounters a notification tap, while the app is runnning. |
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.
nit: "the app encounters a notification tap, while the app is running."
test/notifications/open_test.dart
Outdated
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); | ||
await prepare(tester); | ||
await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage()); | ||
debugDefaultTargetPlatformOverride = null; |
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.
(same comment about maybe using variant: const TargetPlatformVariant({TargetPlatform.iOS})
)
2178fe6
to
616defe
Compare
Thanks for the review @chrisbobbe! Pushed a new revision, PTAL. |
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.
Thanks! Here's a more thorough review of the first four commits:
d1f648c app: Use DeferredBuilderWidget while loading GlobalStore
a5a0fff pigeon [nfc]: Rename pigeon file to notification
-> android_notifications
ab3ff84 notif ios: Navigate when app launched from notification
4b2ade0 notif ios: Navigate when app running but in background
That leaves the last two commits:
28ea77f notif android: Migrate to cross-platform Pigeon API for navigation
616defe docs: Document testing push notifications on iOS Simulator
Actually for your next revision, could you send a new PR with everything except the "Migrate to cross-platform" commit? That one's pretty large, so makes sense to review separately.
ios/Runner/Notifications.g.swift
Outdated
@@ -0,0 +1,167 @@ | |||
// Autogenerated from Pigeon (v24.2.1), do not edit directly. |
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 needs an update for the Pigeon 25 upgrade e2aac35.
lib/widgets/app.dart
Outdated
/// The widget to build when [future] completes, with it's result | ||
/// passed as `result`. |
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.
nit: "its"
@@ -381,7 +381,7 @@ data class StoredNotificationSound ( | |||
) | |||
} | |||
} | |||
private open class NotificationsPigeonCodec : StandardMessageCodec() { | |||
private open class AndroidNotificationsPigeonCodec : StandardMessageCodec() { |
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.
Commit-message nit:
pigeon [nfc]: Rename pigeon file to `notification` -> `android_notifications`
I think the "to" should be deleted? Or moved to replace the "->"?
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.
pigeon [nfc]: Rename pigeon file `notifications.dart` to `android_notifications.dart`
commit-message nit: limit summary line length to 76 (this is 85).
pigeon/notifications.dart
Outdated
/// On iOS, this checks and returns value for the `remoteNotification` key | ||
/// in the `launchOptions` map. The value could be either the raw APNs data | ||
/// dictionary, if the launch of the app was triggered by a notification tap, | ||
/// otherwise it will be null. | ||
/// | ||
/// See: https://developer.apple.com/documentation/uikit/uiapplication/launchoptionskey/remotenotification |
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.
Can be a bit more concise I think:
/// Returns `launchOptions.remoteNotification`,
/// which is the raw APNs data dictionary
/// if the app launch was opened by a notification tap,
/// else null. See Apple doc:
/// https://developer.apple.com/documentation/uikit/uiapplication/launchoptionskey/remotenotification
And this is only used on iOS at this commit, right; the "On iOS" can be added in the later commit where it also starts being used on Android.
test/model/binding.dart
Outdated
FakeNotificationPigeonApi? _notificationPigeonApi; | ||
|
||
@override | ||
FakeNotificationPigeonApi get notificationPigeonApi { | ||
return (_notificationPigeonApi ??= FakeNotificationPigeonApi()); | ||
} |
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.
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.
Also, can use fewer lines:
@override
FakeNotificationPigeonApi get notificationPigeonApi =>
(_notificationPigeonApi ??= FakeNotificationPigeonApi());
ios/Runner/AppDelegate.swift
Outdated
func onNotificationTapEvent(data: NotificationPayloadForOpen) { | ||
if let eventSink = eventSink { | ||
eventSink.success(data) | ||
} | ||
} |
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.
Instead of the one class NotificationPayloadForOpen
, could we have two separate classes NotificationDataFromLaunch
and NotificationTapEvent
? Perhaps with a Payload
typedef helper:
typedef Payload = Map<Object?, Object?>;
class NotificationDataFromLaunch {
const NotificationDataFromLaunch({required this.payload});
/// The raw payload that is attached to the notification,
/// holding the information required to carry out the navigation.
final Payload payload;
}
class NotificationTapEvent {
const NotificationTapEvent({required this.payload});
/// The raw payload that is attached to the notification,
/// holding the information required to carry out the navigation.
final Payload payload;
}
I think the current naming makes the event-channel code harder to read than it needs to be. When reading the Pigeon example code, I see "event" used pretty consistently in the names of things. Here, we have both "data" (onNotificationTapEvent
's param) and "payload", and "payload" is used ambiguously for a payload (NotificationPayloadForOpen.payload
) and something that contains a payload (NotificationPayloadForOpen
itself).
That would mean, for this method:
func onNotificationTapEvent(event: NotificationTapEvent) {
if let eventSink = eventSink {
eventSink.success(event)
}
}
pigeon/notifications.dart
Outdated
@EventChannelApi() | ||
abstract class NotificationHostEvents { |
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.
Would NotificationEventChannelApi
be a better name for this, to make it clearer what kind of thing it is?
lib/model/binding.dart
Outdated
// in global scope of the generated file. This is a helper class to | ||
// namespace the notification related Pigeon API under a single class. | ||
class NotificationPigeonApi { | ||
final _notifInteractionHost = notif_pigeon.NotificationHostApi(); |
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.
Is _notifInteractionHost
the best name for this? How about _hostApi
? We might add more to NotificationHostApi
that's not about interacting with notifications (such as a method to query the current notification permissions). Also, NotificationHostApi
isn't the only code that's about interacting with notifications; notif_pigeon.notificationTapEvents
is too.
lib/notifications/open.dart
Outdated
/// Navigates to the [MessageListPage] of the specific conversation | ||
/// for the provided payload that was attached while creating the | ||
/// notification. | ||
Future<void> _navigateForNotification(NotificationPayloadForOpen payload) async { |
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.
Another reason the NotificationPayloadForOpen
rename would be helpful: currently, it's pretty unclear that this private method is only about notifications that come while the app is open. It'll be helpful for debugging if it's easier to see what code is about the launch notification vs. not.
|
||
testWidgets('(iOS) stream message', (tester) async { | ||
addTearDown(testBinding.reset); | ||
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); |
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 add-self-account line looks boring; can we put it in prepare
?
173f11f
to
9813a4d
Compare
2e40fb0
to
02506ba
Compare
Thanks for the review @gnprice! Pushed an update, PTAL. |
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.
Cool, thanks for splitting these out as separate commits — that's very helpful for reading them. They all look good.
5292653 notif [nfc]: Rename NotificationOpenPayload methods
d130717 notif [nfc]: Refactor NotificationOpenService.routeForNotification
b2e0ed0 notif: Show a dialog if received malformed Android Notification URL
I've now finished reading the whole thing, and generally it all looks good; just a handful of comments below. A couple of them are for follow-ups we can pursue after launch.
There's one commit making a bunch of test changes which I'd like split up to make clearer — as is, I don't yet fully understand all the things it's doing, which means it's hard to be sure whether it's accidentally undermining some of the tests. (Which in turn would mean there could be a bug that the tests are no longer able to catch.)
test/notifications/open_test.dart
Outdated
if (!dropStartingRoutes) { | ||
check(pushedRoutes).isEmpty(); | ||
return; | ||
} | ||
await tester.pump(); | ||
takeStartingRoutes(account: account); | ||
check(pushedRoutes).isEmpty(); | ||
} | ||
|
||
Uri androidNotificationUrlForMessage(Account account, Message message) { |
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 commit f12f911 does several different things:
notif test: Refactor tests for NotificationOpenService
---
test/notifications/open_test.dart | 103 +++++++++++++++++++++++------------------------------
1 file changed, 45 insertions(+), 58 deletions(-)
- make
init
not handle adding the account to the store, moving that to its callers - OTOH make
prepare
add the account to the store by default, which it previously never did, moving that from its callers - add an
account
option toprepare
, for using something other thaneg.selfAccount
- rename
early
todropStartingRoutes
and flip the direction of it - split out this
androidNotificationUrlForMessage
method fromopenNotification
- a couple of other things
I understand some of the changes it's making. But it's hard to track all of them, and as a reviewer to check that they all line up and identify any changes in behavior they might make, because there are so many things combined in one.
It's also hard to tell what the motivation is for many of them, partly because it seems like there are probably two or three different motivations for different changes.
So these would be helpful to split into several commits:
- Those first three points, about managing whether to involve an account and which one, would be at least one commit of their own. Probably can be further clarified by splitting into two or three commits.
- The
early
/dropStartingRoutes
point should be an easy small commit of its own. - The
androidNotificationUrlForMessage
split makes another. - Then at least one other commit for the remaining changes (like
setupNotificationDataForLaunch
); not sure how many because I'm not sure what all the remaining changes are.
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.
I've removed the unnecessary refactors in the latest revision but kept the changes for androidNotificationUrlForMessage
, openNotification
and setupNotificationDataForLaunch
in the test refactor commit (075bae8) as they were essential for simpler diff for these tests in the later commits.
}); | ||
|
||
group('parseIosApnsPayload', () { | ||
test('smoke one-one DM', () { |
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.
Cool, these tests are a good start.
Because of the nature of this function as parsing code (echoing #1379 (comment)), and because notifications are important, I'd like to have more thorough testing for it than only smoke tests.
But let's make that a follow-up issue we can pursue after launch. Even if there are edge cases this code doesn't do anything on, that won't be a regression from what's in main. (And if there are edge cases where it does something but it's the wrong thing, going to the wrong narrow is probably not much worse than doing nothing.)
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.
Filed: #1563
switch (defaultTargetPlatform) { | ||
case TargetPlatform.android: | ||
final intentDataUrl = androidNotificationUrlForMessage(account, message); | ||
unawaited( | ||
WidgetsBinding.instance.handlePushRoute(intentDataUrl.toString())); | ||
await tester.idle(); // let navigateForNotification find navigator | ||
|
||
case TargetPlatform.iOS: |
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.
Cool, I like this mechanism for reusing the same test code for both platforms.
case TargetPlatform.iOS: | ||
_notifDataFromLaunch = await _notifPigeonApi.getNotificationDataFromLaunch(); | ||
_notifPigeonApi.notificationTapEventsStream() | ||
.listen(_navigateForNotification); |
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.
What happens if the user opens the app from the home screen, and then immediately taps a notification, while the app is still just beginning to start up and so before it reaches this listen
call?
In that case the notification won't get passed at startup to application(_:didFinishLaunchingWithOptions:)
. Instead it'll go to userNotificationCenter(_:didReceive:withCompletionHandler:)
. Will it make it to the app's Dart code so that the right conversation gets opened, or will it get dropped because there isn't a listener yet?
Looking at the Swift code, I think it will get dropped:
override func onListen(withArguments arguments: Any?, sink: PigeonEventSink<NotificationTapEvent>) {
eventSink = sink
}
func onNotificationTapEvent(payload: [AnyHashable : Any]) {
eventSink?.success(NotificationTapEvent(payload: payload))
}
I think the ideal behavior would be that instead it gets buffered, stored in the stream, and then when the app reaches this point it consumes any notification that's been buffered there.
If there isn't an obvious tweak to make that would provide that behavior, we can cheerfully leave it as a followup. I think this isn't a super common case.
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.
I tried the buffering implementation but weirdly I was observing some repeated navigations when tapped on a notification. So, I think I will leave this as a follow up, to investigate more about it later.
[greg: cherry-picked from zulip#1379]
[greg: cherry-picked from #1379]
There's a breaking change coming from upstream that I think might affect this work: #1546 (comment) |
02506ba
to
7692841
Compare
…art` This makes it clear that these bindings are for Android only.
And fix a typo.
Also make use of a handy shorthand within `jq`.
And move the notification navigation data parsing utilities to the new class.
To make it clear that they are Android specific.
Update it to receive `NotificationOpenPayload` as an argument instead of the Android Notification URL. Also rename `navigateForNotification` to `navigateForAndroidNotificationUrl`, making it more explicit.
Introduces NotificationOpenPayload.parseIosApnsPayload which can parse the payload that Apple push notification service delivers to the app for displaying a notification. It retrieves the navigation data for the specific message notification.
Introduces a new Pigeon API file, and adds the corresponding bindings in Swift. Unlike the `pigeon/android_notifications.dart` API this doesn't use the ZulipPlugin hack, as that is only needed when we want the Pigeon functions to be available inside a background isolate (see doc in `zulip_plugin/pubspec.yaml`). Since the notification tap will trigger an app launch first (if not running already) anyway, we can be sure that these new functions won't be running on a Dart background isolate, thus not needing the ZulipPlugin hack.
7692841
to
22f6d28
Compare
Thanks for the review @gnprice! Pushed an update, PTAL. |
Fixes #1147.
2nd attempt, first attempt was #1261. This one uses pigeon to move most of the notification payload handling to dart side (and doesn't use/rely on
zulip://notification
URL).