From b7b4c01bde5a33e669f7b97356ab3c55719a2a43 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 11 Oct 2021 14:45:41 +0100 Subject: [PATCH] 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 --- .../notifications/NotifiableEventProcessor.kt | 48 ++++------------- .../NotificationDrawerManager.kt | 36 ++++++++----- .../notifications/NotificationRenderer.kt | 54 ++++++++++--------- .../NotifiableEventProcessorTest.kt | 14 ++--- .../notifications/NotificationRendererTest.kt | 14 ++--- .../fakes/FakeNotifiableEventProcessor.kt | 6 --- .../app/test/fakes/FakeNotificationFactory.kt | 10 ++-- 7 files changed, 77 insertions(+), 105 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventProcessor.kt b/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventProcessor.kt index 3f77ce54ca..bf9e805fc8 100644 --- 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, currentRoomId: String?): ProcessedNotificationEvents { - val roomIdToEventMap: MutableMap> = LinkedHashMap() - val simpleEvents: MutableMap = LinkedHashMap() - val invitationEvents: MutableMap = 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, currentRoomId: String?): Map { + 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>, - val simpleEvents: Map, - val invitationEvents: Map -) diff --git a/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt b/vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt index 042d2f1f88..43d9eff185 100644 --- 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() 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) } } diff --git a/vector/src/main/java/im/vector/app/features/notifications/NotificationRenderer.kt b/vector/src/main/java/im/vector/app/features/notifications/NotificationRenderer.kt index 73ea65debc..80391b1e06 100644 --- 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) { - 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) { + 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.groupByType(): GroupedNotificationEvents { + val roomIdToEventMap: MutableMap> = LinkedHashMap() + val simpleEvents: MutableMap = LinkedHashMap() + val invitationEvents: MutableMap = 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>, + val simpleEvents: Map, + val invitationEvents: Map +) diff --git a/vector/src/test/java/im/vector/app/features/notifications/NotifiableEventProcessorTest.kt b/vector/src/test/java/im/vector/app/features/notifications/NotifiableEventProcessorTest.kt index 6f47e71500..3e66f82bc3 100644 --- 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 = emptyMap(), invitationEvents: Map = emptyMap(), roomEvents: Map> = emptyMap() -) = ProcessedNotificationEvents( +) = GroupedNotificationEvents( roomEvents = roomEvents, simpleEvents = simpleEvents, invitationEvents = invitationEvents, diff --git a/vector/src/test/java/im/vector/app/features/notifications/NotificationRendererTest.kt b/vector/src/test/java/im/vector/app/features/notifications/NotificationRendererTest.kt index 1c68dc4f68..bd0d1e8d3f 100644 --- 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() -private val A_PROCESSED_EVENTS = ProcessedNotificationEvents(emptyMap(), emptyMap(), emptyMap()) +private val AN_EVENT_LIST = mapOf() +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() @@ -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 = 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, diff --git a/vector/src/test/java/im/vector/app/test/fakes/FakeNotifiableEventProcessor.kt b/vector/src/test/java/im/vector/app/test/fakes/FakeNotifiableEventProcessor.kt index 93f5e40524..6143c7a907 100644 --- 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() - fun givenProcessedEventsFor(events: MutableList, currentRoomId: String?, processedEvents: ProcessedNotificationEvents) { - every { instance.modifyAndProcess(events, currentRoomId) } returns processedEvents - } } diff --git a/vector/src/test/java/im/vector/app/test/fakes/FakeNotificationFactory.kt b/vector/src/test/java/im/vector/app/test/fakes/FakeNotificationFactory.kt index da2dbc27da..cc6f84f813 100644 --- 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() - fun givenNotificationsFor(processedEvents: ProcessedNotificationEvents, + fun givenNotificationsFor(groupedEvents: GroupedNotificationEvents, myUserId: String, myUserDisplayName: String, myUserAvatarUrl: String?, @@ -38,9 +38,9 @@ class FakeNotificationFactory { simpleNotifications: List, 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(