From d1e9acad209c111500d2d30b7876869011dfd0bd Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Tue, 6 Oct 2026 17:40:14 +0200 Subject: [PATCH] fix(calendar): re-query after own writes and on return to foreground (#364) Some providers don't notify the observer, which left every view stale until a restart. Also filters soft-deleted rows out of the instances query and stops dropping change ticks. --- .../jeanlucmakiola/calendula/MainActivity.kt | 10 ++++ .../data/calendar/CalendarDataSource.kt | 7 ++- .../data/calendar/CalendarRepository.kt | 3 ++ .../data/calendar/CalendarRepositoryImpl.kt | 51 ++++++++++++------- .../calendar/CalendarRepositoryImplTest.kt | 39 ++++++++++++++ docs/ARCHITECTURE.md | 6 ++- 6 files changed, 95 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/MainActivity.kt b/app/src/main/java/de/jeanlucmakiola/calendula/MainActivity.kt index 347b9101..218feb3a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/MainActivity.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/MainActivity.kt @@ -26,6 +26,7 @@ import androidx.core.net.toUri import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import dagger.hilt.android.AndroidEntryPoint +import de.jeanlucmakiola.calendula.data.calendar.CalendarRepository import de.jeanlucmakiola.calendula.data.prefs.ThemeMode import de.jeanlucmakiola.calendula.data.prefs.is24Hour import de.jeanlucmakiola.calendula.domain.EventForm @@ -56,6 +57,7 @@ import kotlinx.datetime.TimeZone import kotlinx.datetime.toLocalDateTime import kotlin.time.Clock import kotlin.time.Instant +import javax.inject.Inject /** A prefilled create form from an external launch, with the source it came from. */ private data class InsertRequest(val form: EventForm, val source: ImportSource) @@ -63,6 +65,8 @@ private data class InsertRequest(val form: EventForm, val source: ImportSource) @AndroidEntryPoint class MainActivity : AppCompatActivity() { + @Inject lateinit var calendarRepository: CalendarRepository + // Which of light/dark the system bars are drawn for. The styles installed // in onCreate read this field live, so androidx's config-change replay // picks up the in-app override instead of the night resource qualifier. @@ -245,6 +249,12 @@ class MainActivity : AppCompatActivity() { ) } + override fun onStart() { + super.onStart() + // Changes made while we were away may never have notified us (#364). + calendarRepository.refresh() + } + override fun onResume() { super.onResume() // Reaching a running UI means startup succeeded; reset the loop trail. diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt index 6f873afe..b5f3f5cf 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt @@ -610,8 +610,11 @@ class AndroidCalendarDataSource @Inject constructor( // cancelled exception for the one instance (#47). A NULL status is a // normal, un-cancelled event, so it must survive the filter — a bare // `!= CANCELED` would drop it (NULL != 2 is NULL, not true). - "${CalendarContract.Instances.STATUS} IS NULL OR " + - "${CalendarContract.Instances.STATUS} != ${CalendarContract.Events.STATUS_CANCELED}", + // DELETED = 0: a provider that soft-deletes may keep the row's + // instances until a purge (#364). + "(${CalendarContract.Instances.STATUS} IS NULL OR " + + "${CalendarContract.Instances.STATUS} != ${CalendarContract.Events.STATUS_CANCELED}) AND " + + "${CalendarContract.Events.DELETED} = 0", null, CalendarContract.Instances.BEGIN + " ASC", )?.use { c -> c.mapAllNotNull { CursorColumnReader(c).toEventInstance() } } ?: emptyList() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt index ac33e137..e9236723 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt @@ -15,6 +15,9 @@ import kotlin.time.Instant interface CalendarRepository { fun calendars(): Flow> fun instances(range: ClosedRange): Flow> + + /** Re-query every open [calendars] / [instances] flow. */ + fun refresh() suspend fun eventDetail(eventId: Long): EventDetail /** 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 e02bc1de..3c59714f 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 @@ -14,6 +14,7 @@ import de.jeanlucmakiola.calendula.domain.SearchCandidate import de.jeanlucmakiola.calendula.domain.ics.IcsImportSummary import de.jeanlucmakiola.calendula.domain.ics.ParsedIcsEvent import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.channels.BufferOverflow import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.combine @@ -51,21 +52,37 @@ class CalendarRepositoryImpl @Inject constructor( private suspend fun allDayReminderTimeMinutes(): Int = settingsPrefs.allDayReminderTimeMinutes.first() + // DROP_OLDEST: tryEmit never fails, so a tick that lands while a subscriber + // still holds the previous one is never lost (#364). private val ticks = MutableSharedFlow( replay = 0, extraBufferCapacity = 1, + onBufferOverflow = BufferOverflow.DROP_OLDEST, ) /** - * Bumped on every provider notification, so one tick's calendar read can be + * Bumped on every [refresh], so one tick's calendar read can be * shared by everything that needs it (see [calendarsSnapshot]). */ private val generation = AtomicLong(0L) init { - dataSource.registerChangeListener { - generation.incrementAndGet() - ticks.tryEmit(Unit) + dataSource.registerChangeListener(::refresh) + } + + // Writes re-query on their own too: some providers never notify the + // observer, which left every view stale until a restart (#364). + override fun refresh() { + generation.incrementAndGet() + ticks.tryEmit(Unit) + } + + /** Run a provider write on [io], then re-query whether or not it threw. */ + private suspend fun write(block: suspend () -> T): T = withContext(io) { + try { + block() + } finally { + refresh() } } @@ -181,7 +198,7 @@ class CalendarRepositoryImpl @Inject constructor( displayName: String, color: Int, description: String?, - ): Long = withContext(io) { + ): Long = write { dataSource.createLocalCalendar(displayName, color, description) } @@ -190,13 +207,13 @@ class CalendarRepositoryImpl @Inject constructor( displayName: String, color: Int, description: String?, - ) = withContext(io) { dataSource.updateCalendar(id, displayName, color, description) } + ) = write { dataSource.updateCalendar(id, displayName, color, description) } override suspend fun deleteCalendar(id: Long) = - withContext(io) { dataSource.deleteCalendar(id) } + write { dataSource.deleteCalendar(id) } override suspend fun setCalendarsVisible(ids: Collection, visible: Boolean) = - withContext(io) { + write { if (dataSource.canWriteCalendars()) { ids.forEach { dataSource.setCalendarVisible(it, visible) } // Nothing of ours is left waiting for the provider once the @@ -217,7 +234,7 @@ class CalendarRepositoryImpl @Inject constructor( override suspend fun importEvents( targetCalendarId: Long, events: List, - ): IcsImportSummary = withContext(io) { + ): IcsImportSummary = write { val existing = dataSource.existingUids(targetCalendarId) // Both are per-calendar, not per-event: looking them up once keeps a // thousand-event restore to two extra queries. The palette is the @@ -268,7 +285,7 @@ class CalendarRepositoryImpl @Inject constructor( ) } - override suspend fun createEvent(form: EventForm): Long = withContext(io) { + override suspend fun createEvent(form: EventForm): Long = write { dataSource.insertEvent(form, allDayReminderTimeMinutes()) } @@ -276,11 +293,11 @@ class CalendarRepositoryImpl @Inject constructor( eventId: Long, original: EventForm, updated: EventForm, - ) = withContext(io) { + ) = write { dataSource.updateEvent(eventId, original, updated, allDayReminderTimeMinutes()) } - override suspend fun deleteEvent(eventId: Long) = withContext(io) { + override suspend fun deleteEvent(eventId: Long) = write { dataSource.deleteEvent(eventId) } @@ -289,7 +306,7 @@ class CalendarRepositoryImpl @Inject constructor( targetCalendarId: Long, original: EventForm, updated: EventForm, - ): Long = withContext(io) { + ): Long = write { dataSource.moveEvent( eventId, targetCalendarId, original, updated, allDayReminderTimeMinutes(), ) @@ -300,7 +317,7 @@ class CalendarRepositoryImpl @Inject constructor( beginMillis: Long, original: EventForm, form: EventForm, - ): Long = withContext(io) { + ): Long = write { dataSource.updateOccurrence( eventId, beginMillis, original, form, allDayReminderTimeMinutes(), ) @@ -311,20 +328,20 @@ class CalendarRepositoryImpl @Inject constructor( beginMillis: Long, original: EventForm, updated: EventForm, - ): Long = withContext(io) { + ): Long = write { dataSource.updateEventFromOccurrence( eventId, beginMillis, original, updated, allDayReminderTimeMinutes(), ) } - override suspend fun deleteOccurrence(eventId: Long, beginMillis: Long) = withContext(io) { + override suspend fun deleteOccurrence(eventId: Long, beginMillis: Long) = write { dataSource.deleteOccurrence(eventId, beginMillis) } override suspend fun deleteEventFromOccurrence( eventId: Long, beginMillis: Long, - ) = withContext(io) { + ) = write { dataSource.deleteEventFromOccurrence(eventId, beginMillis) } 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 f67f6745..076d7167 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 @@ -95,6 +95,45 @@ class CalendarRepositoryImplTest { } } + @Test + fun `instances re-emit after a write the provider never notified about`(@TempDir tempDir: Path) = runTest { + var current = listOf(makeEvent(10L)) + val fake = FakeCalendarDataSource().apply { instancesResult = { _, _ -> current } } + val repo = CalendarRepositoryImpl(fake, newPrefs(tempDir), newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + + val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) + repo.instances(range).test { + assertThat(awaitItem().map { it.eventId }).containsExactly(10L) + + current = emptyList() + repo.deleteEvent(10L) + + assertThat(awaitItem()).isEmpty() + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `instances re-emit after a write that failed`(@TempDir tempDir: Path) = runTest { + var current = listOf(makeEvent(10L)) + val fake = FakeCalendarDataSource().apply { + instancesResult = { _, _ -> current } + writeError = WriteFailedException("boom") + } + val repo = CalendarRepositoryImpl(fake, newPrefs(tempDir), newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + + val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) + repo.instances(range).test { + awaitItem() + + current = emptyList() + assertThrows { repo.deleteOccurrence(10L, 1_000L) } + + assertThat(awaitItem()).isEmpty() + cancelAndIgnoreRemainingEvents() + } + } + @Test fun `instances forwards epoch-millis bounds to data source`(@TempDir tempDir: Path) = runTest { var observedBegin: Long? = null diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f444241a..f5d31dcd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -12,8 +12,10 @@ the package list (recurring writes, save conflicts, reminder delivery). straight back to it. Sync is DAVx5's / Google's / the system's job. 2. **Observer-driven UI.** A `ContentObserver` on the provider triggers re-queries; every screen recomposes from fresh provider state. After a - write, nothing is patched by hand — the provider notifies, the views - refresh. This also covers external changes (sync) for free. + write, nothing is patched by hand — the views refresh from the provider. + This also covers external changes (sync) for free. Some providers don't + reliably notify (#364), so the repository also re-queries after each of + its own writes, and `MainActivity` does on every return to the foreground. 3. **JVM-first testing.** Everything between the UI and the `ContentResolver` is shaped so it runs as a plain JUnit 5 test: pure domain logic, cursor-free mappers, a `FakeCalendarDataSource` for