docs: trim the #234 comments to the load-bearing facts

The detach path's KDoc and inline comments restated the bug narrative that
already lives in the commit message and the issue. Keep what a reader of
the code needs — why the branch exists, what the detached row loses, and
why the insert precedes the EXDATE update — and drop the rest.
This commit is contained in:
2026-08-27 20:21:34 +02:00
parent bc49730ea5
commit c0b7ad29ef
5 changed files with 58 additions and 107 deletions

View File

@@ -1200,12 +1200,9 @@ class AndroidCalendarDataSource @Inject constructor(
allDayReminderTimeMinutes: Int, allDayReminderTimeMinutes: Int,
): Long { ): Long {
val row = querySeriesRow(eventId) val row = querySeriesRow(eventId)
// Deliberately stricter than deleteOccurrence's bare _sync_id check: the // Stricter than deleteOccurrence's bare _sync_id check: EXDATE only means
// detach path drops the occurrence with EXDATE, which only means anything // something on a row that recurs, so a non-recurring one keeps the
// on a row that actually recurs. Without an RRULE there is nothing to // exception path rather than getting a recurrence set written onto it.
// exclude from, so such a row keeps the existing path rather than getting
// a recurrence set written onto a one-off event. The UI only offers the
// scope choice for a recurring event, so neither case is reachable today.
if (row.syncId == null && !row.rrule.isNullOrBlank()) { if (row.syncId == null && !row.rrule.isNullOrBlank()) {
return detachOccurrence(eventId, beginMillis, row, form, allDayReminderTimeMinutes) return detachOccurrence(eventId, beginMillis, row, form, allDayReminderTimeMinutes)
} }
@@ -1233,46 +1230,24 @@ class AndroidCalendarDataSource @Inject constructor(
* standalone event on the same calendar. * standalone event on the same calendar.
* *
* A modified exception attaches to its parent only through `ORIGINAL_SYNC_ID`, * A modified exception attaches to its parent only through `ORIGINAL_SYNC_ID`,
* exactly like the cancelled one [deleteOccurrence] documents. With no * exactly like the cancelled one [deleteOccurrence] documents; with no
* `_sync_id` the link never forms: the insert either fails outright or lands * `_sync_id` the link never forms and the edit is lost (Codeberg #234).
* an orphan row, and the parent's expansion collapses — which is why editing * EXDATE plus a standalone row needs no link — what a detached instance
* one occurrence of such a series did nothing at all, and occasionally left a * degrades to without a `RECURRENCE-ID` to carry it.
* 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 * The detached row keeps no stored link back to its series, so: it no longer
* instance degrades to when there is no `RECURRENCE-ID` to carry it: the user * travels with it ([moveEvent] copies the master and its `ORIGINAL_ID`
* sees one edited event where the occurrence was, and the rest of the series * children, and this is neither); its EXDATE hole is an absolute instant, so
* untouched. * re-timing the whole series brings the occurrence back beside the copy (a
* #47 delete resurrects the same way); and it is built from the form, not
* cloned, so `ORGANIZER`, `STATUS` and the attendee rows [reconcileAttendees]
* preserves are dropped — the same limitation as [moveEvent].
* *
* What that costs, plainly, because none of it is recoverable later — the * Insert first, so a failure leaves the series untouched
* detached row has *no* stored link back to its series, which is the whole * ([updateEventFromOccurrence]'s discipline); roll the new row back if the
* reason this path exists: * EXDATE update then fails, since it would be a visible duplicate. The
* - It stops travelling with the series. Moving the series to another * reverse order risks the worse outcome — an excluded occurrence with no
* calendar leaves it behind ([moveEvent] copies the master and its * replacement, i.e. an edit that quietly deletes.
* `ORIGINAL_ID` children; this is neither), and deleting the whole series
* leaves it standing where an exception row would have gone with the parent.
* - Its EXDATE hole is an absolute instant. Editing the series' *time* for all
* events moves the generated instances but not the hole, so the occurrence
* comes back alongside the detached copy. That staleness predates this path
* — a #47 delete resurrects the same way — but a duplicate is a louder
* symptom than a resurrection, and it wants fixing at the series-update end.
* - It is built from the form, not cloned from the parent, so columns the form
* doesn't model are dropped rather than inherited: `ORGANIZER`, `STATUS`,
* and the organizer/resource attendee rows [reconcileAttendees] otherwise
* preserves. Same limitation as [moveEvent]. Rare on the calendars that
* reach this path, which are locally authored, but not impossible.
*
* 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( private fun detachOccurrence(
eventId: Long, eventId: Long,
@@ -1281,17 +1256,15 @@ class AndroidCalendarDataSource @Inject constructor(
form: EventForm, form: EventForm,
allDayReminderTimeMinutes: Int, allDayReminderTimeMinutes: Int,
): Long { ): Long {
// Already detached (or deleted): the series no longer contains this // Already detached (or deleted) from a stale screen still pointing at the
// occurrence, so there is nothing here to edit. Reached from a stale // parent: the EXDATE merge would fold the repeat away and still report a
// screen still pointing at the parent — without this the EXDATE merge // changed row, quietly leaving a *second* standalone copy.
// would fold the repeat away, the update would still report a changed
// row, and the save would quietly leave a *second* standalone copy.
if (exdateContains(row.exdate, beginMillis, isAllDay = row.allDay != 0)) { if (exdateContains(row.exdate, beginMillis, isAllDay = row.allDay != 0)) {
throw NoSuchEventException(eventId) throw NoSuchEventException(eventId)
} }
// Carries the form's reminders, guests and colour like any new event // Reminders, guests and colour come along like any new event, and so does
// and a fresh UID, because the detached row really is a separate event // a fresh UID the detached row is a separate event now, and sharing the
// now: sharing the parent's would collide with it in .ics restore dedup. // parent's would collide with it in .ics restore dedup.
val detachedId = insertEvent(form.toDetachedOccurrence(), allDayReminderTimeMinutes) val detachedId = insertEvent(form.toDetachedOccurrence(), allDayReminderTimeMinutes)
val values = buildOccurrenceExdateValues( val values = buildOccurrenceExdateValues(
existingExdate = row.exdate, existingExdate = row.exdate,
@@ -1302,10 +1275,9 @@ class AndroidCalendarDataSource @Inject constructor(
timezone = row.timezone, timezone = row.timezone,
allDay = row.allDay, allDay = row.allDay,
) )
// Rows touched, not occurrences excluded: this is 1 whenever the series // Rows touched, not occurrences excluded 1 whenever the series row still
// row still exists. It catches the row disappearing under us, not an // exists. It catches the row disappearing under us, not an EXDATE the
// EXDATE the provider's expansion fails to match — that would report // provider's expansion fails to match.
// success and leave the duplicate. Same rollback idiom as moveEvent.
val updatedRows = try { val updatedRows = try {
resolver.update( resolver.update(
ContentUris.withAppendedId(CalendarContract.Events.CONTENT_URI, eventId), ContentUris.withAppendedId(CalendarContract.Events.CONTENT_URI, eventId),
@@ -1326,10 +1298,8 @@ class AndroidCalendarDataSource @Inject constructor(
/** /**
* Undo the standalone row [detachOccurrence] inserted before its EXDATE * Undo the standalone row [detachOccurrence] inserted before its EXDATE
* update failed. Best effort: the caller is already throwing, and a rollback * update failed. Best effort: the caller is already throwing, and the worst
* that itself fails must not replace the real failure with a confusing one. * case is the duplicate we were avoiding — never a lost occurrence.
* 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) { private fun rollBackDetached(detachedId: Long) {
runCatching { deleteEvent(detachedId) }.onFailure { runCatching { deleteEvent(detachedId) }.onFailure {

View File

@@ -248,18 +248,13 @@ internal fun buildOccurrenceExceptionValues(
* series rule dropped so [buildEventInsertValues] writes a standalone one-off * series rule dropped so [buildEventInsertValues] writes a standalone one-off
* row (DTSTART + DTEND, no RRULE/DURATION) at the occurrence's own times. * 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`**, * The "edit only this event" shape for a series with **no `_sync_id`**, where an
* where an exception row can't be used at all — see [buildOccurrenceExdateValues] * exception row can't attach to its parent at all (Codeberg #234).
* 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 * The exception path gets the rule dropped for free the provider clears the
* clears the RRULE it cloned from the parent when an exception carries * RRULE it cloned when an exception carries DTSTART + DURATION
* DTSTART + DURATION ([buildOccurrenceExceptionValues]). Here nothing is cloned, * ([buildOccurrenceExceptionValues]). Here nothing is cloned, so it is stripped
* so the rule has to be stripped by hand leaving it on would insert a second * by hand; leaving it on would insert a second *series* overlapping the first.
* *series* overlapping the first, which is the duplication this path exists to
* avoid.
*/ */
internal fun EventForm.toDetachedOccurrence(): EventForm = copy(rrule = null) internal fun EventForm.toDetachedOccurrence(): EventForm = copy(rrule = null)
@@ -457,14 +452,11 @@ internal fun buildOccurrenceExdateValues(
/** /**
* Whether [existingExdate] already excludes the occurrence at [occurrenceMillis] * Whether [existingExdate] already excludes the occurrence at [occurrenceMillis]
* — i.e. this occurrence has already been dropped from the series (deleted, or * — i.e. it has already been dropped from the series, deleted or detached.
* detached into its own event).
* *
* Guards the detach path against running twice for the same occurrence, which a * Guards the detach path against running twice from a stale screen: the EXDATE
* stale detail screen still pointing at the parent can otherwise reach. The * merge folds the repeat away silently and the update still reports one row
* EXDATE merge folds the repeat away silently and the parent update still * changed, so a second save would leave a second standalone copy.
* reports one row changed, so without this check the second save would leave a
* *second* standalone copy and call it a success.
*/ */
internal fun exdateContains( internal fun exdateContains(
existingExdate: String?, existingExdate: String?,

View File

@@ -265,10 +265,9 @@ fun EventEditScreen(
viewModel.reset() viewModel.reset()
onSaved() onSaved()
} }
// A failed save leaves the user on a form that looks exactly as it // A failed save leaves the form looking unchanged, so the snackbar is
// did, so the snackbar is the only sign anything happened — it gets // the only sign anything happened — long rather than the default
// the long duration rather than the default flash (Codeberg #234: // flash (Codeberg #234: it read as "nothing happens at all").
// the failure read as "nothing happens at all").
SaveUiState.Failed -> { SaveUiState.Failed -> {
viewModel.consumeSaveResult() viewModel.consumeSaveResult()
snackbarHostState.showSnackbar(saveFailedMessage, duration = SnackbarDuration.Long) snackbarHostState.showSnackbar(saveFailedMessage, duration = SnackbarDuration.Long)

View File

@@ -755,14 +755,13 @@ class EventEditViewModel @Inject constructor(
} catch (e: SecurityException) { } catch (e: SecurityException) {
SaveUiState.NeedsPermission SaveUiState.NeedsPermission
} catch (e: NoSuchEventException) { } catch (e: NoSuchEventException) {
// The write found the event (or the occurrence) already gone // The event or occurrence is already gone: the same answer the
// the same answer the pre-check gives, and a far better one than // pre-check gives, and better than a bare "couldn't save".
// a bare "couldn't save" for something that no longer exists.
SaveUiState.Gone SaveUiState.Gone
} catch (e: Exception) { } catch (e: Exception) {
// The user only gets a generic snackbar, so without this a // The user only gets a generic snackbar, so without this a failed
// failed write leaves no trace at all to report (Codeberg #234). // write leaves nothing to report (Codeberg #234). Scope and event
// Scope and event id only — never the form's content. // id only — never the form's content.
Log.w(TAG, "Save failed (scope=$scope, eventId=${target?.eventId})", e) Log.w(TAG, "Save failed (scope=$scope, eventId=${target?.eventId})", e)
SaveUiState.Failed SaveUiState.Failed
} }

View File

@@ -571,14 +571,11 @@ class EventWriteMapperTest {
times = detached.toWriteTimes(berlin), times = detached.toWriteTimes(berlin),
) )
assertThat(values[CalendarContract.Events.TITLE]).isEqualTo("Moved") assertThat(values[CalendarContract.Events.TITLE]).isEqualTo("Moved")
// The rule must not survive: without a _sync_id nothing clears an // A surviving rule would insert a second *series* overlapping the first
// 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). // (Codeberg #234's stray duplicate).
assertThat(values).doesNotContainKey(CalendarContract.Events.RRULE) assertThat(values).doesNotContainKey(CalendarContract.Events.RRULE)
assertThat(values).doesNotContainKey(CalendarContract.Events.DURATION) assertThat(values).doesNotContainKey(CalendarContract.Events.DURATION)
// A one-off row carries DTEND the invariant buildEventInsertValues // A one-off row carries DTEND rather than a duration.
// holds for every non-recurring event.
assertThat(values[CalendarContract.Events.DTSTART]).isEqualTo(1_781_164_800_000L) 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.DTEND]).isEqualTo(1_781_170_200_000L)
assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("Europe/Berlin") assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("Europe/Berlin")
@@ -586,9 +583,8 @@ class EventWriteMapperTest {
@Test @Test
fun `a detached occurrence carries every edited field onto the new row`() { fun `a detached occurrence carries every edited field onto the new row`() {
// The detached row is built from the form rather than cloned from the // Built from the form, not cloned from the parent: a field dropped here
// parent, so anything the user edited has to survive the trip — a field // is an edit silently lost.
// dropped here is an edit silently lost.
val edited = form(timezone = "America/New_York").copy( val edited = form(timezone = "America/New_York").copy(
title = " Standup ", title = " Standup ",
location = "Room 2", location = "Room 2",
@@ -612,12 +608,10 @@ class EventWriteMapperTest {
.isEqualTo(CalendarContract.Events.AVAILABILITY_FREE) .isEqualTo(CalendarContract.Events.AVAILABILITY_FREE)
assertThat(values[CalendarContract.Events.ACCESS_LEVEL]) assertThat(values[CalendarContract.Events.ACCESS_LEVEL])
.isEqualTo(CalendarContract.Events.ACCESS_PRIVATE) .isEqualTo(CalendarContract.Events.ACCESS_PRIVATE)
// The pinned zone survives too — a detached occurrence must not be // The pinned zone survives — never re-anchored to the device.
// silently re-anchored to the device.
assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("America/New_York") assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("America/New_York")
assertThat(values[CalendarContract.Events.UID_2445]).isEqualTo("uid@calendula") assertThat(values[CalendarContract.Events.UID_2445]).isEqualTo("uid@calendula")
// Reminders and guests aren't columns the insert path seeds them from // Reminders aren't columns; the insert path seeds them from the form.
// the form, so they only have to survive on the form itself.
assertThat(detached.reminders).containsExactly(10) assertThat(detached.reminders).containsExactly(10)
} }
@@ -645,9 +639,8 @@ class EventWriteMapperTest {
@Test @Test
fun `the detached row lands exactly where the parent's exdate removes it`() { 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 two halves must agree on the instant, or the user sees the
// the occurrence twice or not at all. With the time left untouched, the // occurrence twice or not at all.
// EXDATE stamp and the detached row's DTSTART describe the same instant.
val edited = form().copy(title = "Renamed", rrule = "FREQ=WEEKLY") val edited = form().copy(title = "Renamed", rrule = "FREQ=WEEKLY")
val occurrenceMillis = 1_781_164_800_000L val occurrenceMillis = 1_781_164_800_000L
@@ -702,8 +695,7 @@ class EventWriteMapperTest {
@Test @Test
fun `an occurrence already excluded is recognised, timed and all-day`() { fun `an occurrence already excluded is recognised, timed and all-day`() {
// Detaching twice would fold the repeated EXDATE away and still report a // Detaching twice would leave a second standalone copy.
// changed row, leaving a second standalone copy of one occurrence.
assertThat( assertThat(
exdateContains("20260611T080000Z", 1_781_164_800_000L, isAllDay = false), exdateContains("20260611T080000Z", 1_781_164_800_000L, isAllDay = false),
).isTrue() ).isTrue()
@@ -724,8 +716,7 @@ class EventWriteMapperTest {
@Test @Test
fun `an exclusion is found anywhere in a multi-entry exdate list`() { fun `an exclusion is found anywhere in a multi-entry exdate list`() {
// Whitespace after a comma is legal in the stored column and must not // Whitespace after a comma is legal in the stored column.
// hide an exclusion — that would let a duplicate through.
assertThat( assertThat(
exdateContains( exdateContains(
"20260604T080000Z, 20260611T080000Z,20260618T080000Z", "20260604T080000Z, 20260611T080000Z,20260618T080000Z",