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.
2026-10-06 17:40:14 +02:00
parent 4342c6ab6d
commit d1e9acad20
6 changed files with 95 additions and 21 deletions
@@ -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.
@@ -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()
@@ -15,6 +15,9 @@ import kotlin.time.Instant
 interface CalendarRepository {
    fun calendars(): Flow<List<CalendarSource>>
    fun instances(range: ClosedRange<Instant>): Flow<List<EventInstance>>
+
+    /** Re-query every open [calendars] / [instances] flow. */
+    fun refresh()
    suspend fun eventDetail(eventId: Long): EventDetail

    /**
@@ -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<Unit>(
        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 <T> 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<Long>, 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<ParsedIcsEvent>,
-    ): 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)
    }

@@ -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<WriteFailedException> { 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
@@ -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