Splitting notifications into separate creation and displaying passes #4235

ouchadam · 2021-10-13T10:47:05Z

Previously opened as #4185 and #4220

This entire change is the result of extracting, breaking down and unit testing 1 function NotificationDrawerManager.refreshNotificationDrawerBg

There are 3 main objectives/changes to the notification design

The notification flow becomes a 2 pass system. All notifications are created and then all notifications are notified/cancelled, instead of looping through all the events notifying/cancelling as we go.

This has the benefit of reducing the time between posting individual message and summary group notification changes to the
system UI message queue, which in turn should address our double notification problem 🤞 Doubled notifications #4152

The queueEvents (previously called eventList) is the sole source of truth, we now simply render it as android notifications, when a notification is dismissed it triggers a delete action which causes the event to be reflected in the eventList, which in turn renders the newly missing event as a notification removal.

This avoids redundant notification cancel -> show flows, we only show or cancel event changes

Testable, immutable and simpler to reason, we create all the notifications and then display all the notifications

Creates small classes with single responsibilities, tested where possible. The notification creation helpers RoomGroupMessageCreator & SummaryGroupMessageCreator are not particularly friendly to unit tests due to calling into android builders (although we could abstract that and test...)

Tentatively fixes the double notifications Doubled notifications #4152
Fixes notification related ANR [ANR] NotificationDrawerManager.setCurrentRoom #4214
- Splits the event list processing from the notification rendering, this allows the synchronised block to lock for less time (only whilst we process the events)
Fixes the serialised event list becoming out of sync with the currently rendered notifications, eg receiving multiple room invites
- Adds event list and currently rendered events diffing, this allows us to remove out of sync notifications

BEFORE JONING ROOM EVENT	AFTER JOINING ROOMS EVENT

INCOMING MESSAGES	ROOM INVITE	ACCEPTING / READING

ouchadam · 2021-10-13T10:48:00Z

vector/src/main/java/im/vector/app/features/notifications/InviteNotifiableEvent.kt

        override val isRedacted: Boolean = false
