Write existing overrides directly, match occurrences to the second (#135)

Also round legacy millisecond series once per provider instead of on every
instance write, read the instances cursor once, and fall back when a view lacks
original_instance_id.

app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderRecurrenceTest.kt

@@ -8,6 +8,7 @@ import com.google.common.truth.Truth.assertThat
 import de.jeanlucmakiola.agendula.data.tasks.TasksContract.Tasks
 import de.jeanlucmakiola.agendula.domain.Task
 import de.jeanlucmakiola.agendula.domain.TaskForm
+import de.jeanlucmakiola.agendula.domain.TaskReminder
 import org.junit.After
 import org.junit.Assume.assumeTrue
 import org.junit.Before
@@ -15,6 +16,7 @@ import org.junit.Test
 import org.junit.runner.RunWith
 import kotlin.time.Clock
 import kotlin.time.Duration.Companion.hours
+import kotlin.time.Duration.Companion.milliseconds
 import kotlin.time.Instant
 
 /**
@@ -171,14 +173,43 @@ class ExternalProviderRecurrenceTest {
 
         source.deleteTask(override.taskId)
 
-        // An EXDATE on the master, so a sync removes the occurrence and does not
-        // just drop the edit made to it.
+        // Excluded on the master, not just the override dropped.
         assertThat(storedColumn(id, Tasks.EXDATE)).isNotNull()
         val open = source.tasks(TaskQuery(listId = listId)).filter { it.seriesId == id }
         assertThat(open.map { it.occurrenceStart }).doesNotContain(override.occurrenceStart)
         assertThat(storedRule(id)).isEqualTo("FREQ=DAILY")
     }
 
+    @Test
+    fun editingAnOverrideAgainKeepsItsNewTagsAndReminders() {
+        val id = dailyTask()
+        val override = overrideOf(id)
+        val form = TaskForm(
+            title = override.title,
+            listId = listId,
+            start = override.start,
+            due = override.due,
+            tags = listOf("urgent"),
+            reminders = listOf(TaskReminder(minutesBefore = 15)),
+        )
+
+        source.updateInstance(override.taskId, override.occurrenceStart!!, form, form.reminders)
+
+        assertThat(source.task(override.taskId)!!.tags).containsExactly("urgent")
+        assertThat(source.reminders()[override.taskId]).containsExactly(TaskReminder(minutesBefore = 15))
+        assertThat(source.task(id)?.tags.orEmpty()).isEmpty()
+    }
+
+    @Test
+    fun anOccurrenceIsFoundByItsAnchorToTheSecond() {
+        val id = dailyTask()
+        val override = overrideOf(id)
+
+        val found = source.task(id, override.occurrenceStart!! + 345.milliseconds)
+
+        assertThat(found?.taskId).isEqualTo(override.taskId)
+    }
+
     private fun storedStart(taskId: Long): Long? = storedColumn(taskId, Tasks.DTSTART)?.toLong()
 
     private fun storedColumn(taskId: Long, column: String): String? = context.contentResolver.query(

app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/AndroidTasksDataSource.kt

@@ -7,6 +7,8 @@ import android.content.ContentUris
 import android.content.ContentValues
 import android.content.Context
 import android.database.ContentObserver
+import android.database.Cursor
+import android.database.sqlite.SQLiteException
 import android.net.Uri
 import android.os.Handler
 import android.os.Looper
@@ -79,8 +81,8 @@ class AndroidTasksDataSource @Inject constructor(
 
     override fun task(taskId: Long, occurrenceStart: Instant?): Task? {
         if (occurrenceStart == null) return task(taskId)
-        val rows = queryInstances(OCCURRENCE_OF, arrayOf(taskId.toString(), taskId.toString()))
-        return rows.firstOrNull { it.occurrenceStart == occurrenceStart }
+        val rows = instanceTasks(queryOccurrences(taskId))
+        return rows.firstOrNull { it.occurrenceStart?.let { start -> sameSecond(start, occurrenceStart) } == true }
             ?: rows.firstOrNull { it.taskId == taskId && it.distanceFromCurrent == 0 }
             ?: rows.firstOrNull { it.taskId == taskId }
     }
@@ -109,28 +111,23 @@ class AndroidTasksDataSource @Inject constructor(
         }
     } ?: 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
-        // provider versions (e.g. tasks.org's bundled OpenTasks has no
-        // `is_recurring`). The by-name mapper reads whatever is present and
-        // nulls the rest; the repository re-sorts, so we don't depend on a
-        // provider sort column either.
-        val tasks = resolver.query(uri, null, selection, args, null)
-            ?.use { c ->
-                val reader = CursorColumnReader(c)
-                // The view hides the rule (`null AS rrule`), so it is read from
-                // the tasks table — the master's, for an override — and handed
-                // to the mapper.
-                fun ruleOwner() = reader.getLong(Tasks.ORIGINAL_INSTANCE_ID) ?: reader.getLong(Instances.TASK_ID)
-                val owners = buildSet { while (c.moveToNext()) ruleOwner()?.let(::add) }
-                val rules = seriesRules(owners)
-                c.moveToPosition(-1)
-                buildList {
-                    while (c.moveToNext()) add(TaskMapper.task(reader, ruleOwner()?.let(rules::get)))
-                }
-            } ?: emptyList()
-        if (tasks.isEmpty()) return tasks
+    /**
+     * All columns and no sort: the instances view differs across provider
+     * versions (tasks.org's has no `is_recurring`), the by-name mapper nulls what
+     * is missing, and the repository re-sorts.
+     */
+    private fun queryInstances(selection: String?, args: Array<String>?): List<Task> =
+        instanceTasks(resolver.query(TasksContract.instancesUri(authority()), null, selection, args, null))
+
+    /** Maps [cursor]'s instance rows, with the rule each series keeps on its `tasks` row, and their tags. */
+    private fun instanceTasks(cursor: Cursor?): List<Task> {
+        val rows = cursor?.use { c ->
+            val reader = CursorColumnReader(c)
+            buildList { while (c.moveToNext()) add(TaskMapper.instance(reader)) }
+        }
+        if (rows.isNullOrEmpty()) return emptyList()
+        val rules = seriesRules(rows.map { it.ruleOwner })
+        val tasks = rows.map { TaskMapper.withRule(it, rules[it.ruleOwner]) }
         val tags = categories(tasks.map { it.taskId })
         return tasks.map { task -> tags[task.taskId]?.let { task.copy(tags = it) } ?: task }
     }
@@ -239,10 +236,7 @@ class AndroidTasksDataSource @Inject constructor(
     }
 
     override fun updateSeries(seriesId: Long, occurrenceStart: Instant, form: TaskForm, reminders: List<TaskReminder>?) {
-        val occurrence = occurrence(seriesId, occurrenceStart) { r ->
-            r.getLong(Instances.INSTANCE_START)?.let(Instant::fromEpochMilliseconds) to
-                r.getLong(Instances.INSTANCE_DUE)?.let(Instant::fromEpochMilliseconds)
-        } ?: task(seriesId)?.let { it.start to it.due }
+        val occurrence = task(seriesId, occurrenceStart)
         val anchors = resolver.query(
             taskUri(authority(), seriesId), arrayOf(Tasks.DTSTART, Tasks.DUE), null, null, null,
         )?.use { c ->
@@ -256,8 +250,8 @@ class AndroidTasksDataSource @Inject constructor(
             else -> anchor + (now - was).inWholeMilliseconds
         }?.let(::providerMillis)
         val values = TaskWriteMapper.seriesValues(form, ZoneId.systemDefault().id) +
-            (Tasks.DTSTART to shifted(anchors?.first, occurrence?.first, form.start)) +
-            (Tasks.DUE to shifted(anchors?.second, occurrence?.second, form.due))
+            (Tasks.DTSTART to shifted(anchors?.first, occurrence?.start, form.start)) +
+            (Tasks.DUE to shifted(anchors?.second, occurrence?.due, form.due))
         val rows = resolver.update(taskUri(authority(), seriesId), values.toContentValues(), null, null)
         if (rows == 0) throw TaskWriteFailedException("update series $seriesId")
         setCategories(seriesId, form.tags)
@@ -324,8 +318,7 @@ class AndroidTasksDataSource @Inject constructor(
         val values = mapOf<String, Any?>(Tasks.RRULE to rule)
         val rows = resolver.update(taskUri(authority(), seriesId), values.toContentValues(), null, null)
         if (rows == 0) throw TaskWriteFailedException("end series $seriesId")
-        // Plain row deletes: through the instances URI each would also add an
-        // EXDATE past the rule's new end.
+        // Plain deletes: the instances URI would add EXDATEs past the new end.
         exceptionsFrom(seriesId, occurrenceStart).forEach { resolver.delete(taskUri(authority(), it), null, null) }
     }
 
@@ -353,6 +346,7 @@ class AndroidTasksDataSource @Inject constructor(
     }
 
     override fun updateInstance(taskId: Long, occurrenceStart: Instant, form: TaskForm, reminders: List<TaskReminder>?) {
+        overrideFor(taskId, occurrenceStart)?.let { return updateOverride(it, form, reminders) }
         if (reminders == null) return writeInstance(taskId, occurrenceStart, form)
         // ⚠️ Written to the series, and then put back. Forking an occurrence
         // copies the task's *current* properties onto the new override row, so
@@ -368,6 +362,25 @@ class AndroidTasksDataSource @Inject constructor(
         }
     }
 
+    /**
+     * An occurrence that already has its own row is written there directly. The
+     * fork-and-restore dance in [updateInstance] would put its old tags and
+     * reminders back over the new ones.
+     */
+    private fun updateOverride(overrideId: Long, form: TaskForm, reminders: List<TaskReminder>?) {
+        val values = TaskWriteMapper.instanceValues(form, ZoneId.systemDefault().id)
+        val rows = resolver.update(taskUri(authority(), overrideId), values.toContentValues(), null, null)
+        if (rows == 0) throw TaskWriteFailedException("update override $overrideId")
+        setCategories(overrideId, form.tags)
+        writeReminders(overrideId, reminders)
+    }
+
+    /** The override row standing for [taskId]'s occurrence at [occurrenceStart], if it has one. */
+    private fun overrideFor(taskId: Long, occurrenceStart: Instant): Long? =
+        occurrence(taskId, occurrenceStart) { r ->
+            r.getLong(Instances.TASK_ID)?.takeIf { r.getLong(Tasks.ORIGINAL_INSTANCE_ID) != null }
+        }
+
     private fun writeInstance(taskId: Long, occurrenceStart: Instant, form: TaskForm) {
         val instanceId = instanceIdFor(taskId, occurrenceStart)
             ?: throw TaskWriteFailedException("update instance $taskId@$occurrenceStart: no such occurrence")
@@ -402,58 +415,79 @@ class AndroidTasksDataSource @Inject constructor(
      *
      * The seam addresses occurrences by `(taskId, occurrenceStart)`; writing
      * through the instances URI still needs the row id, so it is looked up here
-     * rather than carried around above the data layer. Selection is on `task_id`
-     * only — the anchor is matched in Kotlin because the column that holds it
-     * (`instance_original_time`) is missing on older provider schemas, where a
-     * WHERE clause naming it would throw instead of falling back.
+     * rather than carried around above the data layer.
      */
     private fun instanceIdFor(taskId: Long, occurrenceStart: Instant): Long? {
-        roundAnchors(taskId)
+        roundLegacyAnchors()
         return occurrence(taskId, occurrenceStart) { it.getLong(Tasks.ID) }
     }
 
     /**
      * [read] applied to the instance row of [taskId]'s occurrence at
      * [occurrenceStart] — its own, or an override's that replaced it — matched in
-     * whole seconds, as the provider expands them.
+     * whole seconds, as the provider expands them. The anchor is matched in
+     * Kotlin because `instance_original_time` is missing on older schemas.
      */
-    private fun <T> occurrence(taskId: Long, occurrenceStart: Instant, read: (ColumnReader) -> T): T? {
-        val uri = TasksContract.instancesUri(authority())
-        val wanted = providerMillis(occurrenceStart.toEpochMilliseconds())
-        return resolver.query(uri, null, OCCURRENCE_OF, arrayOf(taskId.toString(), taskId.toString()), null)?.use { c ->
+    private fun <T> occurrence(taskId: Long, occurrenceStart: Instant, read: (ColumnReader) -> T): T? =
+        queryOccurrences(taskId)?.use { c ->
             val reader = CursorColumnReader(c)
             while (c.moveToNext()) {
                 val anchor = TaskMapper.occurrenceAnchor(reader) ?: continue
-                if (providerMillis(anchor.toEpochMilliseconds()) == wanted) return@use read(reader)
+                if (sameSecond(anchor, occurrenceStart)) return@use read(reader)
             }
             null
         }
-    }
 
     /**
-     * Rounds a task's DTSTART/DUE down to whole seconds if a build before
-     * [providerMillis] stored them with milliseconds — otherwise the provider
-     * cannot move the series past an occurrence written through its instance.
+     * Instance rows of [taskId] and of the overrides standing in for its
+     * occurrences. A provider whose view lacks `original_instance_id` gets the
+     * task's own rows only, rather than a failed query.
      */
-    private fun roundAnchors(taskId: Long) {
-        val uri = taskUri(authority(), taskId)
-        var master: Long? = null
-        val values = resolver.query(uri, arrayOf(Tasks.DTSTART, Tasks.DUE, Tasks.ORIGINAL_INSTANCE_ID), null, null, null)?.use { c ->
-            if (!c.moveToFirst()) return@use null
+    private fun queryOccurrences(taskId: Long): Cursor? {
+        val uri = TasksContract.instancesUri(authority())
+        val id = taskId.toString()
+        return try {
+            resolver.query(uri, null, OCCURRENCE_OF, arrayOf(id, id), null)
+        } catch (_: SQLiteException) {
+            resolver.query(uri, null, "${Instances.TASK_ID} = ?", arrayOf(id), null)
+        }
+    }
+
+    @Volatile
+    private var anchorsRoundedFor: String? = null
+
+    /**
+     * Rounds the series a build before [providerMillis] stored with millisecond
+     * DTSTART/DUE, once per provider and process. The provider cannot move such a
+     * series past an occurrence completed through its instance.
+     */
+    private fun roundLegacyAnchors() {
+        val authority = authority()
+        if (anchorsRoundedFor == authority) return
+        val stale = resolver.query(
+            TasksContract.tasksUri(authority),
+            arrayOf(Tasks.ID, Tasks.DTSTART, Tasks.DUE),
+            "(${Tasks.RRULE} IS NOT NULL OR ${Tasks.RDATE} IS NOT NULL) AND " +
+                "(${Tasks.DTSTART} % 1000 != 0 OR ${Tasks.DUE} % 1000 != 0)",
+            null,
+            null,
+        )?.use { c ->
             val r = CursorColumnReader(c)
-            master = r.getLong(Tasks.ORIGINAL_INSTANCE_ID)
-            buildMap<String, Any?> {
-                for (column in listOf(Tasks.DTSTART, Tasks.DUE)) {
-                    val millis = r.getLong(column) ?: continue
-                    if (providerMillis(millis) != millis) put(column, providerMillis(millis))
+            buildList {
+                while (c.moveToNext()) {
+                    val id = r.getLong(Tasks.ID) ?: continue
+                    val values = buildMap<String, Any?> {
+                        for (column in listOf(Tasks.DTSTART, Tasks.DUE)) {
+                            val millis = r.getLong(column) ?: continue
+                            if (providerMillis(millis) != millis) put(column, providerMillis(millis))
+                        }
+                    }
+                    add(id to values)
                 }
             }
-        }
-        if (!values.isNullOrEmpty() && resolver.update(uri, values.toContentValues(), null, null) == 0) {
-            throw TaskWriteFailedException("round task $taskId")
-        }
-        // Completing an override moves its master on, so the master's anchor matters too.
-        master?.takeIf { it != taskId }?.let(::roundAnchors)
+        }.orEmpty()
+        stale.forEach { (id, values) -> resolver.update(taskUri(authority, id), values.toContentValues(), null, null) }
+        anchorsRoundedFor = authority
     }
 
     override fun setReminders(taskId: Long, reminders: List<TaskReminder>) {
@@ -603,12 +637,18 @@ class AndroidTasksDataSource @Inject constructor(
      */
     override fun deleteTask(taskId: Long) {
         val authority = authority()
-        val isOverride = resolver.query(taskUri(authority, taskId), arrayOf(Tasks.ORIGINAL_INSTANCE_ID), null, null, null)
-            ?.use { c -> c.moveToFirst() && !c.isNull(0) } == true
-        val instanceId = if (!isOverride) null else resolver.query(
-            TasksContract.instancesUri(authority), null, "${Instances.TASK_ID} = ?", arrayOf(taskId.toString()), null,
-        )?.use { c -> if (c.moveToFirst()) CursorColumnReader(c).getLong(Tasks.ID) else null }
-        if (instanceId != null) resolver.delete(TasksContract.instanceUri(authority, instanceId), null, null)
+        val overrideInstance = try {
+            resolver.query(
+                TasksContract.instancesUri(authority),
+                null,
+                "${Instances.TASK_ID} = ? AND ${Tasks.ORIGINAL_INSTANCE_ID} IS NOT NULL",
+                arrayOf(taskId.toString()),
+                null,
+            )?.use { c -> if (c.moveToFirst()) CursorColumnReader(c).getLong(Tasks.ID) else null }
+        } catch (_: SQLiteException) {
+            null
+        }
+        if (overrideInstance != null) resolver.delete(TasksContract.instanceUri(authority, overrideInstance), null, null)
         else resolver.delete(taskUri(authority, taskId), null, null)
     }
 
@@ -685,5 +725,8 @@ class AndroidTasksDataSource @Inject constructor(
 /** An instance row of the task, or of an override standing in for one of its occurrences. */
 private const val OCCURRENCE_OF = "${Instances.TASK_ID} = ? OR ${Tasks.ORIGINAL_INSTANCE_ID} = ?"
 
+private fun sameSecond(a: Instant, b: Instant): Boolean =
+    providerMillis(a.toEpochMilliseconds()) == providerMillis(b.toEpochMilliseconds())
+
 /** SQLite's default bound-argument limit on older Android releases. */
 private const val MAX_SQL_ARGS = 999

app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapper.kt

@@ -20,14 +20,18 @@ object TaskMapper {
      * one-off on tasks.org's bundled provider (DB 22), which also lacks
      * `is_recurring`. An override names its master in `original_instance_id`.
      */
-    fun task(r: ColumnReader, rule: SeriesRule? = null): Task {
+    fun task(r: ColumnReader, rule: SeriesRule? = null): Task = withRule(instance(r), rule)
+
+    /** An `instances` row mapped without its series' rule, for [withRule] once that is known. */
+    fun instance(r: ColumnReader): InstanceRow {
         fun instant(name: String): Instant? =
             r.getLong(name)?.let { Instant.fromEpochMilliseconds(it) }
 
         val rowId = r.getLong(Tasks.ID) ?: 0L
         val masterId = r.getLong(Tasks.ORIGINAL_INSTANCE_ID)
-        val recurring = rule != null || masterId != null || r.getBoolean(Instances.IS_RECURRING)
-        return Task(
+        val recurring = masterId != null || r.getBoolean(Instances.IS_RECURRING)
+        val anchor = occurrenceAnchor(r)
+        val task = Task(
             taskId = r.getLong(Instances.TASK_ID) ?: rowId,
             listId = r.getLong(Tasks.LIST_ID) ?: 0L,
             title = r.getString(Tasks.TITLE).orEmpty(),
@@ -48,12 +52,23 @@ object TaskMapper {
             accountName = r.getString(Tasks.ACCOUNT_NAME),
             parentId = r.getLong(Tasks.PARENT_ID),
             isRecurring = recurring,
-            occurrenceStart = if (recurring) occurrenceAnchor(r) else null,
+            occurrenceStart = if (recurring) anchor else null,
             distanceFromCurrent = r.getInt(Instances.DISTANCE_FROM_CURRENT),
             created = instant(Tasks.CREATED),
             lastModified = instant(Tasks.LAST_MODIFIED),
             seriesId = if (recurring) masterId ?: r.getLong(Instances.TASK_ID) ?: rowId else null,
-            recurrenceRule = rule?.rrule,
+        )
+        return InstanceRow(task, ruleOwner = masterId ?: task.taskId, anchor = anchor)
+    }
+
+    /** [row]'s task as an occurrence of the series [rule] belongs to, or as it is without one. */
+    fun withRule(row: InstanceRow, rule: SeriesRule?): Task {
+        if (rule == null) return row.task
+        return row.task.copy(
+            isRecurring = true,
+            occurrenceStart = row.task.occurrenceStart ?: row.anchor,
+            seriesId = row.task.seriesId ?: row.ruleOwner,
+            recurrenceRule = rule.rrule,
         )
     }
 
@@ -130,3 +145,9 @@ object TaskMapper {
 
 /** A series' recurrence as stored on its master row. */
 data class SeriesRule(val rrule: String?, val rdate: String?)
+
+/**
+ * An instance row's task before its series' rule is applied. [ruleOwner] is the
+ * task whose `tasks` row carries that rule: the master, for an override.
+ */
+data class InstanceRow(val task: Task, val ruleOwner: Long, val anchor: Instant?)

app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapperTest.kt

@@ -86,8 +86,7 @@ class TaskMapperTest {
 
     @Test
     fun `a series is detected from its master rule when the view masks it (#135)`() {
-        // tasks.org's bundled provider (DB 22): `null AS rrule` in the instances
-        // view and no `is_recurring`, so only the tasks row knows it recurs.
+        // tasks.org (DB 22): rule masked in the view, no `is_recurring`.
         val task = TaskMapper.task(
             MapColumnReader(
                 mapOf(
@@ -108,8 +107,7 @@ class TaskMapperTest {
 
     @Test
     fun `the master rule fills the editor even where is_recurring exists`() {
-        // OpenTasks DB 23 flags the row but masks the rule all the same; without
-        // it the editor opens empty and saving the series drops the RRULE.
+        // OpenTasks (DB 23) flags the row but masks the rule too.
         val task = TaskMapper.task(
             MapColumnReader(mapOf(Tasks.ID to 90L, Instances.TASK_ID to 9L, Instances.IS_RECURRING to 1)),
             SeriesRule(rrule = "FREQ=WEEKLY;BYDAY=MO", rdate = null),

app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskWriteMapperTest.kt

@@ -103,9 +103,7 @@ class TaskWriteMapperTest {
 
     @Test
     fun `timed timestamps are kept to the second`() {
-        // A default start of "now" carries milliseconds; the provider expands a
-        // series in whole seconds, and a DTSTART between them stops completing an
-        // occurrence from moving the series on.
+        // The provider expands series in whole seconds.
         val at = Instant.fromEpochMilliseconds(1_784_412_345_678L)
         val values = TaskWriteMapper.taskValues(
             TaskForm(title = "Standup", listId = 1L, start = at, due = at),