Keep list positions unique and tidy, notify through the provider (#136)

Also carry the order over when copying provider lists into the own store.

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,68 @@
+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(source::deleteList)
+
+    private fun ours() = source.taskLists().map { it.id }.filter { it in created }
+
+    @Test
+    fun aReorderIsKeptAndReported() {
+        val (a, b, c) = created
+        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
+
+        val stored = context.getSharedPreferences("provider_list_order", Context.MODE_PRIVATE)
+            .getString(resolver.resolve()!!.authority, null)
+        assertThat(ListOrder.decode(stored).keys).doesNotContain(c)
+    }
+}

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

@@ -52,14 +52,35 @@ 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()
+        val order = ListOrder.decode(listOrder.getString(authority, null))
+        val known = lists.mapTo(HashSet()) { it.id }
+        // A list gone from the provider takes its position along; its id may be reused.
+        if (order.keys.any { it !in known }) storeListOrder(authority, order.filterKeys { it in known })
+        return ListOrder.sorted(lists, order)
     }
 
+    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))
+        }
+        resolver.notifyChange(TasksContract.listsUri(authority), null)
+    }
+
+    private val listOrderLock = Any()
+
     override fun tasks(query: TaskQuery): List<Task> {
         val clauses = mutableListOf<String>()
         val args = mutableListOf<String>()
@@ -545,6 +566,11 @@ class AndroidTasksDataSource @Inject constructor(
     override fun deleteList(listId: Long) {
         val rows = resolver.delete(listSyncUri(listId), null, null)
         if (rows == 0) throw TaskWriteFailedException("delete list $listId")
+        val authority = authority()
+        synchronized(listOrderLock) {
+            val order = ListOrder.decode(listOrder.getString(authority, null))
+            if (listId in order) storeListOrder(authority, order - listId)
+        }
     }
 
     /**
@@ -594,5 +620,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,35 @@
+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; lists without one go last, by name.
+ */
+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] ?: Int.MAX_VALUE }, { it.name.lowercase() })
+            .thenBy { it.name },
+    )
+}

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

@@ -159,7 +159,7 @@ class ExternalImport @Inject constructor(
         val taskDao = database.tasks()
         val alarmDao = database.alarms()
 
-        val listIds = snapshot.lists.associate { list ->
+        val listIds = snapshot.lists.withIndex().associate { (position, list) ->
             list.id to listDao.insert(
                 TaskListEntity(
                     name = list.name,
@@ -168,6 +168,7 @@ class ExternalImport @Inject constructor(
                     isVisible = list.isVisible,
                     isSynced = false,
                     owner = list.owner,
+                    sortOrder = 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 goes last, by name`() {
+        val lists = listOf(list(1, "Zeta"), list(2, "Alpha"), list(3, "Mid"))
+        val order = mapOf(3L to 0)
+
+        assertThat(ListOrder.sorted(lists, order).map { it.id }).containsExactly(3L, 2L, 1L).inOrder()
+    }
+}