-) : NotifiableEvent {
-
-    override var hasBeenDisplayed = false


no longer needed as we diff the eventList and renderedEventsList

ouchadam · 2021-10-13T10:49:34Z

vector/src/main/java/im/vector/app/features/notifications/NotifiableEventProcessor.kt

+        private val autoAcceptInvites: AutoAcceptInvites
+) {
+
+    fun process(eventList: List<NotifiableEvent>, currentRoomId: String?, renderedEventsList: List<ProcessedEvent>): List<ProcessedEvent> {


a simplified version of https://github.com/vector-im/element-android/pull/4235/files#diff-c1e2253b1ef80b85a77d664922bf07c861d56dc472e8fbc51b8587ec264269daL255

no longer mutates the list which allows us to reduce the scope of the synchronised block

handles differences in the renderedEventsList and eventList keeping the notifications in sync

ouchadam · 2021-10-13T10:51:04Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

-
-        return bitmapLoader.getRoomBitmap(roomAvatarPath)
+        if (renderedEventsList == eventsToRender) {
+            Timber.d("Skipping notification update due to event list not changing")


redundant notification updates can now avoid triggering any updates

ouchadam · 2021-10-13T10:53:19Z

vector/src/main/java/im/vector/app/features/notifications/RoomGroupMessageCreator.kt

+        private val notificationUtils: NotificationUtils
+) {
+
+    fun createRoomMessage(events: List<NotifiableMessageEvent>, roomId: String, userDisplayName: String, userAvatarUrl: String?): RoomNotification.Message {


this is the logic from https://github.com/vector-im/element-android/pull/4235/files#diff-c1e2253b1ef80b85a77d664922bf07c861d56dc472e8fbc51b8587ec264269daL296

ouchadam · 2021-10-13T10:53:43Z

vector/src/main/java/im/vector/app/features/notifications/SummaryGroupMessageCreator.kt

+        private val notificationUtils: NotificationUtils
+) {
+
+    fun createSummaryNotification(roomNotifications: List<RoomNotification.Message.Meta>,


this is the logic from https://github.com/vector-im/element-android/pull/4235/files#diff-c1e2253b1ef80b85a77d664922bf07c861d56dc472e8fbc51b8587ec264269daL472

ouchadam · 2021-10-13T10:55:42Z

vector/src/main/java/im/vector/app/features/notifications/NotificationRenderer.kt

+               eventsToProcess: List<ProcessedEvent>) {
+        val (roomEvents, simpleEvents, invitationEvents) = eventsToProcess.groupByType()
+        with(notificationFactory) {
+            val roomNotifications = roomEvents.toNotifications(myUserDisplayName, myUserAvatarUrl)


this is the main change of the PR

all the notifications are created and then displayed in a single pass, the aim being to reduce the time between the notification message queue being updated for the group and child notifications (hopefully eliminating the chance of the system displaying the group as a separate notification 🤞 )

ouchadam · 2021-10-13T10:59:20Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

-            }
+            val shouldUpdate = removeAll { it is NotifiableMessageEvent && it.roomId == roomId }
            if (shouldUpdate) {
-                notificationUtils.cancelNotificationMessage(roomId, ROOM_MESSAGES_NOTIFICATION_ID)


I have a sneaky suspicion this also might have been contributing to the duplicate notifications where we were eagerly cancelling whilst a refreshNotificationDrawerBg may have been running

ouchadam · 2021-10-13T11:00:28Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

+                eventList.addAll(it.onlyKeptEvents())
            }
-            // notice that we can get bit out of sync with actual display but not a big issue
-            firstTime = false


firstTime is no longer needed, the diffing between renderedEventsList and eventList handles this for us

github-actions · 2021-10-13T11:10:00Z

Unit Test Results

  60 files   60 suites 58s ⏱️
115 tests 115 ✔️ 0 💤 0 ❌
340 runs 340 ✔️ 0 💤 0 ❌

Results for commit 0fe1b69.

♻️ This comment has been updated with latest results.

…e handled - also puts in the basis for a separate notification refreshing implementation

- updates the logic to track when events are removed as a way for the notifications to remove themselves, null events mean they've been removed

- also handles when the event diff means the notifications should be removed

…nto a NotificationRender - extract the displaying into its own class to avoid leaking the entire notificationutils - cancel/display notification actions are completely driven by the event or abscense of event from the eventList - attempts to avoid redundant render passes by checking if the eventList has changed since the last render

- the renderer's responsibility it handling events

…r source of truth - when events have finished being displayed they should be removed from the eventList via notification delete actions

…o empty due to only containing removals

…l notifications - adds some comments to explain the positioning

…ions

- this allows us to only synchronise of the event list modifications rather than the entire notification creation/rendering which should in turn reduce some of our ANRs #4214

…ed event id - this state will be used to diff the currently rendered events against the new ones

… allow us to dismiss notifications from removed events

…cting the processed type when creating the notifications

…as our notification source of truth

…ngleton annotation

…used method

ouchadam · 2021-10-14T16:04:19Z

vector/src/main/java/im/vector/app/features/notifications/NotificationRenderer.kt

+                    is RoomNotification.Message -> if (useCompleteNotificationFormat) {
+                        Timber.d("Updating room messages notification ${wrapper.meta.roomId}")
+                        wrapper.shortcutInfo?.let {
+                            ShortcutManagerCompat.pushDynamicShortcut(appContext, it)


Shortcut adding maintained, although now we're only doing it once per room rather than for every message in the room
https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt#L356

we're also able to easily remove the shortcuts when the notification is removed (if needed)

ouchadam · 2021-10-14T16:04:42Z

vector/src/main/java/im/vector/app/features/notifications/RoomGroupMessageCreator.kt

+        }
+
+        val largeBitmap = getRoomBitmap(events)
+        val shortcutInfo = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) {


The creation part of the shortcut change
https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt#L356

ouchadam · 2021-10-14T16:05:26Z

vector/src/main/java/im/vector/app/features/notifications/RoomGroupMessageCreator.kt

+
+    private fun NotificationCompat.MessagingStyle.addMessagesFromEvents(events: List<NotifiableMessageEvent>) {
+        events.forEach { event ->
+            val senderPerson = if (event.outGoingMessage) {


porting over the null person change https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt#L346

bmarty

This is an amazing work, thanks!
I have a few very minor remarks.
I did not run the code yet. Is it the last PR about your notification rework?

bmarty · 2021-10-18T19:34:53Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDisplayer.kt

+import timber.log.Timber
+import javax.inject.Inject
+
+class NotificationDisplayer @Inject constructor(context: Context) {


I like this simple class 👍

bmarty · 2021-10-18T19:36:09Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

+     * we keep track of them in order to know which events have been removed from the eventList
+     * allowing us to cancel any notifications previous displayed by now removed events
+     */
+    private var renderedEventsList = emptyList<Pair<ProcessedType, NotifiableEvent>>()


You could use the typealias ProcessedEvent here (or the data class :)).

bmarty · 2021-10-18T19:36:51Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

-            }
+            val shouldUpdate = removeAll { it is NotifiableMessageEvent && it.roomId == roomId }
            if (shouldUpdate) {
-                notificationUtils.cancelNotificationMessage(roomId, ROOM_MESSAGES_NOTIFICATION_ID)


bmarty · 2021-10-18T19:38:12Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

        }
    }

-    fun displayDiagnosticNotification() {


Was it not used here? I guess yes

^^^ correct, unused

bmarty · 2021-10-18T19:38:53Z

vector/src/main/java/im/vector/app/features/notifications/ProcessedEvent.kt

+
+package im.vector.app.features.notifications
+
+typealias ProcessedEvent = Pair<ProcessedType, NotifiableEvent>


I like typealias, but I do not like .first and .second, which are meaning less. I would prefer to have a simple data class in this case. WDYT?

agreed, will switch to a data class 💯

done! f5de7f7

bmarty · 2021-10-18T19:39:13Z

vector/src/main/java/im/vector/app/features/notifications/NotificationFactory.kt

+import androidx.core.content.pm.ShortcutInfoCompat
+import javax.inject.Inject
+
+private typealias ProcessedMessageEvent = Pair<ProcessedType, NotifiableMessageEvent>


same remark about typealias...

will replace with a data class 👍

bmarty · 2021-10-18T19:40:51Z

vector/src/main/java/im/vector/app/features/notifications/NotificationFactory.kt

+
+    private fun NotifiableMessageEvent.canNotBeDisplayed() = isRedacted
+
+    fun List<Pair<ProcessedType, InviteNotifiableEvent>>.toNotifications(myUserId: String): List<OneShotNotification> {


Nit: Maybe also use @JvmName() here, even if it's not strictly necessary?

good idea 👍

also done as part of f5de7f7

ouchadam · 2021-10-19T08:28:57Z

This is an amazing work, thanks! I have a few very minor remarks. I did not run the code yet. Is it the last PR about your notification rework?

thanks 😄 there's one more tiny PR around changing the notification ids to avoid 0 - 5 but this doesn't rely on the feature branch and can be done against develop

@JvmName

…assing around the process notificatiable events - also includes @JvmName on all conflicting extensions for consistency

bmarty · 2021-10-19T19:28:40Z

vector/src/main/java/im/vector/app/features/notifications/NotifiableEventProcessor.kt

 ) {

-    fun process(eventList: List<NotifiableEvent>, currentRoomId: String?, renderedEventsList: List<ProcessedEvent>): List<ProcessedEvent> {
+    fun process(eventList: List<NotifiableEvent>, currentRoomId: String?, renderedEventsList: ProcessedEvents): ProcessedEvents {


Nit: I know this is not new, but maybe the word List is not necessary in the parameter names. So instead of having renderedEventsList, we could simply have renderedEvents. The plural form is generally enough (to me) to understand that this is not a simple object. This is my point of view, I'm open to different ones :)

Also we can see some inconsistency in the naming convention between eventList and renderedEventsList :)

will fix whilst I'm in the area 👍

bmarty · 2021-10-19T19:34:00Z

This is OK to me to merge this PR into the branch feature/adm/notification-redesign, but just to confirm, will you create a PR to merge feature/adm/notification-redesign into develop?
This will be my opportunity to test the change (I did not test it yet 🙈), and have a final (quick) review

ouchadam · 2021-10-19T20:29:51Z

This is OK to me to merge this PR into the branch feature/adm/notification-redesign, but just to confirm, will you create a PR to merge feature/adm/notification-redesign into develop? This will be my opportunity to test the change (I did not test it yet see_no_evil), and have a final (quick) review

correct! I'll raise the entire feature branch as a PR against develop

bmarty

LGTM, please merge the PR whenever you want!

bmarty · 2021-10-20T09:57:51Z

vector/src/main/java/im/vector/app/features/notifications/NotifiableEventProcessor.kt

-    fun process(eventList: List<NotifiableEvent>, currentRoomId: String?, renderedEventsList: ProcessedEvents): ProcessedEvents {
-        val processedEventList = eventList.map {
+    fun process(queuedEvents: List<NotifiableEvent>, currentRoomId: String?, renderedEvents: ProcessedEvents): ProcessedEvents {
+        val processedEventList = queuedEvents.map {


Please do not change it, but there is still a List suffix in processedEventList (I know I am a pain sometimes 😆 )

ah, missed that 👍

and I'm happy to fix it! will only take a few seconds (maybe longer for the CI 😅 )
0fe1b69

…inconsistencies

ouchadam commented Oct 13, 2021

View reviewed changes

ouchadam requested a review from bmarty October 13, 2021 10:56

ouchadam commented Oct 13, 2021

View reviewed changes

This was referenced Oct 13, 2021

Marking message as read clears all other messages from notification bar #3395

Closed

Constant triggering of notification sounds #4249

Merged

ouchadam force-pushed the feature/adm/notification-redesign branch from ba6e019 to 8429807 Compare October 14, 2021 15:19

ouchadam added 17 commits October 14, 2021 16:31

making the event body non null and immutable to allow less cases to b…

098d4ab

…e handled - also puts in the basis for a separate notification refreshing implementation

creating dedicated class for the processing the serialized events

51d3cc5

- updates the logic to track when events are removed as a way for the notifications to remove themselves, null events mean they've been removed

creating the notifications separate to where they're displayed

dded400

- also handles when the event diff means the notifications should be removed

lifting settings change to cancel all notifications out of the renderer

b3bb42f

- the renderer's responsibility it handling events

removing no longer needed hasBeenDisplayed state, the eventList is ou…

55480e0

…r source of truth - when events have finished being displayed they should be removed from the eventList via notification delete actions

handling creating the summary when notification events are filtered t…

066fd49

…o empty due to only containing removals

removing this usages for project convention

d925044

formatting

1be096b

ensuring that we removing the summary group before removing individua…

c705496

…l notifications - adds some comments to explain the positioning

relying on the notification refreshing to cancel/update the notificat…

bf9a3ef

…ions

splitting the event processing from the rendering

41b3deb

- this allows us to only synchronise of the event list modifications rather than the entire notification creation/rendering which should in turn reduce some of our ANRs #4214

using a process state of keep/removed rather than mapping to an ignor…

b0306ab

…ed event id - this state will be used to diff the currently rendered events against the new ones

diffing the notification events against the currently rendered events…

edff292

… allow us to dismiss notifications from removed events

ensuring that we remove read messages when they come through by respe…

28df5a3

…cting the processed type when creating the notifications

adding documentation around the two notifiable event lists which act …

25c4322

…as our notification source of truth

fixing line lengths

2f8be02

ouchadam added 4 commits October 14, 2021 16:52

adding missing fixture parameter from rebase

672e7f9

inlining single use extension functions

92d05e7

adding missing parameter from rebase and removing no longer needed si…

75e45d1

…ngleton annotation

replacing notification utils usage with the displayer and removing un…

5029937

…used method

ouchadam force-pushed the feature/adm/notification-logic-split branch from 799667f to 5029937 Compare October 14, 2021 15:58

removing unused imports

60a5bb9

ouchadam commented Oct 14, 2021

View reviewed changes

ouchadam added 2 commits October 14, 2021 17:19

updating tests with shortcut placement changes

6393311

increase enum class allowance by 1

63bbc71

bmarty reviewed Oct 18, 2021

View reviewed changes

ouchadam force-pushed the feature/adm/notification-logic-split branch from f9e6718 to 34684cc Compare October 19, 2021 10:42

using dedicated ProcessedEvent data class instead of type alias for p…

f5de7f7

…assing around the process notificatiable events - also includes @JvmName on all conflicting extensions for consistency

ouchadam force-pushed the feature/adm/notification-logic-split branch from 34684cc to f5de7f7 Compare October 19, 2021 10:43

bmarty reviewed Oct 19, 2021

View reviewed changes

bmarty approved these changes Oct 20, 2021

View reviewed changes

renaming event lists to give more context and remove the list suffix/…

0fe1b69

…inconsistencies

ouchadam force-pushed the feature/adm/notification-logic-split branch from 3e10145 to 0fe1b69 Compare October 20, 2021 10:07

ouchadam merged commit d8919e8 into feature/adm/notification-redesign Oct 20, 2021

ouchadam deleted the feature/adm/notification-logic-split branch October 20, 2021 10:36

ouchadam mentioned this pull request Oct 20, 2021

Notifications redesign #4274

Merged

ouchadam mentioned this pull request Nov 25, 2021

[ANR] NotificationDrawerManager.setCurrentRoom #4214

Closed


		package im.vector.app.features.notifications

		typealias ProcessedEvent = Pair<ProcessedType, NotifiableEvent>


		private fun NotifiableMessageEvent.canNotBeDisplayed() = isRedacted

		fun List<Pair<ProcessedType, InviteNotifiableEvent>>.toNotifications(myUserId: String): List<OneShotNotification> {

Splitting notifications into separate creation and displaying passes #4235

Splitting notifications into separate creation and displaying passes #4235

Uh oh!

Conversation

ouchadam commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam commented Oct 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam commented Oct 13, 2021 •

edited

Loading

ouchadam Oct 13, 2021 •

edited

Loading

ouchadam Oct 13, 2021 •

edited

Loading

ouchadam Oct 13, 2021 •

edited

Loading

github-actions bot commented Oct 13, 2021 •

edited

Loading

ouchadam Oct 20, 2021 •

edited

Loading