Merge pull request #4238 from vector-im/feature/adm/delayed-sync-duplicated-notification

Reappearing notifications on slow homeservers

changelog.d/3437.bugfix

@@ -0,0 +1 @@
+Fixes reappearing notifications when dismissing notifications from slow homeservers or delayed /sync responses

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

@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2021 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 im.vector.app.features.notifications
+
+/**
+ * A FIFO circular buffer of T
+ * This class is not thread safe
+ */
+class CircularCache<T : Any>(cacheSize: Int, factory: (Int) -> Array<T?>) {
+
+    companion object {
+        inline fun <reified T : Any> create(cacheSize: Int) = CircularCache(cacheSize) { Array<T?>(cacheSize) { null } }
+    }
+
+    private val cache = factory(cacheSize)
+    private var writeIndex = 0
+
+    fun contains(key: T): Boolean = cache.contains(key)
+
+    fun put(key: T) {
+        if (writeIndex == cache.size) {
+            writeIndex = 0
+        }
+        cache[writeIndex] = key
+        writeIndex++
+    }
+}

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

@@ -84,6 +84,13 @@ class NotificationDrawerManager @Inject constructor(private val context: Context
 
     private var useCompleteNotificationFormat = vectorPreferences.useCompleteNotificationFormat()
 
+    /**
+     * An in memory FIFO cache of the seen events.
+     * Acts as a notification debouncer to stop already dismissed push notifications from
+     * displaying again when the /sync response is delayed.
+     */
+    private val seenEventIds = CircularCache.create<String>(cacheSize = 25)
+
     /**
     Should be called as soon as a new event is ready to be displayed.
     The notification corresponding to this event will not be displayed until
@@ -141,11 +148,17 @@ class NotificationDrawerManager @Inject constructor(private val context: Context
                     }
                 } else {
                     // Not an edit
+                    if (seenEventIds.contains(notifiableEvent.eventId)) {
+                        // we've already seen the event, lets skip
+                        Timber.d("onNotifiableEventReceived(): skipping event, already seen")
+                    } else {
+                        seenEventIds.put(notifiableEvent.eventId)
                         eventList.add(notifiableEvent)
                     }
                 }
             }
         }
+    }
 
     fun onEventRedacted(eventId: String) {
         synchronized(eventList) {

vector/src/test/java/im/vector/app/features/notifications/CircularCacheTest.kt

@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2021 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 im.vector.app.features.notifications
+
+import org.amshove.kluent.shouldBeEqualTo
+import org.junit.Test
+
+class CircularCacheTest {
+
+    @Test
+    fun `when putting more than cache size then cache is limited to cache size`() {
+        val (cache, internalData) = createIntCache(cacheSize = 3)
+
+        cache.putInOrder(1, 1, 1, 1, 1, 1)
+
+        internalData shouldBeEqualTo arrayOf(1, 1, 1)
+    }
+
+    @Test
+    fun `when putting more than cache then acts as FIFO`() {
+        val (cache, internalData) = createIntCache(cacheSize = 3)
+
+        cache.putInOrder(1, 2, 3, 4)
+
+        internalData shouldBeEqualTo arrayOf(4, 2, 3)
+    }
+
+    @Test
+    fun `given empty cache when checking if contains key then is false`() {
+        val (cache, _) = createIntCache(cacheSize = 3)
+
+        val result = cache.contains(1)
+
+        result shouldBeEqualTo false
+    }
+
+    @Test
+    fun `given cached key when checking if contains key then is true`() {
+        val (cache, _) = createIntCache(cacheSize = 3)
+
+        cache.put(1)
+        val result = cache.contains(1)
+
+        result shouldBeEqualTo true
+    }
+
+    private fun createIntCache(cacheSize: Int): Pair<CircularCache<Int>, Array<Int?>> {
+        var internalData: Array<Int?>? = null
+        val factory: (Int) -> Array<Int?> = {
+            Array<Int?>(it) { null }.also { array -> internalData = array }
+        }
+        return CircularCache(cacheSize, factory) to internalData!!
+    }
+
+    private fun CircularCache<Int>.putInOrder(vararg keys: Int) {
+        keys.forEach { put(it) }
+    }
+}