diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/PageStateCache.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/PageStateCache.kt index 5e92a83..9ddaa92 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/common/PageStateCache.kt +++ b/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( private val keep: (cached: K, requested: K) -> Boolean, private val load: (K) -> Flow, ) { - private class Entry(val state: StateFlow, val job: Job) - - private val entries = HashMap>() + private val entries = HashMap() /** The number of entries held, for tests. */ internal val size: Int get() = entries.size fun get(key: K): StateFlow { - 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 { + 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 get() = state.replayCache + + override suspend fun collect(collector: FlowCollector): 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 } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/ui/common/PageStateCacheTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/ui/common/PageStateCacheTest.kt index 1266a71..be52eeb 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/ui/common/PageStateCacheTest.kt +++ b/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(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(backgroundScope, 0, { a, b -> abs(a - b) <= 1 }) { key -> + private fun TestScope.trackingCache() = + PageStateCache(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() + 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) + } }