From 2dfc676b8fd814b3f44b44bbed79acedb9d1711c Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Mon, 9 Nov 2026 18:30:33 +0100 Subject: [PATCH] Keep the list order with tasks.org and OpenTasks (#136) (#143) --- CHANGELOG.md | 2 + .../tasks/ExternalProviderListOrderTest.kt | 76 +++++++++++++++++++ .../data/tasks/AndroidTasksDataSource.kt | 45 +++++++++-- .../agendula/data/tasks/ListOrder.kt | 36 +++++++++ .../agendula/data/tasks/room/TaskListDao.kt | 6 +- .../data/tasks/transfer/ExternalImport.kt | 5 +- .../agendula/data/tasks/ListOrderTest.kt | 56 ++++++++++++++ 7 files changed, 219 insertions(+), 7 deletions(-) create mode 100644 app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderListOrderTest.kt create mode 100644 app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ListOrder.kt create mode 100644 app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ListOrderTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fcd508..2eabbcb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ All notable changes to this project are documented here. The format follows closed the whole series. With tasks.org and OpenTasks alike, the editor now shows the task's repeat rule instead of none, and an occurrence changed on its own stays part of its series (#135). +- With tasks.org or OpenTasks as the storage provider, the order you give your + lists is kept instead of snapping back (#136). ## [1.3.0] - 2026-11-02 diff --git a/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderListOrderTest.kt b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderListOrderTest.kt new file mode 100644 index 0000000..5262d6a --- /dev/null +++ b/app/src/androidTest/java/de/jeanlucmakiola/agendula/data/tasks/ExternalProviderListOrderTest.kt @@ -0,0 +1,76 @@ +package de.jeanlucmakiola.agendula.data.tasks + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat +import org.junit.After +import org.junit.Assume.assumeTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit + +/** + * List order against a real external provider (#136). Skipped unless one is + * installed and its permissions are granted. + */ +@RunWith(AndroidJUnit4::class) +class ExternalProviderListOrderTest { + + private val context = ApplicationProvider.getApplicationContext() + private val resolver = ProviderResolver(AndroidProviderEnvironment(context)).apply { + storageMode = StorageMode.EXTERNAL + } + private val source = AndroidTasksDataSource(context, resolver) + private val created = mutableListOf() + + @Before + fun setUp() { + val provider = resolver.resolve() + assumeTrue("no external tasks provider, or not granted", provider != null && resolver.hasPermission(provider)) + listOf("Order A", "Order B", "Order C").forEach { created += source.createLocalList(it, 0) } + } + + @After + fun tearDown() = created.forEach { runCatching { source.deleteList(it) } } + + private fun ours() = source.taskLists().map { it.id }.filter { it in created } + + /** Waits until the provider has gone quiet, so a change seen afterwards is the test's own. */ + private fun settle() { + var last = System.currentTimeMillis() + val observer = source.registerObserver { last = System.currentTimeMillis() } + while (System.currentTimeMillis() - last < 1_500) Thread.sleep(100) + observer.close() + } + + @Test + fun aReorderIsKeptAndReported() { + val (a, b, c) = created + settle() + val changed = CountDownLatch(1) + val observer = source.registerObserver { changed.countDown() } + + source.reorderLists(listOf(c, a, b)) + + assertThat(changed.await(5, TimeUnit.SECONDS)).isTrue() + observer.close() + assertThat(ours()).containsExactly(c, a, b).inOrder() + assertThat(AndroidTasksDataSource(context, resolver).taskLists().map { it.id }.filter { it in created }) + .containsExactly(c, a, b).inOrder() + } + + @Test + fun aDeletedListLeavesNoPositionBehind() { + val (a, b, c) = created + source.reorderLists(listOf(c, a, b)) + + source.deleteList(c) + created -= c + + source.taskLists() + assertThat(source.storedListOrder().keys).doesNotContain(c) + } +} 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 f653735..03cf612 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 @@ -55,14 +55,47 @@ class AndroidTasksDataSource @Inject constructor( // --- reads ---------------------------------------------------------------- override fun taskLists(): List { - val uri = TasksContract.listsUri(authority()) - val sort = "${Lists.ACCOUNT_NAME}, ${Lists.NAME}" - return resolver.query(uri, TaskProjections.LISTS, null, null, sort)?.use { c -> + val authority = authority() + val lists = resolver.query(TasksContract.listsUri(authority), TaskProjections.LISTS, null, null, null)?.use { c -> val reader = CursorColumnReader(c) - buildList { while (c.moveToNext()) add(TaskMapper.taskList(reader)) } - } ?: emptyList() + buildList { while (c.moveToNext()) add(TaskMapper.taskList(reader)) } + } ?: return emptyList() + val order = synchronized(listOrderLock) { + val stored = storedListOrder(authority) + val known = lists.mapTo(HashSet()) { it.id } + // A list gone from the provider takes its position along, as its id may be + // reused — but not on an empty answer, which is as likely a provider mid-update. + if (lists.isNotEmpty() && stored.keys.any { it !in known }) { + stored.filterKeys { it in known }.also { storeListOrder(authority, it) } + } else { + stored + } + } + return ListOrder.sorted(lists, order) } + /** The order Agendula keeps for [authority]'s lists. */ + internal fun storedListOrder(authority: String = authority()): Map = + ListOrder.decode(listOrder.getString(authority, null)) + + private val listOrder get() = context.getSharedPreferences(LIST_ORDER, Context.MODE_PRIVATE) + + private fun storeListOrder(authority: String, order: Map) { + listOrder.edit().putString(authority, ListOrder.encode(order)).commit() + } + + /** Kept by Agendula, as the provider's lists table has no order of its own. */ + override fun reorderLists(listIds: List) { + val authority = authority() + synchronized(listOrderLock) { + storeListOrder(authority, ListOrder.reordered(taskLists().map { it.id }, listIds)) + } + // Wakes the observers on the lists without asking any sync adapter to upload: nothing changed in the provider. + runCatching { resolver.notifyChange(TasksContract.listsUri(authority), null, 0) } + } + + private val listOrderLock = Any() + override fun tasks(query: TaskQuery): List { val clauses = mutableListOf() val args = mutableListOf() @@ -787,6 +820,8 @@ class AndroidTasksDataSource @Inject constructor( } } +private const val LIST_ORDER = "provider_list_order" + /** 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} = ?" diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ListOrder.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ListOrder.kt new file mode 100644 index 0000000..1627489 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ListOrder.kt @@ -0,0 +1,36 @@ +package de.jeanlucmakiola.agendula.data.tasks + +import de.jeanlucmakiola.agendula.domain.TaskList + +/** + * Agendula's own order for an external provider's lists, which the provider has + * no column for. Stored as `id=position` pairs. A list without one counts as + * position 0, as a new list does in Agendula's own store. + */ +object ListOrder { + + fun decode(raw: String?): Map = raw.orEmpty().split(',').mapNotNull { pair -> + val (id, position) = pair.split('=').takeIf { it.size == 2 } ?: return@mapNotNull null + id.toLongOrNull()?.let { key -> position.toIntOrNull()?.let { key to it } } + }.toMap() + + fun encode(order: Map): String = order.entries.joinToString(",") { "${it.key}=${it.value}" } + + /** + * Positions for every list in [shown] — the lists in their current order — + * after one account's [listIds] were rearranged: those take each other's + * slots in their new order, and every other list keeps its own. + */ + fun reordered(shown: List, listIds: List): Map { + val moved = listIds.iterator() + val moving = listIds.toSet() + val result = shown.map { if (it in moving && moved.hasNext()) moved.next() else it }.toMutableList() + moved.forEachRemaining { if (it !in result) result += it } + return result.withIndex().associate { it.value to it.index } + } + + fun sorted(lists: List, order: Map): List = lists.sortedWith( + compareBy({ it.accountName.lowercase() }, { order[it.id] ?: 0 }, { it.name.lowercase() }) + .thenBy { it.name }, + ) +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt index a5d6bd8..fc71614 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt @@ -18,7 +18,7 @@ interface TaskListDao { """ SELECT l.*, a.display_name AS account_display_name, a.label AS account_label FROM task_lists l LEFT JOIN accounts a ON a.id = l.account_id - ORDER BY a.display_name, l.sort_order, l.name + ORDER BY a.display_name COLLATE NOCASE, l.sort_order, l.name COLLATE NOCASE, l.name """ ) fun lists(): List @@ -32,6 +32,10 @@ interface TaskListDao { ) fun list(listId: Long): TaskListRow? + /** The highest position among device-only lists, -1 when there are none. */ + @Query("SELECT COALESCE(MAX(sort_order), -1) FROM task_lists WHERE account_id IS NULL") + fun lastLocalSortOrder(): Int + @Query("SELECT * FROM task_lists WHERE id = :listId") fun entity(listId: Long): TaskListEntity? diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/transfer/ExternalImport.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/transfer/ExternalImport.kt index 9999e14..b08107b 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/transfer/ExternalImport.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/transfer/ExternalImport.kt @@ -159,7 +159,9 @@ class ExternalImport @Inject constructor( val taskDao = database.tasks() val alarmDao = database.alarms() - val listIds = snapshot.lists.associate { list -> + // After the device-only lists already there, which the copies join. + val after = listDao.lastLocalSortOrder() + 1 + val listIds = snapshot.lists.withIndex().associate { (position, list) -> list.id to listDao.insert( TaskListEntity( name = list.name, @@ -168,6 +170,7 @@ class ExternalImport @Inject constructor( isVisible = list.isVisible, isSynced = false, owner = list.owner, + sortOrder = after + position, ), ) } diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ListOrderTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ListOrderTest.kt new file mode 100644 index 0000000..0695ab3 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ListOrderTest.kt @@ -0,0 +1,56 @@ +package de.jeanlucmakiola.agendula.data.tasks + +import com.google.common.truth.Truth.assertThat +import de.jeanlucmakiola.agendula.domain.TaskList +import org.junit.jupiter.api.Test + +class ListOrderTest { + + private fun list(id: Long, name: String, account: String = "Radicale") = TaskList( + id = id, name = name, color = 0, accountName = account, accountType = "bitfire.at.davdroid", + isSynced = true, isVisible = true, owner = null, + ) + + @Test + fun `an order survives encoding`() { + val order = mapOf(3L to 0, 1L to 1, 2L to 2) + assertThat(ListOrder.decode(ListOrder.encode(order))).isEqualTo(order) + } + + @Test + fun `nothing stored and garbage both decode to no order`() { + assertThat(ListOrder.decode(null)).isEmpty() + assertThat(ListOrder.decode("x=1,2,=3,4=y")).isEmpty() + } + + @Test + fun `lists follow the stored order within their account (#136)`() { + val lists = listOf(list(1, "Groceries"), list(2, "Home"), list(3, "Work"), list(4, "Inbox", account = "Local")) + val order = ListOrder.reordered(listOf(4, 1, 2, 3), listOf(3, 1, 2)) + + assertThat(ListOrder.sorted(lists, order).map { it.id }).containsExactly(4L, 3L, 1L, 2L).inOrder() + } + + @Test + fun `reordering one account leaves every other list in its slot`() { + // 7 and 8 are another account; 5 joined this one after the screen was drawn. + val order = ListOrder.reordered(listOf(7, 8, 1, 5, 2), listOf(2, 1)) + + assertThat(order).isEqualTo(mapOf(7L to 0, 8L to 1, 2L to 2, 5L to 3, 1L to 4)) + assertThat(order.values.toSet()).hasSize(order.size) + } + + @Test + fun `names sort without regard to case`() { + val lists = listOf(list(1, "Zeta"), list(2, "apple")) + assertThat(ListOrder.sorted(lists, emptyMap()).map { it.id }).containsExactly(2L, 1L).inOrder() + } + + @Test + fun `a list without a position counts as the first, as in the own store`() { + val lists = listOf(list(1, "Zeta"), list(2, "Alpha"), list(3, "Mid")) + val order = mapOf(3L to 0, 1L to 1) + + assertThat(ListOrder.sorted(lists, order).map { it.id }).containsExactly(2L, 3L, 1L).inOrder() + } +}