diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt index 0b4dd9d..19dcb5a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt @@ -16,9 +16,12 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.drop import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.merge import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -63,21 +66,38 @@ class CalendarRepositoryImpl @Inject constructor( } } + /** + * Re-query signal for everything filtered by visibility: the provider's own + * notifications, plus every change to the pending switch-off set (an id + * leaves it as its `VISIBLE` write lands, which changes what is shown). + * [calendarsSnapshot] keeps the two in step. + */ + private fun visibilityTicks(): Flow = merge( + ticks.onStart { emit(Unit) }, + // The current value is already covered by the tick above; only later + // changes re-query (the set is deduped, so an unrelated DataStore write + // doesn't). + prefs.pendingDisabledCalendarIds.drop(1).map {}, + ) + // A switch-off the app hasn't been allowed to write yet is folded into the // flag itself, so every consumer — the Settings switch, the filter sheet, // the form and import pickers, the widgets — reads one visibility and can't // disagree with what the user just tapped. The reconciler reads the data // source directly, because it needs the provider's own answer. override fun calendars(): Flow> = - combine( - ticks.onStart { emit(Unit) }.reQuery { calendarsSnapshot() }, - prefs.pendingDisabledCalendarIds, - ) { calendars, pendingDisabled -> + visibilityTicks().reQuery { + val calendars = calendarsSnapshot() + val pendingDisabled = prefs.pendingDisabledCalendarIds.first() if (pendingDisabled.isEmpty()) calendars else calendars.map { if (it.id in pendingDisabled) it.copy(isVisibleInSystem = false) else it } - }.flowOn(io) + } + // Collapse re-emissions that carry an identical list (see + // [instances]). + .distinctUntilChanged() + .flowOn(io) // Instances are filtered by the system's per-calendar VISIBLE flag ∪ the // switch-offs still waiting to be written to it ∪ the app-side hidden set: @@ -89,23 +109,21 @@ class CalendarRepositoryImpl @Inject constructor( // and re-enable invisible calendars. override fun instances(range: ClosedRange): Flow> = combine( - ticks - .onStart { emit(Unit) } - .reQuery { - // Both reads in one pass, so a list of instances is never - // filtered against a visibility snapshot from another tick. - QueriedInstances( - instances = dataSource.instances( - beginMillis = range.start.toEpochMillis(), - endMillis = range.endInclusive.toEpochMillis(), - ), - invisibleCalendarIds = invisibleCalendarIds(), - ) - }, + visibilityTicks().reQuery { + // All three reads in one pass, so a list of instances is never + // filtered against a visibility snapshot from another tick. + QueriedInstances( + instances = dataSource.instances( + beginMillis = range.start.toEpochMillis(), + endMillis = range.endInclusive.toEpochMillis(), + ), + switchedOffCalendarIds = invisibleCalendarIds() + + prefs.pendingDisabledCalendarIds.first(), + ) + }, prefs.hiddenCalendarIds, - prefs.pendingDisabledCalendarIds, - ) { queried, hidden, pendingDisabled -> - val excluded = hidden + pendingDisabled + queried.invisibleCalendarIds + ) { queried, hidden -> + val excluded = hidden + queried.switchedOffCalendarIds if (excluded.isEmpty()) queried.instances else queried.instances.filterNot { it.calendarId in excluded } } @@ -119,7 +137,7 @@ class CalendarRepositoryImpl @Inject constructor( /** One instances query plus the visibility it must be filtered against. */ private data class QueriedInstances( val instances: List, - val invisibleCalendarIds: Set, + val switchedOffCalendarIds: Set, ) /** Calendars switched off at system level — hidden, and never reminded about. */ @@ -129,6 +147,7 @@ class CalendarRepositoryImpl @Inject constructor( private val calendarsLock = Mutex() private var cachedGeneration = -1L + private var cachedPending: Set? = null private var cachedCalendars: List = emptyList() /** @@ -140,12 +159,21 @@ class CalendarRepositoryImpl @Inject constructor( * * An empty result is never cached — it is what a read without the calendar * permission returns, and the grant itself doesn't notify the provider. + * + * The pending switch-off set keys the cache alongside the tick. An id leaves + * that set the moment its `VISIBLE` write lands, while the observer that + * would invalidate the snapshot is only dispatched through the main looper + * afterwards — so a snapshot taken while the id was still pending, read + * against the set that no longer holds it, would report the calendar as *on* + * again and re-admit exactly the events being hidden. */ private suspend fun calendarsSnapshot(): List = calendarsLock.withLock { val current = generation.get() - if (current != cachedGeneration || cachedCalendars.isEmpty()) { + val pending = prefs.pendingDisabledCalendarIds.first() + if (current != cachedGeneration || pending != cachedPending || cachedCalendars.isEmpty()) { cachedCalendars = dataSource.calendars() cachedGeneration = current + cachedPending = pending } cachedCalendars } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt index f96d11f..e615803 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt @@ -8,6 +8,7 @@ import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.map import javax.inject.Inject import javax.inject.Singleton @@ -28,9 +29,13 @@ class CalendarPrefs @Inject constructor( private val store: DataStore, ) { - val hiddenCalendarIds: Flow> = store.data.map { prefs -> - prefs[HIDDEN_IDS_KEY].parseIds() - } + // Both id sets are deduped: the store is shared with SettingsPrefs, so every + // unrelated write (a settings toggle, the last-used calendar) re-emits an + // identical set otherwise — and a change to the pending set now costs a + // fresh provider read in CalendarRepositoryImpl. + val hiddenCalendarIds: Flow> = store.data + .map { prefs -> prefs[HIDDEN_IDS_KEY].parseIds() } + .distinctUntilChanged() suspend fun setHiddenCalendarIds(ids: Set) { store.edit { prefs -> prefs.writeIds(HIDDEN_IDS_KEY, ids) } @@ -49,9 +54,9 @@ class CalendarPrefs @Inject constructor( * provider entry by entry the moment the app may write, and nothing ever * adds to it while it may. */ - val pendingDisabledCalendarIds: Flow> = store.data.map { prefs -> - prefs[DISABLED_IDS_KEY].parseIds() - } + val pendingDisabledCalendarIds: Flow> = store.data + .map { prefs -> prefs[DISABLED_IDS_KEY].parseIds() } + .distinctUntilChanged() suspend fun addPendingDisabledCalendarIds(ids: Collection) = editPendingDisabled { it + ids } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt index 324f092..9baa38f 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt @@ -376,6 +376,8 @@ class CalendarRepositoryImplTest { } // The next tick invalidates it — one fresh read, not one per flow. + // (The list has to change: an identical one is collapsed.) + fake.calendarsResult = listOf(makeCal(1L), makeCal(2L), makeCal(3L)) fake.tick() awaitItem() assertThat(fake.calendarQueries).isEqualTo(2) @@ -383,6 +385,64 @@ class CalendarRepositoryImplTest { } } + @Test + fun `calendars does not re-emit an unchanged list`(@TempDir tempDir: Path) = runTest { + // The store is shared with SettingsPrefs, so an unrelated write would + // otherwise re-run every view's combine for an identical list. + val prefs = newPrefs(tempDir) + val fake = FakeCalendarDataSource().apply { calendarsResult = listOf(makeCal(1L)) } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + + repo.calendars().test { + assertThat(awaitItem().map { it.id }).containsExactly(1L) + + prefs.setLastUsedCalendarId(1L) + fake.tick() + + expectNoEvents() + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `a flushed switch-off never reads as on again before the provider ticks`( + @TempDir tempDir: Path, + ) = runTest { + // The reconciler's shape: write VISIBLE = 0 straight to the provider, + // then release the id app-side. The provider's notification only arrives + // afterwards (it is dispatched through the main looper), so the release + // must not be read against the snapshot from before the write — that + // would flash exactly the events being hidden back into every view. + val prefs = newPrefs(tempDir) + prefs.addPendingDisabledCalendarIds(setOf(2L)) + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L)) + instancesResult = { _, _ -> + listOf(makeEvent(10L, "A", calendarId = 1L), makeEvent(11L, "B", calendarId = 2L)) + } + } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) + + // Warm the snapshot the way an open view would. + repo.instances(range).test { + assertThat(awaitItem().map { it.title }).containsExactly("A") + cancelAndIgnoreRemainingEvents() + } + + fake.setCalendarVisible(2L, false) // no tick(): the observer hasn't fired yet + prefs.removePendingDisabledCalendarIds(setOf(2L)) + + repo.instances(range).test { + assertThat(awaitItem().map { it.title }).containsExactly("A") + cancelAndIgnoreRemainingEvents() + } + repo.calendars().test { + assertThat(awaitItem().single { it.id == 2L }.isVisibleInSystem).isFalse() + cancelAndIgnoreRemainingEvents() + } + } + @Test fun `searchEvents drops results from calendars that are off or hidden`( @TempDir tempDir: Path,