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 5861e55..f22c196 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 @@ -189,6 +189,27 @@ class ExternalProviderRecurrenceTest { assertThat(source.task(override.taskId, override.occurrenceStart)?.taskId).isEqualTo(override.taskId) } + @Test + fun anAllDayDueAddedToTheSeriesStaysOnItsDay() { + val day = Clock.System.now().toEpochMilliseconds() / 86_400_000 * 86_400_000 + val id = source.insertTask( + TaskForm(title = "All day", listId = listId, start = Instant.fromEpochMilliseconds(day), isAllDay = true, rrule = "FREQ=DAILY"), + reminders = null, + ) + val task = source.task(id)!! + // Local midnight the next day, as the date picker hands it over. + val localMidnight = Instant.fromEpochMilliseconds(day + 86_400_000 - java.util.TimeZone.getDefault().getOffset(day)) + + source.updateSeries( + id, + task.occurrenceStart!!, + TaskForm(title = "All day", listId = listId, start = task.start, due = localMidnight, isAllDay = true, rrule = "FREQ=DAILY"), + reminders = null, + ) + + assertThat(storedColumn(id, Tasks.DUE)!!.toLong() % 86_400_000).isEqualTo(0L) + } + @Test fun savingAllFromAnOverrideKeepsTheSeriesWhereItIs() { val id = dailyTask() 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 926af96..f653735 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 @@ -12,6 +12,7 @@ import android.database.sqlite.SQLiteException import android.net.Uri import android.os.Handler import android.os.Looper +import android.util.Log import dagger.hilt.android.qualifiers.ApplicationContext import de.jeanlucmakiola.agendula.data.tasks.TasksContract.Instances import de.jeanlucmakiola.agendula.data.tasks.TasksContract.Lists @@ -75,16 +76,20 @@ class AndroidTasksDataSource @Inject constructor( } override fun task(taskId: Long): Task? { - val rows = queryInstances("${Instances.TASK_ID} = ?", arrayOf(taskId.toString())) - return rows.firstOrNull { it.distanceFromCurrent == 0 } ?: rows.firstOrNull() + roundLegacyAnchors() + val rows = instanceRows(resolver.query(TasksContract.instancesUri(authority()), null, "${Instances.TASK_ID} = ?", arrayOf(taskId.toString()), null)) + return (rows.firstOrNull { it.task.distanceFromCurrent == 0 } ?: rows.firstOrNull())?.let { enrich(listOf(it)).single() } } override fun task(taskId: Long, occurrenceStart: Instant?): Task? { if (occurrenceStart == null) return task(taskId) - 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 } + roundLegacyAnchors() + val rows = instanceRows(queryOccurrences(taskId)) + val chosen = rows.firstOrNull { it.anchor == occurrenceStart } + ?: rows.firstOrNull { it.anchor?.let { a -> sameSecond(a, occurrenceStart) } == true } + ?: rows.firstOrNull { it.task.taskId == taskId && it.task.distanceFromCurrent == 0 } + ?: rows.firstOrNull { it.task.taskId == taskId } + return chosen?.let { enrich(listOf(it)).single() } } override fun subtasks(parentTaskId: Long): List = @@ -122,15 +127,24 @@ class AndroidTasksDataSource @Inject constructor( } /** Maps [cursor]'s instance rows, with the rule each series keeps on its `tasks` row, and their tags. */ - private fun instanceTasks(cursor: Cursor?): List { + private fun instanceTasks(cursor: Cursor?): List = enrich(instanceRows(cursor)) + + /** [cursor]'s instance rows, before their rules and tags are looked up. */ + private fun instanceRows(cursor: Cursor?): List { var flagged = false val rows = cursor?.use { c -> flagged = c.getColumnIndex(Instances.IS_RECURRING) >= 0 val reader = CursorColumnReader(c) buildList { while (c.moveToNext()) add(TaskMapper.instance(reader)) } - } - if (rows.isNullOrEmpty()) return emptyList() - val rules = seriesRules(rows.filter { !flagged || it.task.isRecurring }.map { it.ruleOwner }) + }.orEmpty() + // Where the provider flags recurrence, only flagged rows need their rule looked up. + return if (flagged) rows.map { if (it.task.isRecurring) it else it.copy(ruleOwner = NO_RULE) } else rows + } + + /** [rows] completed with the rule each series keeps on its `tasks` row, and their tags. */ + private fun enrich(rows: List): List { + if (rows.isEmpty()) return emptyList() + val rules = seriesRules(rows.map { it.ruleOwner }.filter { it != NO_RULE }) 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 } @@ -248,14 +262,14 @@ class AndroidTasksDataSource @Inject constructor( val r = CursorColumnReader(c) r.getLong(Tasks.DTSTART) to r.getLong(Tasks.DUE) } - fun shifted(anchor: Long?, was: Instant?, now: Instant?): Long? = when { - now == null -> null - anchor == null || was == null -> now.toEpochMilliseconds() - else -> anchor + (now - was).inWholeMilliseconds - }?.let(::providerMillis) - val values = TaskWriteMapper.seriesValues(form, ZoneId.systemDefault().id) + - (Tasks.DTSTART to shifted(anchors?.first, occurrence?.start, form.start)) + - (Tasks.DUE to shifted(anchors?.second, occurrence?.due, form.due)) + val base = TaskWriteMapper.seriesValues(form, ZoneId.systemDefault().id) + // Without an anchor to shift, the form's own value stands — already pinned for an all-day series. + fun shifted(column: String, anchor: Long?, was: Instant?, now: Instant?): Any? = + if (now == null || anchor == null || was == null) base[column] + else providerMillis(anchor + (now - was).inWholeMilliseconds) + val values = base + + (Tasks.DTSTART to shifted(Tasks.DTSTART, anchors?.first, occurrence?.start, form.start)) + + (Tasks.DUE to shifted(Tasks.DUE, 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) @@ -322,8 +336,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 deletes: the instances URI would add EXDATEs past the new end. - exceptionsFrom(seriesId, occurrenceStart).forEach { resolver.delete(taskUri(authority(), it), null, null) } + exceptionsFrom(seriesId, occurrenceStart).forEach(::deleteTask) } /** Override rows of [seriesId] standing for [from] or a later occurrence. */ @@ -438,11 +451,14 @@ class AndroidTasksDataSource @Inject constructor( private fun occurrence(taskId: Long, occurrenceStart: Instant, read: (ColumnReader) -> T): T? = queryOccurrences(taskId)?.use { c -> val reader = CursorColumnReader(c) + // An exact match first: a row of the same second is a fallback, not a peer. + var near: Int? = null while (c.moveToNext()) { val anchor = TaskMapper.occurrenceAnchor(reader) ?: continue - if (sameSecond(anchor, occurrenceStart)) return@use read(reader) + if (anchor == occurrenceStart) return@use read(reader) + if (near == null && sameSecond(anchor, occurrenceStart)) near = c.position } - null + near?.let { c.moveToPosition(it); read(reader) } } /** @@ -451,35 +467,60 @@ class AndroidTasksDataSource @Inject constructor( * task's own rows only, rather than a failed query. */ private fun queryOccurrences(taskId: Long): Cursor? { - val uri = TasksContract.instancesUri(authority()) + val authority = authority() + 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) + if (ownRowsOnlyFor != authority) { + try { + return resolver.query(uri, null, OCCURRENCE_OF, arrayOf(id, id), null) + } catch (_: SQLiteException) { + ownRowsOnlyFor = authority + } catch (_: IllegalArgumentException) { + ownRowsOnlyFor = authority + } } + return resolver.query(uri, null, "${Instances.TASK_ID} = ?", arrayOf(id), null) } + /** The provider whose instances view turned out to lack `original_instance_id`. */ + @Volatile + private var ownRowsOnlyFor: String? = null + @Volatile private var anchorsRoundedFor: String? = null + private val roundingLock = Any() + /** * Rounds the DTSTART/DUE a build before [providerMillis] stored with * milliseconds, which the provider cannot move past a completed occurrence. - * Once per provider, before the first read; written as the rows' sync adapter, - * since the server only ever had whole seconds, so nothing is dirtied or - * re-uploaded. Overrides need nothing: the provider forks them at its own - * whole-second instance times. + * Once per provider, before the first read: concurrent readers wait for it. + * Written as the rows' sync adapter, since the server only ever had whole + * seconds, so nothing is dirtied or re-uploaded. Overrides need nothing: the + * provider forks them at its own whole-second instance times. + * + * Never fails a read. A row the provider refuses is left as it was and tried + * again on the next start; only a pass that rounded everything is remembered. */ private fun roundLegacyAnchors() { val authority = authority() if (anchorsRoundedFor == authority) return - anchorsRoundedFor = authority - val prefs = context.getSharedPreferences(MIGRATIONS, Context.MODE_PRIVATE) - val key = "anchors_rounded:$authority" - if (prefs.getBoolean(key, false)) return + synchronized(roundingLock) { + if (anchorsRoundedFor == authority) return + val prefs = context.getSharedPreferences(MIGRATIONS, Context.MODE_PRIVATE) + val key = "anchors_rounded:$authority" + val done = prefs.getBoolean(key, false) || runCatching { roundAll(authority) } + .onFailure { Log.w(TAG, "Rounding legacy anchors failed", it) } + .getOrDefault(false) + if (done) prefs.edit().putBoolean(key, true).apply() + anchorsRoundedFor = authority + } + } + + /** True when every stale row was rounded. */ + private fun roundAll(authority: String): Boolean { val columns = listOf(Tasks.DTSTART, Tasks.DUE) - val stale = resolver.query( + val ops = resolver.query( TasksContract.tasksUri(authority), arrayOf(Tasks.ID, Tasks.ACCOUNT_NAME, Tasks.ACCOUNT_TYPE) + columns, columns.joinToString(" OR ") { "$it % 1000 != 0" }, @@ -487,7 +528,7 @@ class AndroidTasksDataSource @Inject constructor( null, )?.use { c -> val r = CursorColumnReader(c) - buildList { + buildList { while (c.moveToNext()) { val id = r.getLong(Tasks.ID) ?: continue val values = buildMap { @@ -496,17 +537,20 @@ class AndroidTasksDataSource @Inject constructor( if (providerMillis(millis) != millis) put(column, providerMillis(millis)) } } - val uri = TasksContract.asSyncAdapter( - taskUri(authority, id), - r.getString(Tasks.ACCOUNT_NAME).orEmpty(), - r.getString(Tasks.ACCOUNT_TYPE).orEmpty(), - ) - add(uri to values) + val name = r.getString(Tasks.ACCOUNT_NAME) + val type = r.getString(Tasks.ACCOUNT_TYPE) + val uri = if (name.isNullOrEmpty() || type.isNullOrEmpty()) taskUri(authority, id) + else TasksContract.asSyncAdapter(taskUri(authority, id), name, type) + add(ContentProviderOperation.newUpdate(uri).withValues(values.toContentValues()).build()) } } - }.orEmpty() - stale.forEach { (uri, values) -> resolver.update(uri, values.toContentValues(), null, null) } - prefs.edit().putBoolean(key, true).apply() + } ?: return false + if (ops.isEmpty()) return true + val batch = runCatching { resolver.applyBatch(authority, ArrayList(ops)) }.getOrNull() + if (batch != null) return batch.all { (it.count ?: 0) > 0 } + // One refused row fails the whole batch; row by row, the rest still go. + return ops.map { op -> runCatching { resolver.applyBatch(authority, arrayListOf(op)).single().count ?: 0 }.getOrDefault(0) > 0 } + .all { it } } override fun setReminders(taskId: Long, reminders: List) { @@ -751,5 +795,10 @@ private fun sameSecond(a: Instant, b: Instant): Boolean = private const val MIGRATIONS = "provider_migrations" +private const val TAG = "AndroidTasksDataSource" + +/** No rule to look up: the provider says the row does not recur. */ +private const val NO_RULE = -1L + /** 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/TasksDataSource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/TasksDataSource.kt index 3970f19..f70b51a 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 @@ -124,7 +124,8 @@ interface TasksDataSource { * * Implementations fall back to [setCompleted] when the row turns out not to * be a series master — an override, or a plain task the caller happened to - * hand an anchor for — so the routing above cannot get this wrong. + * hand an anchor for. A series whose occurrence cannot be found fails with + * [TaskWriteFailedException] instead: the plain write would close all of it. */ fun setCompletedInstance(taskId: Long, occurrenceStart: Instant, completed: Boolean) 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 b262286..0117995 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 @@ -192,7 +192,8 @@ class ProviderWriter internal constructor( 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 store.deleteWithSubtasks(taskId) + val occurrence = current.recurrenceId + if (current.masterId != null && occurrence != null) store.deleteOverride(taskId, occurrence) else store.deleteWithSubtasks(taskId) return true }