Keep the list order through provider hiccups, match the own store's ordering (#136)

An empty or failed list read no longer wipes the stored order, pruning holds
the lock, the reorder notification asks for no network sync, copied lists go
after the device-only ones, and both stores sort names case-insensitively with
unpositioned lists first.

CHANGELOG.md

@@ -7,6 +7,10 @@ All notable changes to this project are documented here. The format follows
 
 ## [Unreleased]
 
+### Fixed
+- 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
 
 ### Added

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<Context>()
+    private val resolver = ProviderResolver(AndroidProviderEnvironment(context)).apply {
+        storageMode = StorageMode.EXTERNAL
+    }
+    private val source = AndroidTasksDataSource(context, resolver)
+    private val created = mutableListOf<Long>()
+
+    @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)
+    }
+}

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

@@ -52,14 +52,47 @@ class AndroidTasksDataSource @Inject constructor(
     // --- reads ----------------------------------------------------------------
 
     override fun taskLists(): List<TaskList> {
-        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<TaskList> { 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<Long, Int> =
+        ListOrder.decode(listOrder.getString(authority, null))
+
+    private val listOrder get() = context.getSharedPreferences(LIST_ORDER, Context.MODE_PRIVATE)
+
+    private fun storeListOrder(authority: String, order: Map<Long, Int>) {
+        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<Long>) {
+        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<Task> {
         val clauses = mutableListOf<String>()
         val args = mutableListOf<String>()
@@ -594,5 +627,7 @@ class AndroidTasksDataSource @Inject constructor(
     }
 }
 
+private const val LIST_ORDER = "provider_list_order"
+
 /** 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/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<Long, Int> = 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<Long, Int>): 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<Long>, listIds: List<Long>): Map<Long, Int> {
+        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<TaskList>, order: Map<Long, Int>): List<TaskList> = lists.sortedWith(
+        compareBy<TaskList>({ it.accountName.lowercase() }, { order[it.id] ?: 0 }, { it.name.lowercase() })
+            .thenBy { it.name },
+    )
+}

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<TaskListRow>
@@ -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?
 

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

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()
+    }
+}