splitting the event processing from the rendering

- 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 https://github.com/vector-im/element-android/issues/4214
2025-02-01 11:46:57 +01:00 · 2021-10-11 14:45:41 +01:00 · 2021-10-11 14:45:41 +01:00 · b7b4c01bde
commit b7b4c01bde
parent 587466e009
7 changed files with 77 additions and 105 deletions
--- a/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventProcessor.kt
+++ b/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventProcessor.kt
@ -17,7 +17,6 @@
 package im.vector.app.features.notifications

 import im.vector.app.features.invite.AutoAcceptInvites
-import timber.log.Timber
 import javax.inject.Inject

 class NotifiableEventProcessor @Inject constructor(
@ -25,49 +24,20 @@ class NotifiableEventProcessor @Inject constructor(
        private val autoAcceptInvites: AutoAcceptInvites
 ) {

-    fun modifyAndProcess(eventList: MutableList<NotifiableEvent>, currentRoomId: String?): ProcessedNotificationEvents {
-        val roomIdToEventMap: MutableMap<String, MutableList<NotifiableMessageEvent>> = LinkedHashMap()
-        val simpleEvents: MutableMap<String, SimpleNotifiableEvent?> = LinkedHashMap()
-        val invitationEvents: MutableMap<String, InviteNotifiableEvent?> = LinkedHashMap()
-
-        val eventIterator = eventList.listIterator()
-        while (eventIterator.hasNext()) {
-            when (val event = eventIterator.next()) {
-                is NotifiableMessageEvent -> {
-                    val roomId = event.roomId
-                    val roomEvents = roomIdToEventMap.getOrPut(roomId) { ArrayList() }
-
-                    // should we limit to last 7 messages per room?
-                    if (shouldIgnoreMessageEventInRoom(currentRoomId, roomId) || outdatedDetector.isMessageOutdated(event)) {
-                        // forget this event
-                        eventIterator.remove()
-                    } else {
-                        roomEvents.add(event)
+    fun process(eventList: List<NotifiableEvent>, currentRoomId: String?): Map<String, NotifiableEvent?> {
+        return eventList.associateBy { it.eventId }
+                .mapValues { (_, value) ->
+                    when (value) {
+                        is InviteNotifiableEvent  -> if (autoAcceptInvites.hideInvites) null else value
+                        is NotifiableMessageEvent -> if (shouldIgnoreMessageEventInRoom(currentRoomId, value.roomId) || outdatedDetector.isMessageOutdated(value)) {
+                            null
+                        } else value
+                        is SimpleNotifiableEvent  -> value
                    }
                }
-                is InviteNotifiableEvent  -> {
-                    if (autoAcceptInvites.hideInvites) {
-                        // Forget this event
-                        eventIterator.remove()
-                        invitationEvents[event.roomId] = null
-                    } else {
-                        invitationEvents[event.roomId] = event
-                    }
-                }
-                is SimpleNotifiableEvent  -> simpleEvents[event.eventId] = event
-                else                      -> Timber.w("Type not handled")
-            }
-        }
-        return ProcessedNotificationEvents(roomIdToEventMap, simpleEvents, invitationEvents)
    }

    private fun shouldIgnoreMessageEventInRoom(currentRoomId: String?, roomId: String?): Boolean {
        return currentRoomId != null && roomId == currentRoomId
    }
 }
-
-data class ProcessedNotificationEvents(
-        val roomEvents: Map<String, List<NotifiableMessageEvent>>,
-        val simpleEvents: Map<String, SimpleNotifiableEvent?>,
-        val invitationEvents: Map<String, InviteNotifiableEvent?>
-)
--- a/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt
+++ b/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt
@ -44,6 +44,7 @@ class NotificationDrawerManager @Inject constructor(private val context: Context
                                                    private val notificationUtils: NotificationUtils,
                                                    private val vectorPreferences: VectorPreferences,
                                                    private val activeSessionDataSource: ActiveSessionDataSource,
+                                                    private val notifiableEventProcessor: NotifiableEventProcessor,
                                                    private val notificationRenderer: NotificationRenderer) {

    private val handlerThread: HandlerThread = HandlerThread("NotificationDrawerManager", Thread.MIN_PRIORITY)
@ -55,6 +56,7 @@ class NotificationDrawerManager @Inject constructor(private val context: Context
    }

    private val eventList = loadEventInfo()
+    private var renderedEventsList = emptyMap<String, NotifiableEvent?>()
    private val avatarSize = context.resources.getDimensionPixelSize(R.dimen.profile_avatar_size)
    private var currentRoomId: String? = null

@ -223,22 +225,32 @@ class NotificationDrawerManager @Inject constructor(private val context: Context
    @WorkerThread
    private fun refreshNotificationDrawerBg() {
        Timber.v("refreshNotificationDrawerBg()")
-        val session = currentSession ?: return

-        val user = session.getUser(session.myUserId)
-        // myUserDisplayName cannot be empty else NotificationCompat.MessagingStyle() will crash
-        val myUserDisplayName = user?.toMatrixItem()?.getBestName() ?: session.myUserId
-        val myUserAvatarUrl = session.contentUrlResolver().resolveThumbnail(user?.avatarUrl, avatarSize, avatarSize, ContentUrlResolver.ThumbnailMethod.SCALE)
+        val newSettings = vectorPreferences.useCompleteNotificationFormat()
+        if (newSettings != useCompleteNotificationFormat) {
+            // Settings has changed, remove all current notifications
+            notificationUtils.cancelAllNotifications()
+            useCompleteNotificationFormat = newSettings
+        }

-        synchronized(eventList) {
-            val newSettings = vectorPreferences.useCompleteNotificationFormat()
-            if (newSettings != useCompleteNotificationFormat) {
-                // Settings has changed, remove all current notifications
-                notificationUtils.cancelAllNotifications()
-                useCompleteNotificationFormat = newSettings
+        val eventsToRender = synchronized(eventList) {
+            notifiableEventProcessor.process(eventList, currentRoomId).also {
+                eventList.clear()
+                eventList.addAll(it.values.filterNotNull())
            }
+        }

-            notificationRenderer.render(currentRoomId, session.myUserId, myUserDisplayName, myUserAvatarUrl, useCompleteNotificationFormat, eventList)
+        if (renderedEventsList == eventsToRender) {
+            Timber.d("Skipping notification update due to event list not changing")
+        } else {
+            renderedEventsList = eventsToRender
+            val session = currentSession ?: return
+            val user = session.getUser(session.myUserId)
+            // myUserDisplayName cannot be empty else NotificationCompat.MessagingStyle() will crash
+            val myUserDisplayName = user?.toMatrixItem()?.getBestName() ?: session.myUserId
+            val myUserAvatarUrl = session.contentUrlResolver().resolveThumbnail(user?.avatarUrl, avatarSize, avatarSize, ContentUrlResolver.ThumbnailMethod.SCALE)
+
+            notificationRenderer.render(session.myUserId, myUserDisplayName, myUserAvatarUrl, useCompleteNotificationFormat, eventsToRender)
        }
    }

--- a/vector/src/main/java/im/vector/app/features/notifications/NotificationRenderer.kt
+++ b/vector/src/main/java/im/vector/app/features/notifications/NotificationRenderer.kt
@ -15,7 +15,6 @@
 */
 package im.vector.app.features.notifications

-import android.content.Context
 import androidx.annotation.WorkerThread
 import im.vector.app.features.notifications.NotificationDrawerManager.Companion.ROOM_EVENT_NOTIFICATION_ID
 import im.vector.app.features.notifications.NotificationDrawerManager.Companion.ROOM_INVITATION_NOTIFICATION_ID
@ -27,36 +26,16 @@ import javax.inject.Inject
 import javax.inject.Singleton

@Singleton
-class NotificationRenderer @Inject constructor(private val notifiableEventProcessor: NotifiableEventProcessor,
-                                               private val notificationDisplayer: NotificationDisplayer,
-                                               private val notificationFactory: NotificationFactory,
-                                               private val appContext: Context) {
-
-    private var lastKnownEventList = -1
+class NotificationRenderer @Inject constructor(private val notificationDisplayer: NotificationDisplayer,
+                                               private val notificationFactory: NotificationFactory) {

    @WorkerThread
-    fun render(currentRoomId: String?,
-               myUserId: String,
+    fun render(myUserId: String,
               myUserDisplayName: String,
               myUserAvatarUrl: String?,
               useCompleteNotificationFormat: Boolean,
-               eventList: MutableList<NotifiableEvent>) {
-        Timber.v("Render notification events - count: ${eventList.size}")
-        val notificationEvents = notifiableEventProcessor.modifyAndProcess(eventList, currentRoomId)
-        if (lastKnownEventList == notificationEvents.hashCode()) {
-            Timber.d("Skipping notification update due to event list not changing")
-        } else {
-            processEvents(notificationEvents, myUserId, myUserDisplayName, myUserAvatarUrl, useCompleteNotificationFormat)
-            lastKnownEventList = notificationEvents.hashCode()
-        }
-    }
-
-    private fun processEvents(notificationEvents: ProcessedNotificationEvents,
-                              myUserId: String,
-                              myUserDisplayName: String,
-                              myUserAvatarUrl: String?,
-                              useCompleteNotificationFormat: Boolean) {
-        val (roomEvents, simpleEvents, invitationEvents) = notificationEvents
+               eventsToProcess: Map<String, NotifiableEvent?>) {
+        val (roomEvents, simpleEvents, invitationEvents) = eventsToProcess.groupByType()
        with(notificationFactory) {
            val roomNotifications = roomEvents.toNotifications(myUserDisplayName, myUserAvatarUrl)
            val invitationNotifications = invitationEvents.toNotifications(myUserId)
@ -128,3 +107,26 @@ class NotificationRenderer @Inject constructor(private val notifiableEventProces
        }
    }
 }
+
+private fun Map<String, NotifiableEvent?>.groupByType(): GroupedNotificationEvents {
+    val roomIdToEventMap: MutableMap<String, MutableList<NotifiableMessageEvent>> = LinkedHashMap()
+    val simpleEvents: MutableMap<String, SimpleNotifiableEvent?> = LinkedHashMap()
+    val invitationEvents: MutableMap<String, InviteNotifiableEvent?> = LinkedHashMap()
+    forEach { (_, value) ->
+        when (value) {
+            is InviteNotifiableEvent  -> invitationEvents[value.roomId]
+            is NotifiableMessageEvent -> {
+                val roomEvents = roomIdToEventMap.getOrPut(value.roomId) { ArrayList() }
+                roomEvents.add(value)
+            }
+            is SimpleNotifiableEvent  -> simpleEvents[value.eventId] = value
+        }
+    }
+    return GroupedNotificationEvents(roomIdToEventMap, simpleEvents, invitationEvents)
+}
+
+data class GroupedNotificationEvents(
+        val roomEvents: Map<String, List<NotifiableMessageEvent>>,
+        val simpleEvents: Map<String, SimpleNotifiableEvent?>,
+        val invitationEvents: Map<String, InviteNotifiableEvent?>
+)
--- a/vector/src/test/java/im/vector/app/features/notifications/NotifiableEventProcessorTest.kt
+++ b/vector/src/test/java/im/vector/app/features/notifications/NotifiableEventProcessorTest.kt
@ -37,7 +37,7 @@ class NotifiableEventProcessorTest {
                aSimpleNotifiableEvent(eventId = "event-2")
        )

-        val result = eventProcessor.modifyAndProcess(events, currentRoomId = NOT_VIEWING_A_ROOM)
+        val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM)

        result shouldBeEqualTo aProcessedNotificationEvents(
                simpleEvents = mapOf(
@ -56,7 +56,7 @@ class NotifiableEventProcessorTest {
                anInviteNotifiableEvent(roomId = "room-2")
        )

-        val result = eventProcessor.modifyAndProcess(events, currentRoomId = NOT_VIEWING_A_ROOM)
+        val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM)

        result shouldBeEqualTo aProcessedNotificationEvents(
                invitationEvents = mapOf(
@ -75,7 +75,7 @@ class NotifiableEventProcessorTest {
                anInviteNotifiableEvent(roomId = "room-2")
        )

-        val result = eventProcessor.modifyAndProcess(events, currentRoomId = NOT_VIEWING_A_ROOM)
+        val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM)

        result shouldBeEqualTo aProcessedNotificationEvents(
                invitationEvents = mapOf(
@ -91,7 +91,7 @@ class NotifiableEventProcessorTest {
        val (events) = createEventsList(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1"))
        outdatedDetector.givenEventIsOutOfDate(events[0])

-        val result = eventProcessor.modifyAndProcess(events, currentRoomId = NOT_VIEWING_A_ROOM)
+        val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM)

        result shouldBeEqualTo aProcessedNotificationEvents(
                roomEvents = mapOf(
@ -106,7 +106,7 @@ class NotifiableEventProcessorTest {
        val (events, originalEvents) = createEventsList(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1"))
        outdatedDetector.givenEventIsInDate(events[0])

-        val result = eventProcessor.modifyAndProcess(events, currentRoomId = NOT_VIEWING_A_ROOM)
+        val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM)

        result shouldBeEqualTo aProcessedNotificationEvents(
                roomEvents = mapOf(
@ -120,7 +120,7 @@ class NotifiableEventProcessorTest {
    fun `given viewing the same room as message event when processing then removes message`() {
        val (events) = createEventsList(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1"))

-        val result = eventProcessor.modifyAndProcess(events, currentRoomId = "room-1")
+        val result = eventProcessor.process(events, currentRoomId = "room-1")

        result shouldBeEqualTo aProcessedNotificationEvents(
                roomEvents = mapOf(
@ -140,7 +140,7 @@ fun createEventsList(vararg event: NotifiableEvent): Pair<MutableList<Notifiable
 fun aProcessedNotificationEvents(simpleEvents: Map<String, SimpleNotifiableEvent?> = emptyMap(),
                                 invitationEvents: Map<String, InviteNotifiableEvent?> = emptyMap(),
                                 roomEvents: Map<String, List<NotifiableMessageEvent>> = emptyMap()
-) = ProcessedNotificationEvents(
+) = GroupedNotificationEvents(
        roomEvents = roomEvents,
        simpleEvents = simpleEvents,
        invitationEvents = invitationEvents,
--- a/vector/src/test/java/im/vector/app/features/notifications/NotificationRendererTest.kt
+++ b/vector/src/test/java/im/vector/app/features/notifications/NotificationRendererTest.kt
@ -17,13 +17,11 @@
 package im.vector.app.features.notifications

 import android.app.Notification
-import im.vector.app.test.fakes.FakeNotifiableEventProcessor
 import im.vector.app.test.fakes.FakeNotificationDisplayer
 import im.vector.app.test.fakes.FakeNotificationFactory
 import io.mockk.mockk
 import org.junit.Test

-private const val A_CURRENT_ROOM_ID = "current-room-id"
 private const val MY_USER_ID = "my-user-id"
 private const val MY_USER_DISPLAY_NAME = "display-name"
 private const val MY_USER_AVATAR_URL = "avatar-url"
@ -31,8 +29,8 @@ private const val AN_EVENT_ID = "event-id"
 private const val A_ROOM_ID = "room-id"
 private const val USE_COMPLETE_NOTIFICATION_FORMAT = true

-private val AN_EVENT_LIST = mutableListOf<NotifiableEvent>()
-private val A_PROCESSED_EVENTS = ProcessedNotificationEvents(emptyMap(), emptyMap(), emptyMap())
+private val AN_EVENT_LIST = mapOf<String, NotifiableEvent?>()
+private val A_PROCESSED_EVENTS = GroupedNotificationEvents(emptyMap(), emptyMap(), emptyMap())
 private val A_SUMMARY_NOTIFICATION = SummaryNotification.Update(mockk())
 private val A_REMOVE_SUMMARY_NOTIFICATION = SummaryNotification.Removed
 private val A_NOTIFICATION = mockk<Notification>()
@ -43,12 +41,10 @@ private val ONE_SHOT_META = OneShotNotification.Append.Meta(key = "ignored", sum

 class NotificationRendererTest {

-    private val notifiableEventProcessor = FakeNotifiableEventProcessor()
    private val notificationDisplayer = FakeNotificationDisplayer()
    private val notificationFactory = FakeNotificationFactory()

    private val notificationRenderer = NotificationRenderer(
-            notifiableEventProcessor = notifiableEventProcessor.instance,
            notificationDisplayer = notificationDisplayer.instance,
            notificationFactory = notificationFactory.instance
    )
@ -182,12 +178,11 @@ class NotificationRendererTest {

    private fun renderEventsAsNotifications() {
        notificationRenderer.render(
-                currentRoomId = A_CURRENT_ROOM_ID,
                myUserId = MY_USER_ID,
                myUserDisplayName = MY_USER_DISPLAY_NAME,
                myUserAvatarUrl = MY_USER_AVATAR_URL,
                useCompleteNotificationFormat = USE_COMPLETE_NOTIFICATION_FORMAT,
-                eventList = AN_EVENT_LIST
+                eventsToProcess = AN_EVENT_LIST
        )
    }

@ -200,9 +195,8 @@ class NotificationRendererTest {
                                   simpleNotifications: List<OneShotNotification> = emptyList(),
                                   useCompleteNotificationFormat: Boolean = USE_COMPLETE_NOTIFICATION_FORMAT,
                                   summaryNotification: SummaryNotification = A_SUMMARY_NOTIFICATION) {
-        notifiableEventProcessor.givenProcessedEventsFor(AN_EVENT_LIST, A_CURRENT_ROOM_ID, A_PROCESSED_EVENTS)
        notificationFactory.givenNotificationsFor(
-                processedEvents = A_PROCESSED_EVENTS,
+                groupedEvents = A_PROCESSED_EVENTS,
                myUserId = MY_USER_ID,
                myUserDisplayName = MY_USER_DISPLAY_NAME,
                myUserAvatarUrl = MY_USER_AVATAR_URL,
--- a/vector/src/test/java/im/vector/app/test/fakes/FakeNotifiableEventProcessor.kt
+++ b/vector/src/test/java/im/vector/app/test/fakes/FakeNotifiableEventProcessor.kt
@ -16,17 +16,11 @@

 package im.vector.app.test.fakes

-import im.vector.app.features.notifications.NotifiableEvent
 import im.vector.app.features.notifications.NotifiableEventProcessor
-import im.vector.app.features.notifications.ProcessedNotificationEvents
-import io.mockk.every
 import io.mockk.mockk

 class FakeNotifiableEventProcessor {

    val instance = mockk<NotifiableEventProcessor>()

-    fun givenProcessedEventsFor(events: MutableList<NotifiableEvent>, currentRoomId: String?, processedEvents: ProcessedNotificationEvents) {
-        every { instance.modifyAndProcess(events, currentRoomId) } returns processedEvents
-    }
 }
--- a/vector/src/test/java/im/vector/app/test/fakes/FakeNotificationFactory.kt
+++ b/vector/src/test/java/im/vector/app/test/fakes/FakeNotificationFactory.kt
@ -18,7 +18,7 @@ package im.vector.app.test.fakes

 import im.vector.app.features.notifications.NotificationFactory
 import im.vector.app.features.notifications.OneShotNotification
-import im.vector.app.features.notifications.ProcessedNotificationEvents
+import im.vector.app.features.notifications.GroupedNotificationEvents
 import im.vector.app.features.notifications.RoomNotification
 import im.vector.app.features.notifications.SummaryNotification
 import io.mockk.every
@ -28,7 +28,7 @@ class FakeNotificationFactory {

    val instance = mockk<NotificationFactory>()

-    fun givenNotificationsFor(processedEvents: ProcessedNotificationEvents,
+    fun givenNotificationsFor(groupedEvents: GroupedNotificationEvents,
                              myUserId: String,
                              myUserDisplayName: String,
                              myUserAvatarUrl: String?,
@ -38,9 +38,9 @@ class FakeNotificationFactory {
                              simpleNotifications: List<OneShotNotification>,
                              summaryNotification: SummaryNotification) {
        with(instance) {
-            every { processedEvents.roomEvents.toNotifications(myUserDisplayName, myUserAvatarUrl) } returns roomNotifications
-            every { processedEvents.invitationEvents.toNotifications(myUserId) } returns invitationNotifications
-            every { processedEvents.simpleEvents.toNotifications(myUserId) } returns simpleNotifications
+            every { groupedEvents.roomEvents.toNotifications(myUserDisplayName, myUserAvatarUrl) } returns roomNotifications
+            every { groupedEvents.invitationEvents.toNotifications(myUserId) } returns invitationNotifications
+            every { groupedEvents.simpleEvents.toNotifications(myUserId) } returns simpleNotifications

            every {
                createSummaryNotification(