makiolaj/calendula
1
0
Fork 1
Code Actions Releases 27 Activity

fix(calendars): don't flash a flushed calendar's events back on

Browse Source 
The reconciler writes VISIBLE=0 and then releases the id from the pending set,
but the ContentObserver that invalidates the repository's cached calendar
snapshot is dispatched through the main looper and arrives later. Until it did,
the released set was read against the snapshot from before the write: the
calendar reported as on again and instances re-admitted exactly the events the
migration was hiding — on a cold start, for as long as the busy main thread took.

The snapshot cache is keyed on the pending set as well as the tick now, so any
read that sees a changed set re-queries the provider; a change to the set also
re-runs the flows, and the pending ids are read in the same pass as the
calendars rather than combined in from a live flow. Both id sets are deduped at
the prefs seam (the store is shared with SettingsPrefs, so every unrelated write
re-emitted them) and calendars() collapses identical lists, which keeps those
re-queries as rare as they should be.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jean-Luc Makiola
2026-07-25 21:34:24 +02:00
parent bb6e3ad336
commit 4ad805e747
3 changed files with 122 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt
View File

@@ -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<Unit> = 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<List<CalendarSource>> =
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<Instant>): Flow<List<EventInstance>> =
combine(
ticks
.onStart { emit(Unit) }
.reQuery {
// Both reads in one pass, so a list of instances is never
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(),
),
invisibleCalendarIds = invisibleCalendarIds(),
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<EventInstance>,
val invisibleCalendarIds: Set<Long>,
val switchedOffCalendarIds: Set<Long>,
)
/** 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<Long>? = null
private var cachedCalendars: List<CalendarSource> = 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<CalendarSource> = 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
}

17
app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt
View File

@@ -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<Preferences>,
) {
val hiddenCalendarIds: Flow<Set<Long>> = 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<Set<Long>> = store.data
.map { prefs -> prefs[HIDDEN_IDS_KEY].parseIds() }
.distinctUntilChanged()
suspend fun setHiddenCalendarIds(ids: Set<Long>) {
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<Set<Long>> = store.data.map { prefs ->
prefs[DISABLED_IDS_KEY].parseIds()
}
val pendingDisabledCalendarIds: Flow<Set<Long>> = store.data
.map { prefs -> prefs[DISABLED_IDS_KEY].parseIds() }
.distinctUntilChanged()
suspend fun addPendingDisabledCalendarIds(ids: Collection<Long>) =
editPendingDisabled { it + ids }

60
app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt
View File

@@ -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,