From 836817cd122178557ab418d383e23ad14a78a416 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Tue, 6 Oct 2026 19:17:24 +0200 Subject: [PATCH] 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 Reviewed-on: https://codeberg.org/jlmakiola/calendula/pulls/370 --- .../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