notif ios: Handle opening of conversation on tap; take 2

[rajveermalviya]

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).

[chrisbobbe]

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. 🙂)

[rajveermalviya]

(Rebased to main, Thanks!)

[chrisbobbe]

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.

[chrisbobbe]

nit: check spelling (here and in commit message)

[chrisbobbe]

(also spelling of _ZulipAppState.initState in the commit message)

[chrisbobbe]

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.

[chrisbobbe]

We normally do this as testBinding.getGlobalStoreUniquely, right?

[chrisbobbe]

Also, for these tests that aren't specifically about GlobalStoreWidget, it would be simpler to use TestZulipApp instead, I think.

[chrisbobbe]

Maybe we can use TestZulipApp for this test?

[rajveermalviya]

Used TestZulipApp in tests where possible, but kept this unchanged because I was getting the same extraneous dep changes mentioned in the above TODO.

[chrisbobbe]

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.

[chrisbobbe]

nit: "its"

[chrisbobbe]

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?

[chrisbobbe]

Maybe we should have an iOS counterpart to

• android: When opening a notification, "back" should not leave the account's context #1210

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.

[rajveermalviya]

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.

[chrisbobbe]

nit: "the app encounters a notification tap, while the app is running."

[chrisbobbe]

(same comment about maybe using variant: const TargetPlatformVariant({TargetPlatform.iOS}))

[rajveermalviya]

Thanks for the review @chrisbobbe! Pushed a new revision, PTAL.

[chrisbobbe]

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.

[chrisbobbe]

This needs an update for the Pigeon 25 upgrade e2aac35.

[chrisbobbe]

nit: "its"

[chrisbobbe]

Commit-message nit:

pigeon [nfc]: Rename pigeon file to `notification` -> `android_notifications`

I think the "to" should be deleted? Or moved to replace the "->"?

[chrisbobbe]

pigeon [nfc]: Rename pigeon file `notifications.dart` to `android_notifications.dart`

commit-message nit: limit summary line length to 76 (this is 85).

[chrisbobbe]

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.

[chrisbobbe]

nit: getter before private field: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#order-other-class-members-in-a-way-that-makes-sense

[chrisbobbe]

Also, can use fewer lines:

  @override
  FakeNotificationPigeonApi get notificationPigeonApi =>
    (_notificationPigeonApi ??= FakeNotificationPigeonApi());

[chrisbobbe]

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)
    }
  }

[chrisbobbe]

Would NotificationEventChannelApi be a better name for this, to make it clearer what kind of thing it is?

[chrisbobbe]

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.

[chrisbobbe]

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.

[chrisbobbe]

This add-self-account line looks boring; can we put it in prepare?

[gnprice]

I started reviewing the new revision, but then realized you hadn't actually said it was ready for review yet. 🙂 Anyway, here's the comments I wrote down so far, all on the first commit:
e6e19e1 app: Move initialization of GlobalStore from GlobalStoreWidget to ZulipApp

But I'll leave further re-review until you say it's ready.

[gnprice]

This test's name no longer seems accurate:

GlobalStoreWidget loads data while showing placeholder

as GlobalStoreWidget no longer does either of those things.

