diff --git a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderRecurrenceTest.kt b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderRecurrenceTest.kt index 4b9c03f..982bb39 100644 --- a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderRecurrenceTest.kt +++ b/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( 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 7899427..c5c65d7 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 @@ -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?): List { - 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?): List = + 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 { + 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?) { - 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(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?) { + 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?) { + 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 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 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 { - 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 { + 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) { @@ -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 diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapper.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapper.kt index 30a7edc..de1074b 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapper.kt +++ b/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?) diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapperTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapperTest.kt index b6905aa..3f65478 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskMapperTest.kt +++ b/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), diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskWriteMapperTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskWriteMapperTest.kt index 1d969a4..4c19139 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/TaskWriteMapperTest.kt +++ b/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),