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 4804f14..503bc6e 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 @@ -1200,12 +1200,9 @@ class AndroidCalendarDataSource @Inject constructor( allDayReminderTimeMinutes: Int, ): Long { val row = querySeriesRow(eventId) - // Deliberately stricter than deleteOccurrence's bare _sync_id check: the - // detach path drops the occurrence with EXDATE, which only means anything - // on a row that actually recurs. Without an RRULE there is nothing to - // 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. + // Stricter than deleteOccurrence's bare _sync_id check: EXDATE only means + // something on a row that recurs, so a non-recurring one keeps the + // exception path rather than getting a recurrence set written onto it. if (row.syncId == null && !row.rrule.isNullOrBlank()) { return detachOccurrence(eventId, beginMillis, row, form, allDayReminderTimeMinutes) } @@ -1233,46 +1230,24 @@ class AndroidCalendarDataSource @Inject constructor( * 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. + * exactly like the cancelled one [deleteOccurrence] documents; with no + * `_sync_id` the link never forms and the edit is lost (Codeberg #234). + * EXDATE plus a standalone row needs no link — what a detached instance + * degrades to without a `RECURRENCE-ID` to carry it. * - * 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. + * The detached row keeps no stored link back to its series, so: it no longer + * travels with it ([moveEvent] copies the master and its `ORIGINAL_ID` + * children, and this is neither); its EXDATE hole is an absolute instant, so + * 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 - * detached row has *no* stored link back to its series, which is the whole - * reason this path exists: - * - It stops travelling with the series. Moving the series to another - * calendar leaves it behind ([moveEvent] copies the master and its - * `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. + * Insert first, so a failure leaves the series untouched + * ([updateEventFromOccurrence]'s discipline); roll the new row back if the + * EXDATE update then fails, since it would be a visible duplicate. The + * reverse order risks the worse outcome — an excluded occurrence with no + * replacement, i.e. an edit that quietly deletes. */ private fun detachOccurrence( eventId: Long, @@ -1281,17 +1256,15 @@ class AndroidCalendarDataSource @Inject constructor( form: EventForm, allDayReminderTimeMinutes: Int, ): Long { - // Already detached (or deleted): the series no longer contains this - // occurrence, so there is nothing here to edit. Reached from a stale - // screen still pointing at the parent — without this the EXDATE merge - // would fold the repeat away, the update would still report a changed - // row, and the save would quietly leave a *second* standalone copy. + // Already detached (or deleted) from a stale screen still pointing at the + // parent: the EXDATE merge would fold the repeat away and still report a + // changed row, quietly leaving a *second* standalone copy. if (exdateContains(row.exdate, beginMillis, isAllDay = row.allDay != 0)) { throw NoSuchEventException(eventId) } - // 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. + // Reminders, guests and colour come along like any new event, and so does + // a fresh UID — the detached row is a separate event now, and sharing the + // parent's would collide with it in .ics restore dedup. val detachedId = insertEvent(form.toDetachedOccurrence(), allDayReminderTimeMinutes) val values = buildOccurrenceExdateValues( existingExdate = row.exdate, @@ -1302,10 +1275,9 @@ class AndroidCalendarDataSource @Inject constructor( timezone = row.timezone, allDay = row.allDay, ) - // Rows touched, not occurrences excluded: this is 1 whenever the series - // row still exists. It catches the row disappearing under us, not an - // EXDATE the provider's expansion fails to match — that would report - // success and leave the duplicate. Same rollback idiom as moveEvent. + // Rows touched, not occurrences excluded — 1 whenever the series row still + // exists. It catches the row disappearing under us, not an EXDATE the + // provider's expansion fails to match. val updatedRows = try { resolver.update( ContentUris.withAppendedId(CalendarContract.Events.CONTENT_URI, eventId), @@ -1326,10 +1298,8 @@ class AndroidCalendarDataSource @Inject constructor( /** * 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. + * update failed. Best effort: the caller is already throwing, and the worst + * case is the duplicate we were avoiding — never a lost occurrence. */ private fun rollBackDetached(detachedId: Long) { runCatching { deleteEvent(detachedId) }.onFailure { 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 484a0d9..8380fba 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 @@ -248,18 +248,13 @@ internal fun buildOccurrenceExceptionValues( * 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. + * The "edit only this event" shape for a series with **no `_sync_id`**, where an + * exception row can't attach to its parent at all (Codeberg #234). * - * 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. + * The exception path gets the rule dropped for free — the provider clears the + * RRULE it cloned when an exception carries DTSTART + DURATION + * ([buildOccurrenceExceptionValues]). Here nothing is cloned, so it is stripped + * by hand; leaving it on would insert a second *series* overlapping the first. */ internal fun EventForm.toDetachedOccurrence(): EventForm = copy(rrule = null) @@ -457,14 +452,11 @@ internal fun buildOccurrenceExdateValues( /** * Whether [existingExdate] already excludes the occurrence at [occurrenceMillis] - * — i.e. this occurrence has already been dropped from the series (deleted, or - * detached into its own event). + * — i.e. it has already been dropped from the series, deleted or detached. * - * Guards the detach path against running twice for the same occurrence, which a - * stale detail screen still pointing at the parent can otherwise reach. The - * EXDATE merge folds the repeat away silently and the parent update still - * reports one row changed, so without this check the second save would leave a - * *second* standalone copy and call it a success. + * Guards the detach path against running twice from a stale screen: the EXDATE + * merge folds the repeat away silently and the update still reports one row + * changed, so a second save would leave a second standalone copy. */ internal fun exdateContains( existingExdate: String?, 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 0956a03..034935c 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 @@ -265,10 +265,9 @@ 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"). + // A failed save leaves the form looking unchanged, so the snackbar is + // the only sign anything happened — long rather than the default + // flash (Codeberg #234: it read as "nothing happens at all"). SaveUiState.Failed -> { viewModel.consumeSaveResult() snackbarHostState.showSnackbar(saveFailedMessage, duration = SnackbarDuration.Long) 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 5944402..d00d80b 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 @@ -755,14 +755,13 @@ class EventEditViewModel @Inject constructor( } catch (e: SecurityException) { SaveUiState.NeedsPermission } catch (e: NoSuchEventException) { - // The write found the event (or the occurrence) already gone — - // the same answer the pre-check gives, and a far better one than - // a bare "couldn't save" for something that no longer exists. + // The event or occurrence is already gone: the same answer the + // pre-check gives, and better than a bare "couldn't save". SaveUiState.Gone } 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. + // The user only gets a generic snackbar, so without this a failed + // write leaves nothing 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 b6813b4..1722a51 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 @@ -571,14 +571,11 @@ class EventWriteMapperTest { 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 + // A surviving rule 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. + // A one-off row carries DTEND rather than a duration. 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") @@ -586,9 +583,8 @@ class EventWriteMapperTest { @Test 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 - // parent, so anything the user edited has to survive the trip — a field - // dropped here is an edit silently lost. + // Built from the form, not cloned from the parent: a field dropped here + // is an edit silently lost. val edited = form(timezone = "America/New_York").copy( title = " Standup ", location = "Room 2", @@ -612,12 +608,10 @@ class EventWriteMapperTest { .isEqualTo(CalendarContract.Events.AVAILABILITY_FREE) assertThat(values[CalendarContract.Events.ACCESS_LEVEL]) .isEqualTo(CalendarContract.Events.ACCESS_PRIVATE) - // The pinned zone survives too — a detached occurrence must not be - // silently re-anchored to the device. + // The pinned zone survives — never re-anchored to the device. assertThat(values[CalendarContract.Events.EVENT_TIMEZONE]).isEqualTo("America/New_York") assertThat(values[CalendarContract.Events.UID_2445]).isEqualTo("uid@calendula") - // Reminders and guests aren't columns — the insert path seeds them from - // the form, so they only have to survive on the form itself. + // Reminders aren't columns; the insert path seeds them from the form. assertThat(detached.reminders).containsExactly(10) } @@ -645,9 +639,8 @@ class EventWriteMapperTest { @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. + // The two halves must agree on the instant, or the user sees the + // occurrence twice or not at all. val edited = form().copy(title = "Renamed", rrule = "FREQ=WEEKLY") val occurrenceMillis = 1_781_164_800_000L @@ -702,8 +695,7 @@ class EventWriteMapperTest { @Test fun `an occurrence already excluded is recognised, timed and all-day`() { - // Detaching twice would fold the repeated EXDATE away and still report a - // changed row, leaving a second standalone copy of one occurrence. + // Detaching twice would leave a second standalone copy. assertThat( exdateContains("20260611T080000Z", 1_781_164_800_000L, isAllDay = false), ).isTrue() @@ -724,8 +716,7 @@ class EventWriteMapperTest { @Test 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 - // hide an exclusion — that would let a duplicate through. + // Whitespace after a comma is legal in the stored column. assertThat( exdateContains( "20260604T080000Z, 20260611T080000Z,20260618T080000Z",