Re-query after own writes and on return to foreground (#364) (#370)

Views now re-query after the app's own writes and whenever the app comes back to the foreground, instead of relying only on the provider's change notification.

On the reporter's Samsung (Android 12, "My Calendar") edits and deletes went through, but no view refreshed until a restart. Tapping today in the agenda, which starts a fresh query, showed the correct data, so the provider had the change and the notification never reached us.

- `CalendarRepositoryImpl`: every write goes through a `write { }` helper that triggers a re-query when it finishes, also when the write threw.
- New `CalendarRepository.refresh()`, called from `MainActivity.onStart()`.
- Change ticks use `DROP_OLDEST`, so a tick is never dropped while a subscriber holds the previous one.
- The instances query filters `Events.DELETED = 0`, for providers that soft-delete and keep the instances around.
- `docs/ARCHITECTURE.md`: the observer principle mentions the extra re-queries.

The issue reads like a save/delete bug, but the writes themselves work. The fix targets the refresh, which is what was broken. The agenda opening on the 1st of the month, also mentioned in the thread, is the existing focus behaviour and isn't changed here.

Closes #364

Co-authored-by: Jean-Luc Makiola <business@jeanlucmakiola.de>
Reviewed-on: https://codeberg.org/jlmakiola/calendula/pulls/370

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.

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()

app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt

@@ -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
 
     /**

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<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)
     }
 

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

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