Probably just move this test to app_test.dart. (cf #1379 (comment) )

[gnprice]

Oh hmm and this version of the test no longer really tests the app's code — instead it's testing the replica in TestZulipApp of that code.

Can that be fixed by just saying ZulipApp here?

[gnprice]

This is kind of a weird feature for this to have. It really only makes sense for that one PerAccountStoreWidget test case that wants to be able to say this:

    // But then if we mount a separate PerAccountStoreWidget...
    // […]

    // (... even one that really is separate, with its own fresh state node ...)

So I'd rather avoid adding it here to the interface of TestZulipApp, which lots and lots of tests are using.

I think there's actually an easy solution for that test case without this: just don't pass accountId, and instead have the test continue supplying its own PerAccountStoreWidget objects.

[gnprice]

This isn't quite as weird a feature, but it's still quite boutique: just one test case uses it. So I'd again rather avoid adding it to this widely-shared helper class.

If you want to convert that test case to use TestZulipApp, I think the simplest solution is to have the test just insert its own Theme widget, so that it effectively ignores the theme that TestZulipApp put on the MaterialApp widget.

[gnprice]

OK, and I've now sat down and spent some time studying the Swift code (and reading docs to understand how it should work). Here's a round of review on that.

[gnprice]

This method never gets called, right?

In particular I notice it doesn't have override like onListen does — so it's not going to get called by any generic Pigeon code. And I think we never would call it, because there's never a point when we now know that there won't be any future notifications that get opened.

Let's leave it out, then. I see it's there in the Pigeon example doc, but I think that's just to illustrate how one would implement such a method if one needed it.

[gnprice]

We should be calling the completion handler unconditionally, I think, from my reading of the doc for the method we're implementing here:
https://developer.apple.com/documentation/usernotifications/unusernotificationcenterdelegate/usernotificationcenter(_:didreceive:withcompletionhandler:)

[gnprice]

What if the user dismisses a notification, instead of tapping it to open?

I believe that will also cause this method to be called. So we should be looking at response.actionIdentifier to determine what to do. See docs:
https://developer.apple.com/documentation/usernotifications/unnotificationresponse

This guide doc also seems helpful:
https://developer.apple.com/documentation/usernotifications/handling-notifications-and-notification-related-actions#Handle-user-selected-actions

[rajveermalviya]

Hmm, I tested this and found that dismissing a notification doesn't cause that method to be called. (Even with the small func implemented to support foreground notifications on iOS)

The the docs for UNNotificationDismissActionIdentifier say:

The system delivers this action only if your app configured the notification’s category object with the customDismissAction option.
…

And from UNNotificationDefaultActionIdentifier:

The delivery of this action doesn’t require any special configuration of notification categories. Use the userNotificationCenter(_:didReceive:withCompletionHandler:) method of your delegate object to receive this action.

So, since we do not setup a notification category using setNotificationCategories, we don't need to handle UNNotificationDismissActionIdentifier currently.

But adding a check anyway (response.actionIdentifier == UNNotificationDefaultActionIdentifier) in case that changes in future.

[gnprice]

Ah I see, good to know. Sounds good, though.

[gnprice]

Can this property actually be nil here?

I think it can't. We set it (to a non-nil value) before setting the delegate, so before this method could possibly get called.

So we can simplify by force-unwrapping, as listener!.

[gnprice]

This would do the same thing, right?

Suggested change

	if let eventSink = eventSink {
	eventSink.success(event)
	}
	eventSink?.success(event)

[gnprice]

I think this API would be a bit cleaner if we adjust it like so:

Suggested change

	listener.onNotificationTapEvent(event: NotificationTapEvent(payload: userInfo))
	listener.onNotificationTapEvent(payload: userInfo)

Then the NotificationTapEvent gets constructed by the method implementation.

That's also consistent with the Pigeon docs' example:

  func onIntEvent(event: Int64) {
    if let eventSink = eventSink {
      eventSink.success(IntEvent(data: event))
    }
  }

  func onStringEvent(event: String) {
    if let eventSink = eventSink {
      eventSink.success(StringEvent(data: event))
    }
  }

[gnprice]

What did you look at to determine this was the way to set up the Pigeon channels?

(I'm not finding a clear answer as I look now in the Pigeon docs.)

[rajveermalviya]

I had adapted this implementation by refering to Flutter's PlatformChannel examples and Pigeon's example.

[gnprice]

Excellent, thanks. This logic looks good, then.

For the style of it, let's use the terser as! form, rather than throwing with a custom error message. It should be an invariant of our app that there'll be a FlutterViewController here, and I think that's a boring enough fact that we don't need to call extra attention to it.

[rajveermalviya]

Thanks for the review @gnprice! Pushed an update, PTAL.

[gnprice]

Thanks for the revision!

Here's comments on just the first commit:
60577d6 app: Move initialization of GlobalStore from GlobalStoreWidget to ZulipApp

It's felt unsatisfying how that change concentrates more functionality on the omnibus widget ZulipApp, particularly as that causes it to get duplicated on TestZulipApp (which introduces the risk of divergence between test and live code). After the comment below about comments that refer to the store getting loaded, I noticed that it seemed like some of those comments were going to need to get more complicated, too, to explain the new situation.

So I went and tried implementing the solution I think I suggested when we looked at this task in a call a few weeks ago: have GlobalStoreWidget keep responsibility for loading the store, and add a parameter to have it await an additional future. I think that works out well. Just pushed back to the PR branch a revision with that change:
9426b82 store: Add "blocking future" option on GlobalStoreWidget

The new commit is nice and simple:

 lib/widgets/store.dart       | 11 ++++++++++
 test/widgets/store_test.dart | 54 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 2 deletions(-)

vs.

 6 files changed, 158 insertions(+), 168 deletions(-)

The effect on the subsequent "Navigate when app launched from notification" commit is small:

    -@@ lib/widgets/app.dart: class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
    -     WidgetsBinding.instance.addObserver(this);
    -     () async {
    -       final globalStore = await ZulipBinding.instance.getGlobalStoreUniquely();
    -+      final notifFuture = NotificationOpenManager.instance.initializationFuture;
    -+      if (notifFuture != null) await notifFuture;
    -       if (!mounted) return;
    -       setState(() {
    -         _globalStore = globalStore;
    +@@ lib/widgets/app.dart: class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
    +   @override
    +   Widget build(BuildContext context) {
    +     return GlobalStoreWidget(
    ++      blockingFuture: NotificationOpenManager.instance.initializationFuture,
    +       child: Builder(builder: (context) {

[gnprice]

Suggested change

	// First, shows a loading page instead of child.
	// First, shows a loading page.

The "instead of child" no longer makes sense in the ZulipApp context.

[gnprice]

This comment seems still relevant.

[gnprice]

There are a few places where comments mention GlobalStoreWidget to refer to the work it does of loading the global store. See git grep -PC2 '\[GlobalStoreWidget\]', and in particular some of the matches in lib/model/binding.dart, test/model/binding.dart, test/widgets/test_app.dart. So those will need to be updated.

[gnprice]

And here's a re-review on the docs commit.

[gnprice]

That's a version from this week, after you originally wrote this doc — did you confirm that it still produces the same output?

[rajveermalviya]

Yes, When I had tried this server version with the fresh dev environment, I had verified that it produced the same output, modulo the message content.

[gnprice]

Great, thanks!

[gnprice]

I think this isn't enough on its own — for getting one's phone to be able to talk to the dev server you'll want the instructions at https://github.com/zulip/zulip-mobile/blob/main/docs/howto/dev-server.md (per #1379 (comment)).

[rajveermalviya]

I had changed to that link because these instructions were specific to testing on iOS Simulator, and expectation was to do these intermediate (optional) steps on the Simulator too.

Changed back to this link, in case reader tries these steps on an actual device, or in your case where server is running on a different machine than the Mac.

[gnprice]

Ah true, the doc does say it's about the simulator. And the key xcrun simctl push step at the end sure sounds like it'll only work on the simulator.

It's probably not real common to be running the dev server on a different machine from the Mac hosting the simulator. So it'd be reasonable to add a remark here saying that if you'll be running the dev server on the Mac that's also hosting the simulator, then those steps boil down to just the normal Zulip dev server instructions (and include a direct link to the latter).

[gnprice]

nit:

Suggested change

	## 1.(Optional) Setup dev server
	## 1. (Optional) Setup dev server

[gnprice]

Huh interesting. That is a bit puzzling, though — looking at the dev_settings.py in that commit, it seems like it'll end up trying to talk to the bouncer at http://push.zulipdev.com:9991 (exactly like the signup command was doing when I tried above), or at an even more unlikely URL like http://push.192.168.0.101:9991 when one sets EXTERNAL_HOST to enable one's phone to get to the server.

[gnprice]

Suggested change

	<!-- TODO(405) Guide to use the new devlogin page instead -->
	<!-- TODO(#405) Guide to use the new devlogin page instead -->

That's the convention we use for TODO links to issues. See existing examples:

$ git grep -P 'TODO.\d+'
$ git grep -P 'TODO.#\d+'

Keeping the convention consistent helps keep the references greppable.

[gnprice]

bump — I'd like this information to not get lost for the next person.

Here's a version that could go in verbatim:

These canned payloads assume that EXTERNAL_HOST has its default value for the dev server. If you've set EXTERNAL_HOST to use an IP address in order to enable your device to connect to the dev server, you'll need to adjust the realm_url fields. You can do this by a find-and-replace for localhost; for example, perl -i -0pe s/localhost/10.0.2.2/g tmp/*.json after saving the canned payloads to files tmp/*.json.

[rajveermalviya]

Thanks for the review @gnprice! Pushed a new revision, PTAL.

[gnprice]

ah, thanks for adding the 9th digit here :-)

[gnprice]

OK, completed re-review of the things covered in the last pair of reviews. All looks good; one comment here above:
#1379 (comment)

Still on my plate in reviewing this are:

• re-review the Swift code, revised since notif ios: Handle opening of conversation on tap; take 2 #1379 (review)
• re-review the small early commits, revised since notif ios: Handle opening of conversation on tap; take 2 #1379 (review) (I believe little changed here and I didn't have much to say the first time, so should be quick)
• retry the testing instructions on a fresh dev server, following notif ios: Handle opening of conversation on tap; take 2 #1379 (comment)
• review the Dart changes in those two main commits (having covered the Swift changes earlier)

[rajveermalviya]

(Rebased to fix conflicts.)