fix(#147, #325): review round 3 - a failed moved occurrence stays at its old time, declined occurrences not copied, unlinked rows kept
This commit is contained in:
1 parent
0a36a6d287
commit
10208a332e
14 files changed
+98
-48
No files matched your search
@@ -798,27 +798,30 @@ class AndroidCalendarDataSource @Inject constructor(
|
||||
it != calendar?.color || !getString(EventExportProjection.IDX_EVENT_COLOR_KEY).isNullOrEmpty()
|
||||
}
|
||||
val mapped = event(calendar?.displayName).copy(color = color)
|
||||
val declined = getIntOrNull(EventExportProjection.IDX_SELF_ATTENDEE_STATUS) ==
|
||||
CalendarContract.Attendees.ATTENDEE_STATUS_DECLINED
|
||||
val originalId = getLongOrNull(EventExportProjection.IDX_ORIGINAL_ID)
|
||||
val originalSyncId = getString(EventExportProjection.IDX_ORIGINAL_SYNC_ID)?.takeIf { it.isNotEmpty() }
|
||||
val originalInstance = getLongOrNull(EventExportProjection.IDX_ORIGINAL_INSTANCE_TIME)
|
||||
if (originalId == null && originalSyncId == null && originalInstance == null) {
|
||||
// Without a link the provider shows the row as an event of its own.
|
||||
if (originalId == null && originalSyncId == null) {
|
||||
series += MasterRow(
|
||||
eventId = getLong(EventExportProjection.IDX_ID),
|
||||
calendarId = calendarId,
|
||||
syncId = getString(EventExportProjection.IDX_SYNC_ID)?.takeIf { it.isNotEmpty() },
|
||||
event = mapped,
|
||||
declined = getIntOrNull(EventExportProjection.IDX_SELF_ATTENDEE_STATUS) ==
|
||||
CalendarContract.Attendees.ATTENDEE_STATUS_DECLINED,
|
||||
declined = declined,
|
||||
)
|
||||
} else {
|
||||
// An exception without its occurrence time can't name what it replaces.
|
||||
if (originalInstance == null) return@forEachExportRow
|
||||
val originalInstance = getLongOrNull(EventExportProjection.IDX_ORIGINAL_INSTANCE_TIME)
|
||||
?: return@forEachExportRow
|
||||
overrides += OverrideRow(
|
||||
calendarId = calendarId,
|
||||
originalId = originalId,
|
||||
originalSyncId = originalSyncId,
|
||||
originalInstance = originalInstance.toKotlinInstantFromEpochMillis(),
|
||||
event = mapped,
|
||||
declined = declined,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
+14
-9
@@ -295,9 +295,9 @@ class CalendarRepositoryImpl @Inject constructor(
|
||||
var skipped = 0
|
||||
var failed = 0
|
||||
var notAttempted = 0
|
||||
// Series whose standalone occurrence didn't land: inserted, they would
|
||||
// exclude it for good, since a re-run skips it next to its series.
|
||||
val held = mutableSetOf<String>()
|
||||
// Occurrences whose standalone copy didn't land, by series UID: the series
|
||||
// keeps them instead of excluding them for good.
|
||||
val unexcluded = mutableMapOf<String, MutableList<Instant>>()
|
||||
for ((index, event) in events.withIndex()) {
|
||||
if (index > 0) onProgress(index)
|
||||
// A known UID means the event is already in this calendar — skip,
|
||||
@@ -306,12 +306,8 @@ class CalendarRepositoryImpl @Inject constructor(
|
||||
skipped++
|
||||
continue
|
||||
}
|
||||
if (event.uid != null && event.uid in held) {
|
||||
failed++
|
||||
continue
|
||||
}
|
||||
try {
|
||||
dataSource.insertImportedEvent(event, targetCalendarId, allDayMinutes, palette)
|
||||
dataSource.insertImportedEvent(event.keeping(unexcluded), targetCalendarId, allDayMinutes, palette)
|
||||
imported++
|
||||
} catch (e: CancellationException) {
|
||||
throw e
|
||||
@@ -331,7 +327,9 @@ class CalendarRepositoryImpl @Inject constructor(
|
||||
// rest of the file — foreign exports do carry rows it rejects
|
||||
// outright (a malformed RRULE throws straight out of insert).
|
||||
failed++
|
||||
event.overrideOf?.let(held::add)
|
||||
if (event.overrideOf != null && event.overrideAt != null) {
|
||||
unexcluded.getOrPut(event.overrideOf) { mutableListOf() } += event.overrideAt
|
||||
}
|
||||
Log.w(TAG, "Skipped an unimportable event", e)
|
||||
}
|
||||
}
|
||||
@@ -424,3 +422,10 @@ private fun <T> Flow<Unit>.reQuery(block: suspend () -> T): Flow<T> = flow {
|
||||
*/
|
||||
private fun ParsedIcsEvent.isAlreadyIn(existing: Set<String>): Boolean =
|
||||
(uid != null && uid in existing) || (overrideOf != null && overrideOf in existing)
|
||||
|
||||
/** This series without the exclusions of [unexcluded] occurrences whose standalone copy failed. */
|
||||
private fun ParsedIcsEvent.keeping(unexcluded: Map<String, List<Instant>>): ParsedIcsEvent {
|
||||
val kept = uid?.let(unexcluded::get) ?: return this
|
||||
val stamps = kept.map { formatExdateStamp(it.toEpochMilliseconds(), isAllDay) }.toSet()
|
||||
return copy(exDates = exDates.filterNot { it in stamps })
|
||||
}
|
||||
@@ -7,7 +7,7 @@ import kotlin.time.Duration.Companion.days
|
||||
* The events a calendar copy (#325) inserts into its target, in the shape
|
||||
* `importEvents` takes: the source's rows flattened as [flattenOverrides] does
|
||||
* for a backup, minus cancelled series and one-offs, which would arrive live
|
||||
* (an insert can't be cancelled), and declined invitations. Attendees and organizer are never read.
|
||||
* (an insert can't be cancelled), and declined invitations and occurrences. Attendees and organizer are never read.
|
||||
*
|
||||
* Idempotent per snapshot: `importEvents` skips a UID the target already holds,
|
||||
* and an [IcsEvent.overrideOf] occurrence whose series it holds, so a re-run
|
||||
@@ -18,7 +18,13 @@ fun planCalendarCopy(
|
||||
rows: CalendarRows,
|
||||
exdateStamp: (occurrenceMillis: Long, isAllDay: Boolean) -> String,
|
||||
): List<ParsedIcsEvent> {
|
||||
val live = rows.copy(series = rows.series.filterNot { it.event.status == EventStatus.Cancelled || it.declined })
|
||||
val live = CalendarRows(
|
||||
series = rows.series.filterNot { it.event.status == EventStatus.Cancelled || it.declined },
|
||||
// A declined occurrence is as good as deleted here.
|
||||
overrides = rows.overrides.map {
|
||||
if (it.declined) it.copy(event = it.event.copy(status = EventStatus.Cancelled)) else it
|
||||
},
|
||||
)
|
||||
return flattenOverrides(live, exdateStamp).map { it.toParsedIcsEvent() }
|
||||
}
|
||||
|
||||
@@ -48,5 +54,6 @@ fun IcsEvent.toParsedIcsEvent(): ParsedIcsEvent = ParsedIcsEvent(
|
||||
accessLevel = accessLevel,
|
||||
color = color,
|
||||
overrideOf = overrideOf,
|
||||
overrideAt = overrideAt,
|
||||
exDates = if (recurs) exDates.filter { it.isNotBlank() }.distinct() else emptyList(),
|
||||
)
|
||||
@@ -27,6 +27,8 @@ data class OverrideRow(
|
||||
val originalSyncId: String?,
|
||||
val originalInstance: Instant,
|
||||
val event: IcsEvent,
|
||||
/** The user declined this occurrence. */
|
||||
val declined: Boolean = false,
|
||||
) {
|
||||
val isCancelled: Boolean get() = event.status == EventStatus.Cancelled
|
||||
}
|
||||
@@ -81,6 +83,7 @@ fun flattenOverrides(
|
||||
recurrenceDates = null,
|
||||
exDates = emptyList(),
|
||||
overrideOf = parent.event.uid,
|
||||
overrideAt = override.originalInstance,
|
||||
)
|
||||
}
|
||||
// Two exception rows for one occurrence would derive the same UID; the same
|
||||
|
||||
@@ -48,6 +48,8 @@ data class IcsEvent(
|
||||
* UID of the series it replaces an occurrence of, as `X-CALENDULA-OVERRIDE-OF`.
|
||||
*/
|
||||
val overrideOf: String? = null,
|
||||
/** The occurrence of [overrideOf] this event replaces, as `X-CALENDULA-OVERRIDE-AT`. */
|
||||
val overrideAt: Instant? = null,
|
||||
)
|
||||
|
||||
/** Whether this is a series: by rule, or by listed dates alone. */
|
||||
|
||||
@@ -52,6 +52,8 @@ data class ParsedIcsEvent(
|
||||
* already is, since the series there carries its own override.
|
||||
*/
|
||||
val overrideOf: String? = null,
|
||||
/** The occurrence of [overrideOf] this event replaces (`X-CALENDULA-OVERRIDE-AT`). */
|
||||
val overrideAt: Instant? = null,
|
||||
/** The provider's raw `RDATE`, only set by a calendar copy. */
|
||||
val recurrenceDates: String? = null,
|
||||
val accessLevel: AccessLevel = AccessLevel.Default,
|
||||
@@ -224,6 +226,7 @@ class IcsParser(private val deviceZone: TimeZone = TimeZone.currentSystemDefault
|
||||
val exDateLines = mutableListOf<IcsContentLine>()
|
||||
var skipAsOverride = false
|
||||
var overrideOf: String? = null
|
||||
var overrideAt: Instant? = null
|
||||
|
||||
var i = 0
|
||||
while (i < body.size) {
|
||||
@@ -253,6 +256,7 @@ class IcsParser(private val deviceZone: TimeZone = TimeZone.currentSystemDefault
|
||||
"RECURRENCE-ID" -> skipAsOverride = true
|
||||
"ATTENDEE" -> warnings.add(IcsParseWarning.AttendeesIgnored)
|
||||
"X-CALENDULA-OVERRIDE-OF" -> overrideOf = unescapeText(line.value).trim().ifEmpty { null }
|
||||
"X-CALENDULA-OVERRIDE-AT" -> overrideAt = parseBasicDateTime(line.value.trim())?.toInstant(TimeZone.UTC)
|
||||
"X-CALENDULA-CALENDAR" ->
|
||||
calendarName = unescapeText(line.value).trim().ifEmpty { calendarName }
|
||||
// Fossify names the source calendar per event; it never writes
|
||||
@@ -313,6 +317,7 @@ class IcsParser(private val deviceZone: TimeZone = TimeZone.currentSystemDefault
|
||||
},
|
||||
isTask = isTask,
|
||||
overrideOf = overrideOf,
|
||||
overrideAt = overrideAt,
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -58,6 +58,7 @@ class IcsWriter(private val prodId: String = ICS_PROD_ID) {
|
||||
?.let { add("X-CALENDULA-CALENDAR:${escapeText(it)}") }
|
||||
event.overrideOf?.takeIf { it.isNotBlank() }
|
||||
?.let { add("X-CALENDULA-OVERRIDE-OF:${escapeText(it)}") }
|
||||
event.overrideAt?.let { add("X-CALENDULA-OVERRIDE-AT:${it.toUtcBasic()}") }
|
||||
event.reminderMinutes.filter { it >= 0 }.distinct().forEach { minutes ->
|
||||
appendAlarm(minutes, event.summary)
|
||||
}
|
||||
|
||||
@@ -305,6 +305,7 @@ private fun BackupContent(
|
||||
leading = { LeadingAvatar(Icons.Default.FileUpload) },
|
||||
onClick = onRestoreClick,
|
||||
)
|
||||
if (copyRun != CopyRunState.Idle) CopyEventsRow(copyRun, Position.Alone, onCopy)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -324,23 +325,7 @@ private fun BackupContent(
|
||||
onClick = onRestoreClick,
|
||||
)
|
||||
// A running or finished copy keeps its row, the way back to its sheet.
|
||||
if (canCopy || copyRun != CopyRunState.Idle) {
|
||||
GroupedRow(
|
||||
title = stringResource(R.string.calendars_copy_action),
|
||||
summary = when (copyRun) {
|
||||
is CopyRunState.Running -> copyProgressText(copyRun)
|
||||
is CopyRunState.Done -> stringResource(
|
||||
if (copyRun.stopped) R.string.calendars_copy_stopped_title else R.string.calendars_copy_done,
|
||||
)
|
||||
CopyRunState.Cancelled -> stringResource(R.string.calendars_copy_cancelled)
|
||||
CopyRunState.Failed -> stringResource(R.string.calendars_copy_failed)
|
||||
CopyRunState.Idle -> stringResource(R.string.calendars_copy_summary)
|
||||
},
|
||||
position = Position.Middle,
|
||||
leading = { LeadingAvatar(Icons.Default.ContentCopy) },
|
||||
onClick = onCopy,
|
||||
)
|
||||
}
|
||||
if (canCopy || copyRun != CopyRunState.Idle) CopyEventsRow(copyRun, Position.Middle, onCopy)
|
||||
GroupedRow(
|
||||
title = stringResource(R.string.calendars_auto_backup),
|
||||
summary = stringResource(R.string.calendars_auto_backup_hint),
|
||||
@@ -406,6 +391,25 @@ private fun BackupContent(
|
||||
}
|
||||
}
|
||||
|
||||
@Composable
|
||||
private fun CopyEventsRow(copyRun: CopyRunState, position: Position, onClick: () -> Unit) {
|
||||
GroupedRow(
|
||||
title = stringResource(R.string.calendars_copy_action),
|
||||
summary = when (copyRun) {
|
||||
is CopyRunState.Running -> copyProgressText(copyRun)
|
||||
is CopyRunState.Done -> stringResource(
|
||||
if (copyRun.stopped) R.string.calendars_copy_stopped_title else R.string.calendars_copy_done,
|
||||
)
|
||||
CopyRunState.Cancelled -> stringResource(R.string.calendars_copy_cancelled)
|
||||
CopyRunState.Failed -> stringResource(R.string.calendars_copy_failed)
|
||||
CopyRunState.Idle -> stringResource(R.string.calendars_copy_summary)
|
||||
},
|
||||
position = position,
|
||||
leading = { LeadingAvatar(Icons.Default.ContentCopy) },
|
||||
onClick = onClick,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Choose which calendars to include in a one-time `.ics` export. Starts on the
|
||||
* calendars a backup covers by default, or on all of them when none is; the Export action opens the SAF save dialog and hands back
|
||||
|
||||
@@ -65,8 +65,9 @@ fun backupUiState(calendars: List<CalendarSource>): BackupUiState {
|
||||
*/
|
||||
fun copySources(calendars: List<CalendarSource>): List<CalendarSource> {
|
||||
val targets = calendars.filter { it.isEventTarget }
|
||||
val onlyTarget = targets.singleOrNull()?.id
|
||||
if (targets.isEmpty()) return emptyList()
|
||||
return calendars.filter { source -> source.isBackupCandidate && targets.any { it.id != source.id } }
|
||||
return calendars.filter { it.isBackupCandidate && it.id != onlyTarget }
|
||||
}
|
||||
|
||||
/** Calendars a copy from [sourceId] can write into: the import targets, minus the source itself. */
|
||||
|
||||
@@ -16,6 +16,7 @@ import androidx.compose.material3.TextButton
|
||||
import androidx.compose.runtime.Composable
|
||||
import androidx.compose.runtime.getValue
|
||||
import androidx.compose.runtime.mutableStateOf
|
||||
import androidx.compose.runtime.remember
|
||||
import androidx.compose.runtime.saveable.rememberSaveable
|
||||
import androidx.compose.runtime.setValue
|
||||
import androidx.compose.ui.Alignment
|
||||
@@ -52,9 +53,9 @@ fun CopyEventsSheet(
|
||||
onDismiss: () -> Unit,
|
||||
) {
|
||||
val list = (calendars as? CopyCalendars.Ready)?.calendars.orEmpty()
|
||||
val sources = copySources(list)
|
||||
val sources = remember(list) { copySources(list) }
|
||||
val source = sources.firstOrNull { it.id == selection.sourceId }
|
||||
val targets = copyTargets(list, source?.id)
|
||||
val targets = remember(list, source?.id) { copyTargets(list, source?.id) }
|
||||
val target = targets.firstOrNull { it.id == selection.targetId }
|
||||
val running = run is CopyRunState.Running
|
||||
var picking by rememberSaveable { mutableStateOf<CopyEnd?>(null) }
|
||||
|
||||
@@ -891,7 +891,7 @@
|
||||
<string name="calendars_restore_unavailable">No calendar can take the imported events yet.</string>
|
||||
<string name="calendars_copy_action">Copy events</string>
|
||||
<string name="calendars_copy_summary">Copy one calendar\'s events into another, for example to move off Google.</string>
|
||||
<string name="calendars_copy_hint">Copies every event, including repeating ones with the occurrences you changed or deleted. Guests and invitations stay behind, and the original calendar is left as it is.</string>
|
||||
<string name="calendars_copy_hint">Copies your events, including repeating ones with the occurrences you changed or deleted. Cancelled events and invitations you declined are left out, guests stay behind, and the original calendar is left as it is.</string>
|
||||
<string name="calendars_copy_from">From</string>
|
||||
<string name="calendars_copy_to">To</string>
|
||||
<string name="calendars_copy_choose">Choose a calendar</string>
|
||||
|
||||
+8
-4
@@ -1047,19 +1047,23 @@ class CalendarRepositoryImplTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a series waits for a standalone occurrence that didn't land`(
|
||||
fun `a series keeps an occurrence whose standalone copy didn't land`(
|
||||
@TempDir tempDir: Path,
|
||||
) = runTest {
|
||||
val fake = FakeCalendarDataSource().apply { failingImportSummaries += "moved" }
|
||||
val repo = testRepository(fake, tempDir, Dispatchers.Unconfined)
|
||||
val at = Instant.parse("2026-01-12T09:00:00Z")
|
||||
|
||||
val summary = repo.importEvents(
|
||||
3L,
|
||||
listOf(parsedEvent("1-s", summary = "moved").copy(overrideOf = "s"), parsedEvent("s"), parsedEvent("t")),
|
||||
listOf(
|
||||
parsedEvent("1-s", summary = "moved").copy(overrideOf = "s", overrideAt = at),
|
||||
parsedEvent("s").copy(recurrenceRule = "FREQ=WEEKLY", exDates = listOf("20260112T090000Z", "20260119T090000Z")),
|
||||
),
|
||||
)
|
||||
|
||||
assertThat(fake.importedEvents.map { it.first.uid }).containsExactly("t")
|
||||
assertThat(summary.failed).isEqualTo(2)
|
||||
assertThat(summary.failed).isEqualTo(1)
|
||||
assertThat(fake.importedEvents.single().first.exDates).containsExactly("20260119T090000Z")
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
@@ -181,9 +181,11 @@ class CalendarRowsTest {
|
||||
|
||||
@Test
|
||||
fun `the override link survives the file`() {
|
||||
val text = IcsWriter().writeCalendar(listOf(event("d").copy(overrideOf = "s;1")), start)
|
||||
val text = IcsWriter().writeCalendar(listOf(event("d").copy(overrideOf = "s;1", overrideAt = occurrence(1))), start)
|
||||
|
||||
assertThat(IcsParser().parse(text).events.single().overrideOf).isEqualTo("s;1")
|
||||
val parsed = IcsParser().parse(text).events.single()
|
||||
assertThat(parsed.overrideOf).isEqualTo("s;1")
|
||||
assertThat(parsed.overrideAt).isEqualTo(occurrence(1))
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -215,4 +217,16 @@ class CalendarRowsTest {
|
||||
|
||||
assertThat(planCalendarCopy(rows, ::formatExdateStamp).map { it.uid }).containsExactly("k")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a copy treats a declined occurrence as deleted`() {
|
||||
val rows = CalendarRows(
|
||||
listOf(series("s")),
|
||||
listOf(OverrideRow(1, 1, null, occurrence(1), event("x"), declined = true)),
|
||||
)
|
||||
|
||||
val plan = planCalendarCopy(rows, ::formatExdateStamp)
|
||||
assertThat(plan.map { it.uid }).containsExactly("s")
|
||||
assertThat(plan.single().exDates).containsExactly("20260112T090000Z")
|
||||
}
|
||||
}
|
||||
@@ -507,10 +507,10 @@ times. Its UID is derived from the series UID and the occurrence, so it is the
|
||||
same in every backup, and it carries the series UID as `X-CALENDULA-OVERRIDE-OF`.
|
||||
`importEvents` skips such an occurrence wherever its series already is — that
|
||||
series there has the override itself — so restoring into the calendar a backup
|
||||
came from adds nothing twice, while a restore into an empty one gets both. If
|
||||
the standalone occurrence fails to insert, its series is held back too (counted
|
||||
as not added), since inserted with the occurrence excluded it would make every
|
||||
re-run skip the occurrence for good.
|
||||
came from adds nothing twice, while a restore into an empty one gets both. The
|
||||
standalone event also names its occurrence (`X-CALENDULA-OVERRIDE-AT`); if it
|
||||
fails to insert, the series is inserted without that exclusion, so the
|
||||
occurrence stays at its original time rather than vanishing for good.
|
||||
Exceptions whose series isn't among the rows are left out, a cancelled
|
||||
series' moved occurrences stay cancelled, and sync ids link only within their
|
||||
own calendar. A file can't carry `RDATE`, so a backup treats a series that
|
||||
@@ -522,7 +522,7 @@ flattened rows straight into `importEvents`, so the UID dedup makes a re-run ski
|
||||
what landed before. It skips the `.ics` text: the writer pins a timed one-off to
|
||||
UTC and carries no colour, visibility or `RDATE`; the direct mapping keeps them.
|
||||
Attendees and organizer are never read, and cancelled events and declined
|
||||
invitations are left out (an insert can't be cancelled; it would arrive live). A re-run is idempotent per
|
||||
invitations are left out (a declined occurrence counts as deleted) (an insert can't be cancelled; it would arrive live). A re-run is idempotent per
|
||||
snapshot: a series already in the target keeps its overrides, so an occurrence
|
||||
changed since keeps its earlier copy rather than gaining a second one.
|
||||
|
||||
|
||||
Reference in new issue
Block a user