-
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
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
There's a breaking change coming from upstream that I think might affect this work: #1546 (comment) |
7692841
to
22f6d28
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.
Thanks for the revision! This all looks good. One nit below, and see also one comment above: #1379 (comment)
Then this is otherwise ready to merge.
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.
notif test: Refactor tests for NotificationOpenService
Cool, this commit is pretty crisp to read, thanks. (Continuing the thread from #1379 (comment) .)
It's an NFC change, right? Even to the tests. (Or does it have an effect on the tests' behavior which I'm missing?) If so, let's specify that in the commit message.
We can also make the message more specific about what the refactor is doing. Like:
notif test [nfc]: Pull out androidNotificationUrlForMessage and setupNotificationDataForLaunch
That's information the reader basically has to work out one way or another in order to understand what the commit is doing and confirm it doesn't affect how the tests behave. So it's helpful to give them that information up front.
22f6d28
to
f753fec
Compare
Thanks for the review @gnprice! Pushed an update, PTAL. |
…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.
…NotificationDataForLaunch
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.
Thanks! Looks good; merging. |
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).