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.
2026-10-15 16:14:28 +02:00
parent 522d5ca35e
commit d123c00def
13 changed files with 467 additions and 77 deletions
@@ -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<String, Any?> {
        val columns = table.columns.map { it.first }
@@ -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()
+    }
 }

@@ -85,6 +85,27 @@ class AndroidTasksDataSource @Inject constructor(
    override fun subtasks(parentTaskId: Long): List<Task> =
        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<Long> = 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<String>?): List<Task> {
        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)
    }

@@ -44,6 +44,7 @@ class ModeRoutingTasksDataSource(
        active().deleteFollowing(seriesId, occurrenceStart)
    override fun subtasks(parentTaskId: Long): List<Task> = active().subtasks(parentTaskId)
    override fun subtaskIds(parentTaskId: Long): List<Long> = 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)

@@ -54,6 +54,21 @@ interface TasksDataSource {
    fun subtaskIds(parentTaskId: Long): List<Long> =
        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<Long>()
+        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)

@@ -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) }

@@ -21,7 +21,9 @@ typealias ProviderRow = Map<String, Any?>

 /**
 * 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<String>): Map<String, Long> {
+    fun categories(taskId: Long, categories: List<String>): Map<String, Long> =
+        categories(taskId, categories, ::tagHash)
+
+    internal fun categories(taskId: Long, categories: List<String>, hash: (String) -> Long): Map<String, Long> {
        val used = HashSet<Long>()
-        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
    }
 }

@@ -32,6 +32,8 @@ val TIME_COLUMNS: Set<String> = 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<String> {
-        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<String> {
+        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<TaskStatus, Int?, Instant?> {
+    private fun completion(current: TaskEntity, values: Values, now: Instant): Triple<TaskStatus, Int?, Instant?> {
+        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<String> {
+        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<String> get() = values.keys

+    val map: ProviderValues get() = values
+
    operator fun contains(key: String) = key in values

    fun string(key: String): String? = values[key]?.toString()
@@ -28,8 +28,8 @@ import javax.inject.Singleton
 import kotlin.time.Clock
 import kotlin.time.Instant

-/** A row URI's selection, `<id column> = N`. */
-private val SINGLE_ROW = Regex("""\s*(\w+)\s*=\s*(\d+)\s*""")
+/** One row by id, `<id column> = N` or `<id column> = ?`. */
+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<Long> = 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<Long>): List<Long> {
+        if (ids.size < 2) return ids
+        fun depth(id: Long): Int {
+            val seen = HashSet<Long>()
+            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<out String>?): 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) {
@@ -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<Long> =
        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()))
@@ -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()
+    }
 }
+
@@ -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<IllegalArgumentException> { 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<IllegalArgumentException> { patch(series, Tasks.EXDATE to "tomorrow") }
+        assertThrows<IllegalArgumentException> { patch(series, Tasks.EXDATE to "20261016,20261017T100000Z") }
+        assertThrows<IllegalArgumentException> { 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()
    }
 }
@@ -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