From f2da047720404da59c911d66db1e3b45924c5392 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 13 Oct 2021 15:36:37 +0100 Subject: [PATCH 1/6] keeping an inmemory cache of the seen ids, fixes delayed sync responses causing already dismissed notifications from being shown again - uses a simple circular buffer to limit the memory usage --- .../notifications/CircularStringCache.kt | 36 +++++++++++++++++++ .../NotificationDrawerManager.kt | 19 ++++++++-- 2 files changed, 52 insertions(+), 3 deletions(-) create mode 100644 vector/src/main/java/im/vector/app/features/notifications/CircularStringCache.kt diff --git a/vector/src/main/java/im/vector/app/features/notifications/CircularStringCache.kt b/vector/src/main/java/im/vector/app/features/notifications/CircularStringCache.kt new file mode 100644 index 0000000000..97b40daf46 --- /dev/null +++ b/vector/src/main/java/im/vector/app/features/notifications/CircularStringCache.kt @@ -0,0 +1,36 @@ +/* + * 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 strings + */ +class CircularStringCache(cacheSize: Int) { + + private val cache = Array(cacheSize) { "" } + private var writeIndex = 0 + + fun contains(key: String): Boolean = cache.contains(key) + + fun put(key: String) { + if (writeIndex == cache.size - 1) { + writeIndex = 0 + } + cache[writeIndex] = key + writeIndex++ + } +} 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 ecb1268892..2df093b615 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 @@ -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 = CircularStringCache(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 @@ -140,8 +147,14 @@ class NotificationDrawerManager @Inject constructor(private val context: Context // Ignore an edit of a not displayed event in the notification drawer } } else { - // Not an edit - eventList.add(notifiableEvent) + if (seenEventIds.contains(notifiableEvent.eventId)) { + // we've already seen the event, lets' skip + Timber.d("onNotifiableEventReceived(): skipping event, already seen}") + } else { + // Not an edit + seenEventIds.put(notifiableEvent.eventId) + eventList.add(notifiableEvent) + } } } } @@ -266,7 +279,7 @@ class NotificationDrawerManager @Inject constructor(private val context: Context is InviteNotifiableEvent -> { if (autoAcceptInvites.hideInvites) { // Forget this event - eventIterator.remove() + eventIterator.remove() } else { invitationEvents.add(event) } From 10b753ad610d69543b10e3c7a3d19690e8fb0bca Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 13 Oct 2021 15:42:59 +0100 Subject: [PATCH 2/6] adding changelog entry --- changelog.d/3437.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3437.bugfix diff --git a/changelog.d/3437.bugfix b/changelog.d/3437.bugfix new file mode 100644 index 0000000000..11e493e1ba --- /dev/null +++ b/changelog.d/3437.bugfix @@ -0,0 +1 @@ +Fixes reappearing notifications when dismissing notifications from slow homeservers or delayed /sync responses \ No newline at end of file From 84b44f6093a69b84b537d583d3191f2f1994cc8a Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Thu, 14 Oct 2021 12:24:06 +0100 Subject: [PATCH 3/6] using generic list for the circular cache instead of a fixed string array --- .../{CircularStringCache.kt => CircularCache.kt} | 8 ++++---- .../features/notifications/NotificationDrawerManager.kt | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) rename vector/src/main/java/im/vector/app/features/notifications/{CircularStringCache.kt => CircularCache.kt} (82%) diff --git a/vector/src/main/java/im/vector/app/features/notifications/CircularStringCache.kt b/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt similarity index 82% rename from vector/src/main/java/im/vector/app/features/notifications/CircularStringCache.kt rename to vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt index 97b40daf46..9a979a2ec8 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/CircularStringCache.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt @@ -19,14 +19,14 @@ package im.vector.app.features.notifications /** * A FIFO circular buffer of strings */ -class CircularStringCache(cacheSize: Int) { +class CircularCache(cacheSize: Int) { - private val cache = Array(cacheSize) { "" } + private val cache = ArrayList(initialCapacity = cacheSize) private var writeIndex = 0 - fun contains(key: String): Boolean = cache.contains(key) + fun contains(key: T): Boolean = cache.contains(key) - fun put(key: String) { + fun put(key: T) { if (writeIndex == cache.size - 1) { writeIndex = 0 } 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 2df093b615..ff44ac4035 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 @@ -89,7 +89,7 @@ class NotificationDrawerManager @Inject constructor(private val context: Context * Acts as a notification debouncer to stop already dismissed push notifications from * displaying again when the /sync response is delayed. */ - private val seenEventIds = CircularStringCache(cacheSize = 25) + private val seenEventIds = CircularCache(cacheSize = 25) /** Should be called as soon as a new event is ready to be displayed. From 00beb27b56226b2f7ea653e3617869d70ffdc3d2 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Thu, 14 Oct 2021 12:25:13 +0100 Subject: [PATCH 4/6] updating class doc to mention its not thread safe --- .../java/im/vector/app/features/notifications/CircularCache.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt b/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt index 9a979a2ec8..de91766fc3 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt @@ -17,7 +17,8 @@ package im.vector.app.features.notifications /** - * A FIFO circular buffer of strings + * A FIFO circular buffer of T + * This class is not thread safe */ class CircularCache(cacheSize: Int) { From 0f0762954721713cb7115f04c0f147609318e248 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Thu, 14 Oct 2021 12:25:54 +0100 Subject: [PATCH 5/6] moving comment position to be above the if and cleaning up log copy --- .../app/features/notifications/NotificationDrawerManager.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 ff44ac4035..1598fa060c 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 @@ -147,11 +147,11 @@ class NotificationDrawerManager @Inject constructor(private val context: Context // Ignore an edit of a not displayed event in the notification drawer } } else { + // Not an edit if (seenEventIds.contains(notifiableEvent.eventId)) { - // we've already seen the event, lets' skip - Timber.d("onNotifiableEventReceived(): skipping event, already seen}") + // we've already seen the event, lets skip + Timber.d("onNotifiableEventReceived(): skipping event, already seen") } else { - // Not an edit seenEventIds.put(notifiableEvent.eventId) eventList.add(notifiableEvent) } From fc793c442b54d6dfa8d8c9c7c0e2cc7a4a6f6326 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Thu, 14 Oct 2021 13:12:16 +0100 Subject: [PATCH 6/6] reverting back to using an array for the circular cache, makes preloading and setting the value simpler - adds unit tests to show it working --- .../features/notifications/CircularCache.kt | 10 ++- .../NotificationDrawerManager.kt | 2 +- .../notifications/CircularCacheTest.kt | 72 +++++++++++++++++++ 3 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 vector/src/test/java/im/vector/app/features/notifications/CircularCacheTest.kt diff --git a/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt b/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt index de91766fc3..1ac66d0ae8 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/CircularCache.kt @@ -20,15 +20,19 @@ package im.vector.app.features.notifications * A FIFO circular buffer of T * This class is not thread safe */ -class CircularCache(cacheSize: Int) { +class CircularCache(cacheSize: Int, factory: (Int) -> Array) { - private val cache = ArrayList(initialCapacity = cacheSize) + companion object { + inline fun create(cacheSize: Int) = CircularCache(cacheSize) { Array(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 - 1) { + if (writeIndex == cache.size) { writeIndex = 0 } cache[writeIndex] = key 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 1598fa060c..5c20ce0832 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 @@ -89,7 +89,7 @@ class NotificationDrawerManager @Inject constructor(private val context: Context * Acts as a notification debouncer to stop already dismissed push notifications from * displaying again when the /sync response is delayed. */ - private val seenEventIds = CircularCache(cacheSize = 25) + private val seenEventIds = CircularCache.create(cacheSize = 25) /** Should be called as soon as a new event is ready to be displayed. diff --git a/vector/src/test/java/im/vector/app/features/notifications/CircularCacheTest.kt b/vector/src/test/java/im/vector/app/features/notifications/CircularCacheTest.kt new file mode 100644 index 0000000000..6428b1025b --- /dev/null +++ b/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, Array> { + var internalData: Array? = null + val factory: (Int) -> Array = { + Array(it) { null }.also { array -> internalData = array } + } + return CircularCache(cacheSize, factory) to internalData!! + } + + private fun CircularCache.putInOrder(vararg keys: Int) { + keys.forEach { put(it) } + } +}