From 9e5e98ccee79b1883cf2a79cd7f200748e17296b Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Fri, 6 Nov 2026 17:52:15 +0100 Subject: [PATCH] Move a series on when its first occurrence is ticked off (#137) Agendula's own store kept the series open and wrote a completed RECURRENCE-ID override, which Infomaniak and Apple Reminders show as still open. The series now advances to its next occurrence and a completed copy is kept; reopening that occurrence moves it back. --- CHANGELOG.md | 7 + .../data/tasks/provider/ProviderWriterTest.kt | 12 +- .../tasks/room/RoomTasksDataSourceTest.kt | 99 +++++++++++- .../data/tasks/room/RoomTasksDataSource.kt | 145 ++++++++++++++++-- 4 files changed, 244 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a65170..579a67b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +### Changed +- Ticking off the current occurrence of a repeating task moves the series on + to its next date and keeps a completed copy, as tasks.org and Apple + Reminders do. Other apps and the web views of servers such as Infomaniak now + see it as done instead of still open. Unticking it moves the series back + (#137). + ## [1.3.0] - 2026-11-02 ### Added diff --git a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriterTest.kt b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriterTest.kt index f4383a9..f60f8a3 100644 --- a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriterTest.kt +++ b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriterTest.kt @@ -685,9 +685,11 @@ class ProviderWriterTest { store().setCompletedInstance(series, today, false) val override = db.tasks().overrides(series).single() db.tasks().update(override.copy(title = "Standup (moved room)")) - val row = readRow(ProviderTable.INSTANCES, instanceId(today)) + // The second occurrence: completing the first moves the series on instead. + val second = today + 1.days + val row = readRow(ProviderTable.INSTANCES, instanceId(second)) - writer.update(ProviderTable.INSTANCES, row + (Tasks.STATUS to 2), byId(ProviderTable.INSTANCES, instanceId(today)), null) + writer.update(ProviderTable.INSTANCES, row + (Tasks.STATUS to 2), byId(ProviderTable.INSTANCES, instanceId(second)), null) assertThat(task(series)!!.title).isEqualTo("Standup") assertThat(task(override.id)!!.title).isEqualTo("Standup (moved room)") @@ -734,9 +736,11 @@ class ProviderWriterTest { val series = db.tasks().insert( TaskEntity(listId = home, uid = "f", title = "Floating", due = today, rrule = "FREQ=DAILY;COUNT=3", timezone = null), ) - val row = readRow(ProviderTable.INSTANCES, instanceId(today)) + // The second occurrence: completing the first moves the series on instead. + val second = today + 1.days + val row = readRow(ProviderTable.INSTANCES, instanceId(second)) - writer.update(ProviderTable.INSTANCES, row + (Tasks.STATUS to 2), byId(ProviderTable.INSTANCES, instanceId(today)), null) + writer.update(ProviderTable.INSTANCES, row + (Tasks.STATUS to 2), byId(ProviderTable.INSTANCES, instanceId(second)), null) writer.update(ProviderTable.TASKS, mapOf(Tasks.TITLE to "Still floating"), byId(ProviderTable.TASKS, series), null) assertThat(db.tasks().overrides(series).single().status).isEqualTo(TaskStatus.COMPLETED) diff --git a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt index 75a3a9e..068864e 100644 --- a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt +++ b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSourceTest.kt @@ -211,7 +211,8 @@ class RoomTasksDataSourceTest { @Test fun completingOneOccurrenceLeavesTheRestOfTheSeriesOpen() { val id = source.insertTask(form(title = "Water the plants")) - makeRecurring(id, now) + // Overdue occurrences before it, so it is not the first and forks an override. + makeRecurring(id, now - 14.days) val open = { source.tasks(TaskQuery(listId = listId)).filter { it.taskId == id } } val before = open() val target = before.first { it.distanceFromCurrent == 0 } @@ -237,7 +238,8 @@ class RoomTasksDataSourceTest { @Test fun reopeningACompletedOccurrenceReusesItsOverride() { val id = source.insertTask(form(title = "Water the plants")) - makeRecurring(id, now) + // Overdue occurrences before it, so it is not the first and forks an override. + makeRecurring(id, now - 14.days) val target = source.tasks(TaskQuery(listId = listId)) .first { it.taskId == id && it.distanceFromCurrent == 0 } @@ -254,7 +256,8 @@ class RoomTasksDataSourceTest { @Test fun cancellingOneOccurrenceLeavesTheRestOfTheSeriesOpen() { val id = source.insertTask(form(title = "Water the plants")) - makeRecurring(id, now) + // Overdue occurrences before it, so it is not the first and forks an override. + makeRecurring(id, now - 14.days) val target = source.tasks(TaskQuery(listId = listId)) .first { it.taskId == id && it.distanceFromCurrent == 0 } @@ -269,6 +272,90 @@ class RoomTasksDataSourceTest { assertThat(db.tasks().override(id, target.occurrenceStart)!!.status).isEqualTo(TaskStatus.NEEDS_ACTION) } + @Test + fun completingTheFirstOccurrenceMovesTheSeriesOnAndKeepsACopy() { + val at = now + val id = source.insertTask(form(title = "Water the plants")) + makeRecurring(id, at, rule = "FREQ=WEEKLY;COUNT=4") + + source.setCompletedInstance(id, at, completed = true) + + val master = db.tasks().entity(id)!! + assertThat(master.status).isEqualTo(TaskStatus.NEEDS_ACTION) + assertThat(master.dtstart).isEqualTo(at + 7.days) + assertThat(master.due).isEqualTo(at + 8.days) + assertThat(master.rrule).isEqualTo("FREQ=WEEKLY;COUNT=3") + assertThat(master.isDirty).isTrue() + assertThat(db.tasks().overrides(id)).isEmpty() + val copy = db.tasks().allIn(listId).single { it.id != id } + assertThat(copy.status).isEqualTo(TaskStatus.COMPLETED) + assertThat(copy.uid).isNotEqualTo(master.uid) + assertThat(copy.rrule).isNull() + assertThat(copy.masterId).isNull() + assertThat(copy.dtstart).isEqualTo(at) + assertThat(copy.title).isEqualTo("Water the plants") + } + + @Test + fun reopeningTheOccurrenceMovesTheSeriesBack() { + val at = now + val id = source.insertTask(form(title = "Water the plants")) + makeRecurring(id, at, rule = "FREQ=WEEKLY;COUNT=4") + source.setCompletedInstance(id, at, completed = true) + + source.setCompletedInstance(id, at, completed = false) + + val master = db.tasks().entity(id)!! + assertThat(master.dtstart).isEqualTo(at) + assertThat(master.rrule).isEqualTo("FREQ=WEEKLY;COUNT=4") + assertThat(db.tasks().allIn(listId).filter { !it.isDeleted }.map { it.id }).containsExactly(id) + } + + @Test + fun completingAnEditedFirstOccurrenceKeepsItsEdits() { + val at = now + val id = source.insertTask(form(title = "Water the plants")) + makeRecurring(id, at) + source.updateInstance(id, at, TaskForm(title = "Water the cactus", listId = listId, start = at, due = at + 1.days), reminders = null) + val override = db.tasks().override(id, at)!! + + source.setCompletedInstance(override.id, at, completed = true) + + assertThat(db.tasks().overrides(id)).isEmpty() + assertThat(db.tasks().entity(id)!!.dtstart).isEqualTo(at + 7.days) + val copy = db.tasks().allIn(listId).single { it.id != id } + assertThat(copy.title).isEqualTo("Water the cactus") + assertThat(copy.status).isEqualTo(TaskStatus.COMPLETED) + } + + @Test + fun completingTheLastOccurrenceClosesTheSeries() { + val at = now + val id = source.insertTask(form(title = "Water the plants")) + makeRecurring(id, at, rule = "FREQ=WEEKLY;COUNT=1") + + source.setCompletedInstance(id, at, completed = true) + + assertThat(db.tasks().entity(id)!!.status).isEqualTo(TaskStatus.COMPLETED) + assertThat(db.tasks().allIn(listId).map { it.id }).containsExactly(id) + + source.setCompletedInstance(id, at, completed = false) + + assertThat(db.tasks().entity(id)!!.status).isEqualTo(TaskStatus.NEEDS_ACTION) + } + + @Test + fun cancellingTheFirstOccurrenceMovesTheSeriesOn() { + val at = now + val id = source.insertTask(form(title = "Water the plants")) + makeRecurring(id, at) + + source.setCancelledInstance(id, at, cancelled = true) + + assertThat(db.tasks().entity(id)!!.dtstart).isEqualTo(at + 7.days) + assertThat(db.tasks().allIn(listId).single { it.id != id }.status).isEqualTo(TaskStatus.CANCELLED) + } + @Test fun completingANonRecurringTaskThroughTheInstancePathWritesTheRowItself() { val id = source.insertTask(form(title = "Buy milk", due = now + 1.days)) @@ -657,10 +744,12 @@ class RoomTasksDataSourceTest { val outOfWindow = db.tasks().insert( TaskEntity(listId = listId, uid = "old", title = "old", dtstart = longAgo, rrule = "FREQ=DAILY;COUNT=2", parentId = parent), ) + val at = now val series = db.tasks().insert( - TaskEntity(listId = listId, uid = "s", title = "s", dtstart = now, rrule = "FREQ=DAILY;COUNT=3", parentId = parent), + TaskEntity(listId = listId, uid = "s", title = "s", dtstart = at, rrule = "FREQ=DAILY;COUNT=3", parentId = parent), ) - source.setCompletedInstance(series, now, true) + // Not the first occurrence, so it forks an override rather than a copy. + source.setCompletedInstance(series, at + 1.days, true) val plain = source.insertTask(form("plain").copy(parentId = parent)) assertThat(source.subtaskIds(parent)).containsExactly(outOfWindow, series, plain) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt index 60edba7..cd0d242 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt @@ -28,6 +28,11 @@ private val WINDOW_FORWARD = 730.days /** Residue lines that describe the old series and must not follow the new one. */ private val SERIES_LINES = listOf("EXDATE", "RDATE", "RRULE", "RECURRENCE-ID", "UID") +private const val OCCURRENCE_MARK = "X-AGENDULA-OCCURRENCE-OF:" + +/** How far ahead to look for a series' next occurrence: past any yearly gap, with room to spare. */ +private val SERIES_LOOKAHEAD = (366 * 10).days + /** Enough to count every occurrence before a split point without bounding a real series. */ internal const val SPLIT_COUNT_CEILING = 100_000 @@ -479,18 +484,138 @@ class RoomTasksDataSource @Inject constructor( } /** - * Ticking one occurrence forks a `RECURRENCE-ID` override carrying the - * completion — the same model [updateInstance] writes. Writing the status onto - * the master instead would close the series: the master is what - * [TaskDao.tasks] filters on, so every occurrence, past and future, would - * leave every list at once. + * Ticking a series' first open occurrence moves the series on to its next + * one and keeps a completed copy, as tasks.org and Apple Reminders do — a + * completed `RECURRENCE-ID` override alone leaves the series showing as open + * in most other clients (#137). Unticking that copy's occurrence moves the + * series back. Any other occurrence forks an override carrying the completion, + * the same model [updateInstance] writes. Writing the status onto the master + * instead would close the series. */ - override fun setCompletedInstance(taskId: Long, occurrenceStart: Instant, completed: Boolean) = - writeInstance(taskId, occurrenceStart, "complete") { TaskFormWriter.completed(it, completed, clock.now()) } + override fun setCompletedInstance(taskId: Long, occurrenceStart: Instant, completed: Boolean) = tx { + val close = { it: TaskEntity -> TaskFormWriter.completed(it, completed, clock.now()) } + if (!moveSeries(taskId, occurrenceStart, closing = completed, close)) { + writeInstance(taskId, occurrenceStart, "complete", close) + } + } - /** Same override model as [setCompletedInstance]. */ - override fun setCancelledInstance(taskId: Long, occurrenceStart: Instant, cancelled: Boolean) = - writeInstance(taskId, occurrenceStart, "cancel") { TaskFormWriter.cancelled(it, cancelled, clock.now()) } + /** Same model as [setCompletedInstance]. */ + override fun setCancelledInstance(taskId: Long, occurrenceStart: Instant, cancelled: Boolean) = tx { + val close = { it: TaskEntity -> TaskFormWriter.cancelled(it, cancelled, clock.now()) } + if (!moveSeries(taskId, occurrenceStart, closing = cancelled, close)) { + writeInstance(taskId, occurrenceStart, "cancel", close) + } + } + + /** + * Moves the series behind [taskId] past [occurrenceStart] when [closing] its + * first occurrence, or back onto it when reopening the one it last moved past. + * False when neither applies, so the caller writes an override instead. + */ + private fun moveSeries(taskId: Long, occurrenceStart: Instant, closing: Boolean, close: (TaskEntity) -> TaskEntity): Boolean { + val row = tasks.entity(taskId) ?: return false + val master = row.masterId?.let { tasks.entity(it) } ?: row + if (master.recurrenceSpec() == null) return false + val override = if (row.masterId != null) row else tasks.override(master.id, occurrenceStart) + val anchor = override?.recurrenceId ?: occurrenceStart + val closed = master.status == TaskStatus.COMPLETED || master.status == TaskStatus.CANCELLED + if (closed) { + // Only its last occurrence closes a series; reopening that one reopens it. + if (closing || master.recurrenceAnchor?.let { sameSecond(it, anchor) } != true) return false + tasks.update(close(master)) + override?.let { tasks.update(close(it)) } + writes.onWritten(setOf(master.listId)) + return true + } + return if (closing) advance(master, anchor, override, close) else rewind(master, anchor) + } + + private fun advance(master: TaskEntity, anchor: Instant, override: TaskEntity?, close: (TaskEntity) -> TaskEntity): Boolean { + val (first, next) = firstTwo(master) ?: return false + if (!sameSecond(first, anchor)) return false + if (next == null) { + tasks.update(close(master)) + override?.let { tasks.update(close(it)) } + writes.onWritten(setOf(master.listId)) + return true + } + val now = clock.now() + val (start, due) = override?.let { it.dtstart to it.due } ?: occurrenceTimes(master, anchor) + val copy = close( + (override ?: master).copy( + id = 0, + uid = UUID.randomUUID().toString(), + masterId = null, + recurrenceId = null, + href = null, + etag = null, + rrule = null, + rdate = null, + exdate = null, + dtstart = start, + due = due, + createdAt = now, + sequence = 0, + isDeleted = false, + unknownProperties = (seriesFree(master.unknownProperties) + occurrenceMark(master, anchor)).joinToString("\r\n"), + ), + ) + val copyId = tasks.insert(copy) + alarms.copyToTask(override?.id ?: master.id, copyId, sameResource = false) + override?.let { tasks.delete(it.id) } + // Keeps the anchor's own sub-second part, which expansion drops. + tasks.update(shifted(master, master.recurrenceAnchor!! + (next - first), spent = 1, now)) + writes.onWritten(setOf(master.listId)) + return true + } + + private fun rewind(master: TaskEntity, anchor: Instant): Boolean { + val anchorNow = master.recurrenceAnchor ?: return false + if (anchor >= anchorNow || sameSecond(anchor, anchorNow)) return false + val back = shifted(master, anchor, spent = -1, clock.now()) + val (first, next) = firstTwo(back) ?: return false + if (!sameSecond(first, anchor) || next == null || !sameSecond(next, anchorNow)) return false + tasks.update(back) + val mark = occurrenceMark(master, anchor) + tasks.allIn(master.listId) + .filter { !it.isDeleted && it.unknownProperties?.lines()?.contains(mark) == true } + .forEach { deleteTask(it.id) } + writes.onWritten(setOf(master.listId)) + return true + } + + /** The series' first occurrence and the one after it (null when it has no other). */ + private fun firstTwo(master: TaskEntity): Pair? { + val spec = master.recurrenceSpec() ?: return null + val from = Instant.fromEpochSeconds(spec.anchor.epochSeconds) + val occurrences = RecurrenceExpander.expand(spec, ExpansionWindow(from = from, until = from + SERIES_LOOKAHEAD, maxOccurrences = 2)) + val first = occurrences.firstOrNull() ?: return null + return first to occurrences.getOrNull(1) + } + + /** [master] anchored on [at], its `COUNT` reduced by [spent] and the exceptions before [at] dropped. */ + private fun shifted(master: TaskEntity, at: Instant, spent: Int, now: Instant): TaskEntity { + val (start, due) = occurrenceTimes(master, at) + val zone = master.timezone ?: zone() + return master.copy( + dtstart = start, + due = due, + rrule = master.rrule?.let { TaskFormWriter.remainingCount(it, spent) }, + exdate = if (spent > 0) TaskFormWriter.valuesFrom(master.exdate, at, zone) else master.exdate, + lastModified = now, + isDirty = true, + ) + } + + private fun seriesFree(unknown: String?): List = + unknown?.lines()?.filterNot { line -> SERIES_LINES.any { line.startsWith(it, ignoreCase = true) } || line.startsWith(OCCURRENCE_MARK) } + ?.filter { it.isNotBlank() }.orEmpty() + + /** Expansion runs in whole seconds; stored anchors may carry milliseconds. */ + private fun sameSecond(a: Instant, b: Instant) = a.epochSeconds == b.epochSeconds + + /** Links a completed copy to the occurrence it was split off, so reopening it can move the series back. */ + private fun occurrenceMark(master: TaskEntity, anchor: Instant) = "$OCCURRENCE_MARK${master.uid};${anchor.toEpochMilliseconds()}" /** Applies [write] to the override for [occurrenceStart], forking one if there is none yet. */ private fun writeInstance(