From 79cc6925e83eaab5336ff901e6a7b54c16780a57 Mon Sep 17 00:00:00 2001 From: RomanNum3ral Date: Fri, 17 Jul 2026 08:51:35 -0400 Subject: [PATCH] Fix the two remaining known limitations: recurring item date editing, field-by-field conflict merge - CalendarItem gains recurrenceMasterStart/End/Due, the true unresolved master date, tracked separately from start/end/due (which hold the resolved next occurrence for a recurring item). The edit screen now shows and edits the real master date for recurring items instead of disabling those fields outright, with a note that the change applies to the whole series. Verified against a real recurring event on-device. - New util/ConflictDiff.kt computes exactly which user-editable fields differ between a local edit and the server's version after a 412. The conflict dialog is now a per-field mine/theirs picker (with "All mine"/"All theirs" shortcuts) instead of an all-or-nothing choice. - Extracted statusLabel() to util/StatusLabels.kt so both the edit screen and the new conflict diff can share it. - Two more test files (ConflictDiffTest, extended IcsMapperTest). --- README.md | 38 ++--- .../com/homelab/ncal/data/db/AppDatabase.kt | 2 +- .../com/homelab/ncal/data/db/ItemEntity.kt | 3 + .../homelab/ncal/data/model/CalendarItem.kt | 10 +- .../data/repository/NextcloudRepository.kt | 8 +- .../homelab/ncal/ui/detail/ItemEditScreen.kt | 159 +++++++++++++----- .../ncal/ui/detail/ItemEditViewModel.kt | 56 ++++-- .../com/homelab/ncal/util/ConflictDiff.kt | 89 ++++++++++ .../java/com/homelab/ncal/util/IcsMapper.kt | 25 ++- .../com/homelab/ncal/util/StatusLabels.kt | 10 ++ .../com/homelab/ncal/util/ConflictDiffTest.kt | 81 +++++++++ .../com/homelab/ncal/util/IcsMapperTest.kt | 15 ++ 12 files changed, 413 insertions(+), 83 deletions(-) create mode 100644 ncal/app/src/main/java/com/homelab/ncal/util/ConflictDiff.kt create mode 100644 ncal/app/src/main/java/com/homelab/ncal/util/StatusLabels.kt create mode 100644 ncal/app/src/test/java/com/homelab/ncal/util/ConflictDiffTest.kt diff --git a/README.md b/README.md index d956983..976563f 100644 --- a/README.md +++ b/README.md @@ -57,8 +57,9 @@ is the fourth option: one app, one account, one sync engine. condition of never/after N times/on a date) for both events and tasks, backed by a real iCal `RRULE`. - **Conflict handling** — a stale save (someone else changed or deleted the item since - you opened it) surfaces both versions in a dialog and lets you keep your edit or - take theirs, instead of just failing with an error. + you opened it) surfaces every field that actually differs and lets you pick, per + field, whether your edit or the server's current value wins ("All mine"/"All + theirs" shortcuts too), instead of just failing with an error. - **Reminders** — add one or more reminders to any event or task (at the scheduled time, or a preset offset before it: 5 min up to 1 week). Reminders fire as real system notifications via `AlarmManager` exact alarms — not a best-effort background @@ -75,7 +76,7 @@ is the fourth option: one app, one account, one sync engine. Nextcloud's own sidebar. - **Recurring events and tasks** — `RRULE` (`FREQ=DAILY/WEEKLY/MONTHLY/YEARLY` with `INTERVAL`/`COUNT`/`UNTIL`) is evaluated client-side to re-date each item to its - next upcoming occurrence. See "Known limitations" below for what this doesn't cover. + next upcoming occurrence. See "Editing recurring items" below for what this doesn't cover. ## Home screen widgets @@ -138,23 +139,21 @@ nothing. Run the (small, growing) JVM unit test suite with `./gradlew test` — no emulator needed. -## Known limitations +## Editing recurring items -- **No recurrence editing for a recurring item's start/end time.** You can set/change - *whether* something repeats and how (Daily/Weekly/Monthly/Yearly, interval, end - condition) via the Repeats picker, and edit all of a recurring item's other fields - freely - but the Starts/Ends/Due date fields become read-only the moment an item is - recurring, because [CalendarItem.start]/`end`/`due` hold the *resolved next - occurrence* for a recurring item, not the master date, and writing that back would - silently shift the whole series forward on every edit. Change the actual start date - of a recurring series in Nextcloud's web UI. A rule the picker can't represent - (`BYDAY`/`BYMONTH`/etc, or anything outside Daily/Weekly/Monthly/Yearly) is left - completely untouched rather than flattened into something simpler. Both guarantees - are regression-tested in `IcsMapperTest`. -- **Conflict resolution is a two-choice dialog, not a field-by-field merge.** A stale - write (HTTP 412 - the item changed or was deleted elsewhere since you opened it) - shows both versions and lets you keep your edit or take theirs; there's no way to - merge individual fields from each. +Recurring items get two guarantees, both regression-tested in `IcsMapperTest`: + +- **Editing the series' actual start/end/due time is safe.** [CalendarItem.start]/`end`/ + `due` hold the *resolved next occurrence* for a recurring item (what every list/grid + displays), not the master date - so the edit screen shows/edits a separate + `recurrenceMasterStart`/`End`/`Due` for a recurring item instead, with a note that + the change applies to the whole series. Editing them writes the true master + `DTSTART`/`DTEND`/`DUE`; leaving them untouched round-trips the original value + exactly, so an unrelated edit (e.g. just the title) can never silently shift a + series forward the way it used to. +- **A rule the Repeats picker can't represent is left completely alone.** `BYDAY`/ + `BYMONTH`/etc-based rules (or anything outside Daily/Weekly/Monthly/Yearly) show as + "Does not repeat" in the picker but are never flattened or overwritten by it. ## Where to look first if you want to extend it @@ -170,6 +169,7 @@ needed. - `util/TaskTree.kt` — the parent/child ordering shared by the Tasks screen and widget. - `util/RecurrenceUtils.kt` — re-dates a recurring item to its next occurrence for display. - `data/repository/SaveConflictException.kt` — the 412-conflict data carried to the edit screen's dialog. +- `util/ConflictDiff.kt` — computes which fields actually differ for the merge dialog. - `widget/` — the two Glance home screen widgets. ## Building a signed release diff --git a/ncal/app/src/main/java/com/homelab/ncal/data/db/AppDatabase.kt b/ncal/app/src/main/java/com/homelab/ncal/data/db/AppDatabase.kt index 1d19bba..3672f70 100644 --- a/ncal/app/src/main/java/com/homelab/ncal/data/db/AppDatabase.kt +++ b/ncal/app/src/main/java/com/homelab/ncal/data/db/AppDatabase.kt @@ -8,7 +8,7 @@ import net.sqlcipher.database.SupportFactory @Database( entities = [CollectionEntity::class, ItemEntity::class], - version = 5, + version = 6, exportSchema = false ) abstract class AppDatabase : RoomDatabase() { diff --git a/ncal/app/src/main/java/com/homelab/ncal/data/db/ItemEntity.kt b/ncal/app/src/main/java/com/homelab/ncal/data/db/ItemEntity.kt index de40af2..96b64bb 100644 --- a/ncal/app/src/main/java/com/homelab/ncal/data/db/ItemEntity.kt +++ b/ncal/app/src/main/java/com/homelab/ncal/data/db/ItemEntity.kt @@ -33,6 +33,9 @@ data class ItemEntity( val recurrenceCount: Int?, val recurrenceUntil: Long?, val isRecurring: Boolean, + val recurrenceMasterStart: Long?, + val recurrenceMasterEnd: Long?, + val recurrenceMasterDue: Long?, val rawIcs: String? ) diff --git a/ncal/app/src/main/java/com/homelab/ncal/data/model/CalendarItem.kt b/ncal/app/src/main/java/com/homelab/ncal/data/model/CalendarItem.kt index 4267dfd..cdcdb3b 100644 --- a/ncal/app/src/main/java/com/homelab/ncal/data/model/CalendarItem.kt +++ b/ncal/app/src/main/java/com/homelab/ncal/data/model/CalendarItem.kt @@ -45,7 +45,15 @@ data class CalendarItem( val recurrenceUntil: Long? = null, // mutually exclusive with recurrenceCount val isRecurring: Boolean = false, // true for ANY RRULE, even ones recurrenceFrequency can't represent - // [start]/[end]/[due] are the resolved *next occurrence* when true, not - // the master date, so the edit screen can't let those be edited here + // the master date - see recurrenceMaster* below for that + + // The *true*, unresolved master DTSTART/DTEND/DUE for a recurring item - only meaningful + // when isRecurring. The edit screen shows/edits these instead of start/end/due for a + // recurring item, so editing the series' actual start time is possible without the bug + // where saving would've overwritten the master date with the resolved next occurrence. + val recurrenceMasterStart: Long? = null, + val recurrenceMasterEnd: Long? = null, // events only + val recurrenceMasterDue: Long? = null, // tasks only val rawIcs: String? = null ) { diff --git a/ncal/app/src/main/java/com/homelab/ncal/data/repository/NextcloudRepository.kt b/ncal/app/src/main/java/com/homelab/ncal/data/repository/NextcloudRepository.kt index 5327d8b..a929d19 100644 --- a/ncal/app/src/main/java/com/homelab/ncal/data/repository/NextcloudRepository.kt +++ b/ncal/app/src/main/java/com/homelab/ncal/data/repository/NextcloudRepository.kt @@ -221,7 +221,9 @@ class NextcloudRepository(context: Context) { parentUid = parentUid, reminderMinutes = reminderMinutes.toMinutesList(), recurrenceFrequency = recurrenceFrequency?.let { runCatching { RecurrenceFrequency.valueOf(it) }.getOrNull() }, recurrenceInterval = recurrenceInterval, recurrenceCount = recurrenceCount, recurrenceUntil = recurrenceUntil, - isRecurring = isRecurring, rawIcs = rawIcs + isRecurring = isRecurring, + recurrenceMasterStart = recurrenceMasterStart, recurrenceMasterEnd = recurrenceMasterEnd, recurrenceMasterDue = recurrenceMasterDue, + rawIcs = rawIcs ) private fun CalendarItem.toEntity() = ItemEntity( @@ -231,7 +233,9 @@ class NextcloudRepository(context: Context) { status = status.name, percentComplete = percentComplete, priority = priority, parentUid = parentUid, reminderMinutes = reminderMinutes.toCsv(), recurrenceFrequency = recurrenceFrequency?.name, recurrenceInterval = recurrenceInterval, - recurrenceCount = recurrenceCount, recurrenceUntil = recurrenceUntil, isRecurring = isRecurring, rawIcs = rawIcs + recurrenceCount = recurrenceCount, recurrenceUntil = recurrenceUntil, isRecurring = isRecurring, + recurrenceMasterStart = recurrenceMasterStart, recurrenceMasterEnd = recurrenceMasterEnd, recurrenceMasterDue = recurrenceMasterDue, + rawIcs = rawIcs ) private fun String.toMinutesList(): List = diff --git a/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditScreen.kt b/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditScreen.kt index 040e916..e2bdb19 100644 --- a/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditScreen.kt +++ b/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditScreen.kt @@ -32,6 +32,7 @@ import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.MenuAnchorType import androidx.compose.material3.OutlinedTextField +import androidx.compose.material3.RadioButton import androidx.compose.material3.Scaffold import androidx.compose.material3.Slider import androidx.compose.material3.Text @@ -55,6 +56,7 @@ import com.homelab.ncal.data.model.RecurrenceFrequency import com.homelab.ncal.data.model.TaskStatus import com.homelab.ncal.util.priorityColor import com.homelab.ncal.util.priorityLabel +import com.homelab.ncal.util.statusLabel import java.text.SimpleDateFormat import java.util.Calendar import java.util.Date @@ -147,7 +149,10 @@ fun ItemEditScreen( if (item.type == ItemType.EVENT) { Text("Starts", style = MaterialTheme.typography.labelLarge, modifier = Modifier.padding(top = 16.dp)) if (item.isRecurring) { - RecurringDateNotice(item.start) + RecurringDateTimePicker( + millis = item.recurrenceMasterStart, + onChange = { v -> viewModel.update { it.copy(recurrenceMasterStart = v) } } + ) } else { DateTimePicker( millis = item.start, @@ -156,7 +161,10 @@ fun ItemEditScreen( } Text("Ends", style = MaterialTheme.typography.labelLarge, modifier = Modifier.padding(top = 8.dp)) if (item.isRecurring) { - RecurringDateNotice(item.end) + RecurringDateTimePicker( + millis = item.recurrenceMasterEnd, + onChange = { v -> viewModel.update { it.copy(recurrenceMasterEnd = v) } } + ) } else { DateTimePicker( millis = item.end, @@ -177,7 +185,10 @@ fun ItemEditScreen( } else { Text("Starts", style = MaterialTheme.typography.labelLarge, modifier = Modifier.padding(top = 16.dp)) if (item.isRecurring) { - RecurringDateNotice(item.start) + RecurringDateTimePicker( + millis = item.recurrenceMasterStart, + onChange = { v -> viewModel.update { it.copy(recurrenceMasterStart = v) } } + ) } else { DateTimePicker( millis = item.start, @@ -186,7 +197,10 @@ fun ItemEditScreen( } Text("Due", style = MaterialTheme.typography.labelLarge, modifier = Modifier.padding(top = 8.dp)) if (item.isRecurring) { - RecurringDateNotice(item.due) + RecurringDateTimePicker( + millis = item.recurrenceMasterDue, + onChange = { v -> viewModel.update { it.copy(recurrenceMasterDue = v) } } + ) } else { DateTimePicker( millis = item.due, @@ -321,40 +335,102 @@ fun ItemEditScreen( } state.conflict?.let { conflict -> - AlertDialog( - onDismissRequest = {}, // force an explicit choice - this isn't safely dismissible - title = { Text("This ${if (item.type == ItemType.TASK) "task" else "event"} changed elsewhere") }, - text = { - Column { - Text( - if (conflict.serverVersion != null) { - "It was edited somewhere else (another device, or Nextcloud's own web UI) since you opened it here." - } else { - "It was deleted somewhere else since you opened it here." + if (conflict.serverVersion != null && state.conflictFields.isNotEmpty()) { + ConflictMergeDialog( + itemTypeLabel = if (item.type == ItemType.TASK) "task" else "event", + fields = state.conflictFields, + choices = state.conflictChoices, + onChoiceChange = viewModel::setConflictFieldChoice, + onSetAll = viewModel::setAllConflictFields, + onConfirm = viewModel::resolveConflictMerge + ) + } else { + AlertDialog( + onDismissRequest = {}, // force an explicit choice - this isn't safely dismissible + title = { Text("This ${if (item.type == ItemType.TASK) "task" else "event"} changed elsewhere") }, + text = { + Column { + Text( + if (conflict.serverVersion != null) { + "It was edited somewhere else (another device, or Nextcloud's own web UI) since you opened it here, though nothing you can edit here actually differs." + } else { + "It was deleted somewhere else since you opened it here." + } + ) + Text( + "Your version: “${conflict.localEdit.summary}”", + modifier = Modifier.padding(top = 12.dp) + ) + conflict.serverVersion?.let { + Text("Their version: “${it.summary}”", modifier = Modifier.padding(top = 4.dp)) } - ) - Text( - "Your version: “${conflict.localEdit.summary}”", - modifier = Modifier.padding(top = 12.dp) - ) - conflict.serverVersion?.let { - Text("Their version: “${it.summary}”", modifier = Modifier.padding(top = 4.dp)) + } + }, + confirmButton = { + TextButton(onClick = viewModel::keepMyChanges) { Text("Keep my changes") } + }, + dismissButton = { + TextButton(onClick = viewModel::discardMyChanges) { + Text(if (conflict.serverVersion != null) "Use their version" else "Discard mine") } } - }, - confirmButton = { - TextButton(onClick = viewModel::keepMyChanges) { Text("Keep my changes") } - }, - dismissButton = { - TextButton(onClick = viewModel::discardMyChanges) { - Text(if (conflict.serverVersion != null) "Use their version" else "Discard mine") - } - } - ) + ) + } } } } +@Composable +private fun ConflictMergeDialog( + itemTypeLabel: String, + fields: List, + choices: Map, + onChoiceChange: (key: String, takeTheirs: Boolean) -> Unit, + onSetAll: (takeTheirs: Boolean) -> Unit, + onConfirm: () -> Unit +) { + AlertDialog( + onDismissRequest = {}, // force an explicit choice - this isn't safely dismissible + title = { Text("This $itemTypeLabel changed elsewhere") }, + text = { + Column(Modifier.verticalScroll(rememberScrollState())) { + Text("It was edited somewhere else since you opened it here. Pick which version wins for each field that differs:") + Row(modifier = Modifier.padding(top = 8.dp)) { + TextButton(onClick = { onSetAll(false) }) { Text("All mine") } + TextButton(onClick = { onSetAll(true) }) { Text("All theirs") } + } + fields.forEach { field -> + val takeTheirs = choices[field.key] == true + Column(modifier = Modifier.padding(top = 12.dp)) { + Text(field.label, style = MaterialTheme.typography.labelLarge) + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier + .fillMaxWidth() + .clickable { onChoiceChange(field.key, false) } + ) { + RadioButton(selected = !takeTheirs, onClick = { onChoiceChange(field.key, false) }) + Text("Mine: ${field.mineDisplay}") + } + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier + .fillMaxWidth() + .clickable { onChoiceChange(field.key, true) } + ) { + RadioButton(selected = takeTheirs, onClick = { onChoiceChange(field.key, true) }) + Text("Theirs: ${field.theirsDisplay}") + } + } + } + } + }, + confirmButton = { + TextButton(onClick = onConfirm) { Text("Save merged version") } + } + ) +} + @OptIn(ExperimentalMaterial3Api::class) @Composable private fun CalendarPicker( @@ -444,15 +520,18 @@ private fun PriorityPicker(selected: Int, onSelect: (Int) -> Unit) { } } +/** Edits a recurring item's true master date ([CalendarItem.recurrenceMasterStart] etc.) - NOT + * the same field the non-recurring [DateTimePicker] call sites bind to, since that one holds + * the resolved next occurrence rather than the series' actual start. */ @Composable -private fun RecurringDateNotice(millis: Long?) { - val display = millis?.let { SimpleDateFormat("EEE, MMM d yyyy h:mm a", Locale.getDefault()).format(Date(it)) } ?: "Not set" - Column(modifier = Modifier.padding(vertical = 8.dp)) { - Text(display) +private fun RecurringDateTimePicker(millis: Long?, onChange: (Long?) -> Unit) { + Column { + DateTimePicker(millis = millis, onChange = onChange) Text( - "This is a recurring item - changing when the series starts isn't supported here yet. Edit it in Nextcloud's web UI if you need to.", + "This changes the whole series, not just one occurrence.", style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurfaceVariant + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(bottom = 4.dp) ) } } @@ -613,12 +692,6 @@ private fun RecurrenceEndPicker( } } -private fun statusLabel(status: TaskStatus): String = when (status) { - TaskStatus.NEEDS_ACTION -> "Not started" - TaskStatus.IN_PROCESS -> "In progress" - TaskStatus.COMPLETED -> "Completed" - TaskStatus.CANCELLED -> "Cancelled" -} @OptIn(ExperimentalMaterial3Api::class) @Composable diff --git a/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditViewModel.kt b/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditViewModel.kt index 66fd24d..0c71ab3 100644 --- a/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditViewModel.kt +++ b/ncal/app/src/main/java/com/homelab/ncal/ui/detail/ItemEditViewModel.kt @@ -8,6 +8,8 @@ import com.homelab.ncal.data.model.CalendarItem import com.homelab.ncal.data.model.ItemType import com.homelab.ncal.data.repository.NextcloudRepository import com.homelab.ncal.data.repository.SaveConflictException +import com.homelab.ncal.util.ConflictField +import com.homelab.ncal.util.diffConflictFields import com.homelab.ncal.util.toUserMessage import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -25,7 +27,9 @@ data class ItemEditUiState( val error: String? = null, val done: Boolean = false, val deleted: Boolean = false, - val conflict: SaveConflictException? = null + val conflict: SaveConflictException? = null, + val conflictFields: List = emptyList(), + val conflictChoices: Map = emptyMap() // field key -> true means "take theirs" ) { /** Tasks eligible to be set as this task's parent: same calendar, not itself, not one of its own descendants. */ val availableParentTasks: List get() { @@ -122,8 +126,12 @@ class ItemEditViewModel( repo.save(item) _state.value = _state.value.copy(saving = false, done = true) } catch (e: SaveConflictException) { - android.util.Log.d("NCalConflict", "save() 412 conflict for href=${item.href}, server version present=${e.serverVersion != null}") - _state.value = _state.value.copy(saving = false, conflict = e) + val fields = e.serverVersion?.let { diffConflictFields(item, it) } ?: emptyList() + android.util.Log.d( + "NCalConflict", + "save() 412 conflict for href=${item.href}, server version present=${e.serverVersion != null}, differing fields=${fields.map { it.key }}" + ) + _state.value = _state.value.copy(saving = false, conflict = e, conflictFields = fields, conflictChoices = emptyMap()) } catch (e: Exception) { android.util.Log.e("NCalError", "save() failed for href=${item.href} calendar=${item.calendarUrl}", e) _state.value = _state.value.copy(saving = false, error = e.toUserMessage()) @@ -131,28 +139,56 @@ class ItemEditViewModel( } } - /** Conflict resolution: retry the write with the server's current etag, overwriting whatever - * changed there with this edit. */ + /** Conflict resolution (simple path - used when there's nothing to diff, e.g. the item was + * deleted server-side): retry the write with the server's current etag, overwriting + * whatever changed there with this edit. */ fun keepMyChanges() { val conflict = _state.value.conflict ?: return _state.value = _state.value.copy( - conflict = null, + conflict = null, conflictFields = emptyList(), conflictChoices = emptyMap(), item = conflict.localEdit.copy(etag = conflict.serverVersion?.etag) ) save() } - /** Conflict resolution: discard this edit and load whatever's on the server now (or leave - * the screen entirely if it was deleted there) instead. */ + /** Conflict resolution (simple path): discard this edit and load whatever's on the server + * now (or leave the screen entirely if it was deleted there) instead. */ fun discardMyChanges() { val conflict = _state.value.conflict ?: return _state.value = if (conflict.serverVersion != null) { - _state.value.copy(conflict = null, item = conflict.serverVersion) + _state.value.copy(conflict = null, conflictFields = emptyList(), conflictChoices = emptyMap(), item = conflict.serverVersion) } else { - _state.value.copy(conflict = null, deleted = true) + _state.value.copy(conflict = null, conflictFields = emptyList(), conflictChoices = emptyMap(), deleted = true) } } + /** Per-field conflict resolution: toggle whether [key] should take the server's value + * ([takeTheirs] = true) or keep the local edit's value (false, the default). */ + fun setConflictFieldChoice(key: String, takeTheirs: Boolean) { + _state.value = _state.value.copy(conflictChoices = _state.value.conflictChoices + (key to takeTheirs)) + } + + fun setAllConflictFields(takeTheirs: Boolean) { + _state.value = _state.value.copy( + conflictChoices = _state.value.conflictFields.associate { it.key to takeTheirs } + ) + } + + /** Applies the chosen per-field picks onto the local edit and retries the save with the + * server's current etag. */ + fun resolveConflictMerge() { + val conflict = _state.value.conflict ?: return + val theirs = conflict.serverVersion ?: return + var merged = conflict.localEdit.copy(etag = theirs.etag) + _state.value.conflictFields.forEach { field -> + if (_state.value.conflictChoices[field.key] == true) { + merged = field.takeTheirs(merged, theirs) + } + } + _state.value = _state.value.copy(conflict = null, conflictFields = emptyList(), conflictChoices = emptyMap(), item = merged) + save() + } + fun delete() { val item = _state.value.item ?: return if (item.isNew) return diff --git a/ncal/app/src/main/java/com/homelab/ncal/util/ConflictDiff.kt b/ncal/app/src/main/java/com/homelab/ncal/util/ConflictDiff.kt new file mode 100644 index 0000000..748cdcc --- /dev/null +++ b/ncal/app/src/main/java/com/homelab/ncal/util/ConflictDiff.kt @@ -0,0 +1,89 @@ +package com.homelab.ncal.util + +import com.homelab.ncal.data.model.CalendarItem +import com.homelab.ncal.data.model.ItemType +import java.text.SimpleDateFormat +import java.util.Date +import java.util.Locale + +/** + * One user-editable field that differs between a local edit and the server's current version + * after a save conflict (HTTP 412). [takeTheirs] applies just this field from `theirs` onto + * `target`, leaving everything else in `target` untouched - so a set of chosen fields can be + * folded onto the local edit one at a time to build a real per-field merge. + */ +data class ConflictField( + val key: String, + val label: String, + val mineDisplay: String, + val theirsDisplay: String, + val takeTheirs: (target: CalendarItem, theirs: CalendarItem) -> CalendarItem +) + +private fun formatConflictDate(millis: Long?): String = + millis?.let { SimpleDateFormat("EEE, MMM d yyyy h:mm a", Locale.getDefault()).format(Date(it)) } ?: "Not set" + +private fun displayOrEmpty(value: String): String = value.ifBlank { "(empty)" } + +/** + * Every user-editable field that differs between [mine] (the edit that just failed to save) and + * [theirs] (the server's current version), in the same order they appear on the edit form. Only + * differing fields are included, so the merge UI stays focused on what actually needs a decision. + */ +fun diffConflictFields(mine: CalendarItem, theirs: CalendarItem): List { + val fields = mutableListOf() + + fun add(key: String, label: String, mineDisplay: String, theirsDisplay: String, takeTheirs: (CalendarItem, CalendarItem) -> CalendarItem) { + fields += ConflictField(key, label, mineDisplay, theirsDisplay, takeTheirs) + } + + if (mine.summary != theirs.summary) { + add("summary", "Title", mine.summary, theirs.summary) { t, th -> t.copy(summary = th.summary) } + } + if (mine.description != theirs.description) { + add("description", "Description", displayOrEmpty(mine.description), displayOrEmpty(theirs.description)) { t, th -> t.copy(description = th.description) } + } + if (mine.location != theirs.location) { + add("location", "Location", displayOrEmpty(mine.location), displayOrEmpty(theirs.location)) { t, th -> t.copy(location = th.location) } + } + if (mine.url != theirs.url) { + add("url", "URL", displayOrEmpty(mine.url), displayOrEmpty(theirs.url)) { t, th -> t.copy(url = th.url) } + } + if (mine.priority != theirs.priority) { + add("priority", "Priority", priorityLabel(mine.priority), priorityLabel(theirs.priority)) { t, th -> t.copy(priority = th.priority) } + } + + if (mine.type == ItemType.EVENT) { + if (mine.start != theirs.start) { + add("start", "Starts", formatConflictDate(mine.start), formatConflictDate(theirs.start)) { t, th -> t.copy(start = th.start) } + } + if (mine.end != theirs.end) { + add("end", "Ends", formatConflictDate(mine.end), formatConflictDate(theirs.end)) { t, th -> t.copy(end = th.end) } + } + if (mine.allDay != theirs.allDay) { + add("allDay", "All day", if (mine.allDay) "Yes" else "No", if (theirs.allDay) "Yes" else "No") { t, th -> t.copy(allDay = th.allDay) } + } + } else { + if (mine.start != theirs.start) { + add("start", "Starts", formatConflictDate(mine.start), formatConflictDate(theirs.start)) { t, th -> t.copy(start = th.start) } + } + if (mine.due != theirs.due) { + add("due", "Due", formatConflictDate(mine.due), formatConflictDate(theirs.due)) { t, th -> t.copy(due = th.due) } + } + if (mine.status != theirs.status) { + add("status", "Status", statusLabel(mine.status), statusLabel(theirs.status)) { t, th -> t.withStatus(th.status) } + } + if (mine.percentComplete != theirs.percentComplete) { + add("percentComplete", "% complete", "${mine.percentComplete}%", "${theirs.percentComplete}%") { t, th -> t.copy(percentComplete = th.percentComplete) } + } + if (mine.parentUid != theirs.parentUid) { + add("parentUid", "Parent task", mine.parentUid ?: "None", theirs.parentUid ?: "None") { t, th -> t.copy(parentUid = th.parentUid) } + } + } + + if (mine.reminderMinutes.toSet() != theirs.reminderMinutes.toSet()) { + add("reminders", "Reminders", "${mine.reminderMinutes.size} set", "${theirs.reminderMinutes.size} set") { t, th -> t.copy(reminderMinutes = th.reminderMinutes) } + } + + return fields +} diff --git a/ncal/app/src/main/java/com/homelab/ncal/util/IcsMapper.kt b/ncal/app/src/main/java/com/homelab/ncal/util/IcsMapper.kt index 951e669..02abf0d 100644 --- a/ncal/app/src/main/java/com/homelab/ncal/util/IcsMapper.kt +++ b/ncal/app/src/main/java/com/homelab/ncal/util/IcsMapper.kt @@ -128,6 +128,8 @@ object IcsMapper { recurrenceCount = recurrence?.count, recurrenceUntil = recurrence?.until?.time, isRecurring = recurrence != null, + recurrenceMasterStart = originalStart, + recurrenceMasterEnd = originalEnd, rawIcs = ics ) } @@ -182,6 +184,8 @@ object IcsMapper { recurrenceCount = recurrence?.count, recurrenceUntil = recurrence?.until?.time, isRecurring = recurrence != null, + recurrenceMasterStart = originalStart, + recurrenceMasterDue = originalDue, rawIcs = ics ) } @@ -203,11 +207,12 @@ object IcsMapper { * * Recurring items get two extra guards, both load-bearing: * - [CalendarItem.start]/[due]/[end] hold the *resolved next occurrence* for a recurring - * item (see [parse]), not the master DTSTART/DTEND/DUE. Writing them back verbatim would - * shift the whole series forward on every single edit. So for an item that was *already* - * recurring going in, the master date(s) are left exactly as parsed - only a freshly - * recurring-for-the-first-time item (or a plain non-recurring one) takes its date from - * the model. + * item (see [parse]), not the master DTSTART/DTEND/DUE - writing them back verbatim would + * shift the whole series forward on every single edit. So an item that was *already* + * recurring going in takes its master date from [CalendarItem.recurrenceMasterStart]/ + * `End`/`Due` instead (what the edit screen actually shows/edits for a recurring item); + * a freshly recurring-for-the-first-time item (or a plain non-recurring one) takes it + * from the model's start/end/due as usual, since there's no prior master date to preserve. * - A rule the picker can't fully represent (BYDAY/BYMONTH/etc, or a frequency other than * the 4 simple ones - see [Recurrence.toRecurrenceFrequency]) is left completely alone * whenever the model's `recurrenceFrequency` is null, rather than being replaced by @@ -226,7 +231,10 @@ object IcsMapper { ev.setDescription(item.description.takeIf { it.isNotBlank() }) ev.setLocation(item.location.takeIf { it.isNotBlank() }) ev.setUrl(item.url.takeIf { it.isNotBlank() }) - if (!wasRecurring) { + if (wasRecurring) { + ev.setDateStart(item.recurrenceMasterStart?.let { Date(it) }, !item.allDay) + ev.setDateEnd(item.recurrenceMasterEnd?.let { Date(it) }, !item.allDay) + } else { ev.setDateStart(item.start?.let { Date(it) }, !item.allDay) ev.setDateEnd(item.end?.let { Date(it) }, !item.allDay) } @@ -250,7 +258,10 @@ object IcsMapper { td.setDescription(item.description.takeIf { it.isNotBlank() }) td.setLocation(item.location.takeIf { it.isNotBlank() }) td.setUrl(item.url.takeIf { it.isNotBlank() }) - if (!wasRecurring) { + if (wasRecurring) { + td.setDateStart(item.recurrenceMasterStart?.let { Date(it) }) + td.setDateDue(item.recurrenceMasterDue?.let { Date(it) }) + } else { td.setDateStart(item.start?.let { Date(it) }) td.setDateDue(item.due?.let { Date(it) }) } diff --git a/ncal/app/src/main/java/com/homelab/ncal/util/StatusLabels.kt b/ncal/app/src/main/java/com/homelab/ncal/util/StatusLabels.kt new file mode 100644 index 0000000..8b72586 --- /dev/null +++ b/ncal/app/src/main/java/com/homelab/ncal/util/StatusLabels.kt @@ -0,0 +1,10 @@ +package com.homelab.ncal.util + +import com.homelab.ncal.data.model.TaskStatus + +fun statusLabel(status: TaskStatus): String = when (status) { + TaskStatus.NEEDS_ACTION -> "Not started" + TaskStatus.IN_PROCESS -> "In progress" + TaskStatus.COMPLETED -> "Completed" + TaskStatus.CANCELLED -> "Cancelled" +} diff --git a/ncal/app/src/test/java/com/homelab/ncal/util/ConflictDiffTest.kt b/ncal/app/src/test/java/com/homelab/ncal/util/ConflictDiffTest.kt new file mode 100644 index 0000000..f09f8fb --- /dev/null +++ b/ncal/app/src/test/java/com/homelab/ncal/util/ConflictDiffTest.kt @@ -0,0 +1,81 @@ +package com.homelab.ncal.util + +import com.homelab.ncal.data.model.CalendarItem +import com.homelab.ncal.data.model.ItemType +import com.homelab.ncal.data.model.TaskStatus +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +class ConflictDiffTest { + + private fun event(summary: String = "Standup", location: String = "", start: Long? = 1000L, end: Long? = 2000L) = CalendarItem( + uid = "u1", href = "https://example.com/e1.ics", etag = "\"1\"", calendarUrl = "https://example.com/cal/", + type = ItemType.EVENT, summary = summary, location = location, start = start, end = end + ) + + @Test + fun `identical items produce no diff fields`() { + val mine = event() + val theirs = event() + assertTrue(diffConflictFields(mine, theirs).isEmpty()) + } + + @Test + fun `only the fields that actually differ are reported`() { + val mine = event(summary = "Standup", location = "Room A") + val theirs = event(summary = "Standup", location = "Room B") + + val fields = diffConflictFields(mine, theirs) + + assertEquals(listOf("location"), fields.map { it.key }) + assertEquals("Room A", fields.single().mineDisplay) + assertEquals("Room B", fields.single().theirsDisplay) + } + + @Test + fun `takeTheirs applies only that one field, not the whole item`() { + val mine = event(summary = "Mine title", location = "Mine location") + val theirs = event(summary = "Theirs title", location = "Theirs location") + + val fields = diffConflictFields(mine, theirs) + val locationField = fields.first { it.key == "location" } + + val merged = locationField.takeTheirs(mine, theirs) + + assertEquals("Theirs location", merged.location) + assertEquals("Mine title", merged.summary) // untouched - only "location" was applied + } + + @Test + fun `task-only fields are compared for tasks and not events`() { + val mine = CalendarItem( + uid = "t1", href = "https://example.com/t1.ics", etag = "\"1\"", calendarUrl = "https://example.com/cal/", + type = ItemType.TASK, summary = "Task", status = TaskStatus.NEEDS_ACTION, percentComplete = 0 + ) + val theirs = mine.withStatus(TaskStatus.COMPLETED) + + val fields = diffConflictFields(mine, theirs) + + assertTrue(fields.any { it.key == "status" }) + assertTrue(fields.any { it.key == "percentComplete" }) + // event-only fields must never appear for a task, even if irrelevant values differ + assertTrue(fields.none { it.key == "allDay" }) + } + + @Test + fun `applying a status takeTheirs keeps completed and percentComplete in sync`() { + val mine = CalendarItem( + uid = "t1", href = "https://example.com/t1.ics", etag = "\"1\"", calendarUrl = "https://example.com/cal/", + type = ItemType.TASK, summary = "Task", status = TaskStatus.NEEDS_ACTION, percentComplete = 0 + ) + val theirs = mine.withStatus(TaskStatus.COMPLETED) + + val statusField = diffConflictFields(mine, theirs).first { it.key == "status" } + val merged = statusField.takeTheirs(mine, theirs) + + assertEquals(TaskStatus.COMPLETED, merged.status) + assertTrue(merged.completed) + assertEquals(100, merged.percentComplete) + } +} diff --git a/ncal/app/src/test/java/com/homelab/ncal/util/IcsMapperTest.kt b/ncal/app/src/test/java/com/homelab/ncal/util/IcsMapperTest.kt index f29f5c4..55520c7 100644 --- a/ncal/app/src/test/java/com/homelab/ncal/util/IcsMapperTest.kt +++ b/ncal/app/src/test/java/com/homelab/ncal/util/IcsMapperTest.kt @@ -68,6 +68,21 @@ class IcsMapperTest { assertTrue("master DTEND must stay exactly as originally authored", rebuiltIcs.contains("DTEND:20200106T100000Z")) } + @Test + fun `deliberately changing a recurring event's master start via recurrenceMasterStart is applied`() { + val item = IcsMapper.parse(recurringEventIcs, href = "https://example.com/e1.ics", etag = null, calendarUrl = "https://example.com/cal/")!! + assertEquals(1578301200000L, item.recurrenceMasterStart) // 2020-01-06T09:00:00Z, parsed from the fixture + + // Simulate the user actually moving the Repeats picker's "Starts" field, not just editing + // an unrelated field - this is the one case where the master date SHOULD change. + val newMasterStart = 1578304800000L // 2020-01-06T10:00:00Z, one hour later + val edited = item.copy(recurrenceMasterStart = newMasterStart) + val rebuiltIcs = IcsMapper.toIcs(edited) + + assertTrue("master DTSTART must reflect the deliberate change", rebuiltIcs.contains("DTSTART:20200106T100000Z")) + assertTrue("RRULE must still survive", rebuiltIcs.contains("RRULE") && rebuiltIcs.contains("FREQ=WEEKLY")) + } + @Test fun `unsupported BYDAY-based rule is left completely untouched when the picker fields are null`() { val item = IcsMapper.parse(recurringEventIcs, href = "https://example.com/e1.ics", etag = null, calendarUrl = "https://example.com/cal/")!!