Fix: make sure we ignore notifications for open rooms (#867)

* Make sure we ignore notifications for open rooms
- Listen to process lifecycle changes in `AppForegroundStateService`. Use initializers to reliable create it.
- Merge `AppNavigationState` with `AppForegroundState`. Renamed the previous `AppNavigationState` to `NavigationState`, created a new `AppNavigationState` which contains both the navigation state and the foreground state.

Author: jmartinesp

app/build.gradle.kts

@@ -209,6 +209,7 @@ dependencies {
     implementation(libs.androidx.core)
     implementation(libs.androidx.corektx)
     implementation(libs.androidx.lifecycle.runtime)
+    implementation(libs.androidx.lifecycle.process)
     implementation(libs.androidx.activity.compose)
     implementation(libs.androidx.startup)
     implementation(libs.androidx.preference)

app/src/main/AndroidManifest.xml

@@ -32,6 +32,18 @@
         android:theme="@style/Theme.ElementX"
         tools:targetApi="33">
 
+        <provider
+            android:name="androidx.startup.InitializationProvider"
+            android:authorities="${applicationId}.androidx-startup"
+            android:exported="false"
+            tools:node="merge">
+
+            <meta-data
+                android:name='androidx.lifecycle.ProcessLifecycleInitializer'
+                android:value='androidx.startup' />
+
+        </provider>
+
         <activity
             android:name=".MainActivity"
             android:configChanges="orientation|screenSize|screenLayout|keyboardHidden|uiMode"

appnav/src/test/kotlin/io/element/android/appnav/RoomFlowNodeTest.kt

@@ -32,7 +32,7 @@ import io.element.android.features.roomdetails.api.RoomDetailsEntryPoint
 import io.element.android.libraries.architecture.childNode
 import io.element.android.libraries.matrix.api.room.RoomMembershipObserver
 import io.element.android.libraries.matrix.test.room.FakeMatrixRoom
-import io.element.android.services.appnavstate.test.NoopAppNavigationStateService
+import io.element.android.services.appnavstate.test.FakeAppNavigationStateService
 import org.junit.Rule
 import org.junit.Test
 
@@ -82,7 +82,7 @@ class RoomFlowNodeTest {
         plugins = plugins,
         messagesEntryPoint = messagesEntryPoint,
         roomDetailsEntryPoint = roomDetailsEntryPoint,
-        appNavigationStateService = NoopAppNavigationStateService(),
+        appNavigationStateService = FakeAppNavigationStateService(),
         roomMembershipObserver = RoomMembershipObserver()
     )
 

libraries/push/impl/build.gradle.kts

@@ -31,7 +31,6 @@ dependencies {
     implementation(libs.dagger)
     implementation(libs.androidx.corektx)
     implementation(libs.androidx.datastore.preferences)
-    implementation(libs.androidx.lifecycle.process)
     implementation(libs.androidx.security.crypto)
     implementation(libs.network.retrofit)
     implementation(libs.serialization.json)

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt

@@ -32,9 +32,7 @@ import io.element.android.libraries.matrix.api.user.MatrixUser
 import io.element.android.libraries.push.api.notifications.NotificationDrawerManager
 import io.element.android.libraries.push.api.store.PushDataStore
 import io.element.android.libraries.push.impl.notifications.model.NotifiableEvent
-import io.element.android.libraries.push.impl.notifications.model.NotifiableMessageEvent
-import io.element.android.libraries.push.impl.notifications.model.shouldIgnoreEventInRoom
-import io.element.android.services.appnavstate.api.AppNavigationState
+import io.element.android.services.appnavstate.api.NavigationState
 import io.element.android.services.appnavstate.api.AppNavigationStateService
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.delay
@@ -65,7 +63,6 @@ class DefaultNotificationDrawerManager @Inject constructor(
      * Lazily initializes the NotificationState as we rely on having a current session in order to fetch the persisted queue of events.
      */
     private val notificationState by lazy { createInitialNotificationState() }
-    private var currentAppNavigationState: AppNavigationState? = null
     private val firstThrottler = FirstThrottler(200)
 
     // TODO EAx add a setting per user for this
@@ -74,26 +71,25 @@ class DefaultNotificationDrawerManager @Inject constructor(
     init {
         // Observe application state
         coroutineScope.launch {
-            appNavigationStateService.appNavigationStateFlow
-                .collect { onAppNavigationStateChange(it) }
+            appNavigationStateService.appNavigationState
+                .collect { onAppNavigationStateChange(it.navigationState) }
         }
     }
 
-    private fun onAppNavigationStateChange(appNavigationState: AppNavigationState) {
-        currentAppNavigationState = appNavigationState
-        when (appNavigationState) {
-            AppNavigationState.Root -> {}
-            is AppNavigationState.Session -> {}
-            is AppNavigationState.Space -> {}
-            is AppNavigationState.Room -> {
+    private fun onAppNavigationStateChange(navigationState: NavigationState) {
+        when (navigationState) {
+            NavigationState.Root -> {}
+            is NavigationState.Session -> {}
+            is NavigationState.Space -> {}
+            is NavigationState.Room -> {
                 // Cleanup notification for current room
-                clearMessagesForRoom(appNavigationState.parentSpace.parentSession.sessionId, appNavigationState.roomId)
+                clearMessagesForRoom(navigationState.parentSpace.parentSession.sessionId, navigationState.roomId)
             }
-            is AppNavigationState.Thread -> {
+            is NavigationState.Thread -> {
                 onEnteringThread(
-                    appNavigationState.parentRoom.parentSpace.parentSession.sessionId,
-                    appNavigationState.parentRoom.roomId,
-                    appNavigationState.threadId
+                    navigationState.parentRoom.parentSpace.parentSession.sessionId,
+                    navigationState.parentRoom.roomId,
+                    navigationState.threadId
                 )
             }
         }
@@ -225,7 +221,7 @@ class DefaultNotificationDrawerManager @Inject constructor(
     private suspend fun refreshNotificationDrawerBg() {
         Timber.v("refreshNotificationDrawerBg()")
         val eventsToRender = notificationState.updateQueuedEvents(this) { queuedEvents, renderedEvents ->
-            notifiableEventProcessor.process(queuedEvents.rawEvents(), currentAppNavigationState, renderedEvents).also {
+            notifiableEventProcessor.process(queuedEvents.rawEvents(), renderedEvents).also {
                 queuedEvents.clearAndAdd(it.onlyKeptEvents())
             }
         }
@@ -275,8 +271,4 @@ class DefaultNotificationDrawerManager @Inject constructor(
             notificationRenderer.render(currentUser, useCompleteNotificationFormat, notifiableEvents)
         }
     }
-
-    fun shouldIgnoreMessageEventInRoom(resolvedEvent: NotifiableMessageEvent): Boolean {
-        return resolvedEvent.shouldIgnoreEventInRoom(currentAppNavigationState)
-    }
 }

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventProcessor.kt

@@ -23,26 +23,27 @@ import io.element.android.libraries.push.impl.notifications.model.NotifiableEven
 import io.element.android.libraries.push.impl.notifications.model.NotifiableMessageEvent
 import io.element.android.libraries.push.impl.notifications.model.SimpleNotifiableEvent
 import io.element.android.libraries.push.impl.notifications.model.shouldIgnoreEventInRoom
-import io.element.android.services.appnavstate.api.AppNavigationState
+import io.element.android.services.appnavstate.api.AppNavigationStateService
 import timber.log.Timber
 import javax.inject.Inject
 
 private typealias ProcessedEvents = List<ProcessedEvent<NotifiableEvent>>
 
 class NotifiableEventProcessor @Inject constructor(
     private val outdatedDetector: OutdatedEventDetector,
+    private val appNavigationStateService: AppNavigationStateService,
 ) {
 
     fun process(
         queuedEvents: List<NotifiableEvent>,
-        appNavigationState: AppNavigationState?,
         renderedEvents: ProcessedEvents,
     ): ProcessedEvents {
+        val appState = appNavigationStateService.appNavigationState.value
         val processedEvents = queuedEvents.map {
             val type = when (it) {
                 is InviteNotifiableEvent -> ProcessedEvent.Type.KEEP
                 is NotifiableMessageEvent -> when {
-                    it.shouldIgnoreEventInRoom(appNavigationState) -> {
+                    it.shouldIgnoreEventInRoom(appState) -> {
                         ProcessedEvent.Type.REMOVE
                             .also { Timber.d("notification message removed due to currently viewing the same room or thread") }
                     }
@@ -55,7 +56,7 @@ class NotifiableEventProcessor @Inject constructor(
                     else -> ProcessedEvent.Type.KEEP
                 }
                 is FallbackNotifiableEvent -> when {
-                    it.shouldIgnoreEventInRoom(appNavigationState) -> {
+                    it.shouldIgnoreEventInRoom(appState) -> {
                         ProcessedEvent.Type.REMOVE
                             .also { Timber.d("notification fallback removed due to currently viewing the same room or thread") }
                     }

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/model/NotifiableMessageEvent.kt

@@ -16,8 +16,6 @@
 package io.element.android.libraries.push.impl.notifications.model
 
 import android.net.Uri
-import androidx.lifecycle.Lifecycle
-import androidx.lifecycle.ProcessLifecycleOwner
 import io.element.android.libraries.matrix.api.core.EventId
 import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.api.core.SessionId
@@ -69,18 +67,13 @@ data class NotifiableMessageEvent(
 /**
  * Used to check if a notification should be ignored based on the current app and navigation state.
  */
-fun NotifiableEvent.shouldIgnoreEventInRoom(
-    appNavigationState: AppNavigationState?
-): Boolean {
-    val currentSessionId = appNavigationState?.currentSessionId() ?: return false
-    return when (val currentRoomId = appNavigationState.currentRoomId()) {
+fun NotifiableEvent.shouldIgnoreEventInRoom(appNavigationState: AppNavigationState): Boolean {
+    val currentSessionId = appNavigationState.navigationState.currentSessionId() ?: return false
+    return when (val currentRoomId = appNavigationState.navigationState.currentRoomId()) {
         null -> false
-        else -> isAppInForeground
+        else -> appNavigationState.isInForeground
             && sessionId == currentSessionId
             && roomId == currentRoomId
-            && (this as? NotifiableMessageEvent)?.threadId == appNavigationState.currentThreadId()
+            && (this as? NotifiableMessageEvent)?.threadId == appNavigationState.navigationState.currentThreadId()
     }
 }
-
-private val isAppInForeground: Boolean
-    get() = ProcessLifecycleOwner.get().lifecycle.currentState.isAtLeast(Lifecycle.State.STARTED)

libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventProcessorTest.kt

@@ -30,26 +30,30 @@ import io.element.android.libraries.push.impl.notifications.fixtures.aNotifiable
 import io.element.android.libraries.push.impl.notifications.fixtures.aSimpleNotifiableEvent
 import io.element.android.libraries.push.impl.notifications.fixtures.anInviteNotifiableEvent
 import io.element.android.libraries.push.impl.notifications.model.NotifiableEvent
-import io.element.android.services.appnavstate.test.anAppNavigationState
+import io.element.android.services.appnavstate.api.NavigationState
+import io.element.android.services.appnavstate.api.AppNavigationState
+import io.element.android.services.appnavstate.test.FakeAppNavigationStateService
+import io.element.android.services.appnavstate.test.aNavigationState
+import kotlinx.coroutines.flow.MutableStateFlow
 import org.junit.Test
 
-private val NOT_VIEWING_A_ROOM = anAppNavigationState()
-private val VIEWING_A_ROOM = anAppNavigationState(A_SESSION_ID, A_SPACE_ID, A_ROOM_ID)
-private val VIEWING_A_THREAD = anAppNavigationState(A_SESSION_ID, A_SPACE_ID, A_ROOM_ID, A_THREAD_ID)
+private val NOT_VIEWING_A_ROOM = aNavigationState()
+private val VIEWING_A_ROOM = aNavigationState(A_SESSION_ID, A_SPACE_ID, A_ROOM_ID)
+private val VIEWING_A_THREAD = aNavigationState(A_SESSION_ID, A_SPACE_ID, A_ROOM_ID, A_THREAD_ID)
 
 class NotifiableEventProcessorTest {
 
     private val outdatedDetector = FakeOutdatedEventDetector()
-    private val eventProcessor = NotifiableEventProcessor(outdatedDetector.instance)
 
     @Test
     fun `given simple events when processing then keep simple events`() {
         val events = listOf(
             aSimpleNotifiableEvent(eventId = AN_EVENT_ID),
             aSimpleNotifiableEvent(eventId = AN_EVENT_ID_2)
         )
+        val eventProcessor = createProcessor(navigationState = NOT_VIEWING_A_ROOM)
 
-        val result = eventProcessor.process(events, appNavigationState = NOT_VIEWING_A_ROOM, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -62,8 +66,9 @@ class NotifiableEventProcessorTest {
     @Test
     fun `given redacted simple event when processing then remove redaction event`() {
         val events = listOf(aSimpleNotifiableEvent(eventId = AN_EVENT_ID, type = EventType.REDACTION))
+        val eventProcessor = createProcessor(navigationState = NOT_VIEWING_A_ROOM)
 
-        val result = eventProcessor.process(events, appNavigationState = NOT_VIEWING_A_ROOM, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -78,8 +83,9 @@ class NotifiableEventProcessorTest {
             anInviteNotifiableEvent(roomId = A_ROOM_ID),
             anInviteNotifiableEvent(roomId = A_ROOM_ID_2)
         )
+        val eventProcessor = createProcessor(navigationState = NOT_VIEWING_A_ROOM)
 
-        val result = eventProcessor.process(events, appNavigationState = NOT_VIEWING_A_ROOM, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -94,7 +100,9 @@ class NotifiableEventProcessorTest {
         val events = listOf(aNotifiableMessageEvent(eventId = AN_EVENT_ID, roomId = A_ROOM_ID))
         outdatedDetector.givenEventIsOutOfDate(events[0])
 
-        val result = eventProcessor.process(events, appNavigationState = NOT_VIEWING_A_ROOM, renderedEvents = emptyList())
+        val eventProcessor = createProcessor(navigationState = NOT_VIEWING_A_ROOM)
+
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -107,8 +115,9 @@ class NotifiableEventProcessorTest {
     fun `given in date message event when processing then keep message event`() {
         val events = listOf(aNotifiableMessageEvent(eventId = AN_EVENT_ID, roomId = A_ROOM_ID))
         outdatedDetector.givenEventIsInDate(events[0])
+        val eventProcessor = createProcessor(navigationState = NOT_VIEWING_A_ROOM)
 
-        val result = eventProcessor.process(events, appNavigationState = NOT_VIEWING_A_ROOM, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -121,8 +130,9 @@ class NotifiableEventProcessorTest {
     fun `given viewing the same room main timeline when processing main timeline message event then removes message`() {
         val events = listOf(aNotifiableMessageEvent(eventId = AN_EVENT_ID, roomId = A_ROOM_ID, threadId = null))
         events.forEach { outdatedDetector.givenEventIsOutOfDate(it) }
+        val eventProcessor = createProcessor(isInForeground = true, navigationState = VIEWING_A_ROOM)
 
-        val result = eventProcessor.process(events, VIEWING_A_ROOM, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -135,8 +145,9 @@ class NotifiableEventProcessorTest {
     fun `given viewing the same thread timeline when processing thread message event then removes message`() {
         val events = listOf(aNotifiableMessageEvent(eventId = AN_EVENT_ID, roomId = A_ROOM_ID, threadId = A_THREAD_ID))
         events.forEach { outdatedDetector.givenEventIsOutOfDate(it) }
+        val eventProcessor = createProcessor(isInForeground = true, navigationState = VIEWING_A_THREAD)
 
-        val result = eventProcessor.process(events, VIEWING_A_THREAD, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -149,8 +160,9 @@ class NotifiableEventProcessorTest {
     fun `given viewing main timeline of the same room when processing thread timeline message event then keep message`() {
         val events = listOf(aNotifiableMessageEvent(eventId = AN_EVENT_ID, roomId = A_ROOM_ID, threadId = A_THREAD_ID))
         outdatedDetector.givenEventIsInDate(events[0])
+        val eventProcessor = createProcessor(isInForeground = true, navigationState = VIEWING_A_ROOM)
 
-        val result = eventProcessor.process(events, VIEWING_A_ROOM, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -163,8 +175,9 @@ class NotifiableEventProcessorTest {
     fun `given viewing thread timeline of the same room when processing main timeline message event then keep message`() {
         val events = listOf(aNotifiableMessageEvent(eventId = AN_EVENT_ID, roomId = A_ROOM_ID))
         outdatedDetector.givenEventIsInDate(events[0])
+        val eventProcessor = createProcessor(isInForeground = true, navigationState = VIEWING_A_THREAD)
 
-        val result = eventProcessor.process(events, VIEWING_A_THREAD, renderedEvents = emptyList())
+        val result = eventProcessor.process(events, renderedEvents = emptyList())
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -180,8 +193,9 @@ class NotifiableEventProcessorTest {
             ProcessedEvent(ProcessedEvent.Type.KEEP, events[0]),
             ProcessedEvent(ProcessedEvent.Type.KEEP, anInviteNotifiableEvent(eventId = AN_EVENT_ID_2))
         )
+        val eventProcessor = createProcessor(navigationState = NOT_VIEWING_A_ROOM)
 
-        val result = eventProcessor.process(events, appNavigationState = NOT_VIEWING_A_ROOM, renderedEvents = renderedEvents)
+        val result = eventProcessor.process(events, renderedEvents = renderedEvents)
 
         assertThat(result).isEqualTo(
             listOfProcessedEvents(
@@ -194,4 +208,14 @@ class NotifiableEventProcessorTest {
     private fun listOfProcessedEvents(vararg event: Pair<ProcessedEvent.Type, NotifiableEvent>) = event.map {
         ProcessedEvent(it.first, it.second)
     }
+
+    private fun createProcessor(
+        isInForeground: Boolean = false,
+        navigationState: NavigationState
+    ): NotifiableEventProcessor {
+        return NotifiableEventProcessor(
+            outdatedDetector.instance,
+            FakeAppNavigationStateService(MutableStateFlow(AppNavigationState(navigationState, isInForeground))),
+        )
+    }
 }

services/appnavstate/api/build.gradle.kts

@@ -24,5 +24,8 @@ android {
 
 dependencies {
     implementation(libs.coroutines.core)
+    implementation(libs.androidx.lifecycle.runtime)
+    implementation(libs.androidx.lifecycle.process)
+    implementation(libs.androidx.startup)
     implementation(projects.libraries.matrix.api)
 }

services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppForegroundStateService.kt

@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2023 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.services.appnavstate.api
+
+import kotlinx.coroutines.flow.StateFlow
+
+/**
+ * A service that tracks the foreground state of the app.
+ */
+interface AppForegroundStateService {
+    /**
+     * Any updates to the foreground state of the app will be emitted here.
+     */
+    val isInForeground: StateFlow<Boolean>
+
+    /**
+     * Start observing the foreground state.
+     */
+    fun start()
+}