notifications: Remove Android summary notification. #5459
Conversation
There was a problem hiding this comment.
Thanks @rachelhyman! One comment on the code, below; please take a look.
Just to elaborate a bit on what the effect of this change seems to be:
From a bit more testing (on an emulated Android 11 device), it looks like notifications for different stream conversations also no longer get grouped:
So probably the story is that each conversation becomes a separate notification that isn't grouped with any of the others.
Once put that way, this behavior sounds familiar -- ISTR finding when we first moved to this new notifications API (with this feature of grouping notifications in the first place) that even though we set a group key on each individual notification, they don't actually get grouped unless we also create a summary notification for the group.
| NotificationManagerCompat.from(context).apply { | ||
| // This posts the notifications. If there is an existing notification | ||
| // with the same tag and ID as one of these calls to `notify`, this will | ||
| // replace it with the updated notification we've just constructed. | ||
| notify(groupKey, NOTIFICATION_ID, summaryNotification) |
There was a problem hiding this comment.
Since we're no longer creating summary notifications, we should also remove the corresponding logic for them at the other end when notifications get removed because the messages were read. That can be found in removeNotification, above in this file.
There was a problem hiding this comment.
Removed the summary notification logic (and updated one comment) in removeNotification, if you'd like to take another look @gnprice.
This sounds exactly correct--the docs indicate that grouped notifications "must" include a summary notification as well. Re: your feedback on removing the summary notification logic in |
They say that but in the context of how "[t]o support older versions" than Android 7:
That reads to me as saying that the summary notification is needed for older versions, not for Android 7 and up (which is all we support anyway). But as we see, the actual behavior seems to be that it's always required. I think what happened is something like: they wanted to say the summary notification is required, then added some words to give a rationale for why that's the case, but weren't careful about it. (I doubt older versions are the real reason for the requirement, either -- in general Android seems pretty willing to add features you can use in ways that won't work on older versions.)
Yep, that's right! |
|
Oh, and I located the conversation where we previously discovered this fact that a summary notification is required in order to get grouping: |
Ah, but then while looking at another part of the page I see that they make the actual situation explicit later! That's helpful (emphasis in original):
I was curious if we just missed that when we were last puzzled about this, and the Wayback Machine has the answer: that text wasn't there at the time. Glad they've clarified it. Also apparently that version said "To support older versions, you can also add a summary notification" etc. -- i.e. they had "can" where it now says "must". So no wonder we thought it was saying we didn't need it on Android 7+. |
|
Thanks for the additional context and sleuthing! Oddly enough, the summary notification does seem to be used on my device (Android 11 and not a wearable, of course), contra what the documentation indicates. I had confirmed this by changing the summary text and seeing it appear when I re-ran. I wish I was able to find more explicit confirmation that the "tap body to open" behavior comes for free with autogrouped notifications, and that there's no way to get that behavior with inbox-style notifications. I've mostly gathered this based off testing other apps, including one I can confirm isn't doing manual notification grouping and does have the desired behavior with tap to open. I should have an update to the PR by tonight that handles the logic in removeNotification. |
rachelhyman
force-pushed
the
5242-notification-click
branch
from
fe7d113 to
5a951a2
Compare
Removes the summary notification from the notification group in NotificationManager.kt, and removes logic around removing the summary notification when Zulip messages are read. This change allows the user to tap on the body of the notification to open the app, which was not possible with the summary notification (only the chevron would expand the notification, which would then allow the user to interact with the individual notifications below it. Please note that removing the summary notification breaks proper grouping: multiple private messages from the same user will correctly be grouped together, but notifications for different stream conversations appear separately. This was tested manually. Fixes part of zulip#5242.
rachelhyman
force-pushed
the
5242-notification-click
branch
from
5a951a2 to
3e1d54d
Compare
|
Closing in favor of #5472. |
Removes the summary notification from the group in
NotificationManager.kt. This allows the user to tap on the body of the
notification to open the app, which was not possible with the summary
notification (only the chevron would expand the
notification, which would then allow the user to interact with the
individual notifications below it.
Please note that removing the summary notification breaks proper
grouping: multiple private messages from the same user will correctly
be grouped together, but stream notifications appear in a separate
notification. This was tested manually on a OnePlus9 on Android 11.
Fixes part of #5242.