Keep month pages updating after a drag (#440, #454) (#467)

A month page could stop updating for good. `PageStateCache` cancelled a month's query when the requested month moved far enough away, but the pager keeps pages composed further out than that window. Even a small drag that snaps back loads two months ahead (it prefetches), so the neighbouring month was dropped while still on screen. That page then never showed new or edited events until you swiped two months away and back.

This explains both reports. In split mode the day pane reads from the same stale page, and opening an event from it handed the editor the old start and end times.

`PageStateCache` now runs a page's query only while something collects it, stops it 5 s after the last collector leaves, and starts it again when it's collected again. Letting an entry go only removes it from the cache; it never stops a state a page still holds. A dropped entry nobody collects holds no coroutine. Week and day views use the same cache and get the same behaviour.

Widening the month window was tried first and wasn't enough, since how far the pager composes depends on drag and prefetch.

Closes #440
Closes #454

Co-authored-by: Jean-Luc Makiola <business@jeanlucmakiola.de>
Reviewed-on: https://codeberg.org/jlmakiola/calendula/pulls/467

app/src/main/java/de/jeanlucmakiola/calendula/ui/common/PageStateCache.kt

@@ -1,22 +1,26 @@
 package de.jeanlucmakiola.calendula.ui.common
 
-import de.jeanlucmakiola.floret.android.stateWhileSubscribed
 import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.ExperimentalForInheritanceCoroutinesApi
 import kotlinx.coroutines.Job
-import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.FlowCollector
+import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
-import kotlinx.coroutines.flow.stateIn
-import kotlinx.coroutines.plus
+import kotlinx.coroutines.launch
 
 /**
  * One shared state per pager page — a week, a day, a month — so a page and the
  * screen's anchor read the same query, and the pages either side stay loaded
  * across a swipe.
  *
- * Every entry shares on its own job, cancelled when [keep] lets it go: a
- * `stateIn` on the ViewModel's scope would stay running, holding its last state,
- * until the ViewModel itself was cleared. Main thread only.
+ * An entry queries only while something collects it, stopping [STOP_TIMEOUT_MILLIS]
+ * after the last collector leaves, and starts again when one comes back. Letting
+ * an entry go only forgets it: a page still holding it keeps a working state
+ * however far the pager composes or prefetches, and however long the screen sat
+ * stopped (#440, #454). A forgotten entry nobody collects holds no coroutine, so
+ * paging through a year doesn't keep a year of queries around. Main thread only.
  *
  * @param keep whether a cached key is still worth holding once [get] asks for another.
  */
@@ -26,28 +30,54 @@ class PageStateCache<K, S>(
     private val keep: (cached: K, requested: K) -> Boolean,
     private val load: (K) -> Flow<S>,
 ) {
-    private class Entry<S>(val state: StateFlow<S>, val job: Job)
-
-    private val entries = HashMap<K, Entry<S>>()
+    private val entries = HashMap<K, Entry>()
 
     /** The number of entries held, for tests. */
     internal val size: Int get() = entries.size
 
     fun get(key: K): StateFlow<S> {
-        val iterator = entries.iterator()
-        while (iterator.hasNext()) {
-            val (cached, entry) = iterator.next()
-            if (cached != key && !keep(cached, key)) {
-                entry.job.cancel()
-                iterator.remove()
+        // One still collected stays, so the anchor asking for it again shares its query.
+        entries.entries.removeAll { (cached, entry) ->
+            cached != key && !keep(cached, key) && !entry.isCollected
+        }
+        return entries.getOrPut(key) { Entry(key) }
+    }
+
+    @OptIn(ExperimentalForInheritanceCoroutinesApi::class)
+    private inner class Entry(private val key: K) : StateFlow<S> {
+        private val state = MutableStateFlow(initial)
+        private var collectors = 0
+        private var loading: Job? = null
+        private var stopping: Job? = null
+
+        val isCollected: Boolean get() = collectors > 0
+
+        override val value: S get() = state.value
+        override val replayCache: List<S> get() = state.replayCache
+
+        override suspend fun collect(collector: FlowCollector<S>): Nothing {
+            collectors++
+            stopping?.cancel()
+            stopping = null
+            if (loading?.isActive != true) {
+                loading = scope.launch { load(key).collect { state.value = it } }
+            }
+            try {
+                state.collect(collector)
+            } finally {
+                if (--collectors == 0) {
+                    stopping = scope.launch {
+                        delay(STOP_TIMEOUT_MILLIS)
+                        loading?.cancel()
+                        loading = null
+                    }
+                }
             }
         }
-        return entries.getOrPut(key) {
-            val job = SupervisorJob(scope.coroutineContext[Job])
-            Entry(
-                state = load(key).stateWhileSubscribed(scope + job, initial),
-                job = job,
-            )
-        }.state
+    }
+
+    private companion object {
+        /** Long enough to ride out a configuration change, as `stateWhileSubscribed` does. */
+        const val STOP_TIMEOUT_MILLIS = 5_000L
     }
 }

app/src/test/java/de/jeanlucmakiola/calendula/ui/common/PageStateCacheTest.kt

@@ -5,7 +5,9 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.awaitCancellation
 import kotlinx.coroutines.flow.flow
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.UnconfinedTestDispatcher
+import kotlinx.coroutines.test.advanceTimeBy
 import kotlinx.coroutines.test.runTest
 import org.junit.jupiter.api.Test
 import kotlin.math.abs
@@ -13,16 +15,10 @@ import kotlin.math.abs
 @OptIn(ExperimentalCoroutinesApi::class)
 class PageStateCacheTest {
 
-    @Test
-    fun `the same key shares one state`() = runTest {
-        val cache = PageStateCache<Int, Int>(backgroundScope, 0, { a, b -> abs(a - b) <= 1 }) { flow { emit(it) } }
-        assertThat(cache.get(5)).isSameInstanceAs(cache.get(5))
-    }
+    private var running = 0
 
-    @Test
-    fun `a dropped entry stops loading`() = runTest(UnconfinedTestDispatcher()) {
-        var running = 0
-        val cache = PageStateCache<Int, Int>(backgroundScope, 0, { a, b -> abs(a - b) <= 1 }) { key ->
+    private fun TestScope.trackingCache() =
+        PageStateCache<Int, Int>(backgroundScope, -1, { a, b -> abs(a - b) <= 1 }) { key ->
             flow {
                 running++
                 try {
@@ -33,13 +29,11 @@ class PageStateCacheTest {
                 }
             }
         }
-        backgroundScope.launch { cache.get(0).collect {} }
-        assertThat(running).isEqualTo(1)
 
-        // Far enough away that 0 is let go, even though it is still collected.
-        cache.get(10)
-        assertThat(running).isEqualTo(0)
-        assertThat(cache.size).isEqualTo(1)
+    @Test
+    fun `the same key shares one state`() = runTest {
+        val cache = trackingCache()
+        assertThat(cache.get(5)).isSameInstanceAs(cache.get(5))
     }
 
     @Test
@@ -50,4 +44,63 @@ class PageStateCacheTest {
         cache.get(6)
         assertThat(cache.size).isEqualTo(3)
     }
+
+    @Test
+    fun `an entry loads only while collected`() = runTest(UnconfinedTestDispatcher()) {
+        val cache = trackingCache()
+        val state = cache.get(0)
+        assertThat(running).isEqualTo(0)
+
+        val page = backgroundScope.launch { state.collect {} }
+        assertThat(running).isEqualTo(1)
+        assertThat(state.value).isEqualTo(0)
+
+        page.cancel()
+        advanceTimeBy(5_001)
+        assertThat(running).isEqualTo(0)
+    }
+
+    @Test
+    fun `an unwatched entry far away is forgotten`() = runTest(UnconfinedTestDispatcher()) {
+        val cache = trackingCache()
+        cache.get(0)
+        cache.get(10)
+        assertThat(cache.size).isEqualTo(1)
+    }
+
+    @Test
+    fun `an entry a page still collects is kept however far away`() = runTest(UnconfinedTestDispatcher()) {
+        val cache = trackingCache()
+        backgroundScope.launch { cache.get(0).collect {} }
+
+        cache.get(10)
+        assertThat(running).isEqualTo(1)
+        assertThat(cache.size).isEqualTo(2)
+    }
+
+    @Test
+    fun `a forgotten entry a page holds still loads once collected`() = runTest(UnconfinedTestDispatcher()) {
+        val cache = trackingCache()
+        // The page took its state but hasn't started collecting when 0 is let go.
+        val held = cache.get(0)
+        cache.get(10)
+
+        val values = mutableListOf<Int>()
+        backgroundScope.launch { held.collect { values += it } }
+        assertThat(running).isEqualTo(1)
+        assertThat(values.last()).isEqualTo(0)
+    }
+
+    @Test
+    fun `an entry restarts after the screen comes back`() = runTest(UnconfinedTestDispatcher()) {
+        val cache = trackingCache()
+        val held = cache.get(0)
+        backgroundScope.launch { held.collect {} }.cancel()
+        advanceTimeBy(5_001)
+        cache.get(10)
+        assertThat(running).isEqualTo(0)
+
+        backgroundScope.launch { held.collect {} }
+        assertThat(running).isEqualTo(1)
+    }
 }