This differential implements notification grouping based on threadID on Android
Details
Details
- Reviewers
bartek tomek - Commits
- rCOMM597ebe41442c: Implement notification groupping on Android
- Obtain device with Android version at least 24 and build the app.
- Send messages from different clients to the device. Ensure that for each thread there is one expandable notification that encapsulates all sent messages when expanded.
- Read messages from one client on another device (or web). Ensure rescinding removes all notifications for this particular thread and does not break grouping for other threads - notifications for other threads are still encapsulated in expandable.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 199 ↗ | (On Diff #27643) | If rescinds delete all notifications for a particular thread, there will still be group summary left. Without deleting it manually there will be an empty notification bubble visible in notification center. Therefore we have to manually delete group summary notification once all notifications from a group are rescinded. However if there are still some notifications left from a particular group, we cannot delete group summary since we would break grouping. |
Comment Actions
(Shellcheck CI job should pass after pulling in latest changes, already restarted failing iOS build)
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 261–266 ↗ | (On Diff #27658) | IMO inverted flow is more readable + optionally using else instead of return, but no strong opinion on that |
Comment Actions
When I was testing this differential I observed strange behaviour that notification is first displayed independently and added to the group after a couple of seconds. I thought that this is my fault so I was deeply investigating signals and telegrams repositories to find out how to properly implement notifications grouping, but I couldn't resolve the issue. Then I did more detailed testing and I noticed that:
- If I just send a couple of notifications and then expand notification center I can see that notifications are correctly grouped.
- Only if I send notifications with notification center expanded I am observing this weird behaviour.
- If notification center is expanded and notification group is expanded as well then when I send new notification it is immediately added to the group, but when I collapse notification group the latest notification leaves the group and then comes back after a couple of seconds.
This behaviour was so weird that I concluded it might be related to the particular Android device. I was using Realme GT 2 with ColorOS addition to Android. When I installed the app to Samsung A21S which runs pure Android all issues disappeared and notifications were immediately appearing in a correct group.
That said I conclude grouping implementation in this diff is fine and weird behaviour I observed was due to some bug in ColorOS.
Comment Actions
@marcin, quick question – did you observe similar issues (delays to notification grouping) with other apps (eg. Signal) on the Realme GT 2 with ColorOS addition?
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 209 ↗ | (On Diff #27658) | I think we should call setGroupAlertBehavior on this notif too, since it is mentioned in the same section that mentions to call setGroup |
Comment Actions
Today I did more detailed testing with other apps on this device and I noticed that Signal and Messenger actually don't have this issue. Therefore I think it is perhaps worth to spend some additional time trying to get it working on this device too,
Comment Actions
Hey @marcin, in your daily update yesterday you mentioned:
discovered that if we disable banner from comm notifications the issue disappears on Comm. My theory is that when notification appears as a banner it has some time it must spent as a banner before it joins the group.
What did you mean by "banner" in this case?
Comment Actions
When notification arrives it content is displayed at the top of the screen for a couple for seconds (about 5 seconds). Then it disappears and notification is visible only in Notification Drawer. In Android documentations they refer to it as banner. User can choose for any app. if they want its notification to be displayed this way or if they prefer that notifications are only visible in Notification Drawer. Comm has banner property on by default. When I turned it off I noticed that the issue with delayed grouping disappears.
Comment Actions
I can confirm that neither Signal nor Element app exhibit grouping delay behaviour since they don't display individual notifications but one MessagingStyle notification. Today I implemented in a hacky way this behaviour in Comm app and notifications were looking and behaving exactly like those from element or Signal on Realme GT 2. There was no delay to grouping and individual messages were unable to be deleted by swiping (Signal as Element have that too). The hack was to keep received messages contents in a bundle attached to to MessagingStyle notification and re-render notification based on those data. This does not break rescinding. Upon rescind we can simply remove relevant notification content from the bundle.
If we really want to we can amend this solution to this differential but I am opposed to it. As mentioned earlier, it is a hack and the proper way to do this is to design persisted notification state as described in the issue I created today: https://linear.app/comm/issue/ENG-4164/consider-changing-our-approach-to-notification-handling-on-android. One might argue that we can ship the hack now to mitigate the risk of delayed grouping experience and remove it later once the proper persisted notifications state is implemented. However we will probably be needing the proper notification state soon anyway since it is might be necessary to implement notification coalescing that is likely to be my monthly goal for August or September. That said I would land this differential as it is now, which guarantees that every notification will "eventually" be grouped and remove the possibility for delay once we implement robust persisted notification state.
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 261–266 ↗ | (On Diff #27658) | We have convention in our codebase to use early return instead of else to reduce indentation. |
Comment Actions
Exciting to see this launched! We should make sure we announce this to users in the Comm supporters community, since I know some of them had requested this