diff --git a/changelog.d/7634.bugfix b/changelog.d/7634.bugfix new file mode 100644 index 0000000000..a3c829840a --- /dev/null +++ b/changelog.d/7634.bugfix @@ -0,0 +1 @@ +Push notification for thread message is now shown correctly when user observes rooms main timeline diff --git a/vector/src/main/java/im/vector/app/core/pushers/VectorPushHandler.kt b/vector/src/main/java/im/vector/app/core/pushers/VectorPushHandler.kt index b74028d579..0d2cd56995 100644 --- a/vector/src/main/java/im/vector/app/core/pushers/VectorPushHandler.kt +++ b/vector/src/main/java/im/vector/app/core/pushers/VectorPushHandler.kt @@ -28,6 +28,7 @@ import im.vector.app.core.network.WifiDetector import im.vector.app.core.pushers.model.PushData import im.vector.app.core.resources.BuildMeta import im.vector.app.features.notifications.NotifiableEventResolver +import im.vector.app.features.notifications.NotifiableMessageEvent import im.vector.app.features.notifications.NotificationActionIds import im.vector.app.features.notifications.NotificationDrawerManager import im.vector.app.features.settings.VectorDataStore @@ -142,11 +143,6 @@ class VectorPushHandler @Inject constructor( pushData.roomId ?: return pushData.eventId ?: return - // If the room is currently displayed, we will not show a notification, so no need to get the Event faster - if (notificationDrawerManager.shouldIgnoreMessageEventInRoom(pushData.roomId)) { - return - } - if (wifiDetector.isConnectedToWifi().not()) { Timber.tag(loggerTag.value).d("No WiFi network, do not get Event") return @@ -157,6 +153,13 @@ class VectorPushHandler @Inject constructor( val resolvedEvent = notifiableEventResolver.resolveInMemoryEvent(session, event, canBeReplaced = true) + if (resolvedEvent is NotifiableMessageEvent) { + // If the room is currently displayed, we will not show a notification, so no need to get the Event faster + if (notificationDrawerManager.shouldIgnoreMessageEventInRoom(resolvedEvent)) { + return + } + } + resolvedEvent ?.also { Timber.tag(loggerTag.value).d("Fast lane: notify drawer") } ?.let { diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt index 34d7e45028..b73d443832 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt @@ -972,6 +972,7 @@ class TimelineFragment : override fun onResume() { super.onResume() notificationDrawerManager.setCurrentRoom(timelineArgs.roomId) + notificationDrawerManager.setCurrentThread(timelineArgs.threadTimelineArgs?.rootThreadEventId) roomDetailPendingActionStore.data?.let { handlePendingAction(it) } roomDetailPendingActionStore.data = null } @@ -991,6 +992,7 @@ class TimelineFragment : override fun onPause() { super.onPause() notificationDrawerManager.setCurrentRoom(null) + notificationDrawerManager.setCurrentThread(null) } private val emojiActivityResultLauncher = registerStartForActivityResult { activityResult -> 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 6bdd2ab511..81b9844e36 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 @@ -30,13 +30,13 @@ class NotifiableEventProcessor @Inject constructor( private val autoAcceptInvites: AutoAcceptInvites ) { - fun process(queuedEvents: List, currentRoomId: String?, renderedEvents: ProcessedEvents): ProcessedEvents { + fun process(queuedEvents: List, currentRoomId: String?, currentThreadId: String?, renderedEvents: ProcessedEvents): ProcessedEvents { val processedEvents = queuedEvents.map { val type = when (it) { is InviteNotifiableEvent -> if (autoAcceptInvites.hideInvites) REMOVE else KEEP is NotifiableMessageEvent -> when { - shouldIgnoreMessageEventInRoom(currentRoomId, it.roomId) -> REMOVE - .also { Timber.d("notification message removed due to currently viewing the same room") } + it.shouldIgnoreMessageEventInRoom(currentRoomId, currentThreadId) -> REMOVE + .also { Timber.d("notification message removed due to currently viewing the same room or thread") } outdatedDetector.isMessageOutdated(it) -> REMOVE .also { Timber.d("notification message removed due to being read") } else -> KEEP @@ -55,8 +55,4 @@ class NotifiableEventProcessor @Inject constructor( return removedEventsDiff + processedEvents } - - private fun shouldIgnoreMessageEventInRoom(currentRoomId: String?, roomId: String?): Boolean { - return currentRoomId != null && roomId == currentRoomId - } } diff --git a/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventResolver.kt b/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventResolver.kt index d28ab22684..988ab01ef8 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventResolver.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/NotifiableEventResolver.kt @@ -32,6 +32,7 @@ import org.matrix.android.sdk.api.session.crypto.MXCryptoError import org.matrix.android.sdk.api.session.crypto.model.OlmDecryptionResult import org.matrix.android.sdk.api.session.events.model.Event import org.matrix.android.sdk.api.session.events.model.EventType +import org.matrix.android.sdk.api.session.events.model.getRootThreadEventId import org.matrix.android.sdk.api.session.events.model.isEdition import org.matrix.android.sdk.api.session.events.model.isImageMessage import org.matrix.android.sdk.api.session.events.model.supportsNotification @@ -133,7 +134,7 @@ class NotifiableEventResolver @Inject constructor( } } - private suspend fun resolveMessageEvent(event: TimelineEvent, session: Session, canBeReplaced: Boolean, isNoisy: Boolean): NotifiableEvent? { + private suspend fun resolveMessageEvent(event: TimelineEvent, session: Session, canBeReplaced: Boolean, isNoisy: Boolean): NotifiableMessageEvent? { // The event only contains an eventId, and roomId (type is m.room.*) , we need to get the displayable content (names, avatar, text, etc...) val room = session.getRoom(event.root.roomId!! /*roomID cannot be null*/) @@ -155,6 +156,7 @@ class NotifiableEventResolver @Inject constructor( body = body.toString(), imageUriString = event.fetchImageIfPresent(session)?.toString(), roomId = event.root.roomId!!, + threadId = event.root.getRootThreadEventId(), roomName = roomName, matrixID = session.myUserId ) @@ -178,6 +180,7 @@ class NotifiableEventResolver @Inject constructor( body = body, imageUriString = event.fetchImageIfPresent(session)?.toString(), roomId = event.root.roomId!!, + threadId = event.root.getRootThreadEventId(), roomName = roomName, roomIsDirect = room.roomSummary()?.isDirect ?: false, roomAvatarPath = session.contentUrlResolver() diff --git a/vector/src/main/java/im/vector/app/features/notifications/NotifiableMessageEvent.kt b/vector/src/main/java/im/vector/app/features/notifications/NotifiableMessageEvent.kt index 68268739a0..bbd8c6638c 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/NotifiableMessageEvent.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/NotifiableMessageEvent.kt @@ -31,6 +31,7 @@ data class NotifiableMessageEvent( // NotSerializableException when persisting this to storage val imageUriString: String?, val roomId: String, + val threadId: String?, val roomName: String?, val roomIsDirect: Boolean = false, val roomAvatarPath: String? = null, @@ -51,3 +52,10 @@ data class NotifiableMessageEvent( val imageUri: Uri? get() = imageUriString?.let { Uri.parse(it) } } + +fun NotifiableMessageEvent.shouldIgnoreMessageEventInRoom(currentRoomId: String?, currentThreadId: String?): Boolean { + return when (currentRoomId) { + null -> false + else -> roomId == currentRoomId && threadId == currentThreadId + } +} diff --git a/vector/src/main/java/im/vector/app/features/notifications/NotificationBroadcastReceiver.kt b/vector/src/main/java/im/vector/app/features/notifications/NotificationBroadcastReceiver.kt index 180351f806..455f4778e8 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/NotificationBroadcastReceiver.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/NotificationBroadcastReceiver.kt @@ -148,6 +148,7 @@ class NotificationBroadcastReceiver : BroadcastReceiver() { body = message, imageUriString = null, roomId = room.roomId, + threadId = null, // needs to be changed: https://github.com/vector-im/element-android/issues/7475 roomName = room.roomSummary()?.displayName ?: room.roomId, roomIsDirect = room.roomSummary()?.isDirect == true, outGoingMessage = true, 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 2623045cf3..4f05e83bd4 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 @@ -63,6 +63,7 @@ class NotificationDrawerManager @Inject constructor( private val notificationState by lazy { createInitialNotificationState() } private val avatarSize = context.resources.getDimensionPixelSize(R.dimen.profile_avatar_size) private var currentRoomId: String? = null + private var currentThreadId: String? = null private val firstThrottler = FirstThrottler(200) private var useCompleteNotificationFormat = vectorPreferences.useCompleteNotificationFormat() @@ -123,6 +124,22 @@ class NotificationDrawerManager @Inject constructor( } } + /** + * Should be called when the application is currently opened and showing timeline for the given threadId. + * Used to ignore events related to that thread (no need to display notification) and clean any existing notification on this room. + */ + fun setCurrentThread(threadId: String?) { + updateEvents { + val hasChanged = threadId != currentThreadId + currentThreadId = threadId + currentRoomId?.let { roomId -> + if (hasChanged && threadId != null) { + it.clearMessagesForThread(roomId, threadId) + } + } + } + } + fun notificationStyleChanged() { updateEvents { val newSettings = vectorPreferences.useCompleteNotificationFormat() @@ -164,7 +181,7 @@ class NotificationDrawerManager @Inject constructor( private fun refreshNotificationDrawerBg() { Timber.v("refreshNotificationDrawerBg()") val eventsToRender = notificationState.updateQueuedEvents(this) { queuedEvents, renderedEvents -> - notifiableEventProcessor.process(queuedEvents.rawEvents(), currentRoomId, renderedEvents).also { + notifiableEventProcessor.process(queuedEvents.rawEvents(), currentRoomId, currentThreadId, renderedEvents).also { queuedEvents.clearAndAdd(it.onlyKeptEvents()) } } @@ -198,8 +215,8 @@ class NotificationDrawerManager @Inject constructor( notificationRenderer.render(session.myUserId, myUserDisplayName, myUserAvatarUrl, useCompleteNotificationFormat, eventsToRender) } - fun shouldIgnoreMessageEventInRoom(roomId: String?): Boolean { - return currentRoomId != null && roomId == currentRoomId + fun shouldIgnoreMessageEventInRoom(resolvedEvent: NotifiableMessageEvent): Boolean { + return resolvedEvent.shouldIgnoreMessageEventInRoom(currentRoomId, currentThreadId) } companion object { diff --git a/vector/src/main/java/im/vector/app/features/notifications/NotificationEventQueue.kt b/vector/src/main/java/im/vector/app/features/notifications/NotificationEventQueue.kt index f02424803a..8aff9c3bf2 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/NotificationEventQueue.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/NotificationEventQueue.kt @@ -122,15 +122,20 @@ data class NotificationEventQueue( } fun clearMemberShipNotificationForRoom(roomId: String) { - Timber.v("clearMemberShipOfRoom $roomId") + Timber.d("clearMemberShipOfRoom $roomId") queue.removeAll { it is InviteNotifiableEvent && it.roomId == roomId } } fun clearMessagesForRoom(roomId: String) { - Timber.v("clearMessageEventOfRoom $roomId") + Timber.d("clearMessageEventOfRoom $roomId") queue.removeAll { it is NotifiableMessageEvent && it.roomId == roomId } } + fun clearMessagesForThread(roomId: String, threadId: String) { + Timber.d("clearMessageEventOfThread $roomId, $threadId") + queue.removeAll { it is NotifiableMessageEvent && it.roomId == roomId && it.threadId == threadId } + } + fun rawEvents(): List = queue } 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 131a423316..59e42a9568 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 @@ -27,6 +27,7 @@ import org.junit.Test import org.matrix.android.sdk.api.session.events.model.EventType private val NOT_VIEWING_A_ROOM: String? = null +private val NOT_VIEWING_A_THREAD: String? = null class NotifiableEventProcessorTest { @@ -42,7 +43,7 @@ class NotifiableEventProcessorTest { aSimpleNotifiableEvent(eventId = "event-2") ) - val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, renderedEvents = emptyList()) + val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, currentThreadId = NOT_VIEWING_A_THREAD, renderedEvents = emptyList()) result shouldBeEqualTo listOfProcessedEvents( Type.KEEP to events[0], @@ -54,7 +55,7 @@ class NotifiableEventProcessorTest { fun `given redacted simple event when processing then remove redaction event`() { val events = listOf(aSimpleNotifiableEvent(eventId = "event-1", type = EventType.REDACTION)) - val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, renderedEvents = emptyList()) + val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, currentThreadId = NOT_VIEWING_A_THREAD, renderedEvents = emptyList()) result shouldBeEqualTo listOfProcessedEvents( Type.REMOVE to events[0] @@ -69,7 +70,7 @@ class NotifiableEventProcessorTest { anInviteNotifiableEvent(roomId = "room-2") ) - val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, renderedEvents = emptyList()) + val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, currentThreadId = NOT_VIEWING_A_THREAD, renderedEvents = emptyList()) result shouldBeEqualTo listOfProcessedEvents( Type.REMOVE to events[0], @@ -85,7 +86,7 @@ class NotifiableEventProcessorTest { anInviteNotifiableEvent(roomId = "room-2") ) - val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, renderedEvents = emptyList()) + val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, currentThreadId = NOT_VIEWING_A_THREAD, renderedEvents = emptyList()) result shouldBeEqualTo listOfProcessedEvents( Type.KEEP to events[0], @@ -98,7 +99,7 @@ class NotifiableEventProcessorTest { val events = listOf(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1")) outdatedDetector.givenEventIsOutOfDate(events[0]) - val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, renderedEvents = emptyList()) + val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, currentThreadId = NOT_VIEWING_A_THREAD, renderedEvents = emptyList()) result shouldBeEqualTo listOfProcessedEvents( Type.REMOVE to events[0], @@ -110,7 +111,7 @@ class NotifiableEventProcessorTest { val events = listOf(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1")) outdatedDetector.givenEventIsInDate(events[0]) - val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, renderedEvents = emptyList()) + val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, currentThreadId = NOT_VIEWING_A_THREAD, renderedEvents = emptyList()) result shouldBeEqualTo listOfProcessedEvents( Type.KEEP to events[0], @@ -118,16 +119,51 @@ class NotifiableEventProcessorTest { } @Test - fun `given viewing the same room as message event when processing then removes message`() { - val events = listOf(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1")) + fun `given viewing the same room main timeline when processing main timeline message event then removes message`() { + val events = listOf(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1", threadId = null)) - val result = eventProcessor.process(events, currentRoomId = "room-1", renderedEvents = emptyList()) + val result = eventProcessor.process(events, currentRoomId = "room-1", currentThreadId = null, renderedEvents = emptyList()) result shouldBeEqualTo listOfProcessedEvents( Type.REMOVE to events[0], ) } + @Test + fun `given viewing the same thread timeline when processing thread message event then removes message`() { + val events = listOf(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1", threadId = "thread-1")) + + val result = eventProcessor.process(events, currentRoomId = "room-1", currentThreadId = "thread-1", renderedEvents = emptyList()) + + result shouldBeEqualTo listOfProcessedEvents( + Type.REMOVE to events[0], + ) + } + + @Test + fun `given viewing main timeline of the same room when processing thread timeline message event then keep message`() { + val events = listOf(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1", threadId = "thread-1")) + outdatedDetector.givenEventIsInDate(events[0]) + + val result = eventProcessor.process(events, currentRoomId = "room-1", currentThreadId = null, renderedEvents = emptyList()) + + result shouldBeEqualTo listOfProcessedEvents( + Type.KEEP to events[0], + ) + } + + @Test + fun `given viewing thread timeline of the same room when processing main timeline message event then keep message`() { + val events = listOf(aNotifiableMessageEvent(eventId = "event-1", roomId = "room-1")) + outdatedDetector.givenEventIsInDate(events[0]) + + val result = eventProcessor.process(events, currentRoomId = "room-1", currentThreadId = "thread-1", renderedEvents = emptyList()) + + result shouldBeEqualTo listOfProcessedEvents( + Type.KEEP to events[0], + ) + } + @Test fun `given events are different to rendered events when processing then removes difference`() { val events = listOf(aSimpleNotifiableEvent(eventId = "event-1")) @@ -136,7 +172,7 @@ class NotifiableEventProcessorTest { ProcessedEvent(Type.KEEP, anInviteNotifiableEvent(roomId = "event-2")) ) - val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, renderedEvents = renderedEvents) + val result = eventProcessor.process(events, currentRoomId = NOT_VIEWING_A_ROOM, currentThreadId = NOT_VIEWING_A_THREAD, renderedEvents = renderedEvents) result shouldBeEqualTo listOfProcessedEvents( Type.REMOVE to renderedEvents[1].event, diff --git a/vector/src/test/java/im/vector/app/test/fixtures/NotifiableEventFixture.kt b/vector/src/test/java/im/vector/app/test/fixtures/NotifiableEventFixture.kt index 397ca80f84..a6d21a46c9 100644 --- a/vector/src/test/java/im/vector/app/test/fixtures/NotifiableEventFixture.kt +++ b/vector/src/test/java/im/vector/app/test/fixtures/NotifiableEventFixture.kt @@ -63,6 +63,7 @@ fun anInviteNotifiableEvent( fun aNotifiableMessageEvent( eventId: String = "a-message-event-id", roomId: String = "a-message-room-id", + threadId: String? = null, isRedacted: Boolean = false ) = NotifiableMessageEvent( eventId = eventId, @@ -73,6 +74,7 @@ fun aNotifiableMessageEvent( senderId = "sending-id", body = "message-body", roomId = roomId, + threadId = threadId, roomName = "room-name", roomIsDirect = false, canBeReplaced = false,