From a1d1894f84e6fa3fb8433cab1c3cd2ed1829cefa Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 27 Aug 2026 19:57:35 +0200 Subject: [PATCH] fix: detach the occurrence when editing one instance of an unsynced series (#234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Edit only this event" wrote a modified-occurrence exception unconditionally. That is the right shape for a synced series and the wrong one for everything else: an exception attaches to its parent through ORIGINAL_SYNC_ID, so on a row with no _sync_id the link never forms — the insert fails or lands an orphan, the generic catch in EventEditViewModel turns it into a snackbar, and the scope dialog just closes again. To the reporter that read as "nothing happens, ever", with a stray copy of the event left behind the one time the insert did land. This is the same constraint deleteOccurrence has documented since #47, and the same calendars: a local calendar, Calendula's own contact special-date calendars, and — the reporter's case — a Google calendar whose rows the sync adapter has not stamped yet, which is exactly why it "shows as on-device". updateOccurrence now branches on _sync_id the way deleteOccurrence does. A synced series keeps the exception path untouched; its write shape is load- bearing and verified. A series without one gets the occurrence excluded from the parent via EXDATE and the edited values inserted as a standalone event on the same calendar — a detached instance, minus the RECURRENCE-ID the provider has no way to store here. The cost is honest and worth naming: the edited occurrence stops travelling with its series. The alternative is an edit that silently does nothing. Two things shape the write. The parent update reuses buildOccurrenceExdateValues unchanged, so it keeps carrying the whole time/recurrence set — an EXDATE-only update is not a recurrence change to the provider and leaves the expanded instances standing (#47's first quirk). And the form's RRULE is stripped before the insert: the exception path gets an inherited rule cleared for free by DTSTART + DURATION, but nothing clears one here, so leaving it would insert a second *series* overlapping the first. Ordering is chosen for the failure cases, not the happy path. The insert runs first, so a failure there leaves the series completely untouched — the discipline updateEventFromOccurrence already follows. If the EXDATE update then fails, the new row is a visible duplicate of an occurrence still in the series, so it is rolled back (best effort) before the failure surfaces. The reverse order could strand an occurrence excluded from its series with nothing standing in for it, turning an edit into a silent delete. Also makes a failed save legible, since this bug was invisible precisely because it wasn't: the failure snackbar gets the long duration instead of a flash, and the catch logs the scope and event id — never the form's content — so a failure leaves something to report. Adds JVM tests for the detached shape: the rule is dropped and the row becomes a one-off with DTEND, all-day stays on UTC midnights, and the detached row's DTSTART agrees with the EXDATE stamp that removes it from the parent, timed and all-day. The provider behaviour itself still needs a device. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 13 ++ .../data/calendar/CalendarDataSource.kt | 99 +++++++++++++- .../data/calendar/EventWriteMapper.kt | 20 +++ .../calendula/ui/edit/EventEditScreen.kt | 9 +- .../calendula/ui/edit/EventEditViewModel.kt | 7 + .../data/calendar/EventWriteMapperTest.kt | 124 ++++++++++++++++++ 6 files changed, 266 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5435a0a..7735c23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **"Only this event" now actually saves your edit.** On some calendars — + including Google ones that still show as on-device, and any local calendar — + editing a single occurrence of a repeating event did nothing at all: the scope + dialog closed, the edit screen stayed put, and saving again just repeated it. + Android can only attach a single-occurrence change to its series once the + calendar has been synced at least once, so on those calendars the change had + nowhere to go. Calendula now removes that one occurrence from the series and + saves the edit as its own event instead, which is what you see either way. A + save that does fail also says so for longer, rather than flashing past + ([#234]). + ## [2.19.3] — 2026-08-22 ### Added @@ -1471,3 +1483,4 @@ automatically, with zero telemetry and no internet permission. [#192]: https://codeberg.org/jlmakiola/calendula/issues/192 [#196]: https://codeberg.org/jlmakiola/calendula/issues/196 [#214]: https://codeberg.org/jlmakiola/calendula/issues/214 +[#234]: https://codeberg.org/jlmakiola/calendula/issues/234 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 1b8507f..3792608 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 @@ -232,10 +232,15 @@ interface CalendarDataSource { ): Long /** - * Change a single occurrence of a recurring event by inserting a - * modified-occurrence exception at [beginMillis] (the occurrence's - * `Instances.BEGIN`) carrying [form]'s values; returns the exception - * row's `Events._ID`. [allDayReminderTimeMinutes]: see [insertEvent]. + * Change a single occurrence of a recurring event at [beginMillis] (the + * occurrence's `Instances.BEGIN`) to [form]'s values; returns the + * `Events._ID` of the row now holding them. + * + * A series with a `_sync_id` gets a modified-occurrence exception. One + * without gets the occurrence excluded from the parent via EXDATE plus a + * standalone event carrying the edits — an exception cannot link to its + * parent there (Codeberg #234, the same constraint as [deleteOccurrence]). + * [allDayReminderTimeMinutes]: see [insertEvent]. */ fun updateOccurrence( eventId: Long, @@ -1194,6 +1199,10 @@ class AndroidCalendarDataSource @Inject constructor( form: EventForm, allDayReminderTimeMinutes: Int, ): Long { + val row = querySeriesRow(eventId) + if (row.syncId == null && !row.rrule.isNullOrBlank()) { + return detachOccurrence(eventId, beginMillis, row, form, allDayReminderTimeMinutes) + } // The provider clones the series row and applies these values on top. val values = buildOccurrenceExceptionValues( form = form, @@ -1212,6 +1221,88 @@ class AndroidCalendarDataSource @Inject constructor( return exceptionId } + /** + * "Edit only this event" on a series with **no `_sync_id`**: drop the + * occurrence from the parent with EXDATE and insert the edited values as a + * standalone event on the same calendar. + * + * A modified exception attaches to its parent only through `ORIGINAL_SYNC_ID`, + * exactly like the cancelled one [deleteOccurrence] documents. With no + * `_sync_id` the link never forms: the insert either fails outright or lands + * an orphan row, and the parent's expansion collapses — which is why editing + * one occurrence of such a series did nothing at all, and occasionally left a + * stray copy behind (Codeberg #234). The reporter's calendar was a Google one + * that "shows as on-device", i.e. rows the sync adapter had not stamped yet; + * Calendula's own local and contact special-date calendars are permanently in + * this shape. + * + * EXDATE + a standalone row needs no parent link, and is what a detached + * instance degrades to when there is no `RECURRENCE-ID` to carry it: the user + * sees one edited event where the occurrence was, and the rest of the series + * untouched. It does mean the edited occurrence stops travelling with the + * series — moving the series later won't move it — which is the honest cost of + * a link the provider cannot store here. + * + * Order is deliberate. The insert goes first, so a failure there leaves the + * series completely untouched (the same discipline as + * [updateEventFromOccurrence]). If the EXDATE update then fails, the new row + * is a visible duplicate of an occurrence that is still in the series, so it + * is rolled back before the failure surfaces — better a save the user can + * retry than a silent duplicate. The reverse order would risk the opposite: + * an occurrence excluded from the series with no replacement, i.e. an edit + * that quietly deletes. + */ + private fun detachOccurrence( + eventId: Long, + beginMillis: Long, + row: SeriesRow, + form: EventForm, + allDayReminderTimeMinutes: Int, + ): Long { + // Carries the form's reminders, guests and colour like any new event — + // and a fresh UID, because the detached row really is a separate event + // now: sharing the parent's would collide with it in .ics restore dedup. + val detachedId = insertEvent(form.toDetachedOccurrence(), allDayReminderTimeMinutes) + val values = buildOccurrenceExdateValues( + existingExdate = row.exdate, + occurrenceMillis = beginMillis, + dtStartMillis = row.dtStartMillis, + rrule = row.rrule, + duration = row.duration, + timezone = row.timezone, + allDay = row.allDay, + ) + val excluded = try { + resolver.update( + ContentUris.withAppendedId(CalendarContract.Events.CONTENT_URI, eventId), + values.toContentValues(), null, null, + ) + } catch (e: RuntimeException) { + rollBackDetached(detachedId) + throw e + } + if (excluded == 0) { + rollBackDetached(detachedId) + throw WriteFailedException( + "exdate occurrence for edit, event id=$eventId begin=$beginMillis", + ) + } + return detachedId + } + + /** + * Undo the standalone row [detachOccurrence] inserted before its EXDATE + * update failed. Best effort: the caller is already throwing, and a rollback + * that itself fails must not replace the real failure with a confusing one. + * The worst case is the duplicate we were trying to avoid, which the user can + * see and delete — never a lost occurrence. + */ + private fun rollBackDetached(detachedId: Long) { + runCatching { deleteEvent(detachedId) }.onFailure { + Log.w(TAG, "Failed to roll back detached occurrence $detachedId", it) + } + } + override fun updateEventFromOccurrence( eventId: Long, beginMillis: Long, diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt index 8e64db5..483c08e 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapper.kt @@ -243,6 +243,26 @@ internal fun buildOccurrenceExceptionValues( putAll(eventColorColumns(form.colorKey, form.color)) } +/** + * The form as a **detached occurrence**: the same edited values, with the + * series rule dropped so [buildEventInsertValues] writes a standalone one-off + * row (DTSTART + DTEND, no RRULE/DURATION) at the occurrence's own times. + * + * This is the "edit only this event" shape for a series with **no `_sync_id`**, + * where an exception row can't be used at all — see [buildOccurrenceExdateValues] + * for why the parent link never forms. The occurrence is dropped from the parent + * with EXDATE and re-created as its own event, which is what a detached instance + * degrades to without a `RECURRENCE-ID` to carry it. + * + * Dropping the rule mirrors what the exception path gets for free: the provider + * clears the RRULE it cloned from the parent when an exception carries + * DTSTART + DURATION ([buildOccurrenceExceptionValues]). Here nothing is cloned, + * so the rule has to be stripped by hand — leaving it on would insert a second + * *series* overlapping the first, which is the duplication this path exists to + * avoid. + */ +internal fun EventForm.toDetachedOccurrence(): EventForm = copy(rrule = null) + /** * Raw provider snapshot of a master/one-off Events row, enough to re-insert it * verbatim on another calendar (a calendar move is copy+delete — `CALENDAR_ID` diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditScreen.kt index 4f36992..0956a03 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditScreen.kt @@ -64,6 +64,7 @@ import androidx.compose.material3.Scaffold import androidx.compose.material3.SegmentedButton import androidx.compose.material3.SegmentedButtonDefaults import androidx.compose.material3.SingleChoiceSegmentedButtonRow +import androidx.compose.material3.SnackbarDuration import androidx.compose.material3.SnackbarHost import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Surface @@ -264,13 +265,17 @@ fun EventEditScreen( viewModel.reset() onSaved() } + // A failed save leaves the user on a form that looks exactly as it + // did, so the snackbar is the only sign anything happened — it gets + // the long duration rather than the default flash (Codeberg #234: + // the failure read as "nothing happens at all"). SaveUiState.Failed -> { viewModel.consumeSaveResult() - snackbarHostState.showSnackbar(saveFailedMessage) + snackbarHostState.showSnackbar(saveFailedMessage, duration = SnackbarDuration.Long) } SaveUiState.NeedsPermission -> { viewModel.consumeSaveResult() - snackbarHostState.showSnackbar(writeDeniedMessage) + snackbarHostState.showSnackbar(writeDeniedMessage, duration = SnackbarDuration.Long) } // AwaitingScope/AwaitingConflict/Gone render as dialogs below. else -> Unit diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt index 2c3dc61..f184150 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt @@ -1,5 +1,6 @@ package de.jeanlucmakiola.calendula.ui.edit +import android.util.Log import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel @@ -56,6 +57,8 @@ import kotlin.time.Duration.Companion.minutes import kotlin.time.Instant import javax.inject.Inject +private const val TAG = "EventEdit" + /** * Where a prefilled [EventEditViewModel.openImported] form came from. The sources * want different reminder handling (#49), and differ in whether they own the @@ -752,6 +755,10 @@ class EventEditViewModel @Inject constructor( } catch (e: SecurityException) { SaveUiState.NeedsPermission } catch (e: Exception) { + // The user only gets a generic snackbar, so without this a + // failed write leaves no trace at all to report (Codeberg #234). + // Scope and event id only — never the form's content. + Log.w(TAG, "Save failed (scope=$scope, eventId=${target?.eventId})", e) SaveUiState.Failed } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt index e8f4a75..5f8865f 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/EventWriteMapperTest.kt @@ -558,6 +558,130 @@ class EventWriteMapperTest { assertThat(values[CalendarContract.Events.ALL_DAY]).isEqualTo(1) } + // --- toDetachedOccurrence ("edit only this event", no _sync_id) --- + + @Test + fun `a detached occurrence drops the series rule and becomes a one-off row`() { + val edited = form().copy(title = "Moved", rrule = "FREQ=WEEKLY;BYDAY=TH") + val detached = edited.toDetachedOccurrence() + + val values = buildEventInsertValues( + form = detached, + uid = "uid@calendula", + times = detached.toWriteTimes(berlin), + ) + assertThat(values[CalendarContract.Events.TITLE]).isEqualTo("Moved") + // The rule must not survive: without a _sync_id nothing clears an + // inherited RRULE the way the exception path's DTSTART + DURATION does, + // so keeping it would insert a second *series* overlapping the first + // (Codeberg #234's stray duplicate). + assertThat(values).doesNotContainKey(CalendarContract.Events.RRULE) + assertThat(values).doesNotContainKey(CalendarContract.Events.DURATION) + // A one-off row carries DTEND — the invariant buildEventInsertValues + // holds for every non-recurring event. + assertThat(values[CalendarContract.Events.DTSTART]).isEqualTo(1_781_164_800_000L) + assertThat(values[CalendarContract.Events.DTEND]).isEqualTo(1_781_170_200_000L) + assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("Europe/Berlin") + } + + @Test + fun `a detached occurrence keeps everything but the rule`() { + val edited = form(timezone = "America/New_York").copy( + title = "Standup", + location = "Room 2", + description = "notes", + reminders = listOf(10), + availability = Availability.Free, + accessLevel = AccessLevel.Private, + colorKey = "7", + rrule = "FREQ=DAILY", + ) + val detached = edited.toDetachedOccurrence() + + assertThat(detached.rrule).isNull() + assertThat(detached).isEqualTo(edited.copy(rrule = null)) + // Reminders and guests ride along on the form: the insert path seeds them + // from it, so the detached row keeps the reminders the user just edited. + assertThat(detached.reminders).containsExactly(10) + } + + @Test + fun `a detached all-day occurrence stays on UTC midnights`() { + val edited = form( + isAllDay = true, + start = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)), + end = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)), + ).copy(title = "Birthday", rrule = "FREQ=YEARLY") + val detached = edited.toDetachedOccurrence() + + val values = buildEventInsertValues( + form = detached, + uid = "uid@calendula", + times = detached.toWriteTimes(berlin), + ) + assertThat(values[CalendarContract.Events.ALL_DAY]).isEqualTo(1) + assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("UTC") + assertThat(values[CalendarContract.Events.DTSTART]).isEqualTo(1_781_136_000_000L) + // Exclusive DTEND — the next UTC midnight. + assertThat(values[CalendarContract.Events.DTEND]).isEqualTo(1_781_222_400_000L) + assertThat(values).doesNotContainKey(CalendarContract.Events.RRULE) + } + + @Test + fun `the detached row lands exactly where the parent's exdate removes it`() { + // The two halves of the no-_sync_id edit have to agree, or the user sees + // the occurrence twice or not at all. With the time left untouched, the + // EXDATE stamp and the detached row's DTSTART describe the same instant. + val edited = form().copy(title = "Renamed", rrule = "FREQ=WEEKLY") + val occurrenceMillis = 1_781_164_800_000L + + val parent = buildOccurrenceExdateValues( + existingExdate = null, + occurrenceMillis = occurrenceMillis, + dtStartMillis = 1_780_560_000_000L, + rrule = "FREQ=WEEKLY", + duration = "P5400S", + timezone = "Europe/Berlin", + allDay = 0, + ) + val detached = edited.toDetachedOccurrence() + val inserted = buildEventInsertValues( + form = detached, + uid = "uid@calendula", + times = detached.toWriteTimes(berlin), + ) + assertThat(parent[CalendarContract.Events.EXDATE]).isEqualTo("20260611T080000Z") + assertThat(inserted[CalendarContract.Events.DTSTART]).isEqualTo(occurrenceMillis) + // The parent keeps its own anchor and rule — only this occurrence leaves. + assertThat(parent[CalendarContract.Events.DTSTART]).isEqualTo(1_780_560_000_000L) + assertThat(parent[CalendarContract.Events.RRULE]).isEqualTo("FREQ=WEEKLY") + } + + @Test + fun `a detached all-day occurrence matches its date-only exdate stamp`() { + val parent = buildOccurrenceExdateValues( + existingExdate = null, + occurrenceMillis = 1_781_136_000_000L, // 2026-06-11T00:00:00Z + dtStartMillis = 1_749_600_000_000L, + rrule = "FREQ=YEARLY", + duration = "P1D", + timezone = "UTC", + allDay = 1, + ) + val detached = form( + isAllDay = true, + start = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)), + end = LocalDateTime(LocalDate(2026, 6, 11), LocalTime(0, 0)), + ).copy(rrule = "FREQ=YEARLY").toDetachedOccurrence() + val inserted = buildEventInsertValues( + form = detached, + uid = "uid@calendula", + times = detached.toWriteTimes(berlin), + ) + assertThat(parent[CalendarContract.Events.EXDATE]).isEqualTo("20260611") + assertThat(inserted[CalendarContract.Events.DTSTART]).isEqualTo(1_781_136_000_000L) + } + // --- per-event colour --- @Test