notifications: Open to unreads from summary notification. #5472
Conversation
Adds an intent to the summary notification. When the summary notification is clicked in the collapsed state, the app will open and reset itself to the unreads screen. It will also switch to the appropriate account if needed. Because the intent flags (Intent.FLAG_ACTIVITY_NEW_TASK and Intent.FLAG_ACTIVITY_CLEAR_TOP) apply to both the normal notification and the summary one (and are prefaced with explanatory comments), I moved them to a variable before where both notifications are created. This was manually tested for the following cases, with both the app already being on the correct account and being on a different account: - App open in background on non-unreads screen - App open in background on unreads screen - App not open in background I also made sure that tapping on a notification in the expanded state still routes the user to the correct screen. Fixes zulip#5242.
There was a problem hiding this comment.
Thanks @rachelhyman!
In my testing, this doesn't actually result in navigating to the account in question, or navigating to the unreads screen, when the user opens the summary notification. It opens the app or brings it to the foreground, but then the app remains on the account that was previously active, and on the screen it'd been on if it was already running.
This is a little bit subtle to test manually, even though it's a quite common situation in normal use: you have to send the user notification-causing messages in two different conversations (e.g., a 1:1 PM and also a PM to some group.) If the test messages you send are all in the same conversation, then they all get shown in a single notification, and then the summary notification doesn't get involved. Perhaps that's what happened in your testing.
One good way to test your test procedure is to try it on the unmodified app, without the new setContentIntent call you added, and make sure that it correctly fails there.
I've added a comment below pointing at the other bit of existing code for the individual notifications that will need a counterpart for the summary notification. That supplies some information to the JS side of the app; and then for where that information gets consumed, see src/notification/notifOpen.js.
| // all the activities on top of the target one. | ||
| Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP | ||
| ) | ||
| .setFlags(intentFlags) | ||
| .putExtra(EXTRA_NOTIFICATION_DATA, bundleOf(*fcmMessage.dataForOpen())), |
There was a problem hiding this comment.
Here's the line in the existing code where we supply the data that the JS side of the app relies on in order to know where it should navigate to when opening the notification.
There was a problem hiding this comment.
Depending on the outcome of the below, I take it we would want a dataForSummaryOpen that only includes the realm_uri and user_id?
|
Hi @gnprice, thanks for testing it on your end. Hm....I took a screen recording of what I'm seeing on my end with the new code: I did notice that when I have existing notifications that are all from one conversation (like PMs from one person), and then receive a new notification from a different conversation, it takes a minute for them all to "collect" under the summary one. They appear in separate groups at first, and then after a time or two of swiping down to show the notifications screen, they show up as all under the summary notification like I expect. |
|
Interesting, thanks for the video. I believe what's happening there is that the app is getting started again from scratch, with the side effect of resetting the navigation to the unreads/inbox screen. One detail that points to that is the "Connecting…" banner showing up at the top -- that's something that happens when the app starts up, but doesn't have another reason to appear in connection with opening a notification. That demonstration also doesn't appear to involve switching between different accounts. I think this version of the code fundamentally can't accomplish that, because there's no information it attaches to the summary notification that would make it possible for the JS side of the code to know it should switch to a particular account. |
Adds an intent to the summary notification. When the summary
notification is clicked in the collapsed state, the app will open and
reset itself to the unreads screen. It will also switch to the
appropriate account if needed.
Because the intent flags (Intent.FLAG_ACTIVITY_NEW_TASK
and Intent.FLAG_ACTIVITY_CLEAR_TOP) apply to both the normal
notification and the summary one (and are prefaced with explanatory
comments), I moved them to a variable before where both notifications
are created.
This was manually tested for the following cases, with both the app
already being on the correct account and being on a different account:
I also made sure that tapping on a notification in the expanded state
still routes the user to the correct screen.
Fixes #5242.