From d123c00def6bd56ae887e52121f7500ca49fcca9 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 15 Oct 2026 16:14:28 +0200 Subject: [PATCH] Fix review findings on provider writes (#63) Write-back of a read row now lets the changed completion column decide, keeps synced rdate/exdate as stored, floats tz-less inserts, deletes whole subtrees on delete-following, and reports real validation errors. --- .../data/tasks/provider/ProviderWriterTest.kt | 119 ++++++++++++++++++ .../tasks/room/RoomTasksDataSourceTest.kt | 25 ++++ .../data/tasks/AndroidTasksDataSource.kt | 23 +++- .../data/tasks/ModeRoutingTasksDataSource.kt | 1 + .../agendula/data/tasks/TasksDataSource.kt | 15 +++ .../data/tasks/TasksRepositoryImpl.kt | 14 +-- .../data/tasks/provider/ProviderSnapshot.kt | 37 ++++-- .../data/tasks/provider/ProviderTaskWriter.kt | 91 ++++++++++---- .../data/tasks/provider/ProviderWriter.kt | 73 +++++++---- .../data/tasks/room/RoomTasksDataSource.kt | 6 +- .../data/tasks/provider/ProviderIdsTest.kt | 29 +++++ .../tasks/provider/ProviderTaskWriterTest.kt | 101 ++++++++++++++- docs/CONTENT_PROVIDER.md | 10 +- 13 files changed, 467 insertions(+), 77 deletions(-) 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 e9c01a4..f4383a9 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 @@ -676,6 +676,125 @@ class ProviderWriterTest { assertThat(writer.delete(ProviderTable.PROPERTIES, byId(ProviderTable.PROPERTIES, unknown), null)).isEqualTo(0) } + // --- second review regressions ----------------------------------------------- + + @Test + fun tickingOffAModifiedOccurrenceAsReadLeavesTheSeriesAlone() { + val series = dailySeries("Standup") + // Reopening forks an override, which then gets its own title as the app's "this occurrence" edit would. + 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)) + + writer.update(ProviderTable.INSTANCES, row + (Tasks.STATUS to 2), byId(ProviderTable.INSTANCES, instanceId(today)), null) + + assertThat(task(series)!!.title).isEqualTo("Standup") + assertThat(task(override.id)!!.title).isEqualTo("Standup (moved room)") + assertThat(task(override.id)!!.status).isEqualTo(TaskStatus.COMPLETED) + } + + @Test + fun anInstanceWrittenBackWithOnlyProgressChangedIsCompleted() { + val series = dailySeries() + val row = readRow(ProviderTable.INSTANCES, instanceId(today)) + + writer.update(ProviderTable.INSTANCES, row + (Tasks.PERCENT_COMPLETE to 100), byId(ProviderTable.INSTANCES, instanceId(today)), null) + + assertThat(db.tasks().overrides(series).single().status).isEqualTo(TaskStatus.COMPLETED) + } + + @Test + fun aBadZoneOnAnInstanceReportsTheZone() { + dailySeries() + val instance = instanceId(today) + + val error = assertThrows(IllegalArgumentException::class.java) { + writer.update(ProviderTable.INSTANCES, mapOf(Tasks.TZ to "Europe/Berln"), byId(ProviderTable.INSTANCES, instance), null) + } + + assertThat(error).hasMessageThat().contains("Unknown time zone") + } + + @Test + fun anInProcessSeriesCanBeWrittenBackAsRead() { + val series = dailySeries() + db.tasks().update(task(series)!!.copy(status = TaskStatus.IN_PROCESS, percentComplete = 50)) + val row = readRow(ProviderTable.INSTANCES, instanceId(today)) + + writer.update(ProviderTable.INSTANCES, row + (Tasks.TITLE to "Renamed"), byId(ProviderTable.INSTANCES, instanceId(today)), null) + + assertThat(task(series)!!.title).isEqualTo("Renamed") + assertThat(task(series)!!.status).isEqualTo(TaskStatus.IN_PROCESS) + assertThat(db.tasks().overrides(series)).isEmpty() + } + + @Test + fun aFloatingSeriesCanBeWrittenBackAsRead() { + 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)) + + writer.update(ProviderTable.INSTANCES, row + (Tasks.STATUS to 2), byId(ProviderTable.INSTANCES, instanceId(today)), 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) + assertThat(task(series)!!.timezone).isNull() + } + + @Test + fun aParentLoopDeletesInsteadOfRecursingForever() { + val a = insert(Tasks.TITLE to "a") + val b = insert(Tasks.TITLE to "b", Tasks.PARENT_ID to a) + db.tasks().update(task(a)!!.copy(parentId = b)) + + writer.delete(ProviderTable.TASKS, byId(ProviderTable.TASKS, a), null) + + assertThat(task(a)).isNull() + assertThat(task(b)).isNull() + } + + @Test + fun aBulkMoveWorksWhicheverOrderTheRowsComeIn() { + val work = synced() + // The subtask gets the lower id, so it comes first unless the writer reorders. + val child = insert(Tasks.TITLE to "child") + val parent = insert(Tasks.TITLE to "parent") + db.tasks().update(task(child)!!.copy(parentId = parent)) + + val count = writer.update(ProviderTable.TASKS, mapOf(Tasks.LIST_ID to work), "${Tasks.LIST_ID} = ?", arrayOf(home.toString())) + + assertThat(count).isEqualTo(2) + assertThat(task(child)!!.listId).isEqualTo(work) + assertThat(task(parent)!!.listId).isEqualTo(work) + assertThat(task(child)!!.parentId).isEqualTo(parent) + } + + @Test + fun aRowSelectedWithAnArgumentIsWritten() { + val a = insert(Tasks.TITLE to "a") + val b = insert(Tasks.TITLE to "b") + + assertThat(writer.update(ProviderTable.TASKS, mapOf(Tasks.TITLE to "x"), "${Tasks.ID}=?", arrayOf(b.toString()))).isEqualTo(1) + assertThat(writer.update(ProviderTable.TASKS, mapOf(Tasks.TITLE to "x"), "${Tasks.ID} = ?", arrayOf("987654"))).isEqualTo(0) + + assertThat(task(a)!!.title).isEqualTo("a") + assertThat(task(b)!!.title).isEqualTo("x") + } + + @Test + fun garbageExceptionDatesAreRejected() { + val series = dailySeries() + + assertThrows(IllegalArgumentException::class.java) { + writer.update(ProviderTable.TASKS, mapOf(Tasks.EXDATE to "tomorrow"), byId(ProviderTable.TASKS, series), null) + } + assertThat(task(series)!!.exdate).isNull() + } + + private fun store() = RoomTasksDataSource(db) + /** Every column of one published row, as a client that read it would write it back. */ private fun readRow(table: ProviderTable, id: Long): Map { val columns = table.columns.map { it.first } 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 01b2f9a..4782f41 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 @@ -555,5 +555,30 @@ class RoomTasksDataSourceTest { assertThat(source.subtaskIds(parent)).containsExactly(outOfWindow, series, plain) } + + @Test + fun deletingASubtreeEndsOnAParentLoop() { + val a = source.insertTask(form("a")) + val b = source.insertTask(form("b").copy(parentId = a)) + val c = source.insertTask(form("c").copy(parentId = b)) + db.tasks().update(db.tasks().entity(a)!!.copy(parentId = c)) + + source.deleteWithSubtasks(a) + + assertThat(listOf(a, b, c).mapNotNull { db.tasks().entity(it) }).isEmpty() + } + + @Test + fun deletingFollowingFromTheFirstOccurrenceTakesTheWholeSubtree() { + val series = db.tasks().insert( + TaskEntity(listId = listId, uid = "s", title = "s", dtstart = now, rrule = "FREQ=DAILY;COUNT=3"), + ) + val child = source.insertTask(form("child").copy(parentId = series)) + val grandchild = source.insertTask(form("grandchild").copy(parentId = child)) + + source.deleteFollowing(series, now) + + assertThat(listOf(series, child, grandchild).mapNotNull { db.tasks().entity(it) }).isEmpty() + } } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/AndroidTasksDataSource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/AndroidTasksDataSource.kt index b87069c..dc91251 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/AndroidTasksDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/AndroidTasksDataSource.kt @@ -85,6 +85,27 @@ class AndroidTasksDataSource @Inject constructor( override fun subtasks(parentTaskId: Long): List = queryInstances("${Tasks.PARENT_ID} = ?", arrayOf(parentTaskId.toString())) + /** + * Read from the `tasks` table, so a recurring subtask with no occurrence in + * the window still counts. All columns and a lenient `_deleted`, as in + * [exportTasks], since the table's shape varies across providers. + */ + override fun subtaskIds(parentTaskId: Long): List = resolver.query( + TasksContract.tasksUri(authority()), + null, + "${Tasks.PARENT_ID} = ? AND (${Tasks.DELETED} IS NULL OR ${Tasks.DELETED} = 0)", + arrayOf(parentTaskId.toString()), + null, + )?.use { c -> + val r = CursorColumnReader(c) + buildList { + while (c.moveToNext()) { + if (r.getLong(Tasks.ORIGINAL_INSTANCE_ID) != null) continue + r.getLong(Tasks.ID)?.takeIf { it != parentTaskId }?.let(::add) + } + } + } ?: emptyList() + private fun queryInstances(selection: String?, args: Array?): List { val uri = TasksContract.instancesUri(authority()) // projection = null (all columns): the instances view differs across @@ -201,7 +222,7 @@ class AndroidTasksDataSource @Inject constructor( override fun deleteFollowing(seriesId: Long, occurrenceStart: Instant) { val spec = seriesSpec(seriesId) ?: return - if (spec.rrule == null || occurrenceStart <= spec.anchor) return deleteTask(seriesId) + if (spec.rrule == null || occurrenceStart <= spec.anchor) return deleteWithSubtasks(seriesId) endSeriesBefore(seriesId, spec, occurrenceStart) } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ModeRoutingTasksDataSource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ModeRoutingTasksDataSource.kt index 1454885..70f5fce 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ModeRoutingTasksDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ModeRoutingTasksDataSource.kt @@ -44,6 +44,7 @@ class ModeRoutingTasksDataSource( active().deleteFollowing(seriesId, occurrenceStart) override fun subtasks(parentTaskId: Long): List = active().subtasks(parentTaskId) override fun subtaskIds(parentTaskId: Long): List = active().subtaskIds(parentTaskId) + override fun deleteWithSubtasks(taskId: Long) = active().deleteWithSubtasks(taskId) override fun insertTask(form: TaskForm): Long = active().insertTask(form) override fun updateTask(taskId: Long, form: TaskForm) = active().updateTask(taskId, form) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksDataSource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksDataSource.kt index 2467a4a..394c187 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksDataSource.kt @@ -54,6 +54,21 @@ interface TasksDataSource { fun subtaskIds(parentTaskId: Long): List = subtasks(parentTaskId).map { it.seriesId ?: it.taskId }.distinct().filter { it != parentTaskId } + /** + * Delete [taskId] and its subtasks, all the way down; left behind they would + * hang off a tombstone. Each task once, so a parent loop an import brought in + * ends instead of recursing forever. + */ + fun deleteWithSubtasks(taskId: Long) { + val seen = HashSet() + fun delete(id: Long) { + if (!seen.add(id)) return + subtaskIds(id).forEach(::delete) + deleteTask(id) + } + delete(taskId) + } + fun insertTask(form: TaskForm): Long fun updateTask(taskId: Long, form: TaskForm) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksRepositoryImpl.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksRepositoryImpl.kt index 9df349d..057e6c0 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksRepositoryImpl.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksRepositoryImpl.kt @@ -248,7 +248,7 @@ class TasksRepositoryImpl @Inject constructor( val current = dataSource.task(taskId, occurrenceStart) val seriesId = current?.seriesId val occurrence = current?.occurrenceStart - if (seriesId == null || occurrence == null) return@withContext deleteWithSubtasks(taskId) + if (seriesId == null || occurrence == null) return@withContext dataSource.deleteWithSubtasks(taskId) when (scope) { RecurringScope.ThisOccurrence -> // An override row deletes itself into an EXDATE; a generated @@ -256,20 +256,10 @@ class TasksRepositoryImpl @Inject constructor( if (current.taskId != seriesId) dataSource.deleteTask(current.taskId) else dataSource.deleteOccurrence(seriesId, occurrence) RecurringScope.ThisAndFollowing -> dataSource.deleteFollowing(seriesId, occurrence) - RecurringScope.AllOccurrences -> deleteWithSubtasks(seriesId) + RecurringScope.AllOccurrences -> dataSource.deleteWithSubtasks(seriesId) } } - /** - * A parent goes with its subtasks, all the way down; left behind they would - * hang off a tombstone. By task row, not occurrence: deleting an overridden - * occurrence's row would only EXDATE it. - */ - private fun deleteWithSubtasks(taskId: Long) { - dataSource.subtaskIds(taskId).forEach(::deleteWithSubtasks) - dataSource.deleteTask(taskId) - } - override suspend fun createLocalList(name: String, color: Int): Long = withContext(io) { dataSource.createLocalList(name, color) } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderSnapshot.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderSnapshot.kt index 021a57c..ae58715 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderSnapshot.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderSnapshot.kt @@ -21,7 +21,9 @@ typealias ProviderRow = Map /** * Ids that stay put for as long as what they name exists, so a client can read - * an id and write to it later, or several in one batch. + * an id and write to it later, or several in one batch. The one exception is + * two tags on one task whose spellings hash alike (about one in 2³¹): removing + * the one that sorts first moves the other onto its id. * * An instance is its series and its original time; a plain task's one * instance has none. An alarm property is the alarm row; a tag property is its @@ -43,17 +45,34 @@ object ProviderIds { fun alarmRow(propertyId: Long): Long = propertyId ushr 1 - fun categoryTask(propertyId: Long): Long = propertyId ushr 33 - /** The property id of each of [taskId]'s [categories], by spelling. */ - fun categories(taskId: Long, categories: List): Map { + fun categories(taskId: Long, categories: List): Map = + categories(taskId, categories, ::tagHash) + + internal fun categories(taskId: Long, categories: List, hash: (String) -> Long): Map { val used = HashSet() - return categories.associateWith { name -> - var id = (((taskId shl 32) or (name.hashCode().toLong() and 0x7FFFFFFFL)) shl 1) or 1L - // Two spellings with one hash: the later takes the next free odd id. - while (!used.add(id)) id += 2 - id + // Two spellings on one task with one hash: the one that sorts later takes + // the next free slot, wrapping inside the tag bits so it never reaches the + // task's. Sorting keeps that independent of the tags' order. + val ids = categories.sorted().associateWith { name -> + var bits = hash(name) and TAG_BITS + while (!used.add(bits)) bits = (bits + 1) and TAG_BITS + (((taskId shl 31) or bits) shl 1) or 1L } + return categories.associateWith(ids::getValue) + } + + fun categoryTask(propertyId: Long): Long = propertyId ushr 32 + + private const val TAG_BITS = 0x7FFFFFFFL + + /** 64-bit FNV-1a over the spelling's UTF-8, folded to 31 bits. */ + private fun tagHash(name: String): Long { + var hash = -0x340d631b7bdddcdbL + name.encodeToByteArray().forEach { byte -> + hash = (hash xor (byte.toLong() and 0xFF)) * 0x100000001b3L + } + return (hash xor (hash ushr 31) xor (hash ushr 62)) and TAG_BITS } } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriter.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriter.kt index 54b9c92..0fa71c6 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriter.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriter.kt @@ -32,6 +32,8 @@ val TIME_COLUMNS: Set = setOf( /** What a write to one occurrence of a series does to it. */ enum class OccurrenceChange { COMPLETE, CANCEL, REOPEN } +private val ICAL_DATE = Regex("""\d{8}(T\d{6}Z?)?""") + private val ICAL_DURATION = Regex("""[+-]?P(\d+W|(\d+D)?(T(\d+H)?(\d+M)?(\d+S)?)?)""") /** @@ -62,7 +64,9 @@ object ProviderTaskWriter { val v = Values(values) val listId = requireNotNull(v.long(Tasks.LIST_ID)) { "A new task needs a ${Tasks.LIST_ID}" } val base = TaskEntity(listId = listId, uid = v.string(Tasks.UID)?.trim()?.ifEmpty { null } ?: uid, createdAt = now) - return applied(base, values - Tasks.LIST_ID - Tasks.UID, now, tzId) + // As in OpenTasks, a new task inserted without a zone is floating. + val zoned = if (Tasks.TZ in v) values else values + (Tasks.TZ to null) + return applied(base, zoned - Tasks.LIST_ID - Tasks.UID, now, tzId) } /** @@ -75,15 +79,17 @@ object ProviderTaskWriter { return if (edited.copy(lastModified = current.lastModified, isDirty = current.isDirty) == current) current else edited } - /** The [TIME_COLUMNS] in [values] that would change [current]; writing back the ones a row published changes nothing. */ - fun changedTimeColumns(current: TaskEntity, values: ProviderValues, tzId: String): Set { - val times = values.filterKeys { it in TIME_COLUMNS } - val edited = applied(current, times, current.lastModified ?: Instant.DISTANT_PAST, tzId) - val same = edited.dtstart == current.dtstart && edited.due == current.due && - edited.duration == current.duration && edited.isAllDay == current.isAllDay && - edited.timezone == current.timezone && edited.rrule == current.rrule && - edited.rdate == current.rdate && edited.exdate == current.exdate - return if (same) emptySet() else times.keys + /** + * The columns in [values] that would change [current] on their own. Writing + * back what a row published changes nothing, so a client that read a row can + * send all of it with the one value it meant to change. + */ + fun changedColumns(current: TaskEntity, values: ProviderValues, tzId: String): Set { + val stamp = current.lastModified ?: Instant.DISTANT_PAST + return values.keys.filterTo(HashSet()) { key -> + val edited = runCatching { applied(current, mapOf(key to values[key]), stamp, tzId) }.getOrNull() + edited == null || edited.copy(lastModified = current.lastModified, isDirty = current.isDirty) != current + } } private fun applied(current: TaskEntity, values: ProviderValues, now: Instant, tzId: String): TaskEntity { @@ -119,15 +125,16 @@ object ProviderTaskWriter { Tasks.TZ in v -> v.string(Tasks.TZ)?.also { zone -> require(runCatching { ZoneId.of(zone) }.isSuccess) { "Unknown time zone: $zone" } } - current.timezone != null -> current.timezone - dtstart != null || due != null -> tzId - else -> null + dtstart == null && due == null -> null + // A timed task without a zone is floating (RFC 5545 §3.3.5) and stays so. + current.dtstart != null || current.due != null -> if (current.isAllDay) tzId else current.timezone + else -> current.timezone ?: tzId } val rrule = if (Tasks.RRULE in v) normalizedRule(v.string(Tasks.RRULE)) else current.rrule val ruleDropped = Tasks.RRULE in v && rrule == null && current.rrule != null - val rdate = if (Tasks.RDATE in v) v.string(Tasks.RDATE)?.trim()?.ifEmpty { null } else current.rdate.takeUnless { ruleDropped } - val exdate = if (Tasks.EXDATE in v) v.string(Tasks.EXDATE)?.trim()?.ifEmpty { null } else current.exdate.takeUnless { ruleDropped } + val rdate = if (Tasks.RDATE in v) dateList(Tasks.RDATE, v, current.rdate) else current.rdate.takeUnless { ruleDropped } + val exdate = if (Tasks.EXDATE in v) dateList(Tasks.EXDATE, v, current.exdate) else current.exdate.takeUnless { ruleDropped } require(rrule == null || dtstart != null || due != null) { "A recurring task needs a ${Tasks.DTSTART} or ${Tasks.DUE}" } val (status, percent, completedAt) = completion(current, v, now) @@ -165,7 +172,7 @@ object ProviderTaskWriter { * partial progress belongs to a task, not to one of its occurrences. */ fun occurrenceChange(values: ProviderValues): OccurrenceChange? { - val v = Values(values) + val v = Values(values.filterKeys { it in COMPLETION_COLUMNS }) return when { Tasks.STATUS in v -> when (v.int(Tasks.STATUS, 0..3)) { TasksContract.STATUS_COMPLETED -> OccurrenceChange.COMPLETE @@ -186,15 +193,19 @@ object ProviderTaskWriter { /** * Status, progress and completion time after a write. They move together the * way OpenTasks keeps them: a status implies its progress and time, progress - * implies a status, and a completion time alone completes the task. + * implies a status, and a completion time alone completes the task. When a + * row is written back, the column that changed decides, not the ones it + * carries unchanged. */ - private fun completion(current: TaskEntity, v: Values, now: Instant): Triple { + private fun completion(current: TaskEntity, values: Values, now: Instant): Triple { + val v = Values(values.map.filterKeys { it !in COMPLETION_COLUMNS || it in completionChanged(current, values) }) val percent = if (Tasks.PERCENT_COMPLETE in v) v.int(Tasks.PERCENT_COMPLETE, 0..100) else current.percentComplete val completed = if (Tasks.COMPLETED in v) v.instant(Tasks.COMPLETED) else current.completedAt return when { Tasks.STATUS in v -> when (val status = statusFromInt(v.int(Tasks.STATUS, 0..3))) { TaskStatus.COMPLETED -> Triple(status, if (Tasks.PERCENT_COMPLETE in v) percent else 100, completed ?: now) - TaskStatus.NEEDS_ACTION -> Triple(status, if (Tasks.PERCENT_COMPLETE in v) percent else null, null) + TaskStatus.NEEDS_ACTION -> + Triple(status, if (Tasks.PERCENT_COMPLETE in v) percent?.takeIf { it < 100 } else null, null) else -> Triple(status, percent, null) } Tasks.PERCENT_COMPLETE in v -> when { @@ -203,13 +214,47 @@ object ProviderTaskWriter { percent > 0 -> Triple(TaskStatus.IN_PROCESS, percent, null) else -> Triple(TaskStatus.NEEDS_ACTION, percent, null) } - Tasks.COMPLETED in v -> - if (completed != null) Triple(TaskStatus.COMPLETED, 100, completed) - else Triple(TaskStatus.NEEDS_ACTION, null, null) + Tasks.COMPLETED in v -> when { + completed != null -> Triple(TaskStatus.COMPLETED, 100, completed) + // Clearing the time reopens a completed task; an open one has none to clear. + current.status == TaskStatus.COMPLETED -> Triple(TaskStatus.NEEDS_ACTION, null, null) + else -> Triple(current.status, current.percentComplete, null) + } else -> Triple(current.status, current.percentComplete, current.completedAt) } } + /** + * The completion columns in [v] that differ from [current]. All of them when + * none does, so a write that only repeats the row still settles the rest. + */ + private fun completionChanged(current: TaskEntity, v: Values): Set { + val present = COMPLETION_COLUMNS.filterTo(HashSet()) { it in v } + val changed = present.filterTo(HashSet()) { column -> + when (column) { + Tasks.STATUS -> statusFromInt(v.int(Tasks.STATUS, 0..3)) != current.status + Tasks.PERCENT_COMPLETE -> v.int(Tasks.PERCENT_COMPLETE, 0..100) != current.percentComplete + else -> v.instant(Tasks.COMPLETED) != current.completedAt + } + } + return changed.ifEmpty { present } + } + + /** + * An `RDATE`/`EXDATE` value: DATE or DATE-TIME values, comma-separated, of + * one kind. [stored] written back as is stays, whatever shape it synced in. + */ + private fun dateList(column: String, v: Values, stored: String?): String? { + val raw = v.string(column)?.trim()?.ifEmpty { null } ?: return null + if (raw == stored?.trim()) return stored + val values = raw.split(',').map(String::trim) + require(values.all { ICAL_DATE.matches(it) }) { "$column takes iCalendar dates or date-times: $raw" } + // lib-recur reads the whole list or none of it, so it has to be one kind. + val kinds = values.map { if (it.length == 8) "date" else if (it.endsWith('Z')) "utc" else "local" } + require(kinds.distinct().size == 1) { "$column mixes value kinds: $raw" } + return values.joinToString(",") + } + private fun normalizedRule(raw: String?): String? { val rule = raw?.trim()?.removePrefix("RRULE:")?.ifEmpty { null } ?: return null require(runCatching { RecurrenceRule(rule) }.isSuccess) { "Not a recurrence rule: $rule" } @@ -222,6 +267,8 @@ internal class Values(private val values: ProviderValues) { val keys: Set get() = values.keys + val map: ProviderValues get() = values + operator fun contains(key: String) = key in values fun string(key: String): String? = values[key]?.toString() diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriter.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriter.kt index 38943ec..7b1036f 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriter.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderWriter.kt @@ -28,8 +28,8 @@ import javax.inject.Singleton import kotlin.time.Clock import kotlin.time.Instant -/** A row URI's selection, ` = N`. */ -private val SINGLE_ROW = Regex("""\s*(\w+)\s*=\s*(\d+)\s*""") +/** One row by id, ` = N` or ` = ?`. */ +private val SINGLE_ROW = Regex("""\s*(\w+)\s*=\s*(\d+|\?)\s*""") /** * Applies another app's writes to the own store for [AgendulaTaskProvider]. @@ -79,7 +79,8 @@ class ProviderWriter internal constructor( when (table) { ProviderTable.TASKS -> { val writable = ProviderTaskWriter.writable(table, values, WRITABLE_TASK_COLUMNS) - ids(table, selection, args).count { updateTask(it, writable) } + val matched = ids(table, selection, args) + (if (Tasks.LIST_ID in writable) parentsFirst(matched) else matched).count { updateTask(it, writable) } } ProviderTable.INSTANCES -> { val writable = ProviderTaskWriter.writable(table, values, WRITABLE_TASK_COLUMNS) @@ -161,31 +162,44 @@ class ProviderWriter internal constructor( if (edited.parentId != current.parentId || moved) requireParent(edited) store.replaceEntity(current, edited) // A subtree moves as one: a subtask has to be in its parent's list. - if (moved) store.subtaskIds(taskId).forEach { moveSubtree(it, edited.listId) } + if (moved) store.subtaskIds(taskId).forEach { moveSubtree(it, edited.listId, hashSetOf(taskId)) } return true } - private fun moveSubtree(taskId: Long, listId: Long) { + private fun moveSubtree(taskId: Long, listId: Long, seen: MutableSet = HashSet()) { + if (!seen.add(taskId)) return val current = database.tasks().entity(taskId)?.takeUnless { it.isDeleted } ?: return - requireFreeUid(listId, current.uid) - store.replaceEntity(current, current.copy(listId = listId, lastModified = clock.now(), isDirty = true)) - store.subtaskIds(taskId).forEach { moveSubtree(it, listId) } + if (current.listId != listId) { + requireFreeUid(listId, current.uid) + store.replaceEntity(current, current.copy(listId = listId, lastModified = clock.now(), isDirty = true)) + } + store.subtaskIds(taskId).forEach { moveSubtree(it, listId, seen) } + } + + /** + * [ids] with every parent before its subtasks, so a bulk move carries a + * subtree along before reaching its subtasks, whatever order they came in. + */ + private fun parentsFirst(ids: List): List { + if (ids.size < 2) return ids + fun depth(id: Long): Int { + val seen = HashSet() + var parent = database.tasks().entity(id)?.parentId + while (parent != null && seen.add(parent)) parent = database.tasks().entity(parent)?.parentId + return seen.size + } + val depths = ids.associateWith(::depth) + return ids.sortedBy(depths::getValue) } private fun deleteTask(taskId: Long): Boolean { val current = database.tasks().entity(taskId)?.takeUnless { it.isDeleted } ?: return false requireWritableList(current.listId) // An override deletes itself into an EXDATE on its master. - if (current.masterId != null) store.deleteTask(taskId) else deleteWithSubtasks(taskId) + if (current.masterId != null) store.deleteTask(taskId) else store.deleteWithSubtasks(taskId) return true } - /** A parent goes with its subtasks, all the way down, as in the app. */ - private fun deleteWithSubtasks(taskId: Long) { - store.subtaskIds(taskId).forEach(::deleteWithSubtasks) - store.deleteTask(taskId) - } - // --- instances -------------------------------------------------------------- /** One occurrence: the row it reads from, its series, and when it was due to happen. */ @@ -224,18 +238,26 @@ class ProviderWriter internal constructor( if (!row.recurring || occurrence == null) return updateTask(row.taskId, values) val shown = database.tasks().entity(row.taskId)?.takeUnless { it.isDeleted } ?: return false requireWritableList(shown.listId) - // A row read back unchanged carries its times; only a real move is refused. - val moved = ProviderTaskWriter.changedTimeColumns(shown, values, zone()) - require(moved.isEmpty()) { - "One occurrence of a recurring task cannot be moved (${moved.sorted().joinToString()}); write the series through ${Tasks.PATH}" + // A row read back carries every column; only what differs from it is a write. + val changed = ProviderTaskWriter.changedColumns(shown, values, zone()) + val moved = changed intersect TIME_COLUMNS + if (moved.isNotEmpty()) { + // A bad value reports itself rather than as a move. + ProviderTaskWriter.patch(shown, values.filterKeys { it in moved }, clock.now(), zone()) + throw IllegalArgumentException( + "One occurrence of a recurring task cannot be moved (${moved.sorted().joinToString()}); write the series through ${Tasks.PATH}", + ) } - val content = values - COMPLETION_COLUMNS - TIME_COLUMNS + val content = values.filterKeys { it in changed && it !in COMPLETION_COLUMNS } if (content.isNotEmpty()) { updateTask(row.seriesId, content) // A modified occurrence shows its own copy of the content, so it takes the write too. if (row.taskId != row.seriesId) updateTask(row.taskId, content - Tasks.LIST_ID - Tasks.PARENT_ID) } - val change = ProviderTaskWriter.occurrenceChange(values.filterKeys { it in COMPLETION_COLUMNS }) ?: return true + if (changed.none { it in COMPLETION_COLUMNS }) return true + // The column the client changed decides, not the unchanged ones it carries along. + val change = ProviderTaskWriter.occurrenceChange(values.filterKeys { it in changed && it in COMPLETION_COLUMNS }) + ?: return true val status = database.tasks().entity(row.taskId)?.status when (change) { OccurrenceChange.COMPLETE -> @@ -372,9 +394,14 @@ class ProviderWriter internal constructor( } private fun singleId(table: ProviderTable, selection: String?, args: Array?): Long? { - if (!args.isNullOrEmpty()) return null val match = SINGLE_ROW.matchEntire(selection ?: return null) ?: return null - return match.groupValues[2].toLongOrNull()?.takeIf { match.groupValues[1] == table.idColumn } + if (match.groupValues[1] != table.idColumn) return null + val value = match.groupValues[2] + return when { + value == "?" -> args?.singleOrNull()?.trim()?.toLongOrNull() + args.isNullOrEmpty() -> value.toLongOrNull() + else -> null + } } private fun requireFreeUid(listId: Long, uid: String) { 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 ce8e95e..30975b3 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 @@ -145,6 +145,9 @@ class RoomTasksDataSource @Inject constructor( .flatMap { occurrencesOf(it, tasks.overrides(it.task.id), now) } } + /** One transaction, so a failure part-way leaves the subtree whole. */ + override fun deleteWithSubtasks(taskId: Long) = tx { super.deleteWithSubtasks(taskId) } + override fun subtaskIds(parentTaskId: Long): List = tasks.subtasks(parentTaskId).map { it.task.id }.filter { it != parentTaskId } @@ -346,8 +349,7 @@ class RoomTasksDataSource @Inject constructor( val master = tasks.entity(seriesId) ?: return@tx val anchor = master.recurrenceAnchor if (anchor == null || occurrenceStart <= anchor) { - tasks.subtasks(seriesId).forEach { deleteTask(it.task.id) } - deleteTask(seriesId) + deleteWithSubtasks(seriesId) return@tx } tasks.update(TaskFormWriter.endedBefore(master, occurrenceStart, clock.now(), zone())) diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderIdsTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderIdsTest.kt index 4ed11d1..6f9b892 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderIdsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderIdsTest.kt @@ -64,4 +64,33 @@ class ProviderIdsTest { assertThat(ids.values).containsNoDuplicates() assertThat(ids.values.map(ProviderIds::categoryTask)).containsExactly(5L, 5L) } + + @Test + fun `colliding tags keep their ids whatever their order`() { + val hash = { _: String -> 7L } + + assertThat(ProviderIds.categories(5, listOf("b", "a"), hash)) + .isEqualTo(ProviderIds.categories(5, listOf("a", "b"), hash)) + } + + @Test + fun `colliding tags stay inside their task's bits`() { + val ids = ProviderIds.categories(5, listOf("a", "b", "c"), hash = { 0x7FFFFFFFL }) + + assertThat(ids.values).containsNoDuplicates() + ids.values.forEach { + assertThat(ProviderIds.categoryTask(it)).isEqualTo(5L) + assertThat(ProviderIds.isAlarm(it)).isFalse() + } + } + + @Test + fun `tags of neighbouring tasks never share an id`() { + val names = (1..200).map { "tag$it" } + + val ids = (1L..50L).flatMap { ProviderIds.categories(it, names).values } + + assertThat(ids).containsNoDuplicates() + } } + diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriterTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriterTest.kt index fa75dd5..41c4967 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriterTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/provider/ProviderTaskWriterTest.kt @@ -169,6 +169,15 @@ class ProviderTaskWriterTest { .isEqualTo("America/New_York") } + @Test + fun `a new timed task without a zone is floating`() { + val due = millis(Instant.parse("2026-10-20T09:00:00Z")) + + assertThat(ProviderTaskWriter.newTask(mapOf(Tasks.LIST_ID to 3L, Tasks.DUE to due), "u", now, "UTC").timezone).isNull() + assertThat(ProviderTaskWriter.newTask(mapOf(Tasks.LIST_ID to 3L, Tasks.DUE to due, Tasks.TZ to "Europe/Paris"), "u", now, "UTC").timezone) + .isEqualTo("Europe/Paris") + } + @Test fun `an unknown zone is rejected`() { assertThrows { patch(task, Tasks.DUE to 0L, Tasks.TZ to "Mars/Olympus") } @@ -322,6 +331,29 @@ class ProviderTaskWriterTest { assertThat(reopened.completedAt).isNull() } + @Test + fun `a completed row written back with status open reopens it fully`() { + val done = task.copy(status = TaskStatus.COMPLETED, percentComplete = 100, completedAt = earlier) + + val edited = patch(done, Tasks.STATUS to 0, Tasks.PERCENT_COMPLETE to 100, Tasks.COMPLETED to millis(earlier)) + + assertThat(edited.status).isEqualTo(TaskStatus.NEEDS_ACTION) + assertThat(edited.percentComplete).isNull() + assertThat(edited.completedAt).isNull() + } + + @Test + fun `the changed completion column decides over unchanged ones written back`() { + val completed = patch(task, Tasks.STATUS to 0, Tasks.PERCENT_COMPLETE to 100, Tasks.COMPLETED to null) + assertThat(completed.status).isEqualTo(TaskStatus.COMPLETED) + assertThat(completed.completedAt).isEqualTo(now) + + val done = task.copy(status = TaskStatus.COMPLETED, percentComplete = 100, completedAt = earlier) + val reopened = patch(done, Tasks.STATUS to 2, Tasks.PERCENT_COMPLETE to 100, Tasks.COMPLETED to null) + assertThat(reopened.status).isEqualTo(TaskStatus.NEEDS_ACTION) + assertThat(reopened.percentComplete).isNull() + } + @Test fun `a write without completion columns leaves completion alone`() { val done = task.copy(status = TaskStatus.COMPLETED, percentComplete = 100, completedAt = earlier) @@ -386,16 +418,73 @@ class ProviderTaskWriterTest { } @Test - fun `unchanged time columns are not a move`() { + fun `only columns that differ from the row count as changed`() { val due = Instant.parse("2026-10-20T15:00:00Z") - val series = task.copy(due = due, timezone = "UTC", rrule = "FREQ=DAILY") + val series = task.copy(due = due, timezone = "UTC", rrule = "FREQ=DAILY", status = TaskStatus.IN_PROCESS, percentComplete = 50) val readBack = mapOf( - Tasks.DUE to millis(due), Tasks.DTSTART to null, Tasks.TZ to "UTC", Tasks.IS_ALLDAY to 0, + Tasks.TITLE to "Old", Tasks.DUE to millis(due), Tasks.DTSTART to null, Tasks.TZ to "UTC", Tasks.IS_ALLDAY to 0, Tasks.RRULE to "FREQ=DAILY", Tasks.RDATE to null, Tasks.EXDATE to null, Tasks.DURATION to null, + Tasks.STATUS to 1, Tasks.PERCENT_COMPLETE to 50, Tasks.COMPLETED to null, ) - assertThat(ProviderTaskWriter.changedTimeColumns(series, readBack, "Europe/Berlin")).isEmpty() - assertThat(ProviderTaskWriter.changedTimeColumns(series, readBack + (Tasks.DUE to millis(due + 1.hours)), "Europe/Berlin")) - .contains(Tasks.DUE) + assertThat(ProviderTaskWriter.changedColumns(series, readBack, "Europe/Berlin")).isEmpty() + assertThat( + ProviderTaskWriter.changedColumns( + series, + readBack + (Tasks.DUE to millis(due + 1.hours)) + (Tasks.TITLE to "New") + (Tasks.STATUS to 2), + "Europe/Berlin", + ), + ).containsExactly(Tasks.DUE, Tasks.TITLE, Tasks.STATUS) + } + + @Test + fun `a value that does not fit counts as changed`() { + assertThat(ProviderTaskWriter.changedColumns(task, mapOf(Tasks.PRIORITY to 99), "UTC")).containsExactly(Tasks.PRIORITY) + } + + @Test + fun `a floating task stays floating`() { + val floating = task.copy(due = now, timezone = null) + + assertThat(patch(floating, Tasks.TITLE to "t").timezone).isNull() + assertThat(patch(floating, Tasks.DUE to millis(now + 1.hours)).timezone).isNull() + assertThat(patch(floating, Tasks.TITLE to "Old")).isSameInstanceAs(floating) + } + + @Test + fun `an all-day task made timed takes the device zone`() { + val allDay = task.copy(due = Instant.parse("2026-10-20T00:00:00Z"), isAllDay = true) + + val timed = patch(allDay, Tasks.IS_ALLDAY to 0, Tasks.DUE to millis(Instant.parse("2026-10-20T15:00:00Z"))) + + assertThat(timed.timezone).isEqualTo("Europe/Berlin") + } + + @Test + fun `clearing every date clears the zone`() { + assertThat(patch(task.copy(due = now, timezone = "UTC"), Tasks.DUE to null).timezone).isNull() + } + + @Test + fun `exception and extra dates must be iCalendar values of one kind`() { + val series = task.copy(due = now, rrule = "FREQ=DAILY", timezone = "UTC") + + assertThat(patch(series, Tasks.EXDATE to "20261016T100000Z, 20261017T100000Z").exdate) + .isEqualTo("20261016T100000Z,20261017T100000Z") + assertThat(patch(series, Tasks.RDATE to "20261030").rdate).isEqualTo("20261030") + assertThrows { patch(series, Tasks.EXDATE to "tomorrow") } + assertThrows { patch(series, Tasks.EXDATE to "20261016,20261017T100000Z") } + assertThrows { patch(series, Tasks.RDATE to "20261016T100000Z,20261017T100000") } + } + + @Test + fun `stored dates written back as synced are kept`() { + val series = task.copy(due = now, rrule = "FREQ=DAILY", rdate = "20261016T100000Z/PT1H", exdate = "20261016,20261017T100000Z") + + val edited = patch(series, Tasks.TITLE to "New", Tasks.RDATE to series.rdate, Tasks.EXDATE to series.exdate) + + assertThat(edited.rdate).isEqualTo(series.rdate) + assertThat(edited.exdate).isEqualTo(series.exdate) + assertThat(ProviderTaskWriter.changedColumns(series, mapOf(Tasks.RDATE to series.rdate), "UTC")).isEmpty() } } diff --git a/docs/CONTENT_PROVIDER.md b/docs/CONTENT_PROVIDER.md index 4874369..9209489 100644 --- a/docs/CONTENT_PROVIDER.md +++ b/docs/CONTENT_PROVIDER.md @@ -67,12 +67,18 @@ changes nothing does not mark the task for sync. columns the table publishes are ignored, so you can write back a row you read. A column the table does not have is rejected with `IllegalArgumentException`. Agendula's sync state (the server's URL and ETag, the iCalendar `SEQUENCE`, and -properties it does not understand) is not published and cannot be written. +properties it does not understand) is not published and cannot be written. A +timed task without a `tz` is floating and stays floating. A task inserted +without a `tz` is floating, as in OpenTasks; an existing task that first gets a +date takes the device's zone. `rdate` and `exdate` take comma-separated +iCalendar values of one kind: all dates, all UTC date-times or all local +date-times. A value written back exactly as read is kept, whatever its shape. **Status.** `status`, `percent_complete` and `completed` stay consistent the way OpenTasks keeps them. Completing a task sets 100 % and a completion time, progress between 1 and 99 sets "in process", and a completion time alone -completes the task. +completes the task. When you write back a row you read, the column you changed +decides; the others it carries unchanged do not. **Recurring tasks.** Writing `status`, `percent_complete` or `completed` to an `instances` row completes, cancels or reopens that one occurrence; partial