Reappearing notifications on slow homeservers

[ouchadam]

Fixes #3437 - reappearing notifications after dismissing

Adds a in memory cache for keeping track of notification events we've already seen, this allows us to skip displaying already dismissed events received by the push flow which the /sync response then returns at a later point.

The cache is a simple circular buffer of 25 event ids to avoid us tracking every single event over the lifetime of the application. The cache size is a finger in the air estimate, could many users receive and dismiss 25+ push notifications before a sync response completes 🤔

*with a forced 5 second delay when syncing

BEFORE	AFTER

The gif isn't very exciting, basically the after shows the notification no longer reappearing after dismissing

[github-actions]

Unit Test Results

  48 files  +  4    48 suites  +4   50s ⏱️ +2s
  91 tests +  4    91 ✔️ +  4  0 💤 ±0  0 ❌ ±0 
244 runs  +16  244 ✔️ +16  0 💤 ±0  0 ❌ ±0 

Results for commit fc793c4. ± Comparison against base commit 7ec0872.

♻️ This comment has been updated with latest results.

[bmarty]

LGTM, just one small remark and one suggestion that you must feel free to just ignore :)

[bmarty]

Thinking of a small rework, also more generic, but with the disadvantage of exposing the whole MutableList API...

/**
 * A FIFO circular buffer of elements
 */
class CircularCache<T>(cacheSize: Int) : MutableList<T> by ArrayList(cacheSize) {
    private var writeIndex = 0

    override fun add(element: T): Boolean {
        if (writeIndex == size - 1) {
            writeIndex = 0
        }
        set(writeIndex, element)
        writeIndex++
        return true
    }
}

You decide if it's better

[bmarty]

set(writeIndex, element) may crash (not tested)

[bmarty]

Also maybe add as a comment that this is not thread safe (I think)

[ouchadam]

• 👍 for switching to a List, I originally chose Array to try and be as memory efficient as possible (as List will auto resize when it gets close to the capacity) but List allows it to be generic

• I'd prefer to avoid exposing the List to avoid it being used in unexpected ways

• 👍 will document the class as not being thread safe

[ouchadam]

ended up going back to using an Array, a list means we have to handle prefilling or add/remove logic

[bmarty]

This comment refer to the first if so should be moved above this new if

[bmarty]

Sorry if it wasn't clear. I would like to have:

                    // 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)
                    }

[ouchadam]

ah 🤦 will do 👍

[bmarty]

Ah, small } at the end of the String

[bmarty]

💯 for adding tests!

[bmarty]

Thanks for the update!