From 9a69d175a876826f82f92c1a8122812dde107fb3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 12:04:33 -0500 Subject: [PATCH 1/3] feat(settings): reorder accounts by drag in settings Give accounts a user-controlled order (issue #164). Every surface that lists accounts -- the Settings list, the drawer account switcher, and the compose account picker -- reads the same `ORDER BY sortOrder` query, so a reorder in Settings is honored app-wide. In Settings a row can be long-pressed and dragged to a new position; the order persists and survives restart. Data layer: - AccountEntity gains `sortOrder` (@ColumnInfo defaultValue "0"); AccountDao orders by it and adds insertAtEnd / reorder / nextSortOrder / setSortOrder, the mutations wrapped in transactions. - New accounts are appended (current max + 1) via insertAtEnd. Migration (AccountDatabase v1 -> v2): - ACCOUNT_MIGRATION_1_2 adds the column and backfills existing accounts by their previous alphabetical (email) rank, so the already-shown order does not shuffle on upgrade. Registered in AccountDatabaseModule; 2.json is exported and AccountMigrationTest replays and validates it. - AccountDataMigrator (the pre-#111 cache->account-db move) creates the v2-shaped table and applies the same email-rank backfill, since ACCOUNT_MIGRATION_1_2 does not run for that path. UI: - AccountReorderList drives long-press drag over a plain Column (no nested lazy list inside the scrolling settings column, no extra dependency); the pure reorder-index maths (commitDrag) is unit-tested. Co-Authored-By: Claude Opus 4.8 --- .../2.json | 241 ++++++++++++++++++ .../data/local/AccountDataMigratorTest.kt | 29 ++- .../data/local/AccountDatabaseTest.kt | 32 ++- .../data/local/AccountMigrationTest.kt | 77 ++++++ .../kotlin/org/libremail/ui/Fakes.kt | 5 + .../data/local/AccountDataMigrator.kt | 15 +- .../libremail/data/local/AccountDatabase.kt | 2 +- .../libremail/data/local/AccountMigrations.kt | 33 +++ .../libremail/data/local/dao/AccountDao.kt | 31 ++- .../data/local/entity/AccountEntity.kt | 9 + .../data/repository/AccountRepositoryImpl.kt | 6 +- .../org/libremail/di/AccountDatabaseModule.kt | 2 + .../domain/repository/AccountRepository.kt | 7 + .../ui/settings/AccountReorderList.kt | 157 ++++++++++++ .../libremail/ui/settings/SettingsScreen.kt | 8 +- .../ui/settings/SettingsViewModel.kt | 3 + app/src/main/res/values/strings.xml | 1 + .../ui/settings/AccountReorderListTest.kt | 47 ++++ 18 files changed, 693 insertions(+), 12 deletions(-) create mode 100644 app/schemas/org.libremail.data.local.AccountDatabase/2.json create mode 100644 app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt create mode 100644 app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt create mode 100644 app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt create mode 100644 app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListTest.kt diff --git a/app/schemas/org.libremail.data.local.AccountDatabase/2.json b/app/schemas/org.libremail.data.local.AccountDatabase/2.json new file mode 100644 index 0000000..627134f --- /dev/null +++ b/app/schemas/org.libremail.data.local.AccountDatabase/2.json @@ -0,0 +1,241 @@ +{ + "formatVersion": 1, + "database": { + "version": 2, + "identityHash": "6705bfa56d02b55f47c5c66ba10143b0", + "entities": [ + { + "tableName": "accounts", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `email` TEXT NOT NULL, `displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, `sortOrder` INTEGER NOT NULL DEFAULT 0, `imap_host` TEXT NOT NULL, `imap_port` INTEGER NOT NULL, `imap_security` TEXT NOT NULL, `smtp_host` TEXT NOT NULL, `smtp_port` INTEGER NOT NULL, `smtp_security` TEXT NOT NULL, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "email", + "columnName": "email", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "displayName", + "columnName": "displayName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "authType", + "columnName": "authType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sortOrder", + "columnName": "sortOrder", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "imap.host", + "columnName": "imap_host", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "imap.port", + "columnName": "imap_port", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "imap.security", + "columnName": "imap_security", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "smtp.host", + "columnName": "smtp_host", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "smtp.port", + "columnName": "smtp_port", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "smtp.security", + "columnName": "smtp_security", + "affinity": "TEXT", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "credentials", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `encryptedSecret` TEXT NOT NULL, PRIMARY KEY(`accountId`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "encryptedSecret", + "columnName": "encryptedSecret", + "affinity": "TEXT", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId" + ] + } + }, + { + "tableName": "account_settings", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `signature` TEXT NOT NULL, `signatureEnabled` INTEGER NOT NULL, `notificationsEnabled` INTEGER NOT NULL, `retentionCount` INTEGER, `retentionMonths` INTEGER, PRIMARY KEY(`accountId`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "signature", + "columnName": "signature", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "signatureEnabled", + "columnName": "signatureEnabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "notificationsEnabled", + "columnName": "notificationsEnabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "retentionCount", + "columnName": "retentionCount", + "affinity": "INTEGER" + }, + { + "fieldPath": "retentionMonths", + "columnName": "retentionMonths", + "affinity": "INTEGER" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId" + ] + }, + "foreignKeys": [ + { + "table": "accounts", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "accountId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "signatures", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `name` TEXT NOT NULL, `contentHtml` TEXT NOT NULL, `isDefault` INTEGER NOT NULL, PRIMARY KEY(`id`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "name", + "columnName": "name", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "contentHtml", + "columnName": "contentHtml", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "isDefault", + "columnName": "isDefault", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_signatures_accountId", + "unique": false, + "columnNames": [ + "accountId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_signatures_accountId` ON `${TABLE_NAME}` (`accountId`)" + } + ], + "foreignKeys": [ + { + "table": "accounts", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "accountId" + ], + "referencedColumns": [ + "id" + ] + } + ] + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '6705bfa56d02b55f47c5c66ba10143b0')" + ] + } +} \ No newline at end of file diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index eda28f0..a53c863 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -221,11 +221,38 @@ class AccountDataMigratorTest { } } + @Test + fun copiedAccountsGetAStableAlphabeticalInitialSortOrder() = runBlocking { + // Insert three accounts in non-alphabetical order so the assertion proves the initial order is + // by email (the pre-#111 listing) rather than the copy's insertion order (issue #164). sortOrder + // is a destination-only column the v14 cache never had, so it is assigned entirely by the copy. + helper.createDatabase(cacheName, 14).apply { + listOf("c" to "carol@example.org", "a" to "ada@example.org", "b" to "bob@example.org") + .forEach { (id, email) -> + execSQL( + "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, " + + "imap_security, smtp_host, smtp_port, smtp_security) VALUES " + + "('$id', '$email', '$id', 'PASSWORD_IMAP', 'imap.example.org', 993, 'SSL_TLS', " + + "'smtp.example.org', 465, 'SSL_TLS')", + ) + } + close() + } + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + openAccountsDb().apply { + // getAll orders by sortOrder; the copy ranked by email, so ada(0) < bob(1) < carol(2). + assertEquals(listOf("a", "b", "c"), accountDao().getAll().map { it.id }) + close() + } + } + @Test fun migratorDdlMatchesExportedAccountDatabaseSchema() { val schema = JSONObject( InstrumentationRegistry.getInstrumentation().context.assets - .open("org.libremail.data.local.AccountDatabase/1.json") + .open("org.libremail.data.local.AccountDatabase/2.json") .bufferedReader().use { it.readText() }, ).getJSONObject("database") val entities = schema.getJSONArray("entities") diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt index c8131c5..b908d0e 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt @@ -39,9 +39,9 @@ class AccountDatabaseTest { @After fun tearDown() = db.close() - private fun account(id: String = "acct") = AccountEntity( + private fun account(id: String = "acct", email: String = "$id@example.org") = AccountEntity( id = id, - email = "a@example.org", + email = email, displayName = "A", authType = "PASSWORD_IMAP", imap = ServerConfigEmbedded("imap.example.org", 993, "SSL_TLS"), @@ -69,6 +69,34 @@ class AccountDatabaseTest { assertNull("account_settings must cascade-delete with its account", db.accountSettingsDao().get("acct")) } + @Test + fun insertAtEndAppendsAccountsInIncreasingSortOrder() = runBlocking { + db.accountDao().insertAtEnd(account("a")) + db.accountDao().insertAtEnd(account("b")) + db.accountDao().insertAtEnd(account("c")) + + // Each new account lands at the end: current max + 1, starting from 0 (issue #164). + assertEquals(0, db.accountDao().getById("a")?.sortOrder) + assertEquals(1, db.accountDao().getById("b")?.sortOrder) + assertEquals(2, db.accountDao().getById("c")?.sortOrder) + // observeAll now orders by sortOrder, i.e. the append (insertion) order. + assertEquals(listOf("a", "b", "c"), db.accountDao().observeAll().first().map { it.id }) + } + + @Test + fun reorderPersistsAndBothQueriesReflectTheNewOrder() = runBlocking { + listOf("a", "b", "c").forEach { db.accountDao().insertAtEnd(account(it)) } + + db.accountDao().reorder(listOf("c", "a", "b")) + + assertEquals(listOf("c", "a", "b"), db.accountDao().observeAll().first().map { it.id }) + assertEquals(listOf("c", "a", "b"), db.accountDao().getAll().map { it.id }) + // sortOrder is renumbered to the new positions, so the order survives an app restart. + assertEquals(0, db.accountDao().getById("c")?.sortOrder) + assertEquals(1, db.accountDao().getById("a")?.sortOrder) + assertEquals(2, db.accountDao().getById("b")?.sortOrder) + } + @Test fun signaturesCascadeWithTheirAccount() = runBlocking { db.accountDao().upsert(account()) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt new file mode 100644 index 0000000..7593a44 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountMigrationTest.kt @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import androidx.room.testing.MigrationTestHelper +import androidx.sqlite.db.SupportSQLiteDatabase +import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Replays [AccountDatabase]'s migrations against the schema JSONs exported to `app/schemas` (shipped + * to the test APK as assets). AccountDatabase is the non-auth account store split out of the cache in + * issue #111; like the cache database it registers no destructive fallback, so a drifted migration + * would crash upgrading users at first open — this test makes such drift fail in CI instead. Kept + * separate from [MigrationTest] because each database has its own version line and MigrationTestHelper + * binds to a single [androidx.room.RoomDatabase] class. + */ +@RunWith(AndroidJUnit4::class) +class AccountMigrationTest { + + @get:Rule + val helper = MigrationTestHelper( + InstrumentationRegistry.getInstrumentation(), + AccountDatabase::class.java, + emptyList(), + FrameworkSQLiteOpenHelperFactory(), + ) + + /** + * v1 -> v2 (issue #164): `accounts.sortOrder` appears, and existing accounts are backfilled with a + * stable initial order matching the previous alphabetical (`ORDER BY email`) listing. The rows are + * inserted out of alphabetical order to prove the rank is by email, not insertion order. + */ + @Test + fun migrate1To2_addsSortOrderBackfilledByEmailRank() { + helper.createDatabase(TEST_DB, 1).apply { + insertAccount("c", "carol@example.org") + insertAccount("a", "ada@example.org") + insertAccount("b", "bob@example.org") + close() + } + + val db = helper.runMigrationsAndValidate(TEST_DB, 2, true, ACCOUNT_MIGRATION_1_2) + + db.query("SELECT id, sortOrder FROM accounts ORDER BY sortOrder").use { c -> + assertTrue(c.moveToFirst()) + assertEquals("ada ranks first alphabetically, so sortOrder 0", "a", c.getString(0)) + assertEquals(0, c.getInt(1)) + assertTrue(c.moveToNext()) + assertEquals("b", c.getString(0)) + assertEquals(1, c.getInt(1)) + assertTrue(c.moveToNext()) + assertEquals("c", c.getString(0)) + assertEquals(2, c.getInt(1)) + assertFalse("all three pre-upgrade accounts must survive, and nothing else", c.moveToNext()) + } + db.close() + } + + private fun SupportSQLiteDatabase.insertAccount(id: String, email: String) { + execSQL( + "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, imap_security, " + + "smtp_host, smtp_port, smtp_security) VALUES ('$id', '$email', '$id', 'PASSWORD_IMAP', " + + "'imap.example.org', 993, 'SSL_TLS', 'smtp.example.org', 465, 'SSL_TLS')", + ) + } + + private companion object { + const val TEST_DB = "account-migration-test.db" + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt index f59995c..1441411 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt @@ -62,6 +62,11 @@ class FakeAccountRepository( accountsFlow.value = accountsFlow.value.filterNot { it.id == id } } + override suspend fun reorderAccounts(orderedIds: List) { + val byId = accountsFlow.value.associateBy { it.id } + accountsFlow.value = orderedIds.mapNotNull { byId[it] } + } + override suspend fun resetBackfillProgress(accountId: String?) = Unit } diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt index 521bc81..a1be640 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -108,7 +108,8 @@ class AccountDataMigrator @Inject constructor( internal val CREATE_TABLE_SQL = mapOf( "accounts" to "CREATE TABLE IF NOT EXISTS `accounts` (`id` TEXT NOT NULL, `email` TEXT NOT NULL, " + - "`displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, `imap_host` TEXT NOT NULL, " + + "`displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, " + + "`sortOrder` INTEGER NOT NULL DEFAULT 0, `imap_host` TEXT NOT NULL, " + "`imap_port` INTEGER NOT NULL, `imap_security` TEXT NOT NULL, `smtp_host` TEXT NOT NULL, " + "`smtp_port` INTEGER NOT NULL, `smtp_security` TEXT NOT NULL, PRIMARY KEY(`id`))", "credentials" to @@ -168,6 +169,18 @@ class AccountDataMigrator @Inject constructor( val cols = sharedColumns(db, table) db.rawExecSQL("INSERT OR IGNORE INTO `$table` ($cols) SELECT $cols FROM cache.`$table`") } + // sortOrder (issue #164) is a destination-only column the pre-#111 cache never had, so + // the copy above leaves every account at its DEFAULT 0. Give them the same stable + // alphabetical initial order ACCOUNT_MIGRATION_1_2 assigns (rank by email), so a + // pre-#111 upgrade lands in the order it already showed rather than an undefined + // tie-break. Post-#111 installs migrate via ACCOUNT_MIGRATION_1_2 instead and never + // reach this copy; either way the user can then drag to reorder. + if ("accounts" in present) { + db.rawExecSQL( + "UPDATE `accounts` SET `sortOrder` = " + + "(SELECT COUNT(*) FROM `accounts` AS ranked WHERE ranked.`email` < `accounts`.`email`)", + ) + } Log.d(TAG, "moved account tables into the account database: $present") } finally { db.rawExecSQL("DETACH DATABASE cache;") diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt index cdf4e47..3b0a070 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt @@ -40,7 +40,7 @@ import org.libremail.data.local.entity.SignatureEntity AccountSettingsEntity::class, SignatureEntity::class, ], - version = 1, + version = 2, exportSchema = true, ) abstract class AccountDatabase : RoomDatabase() { diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt b/app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt new file mode 100644 index 0000000..0ff0112 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/AccountMigrations.kt @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import androidx.room.migration.Migration +import androidx.sqlite.db.SupportSQLiteDatabase + +/** + * Migrations for [AccountDatabase] — the non-auth account store split out of the cache database in + * issue #111. Kept in a SEPARATE file from the cache database's `MIGRATION_x_y` chain in + * `Migrations.kt` on purpose: both databases number their own schema versions from 1, and + * `MigrationTest` reflectively pulls every `Migration` val out of the `Migrations.kt` file facade to + * assert the cache chain is unbroken — an AccountDatabase migration declared alongside them would + * pollute that assertion with a second `1 -> 2`. A distinct file (and the `ACCOUNT_` prefix) keeps + * the two chains from colliding at the package level. + */ + +/** + * AccountDatabase v1 -> v2 (issue #164): add the user-controlled [AccountEntity.sortOrder]. Existing + * accounts are given a stable initial order matching the previous alphabetical (`ORDER BY email`) + * listing — each account's rank among the others by email — so the order they were already shown in + * doesn't shuffle on upgrade. `DEFAULT 0` matches the entity's `@ColumnInfo(defaultValue = "0")` so a + * fresh install validates identically to a migrated one (the folders `specialUse` MIGRATION_11_12 + * pattern); the backfill then overwrites that default with the real rank. + */ +val ACCOUNT_MIGRATION_1_2 = object : Migration(1, 2) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("ALTER TABLE `accounts` ADD COLUMN `sortOrder` INTEGER NOT NULL DEFAULT 0") + db.execSQL( + "UPDATE `accounts` SET `sortOrder` = " + + "(SELECT COUNT(*) FROM `accounts` AS ranked WHERE ranked.`email` < `accounts`.`email`)", + ) + } +} diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt index 45ff38d..af87c1b 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt @@ -5,15 +5,16 @@ import androidx.room.Dao import androidx.room.Insert import androidx.room.OnConflictStrategy import androidx.room.Query +import androidx.room.Transaction import kotlinx.coroutines.flow.Flow import org.libremail.data.local.entity.AccountEntity @Dao interface AccountDao { - @Query("SELECT * FROM accounts ORDER BY email") + @Query("SELECT * FROM accounts ORDER BY sortOrder") fun observeAll(): Flow> - @Query("SELECT * FROM accounts ORDER BY email") + @Query("SELECT * FROM accounts ORDER BY sortOrder") suspend fun getAll(): List @Query("SELECT * FROM accounts WHERE id = :id LIMIT 1") @@ -22,6 +23,32 @@ interface AccountDao { @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun upsert(account: AccountEntity) + /** The sortOrder that appends a new account to the end of the list (0 when there are none yet). */ + @Query("SELECT COALESCE(MAX(sortOrder), -1) + 1 FROM accounts") + suspend fun nextSortOrder(): Int + + /** + * Insert [account] at the end of the user-defined order (issue #164), stamping it with the current + * max + 1. Both steps run in one transaction so two near-simultaneous adds can't collide on a + * sortOrder. + */ + @Transaction + suspend fun insertAtEnd(account: AccountEntity) { + upsert(account.copy(sortOrder = nextSortOrder())) + } + + @Query("UPDATE accounts SET sortOrder = :sortOrder WHERE id = :id") + suspend fun setSortOrder(id: String, sortOrder: Int) + + /** + * Persist a user-chosen ordering (issue #164): each account is stamped with its index in + * [orderedIds]. Wrapped in a transaction so the list is never observed half-renumbered. + */ + @Transaction + suspend fun reorder(orderedIds: List) { + orderedIds.forEachIndexed { index, id -> setSortOrder(id, index) } + } + @Query("DELETE FROM accounts WHERE id = :id") suspend fun deleteById(id: String) } diff --git a/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt b/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt index ae7efc5..7d22516 100644 --- a/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt +++ b/app/src/main/kotlin/org/libremail/data/local/entity/AccountEntity.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.local.entity +import androidx.room.ColumnInfo import androidx.room.Embedded import androidx.room.Entity import androidx.room.PrimaryKey @@ -14,6 +15,14 @@ data class AccountEntity( val authType: String, @Embedded(prefix = "imap_") val imap: ServerConfigEmbedded, @Embedded(prefix = "smtp_") val smtp: ServerConfigEmbedded, + /** + * The account's position in the user-defined ordering shown across the app (issue #164): the + * Settings list, the drawer's account switcher, and the unified-inbox filter chips all list + * accounts `ORDER BY sortOrder`. New accounts are appended (current max + 1); the user can drag to + * reorder. `DEFAULT 0` matches [ACCOUNT_MIGRATION_1_2] so a fresh install validates identically to + * a migrated one (the folders `specialUse` pattern). + */ + @ColumnInfo(defaultValue = "0") val sortOrder: Int = 0, ) /** Embedded host/port/security columns (prefixed per server in [AccountEntity]). */ diff --git a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt index f682538..5c63327 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt @@ -44,7 +44,7 @@ class AccountRepositoryImpl @Inject constructor( override suspend fun addImapAccount(account: Account, password: String): Result> = runCatching { val folders = imapClient.listFolders(account.toImapParams(secret = password, useXoauth2 = false)) - accountDao.upsert(account.toEntity()) + accountDao.insertAtEnd(account.toEntity()) accountSettingsRepository.ensureDefaults(account.id) credentialStore.saveSecret(account.id, password) mailNotifier.ensureAccountChannel(account) @@ -60,7 +60,7 @@ class AccountRepositoryImpl @Inject constructor( ): Result> = runCatching { val account = Account.outlook(email) val folders = imapClient.listFolders(account.toImapParams(secret = accessToken, useXoauth2 = true)) - accountDao.upsert(account.toEntity()) + accountDao.insertAtEnd(account.toEntity()) accountSettingsRepository.ensureDefaults(account.id) credentialStore.saveSecret(account.id, authStateJson) mailNotifier.ensureAccountChannel(account) @@ -69,6 +69,8 @@ class AccountRepositoryImpl @Inject constructor( folders.map { it.fullName } } + override suspend fun reorderAccounts(orderedIds: List) = accountDao.reorder(orderedIds) + override suspend fun deleteAccount(id: String) { accountDao.deleteById(id) credentialStore.delete(id) diff --git a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt index 60bac01..7e44640 100644 --- a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt @@ -10,6 +10,7 @@ import dagger.hilt.InstallIn import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.components.SingletonComponent import kotlinx.coroutines.runBlocking +import org.libremail.data.local.ACCOUNT_MIGRATION_1_2 import org.libremail.data.local.AccountDatabase import org.libremail.data.local.DatabaseFiles.ACCOUNTS_NAME import org.libremail.data.local.DatabaseProvisioner @@ -43,6 +44,7 @@ object AccountDatabaseModule { @ApplicationContext context: Context, provisioner: DatabaseProvisioner, ): AccountDatabase = Room.databaseBuilder(context, AccountDatabase::class.java, ACCOUNTS_NAME) + .addMigrations(ACCOUNT_MIGRATION_1_2) .openHelperFactory( DeferredOpenHelperFactory { configuration -> runBlocking { provisioner.prepareCache() } diff --git a/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt b/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt index f25c0be..da9f3a0 100644 --- a/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt +++ b/app/src/main/kotlin/org/libremail/domain/repository/AccountRepository.kt @@ -20,6 +20,13 @@ interface AccountRepository { suspend fun deleteAccount(id: String) + /** + * Persist a user-chosen account order (issue #164). [orderedIds] is the new top-to-bottom order; + * every account's stored position is updated to its index in the list. All surfaces that list + * accounts (Settings, the drawer switcher, the unified-inbox filter chips) then follow it. + */ + suspend fun reorderAccounts(orderedIds: List) + /** * Discards full-history backfill progress so it re-evaluates against the current retention floor * after a retention change: tightening re-hits the (tighter) floor cheaply, loosening resumes diff --git a/app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt b/app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt new file mode 100644 index 0000000..8a996f6 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/ui/settings/AccountReorderList.kt @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.settings + +import androidx.compose.animation.core.animateDpAsState +import androidx.compose.foundation.clickable +import androidx.compose.foundation.gestures.detectDragGesturesAfterLongPress +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.offset +import androidx.compose.foundation.layout.padding +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Menu +import androidx.compose.material3.Icon +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Surface +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableFloatStateOf +import androidx.compose.runtime.mutableIntStateOf +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.input.pointer.pointerInput +import androidx.compose.ui.layout.onSizeChanged +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.unit.IntOffset +import androidx.compose.ui.unit.dp +import androidx.compose.ui.zIndex +import org.libremail.R +import org.libremail.domain.model.Account +import kotlin.math.roundToInt + +/** + * The Settings accounts list with long-press drag-to-reorder (issue #164). Tapping a row still opens + * that account; a long-press lifts it and the drag follows the finger, and on release the new + * top-to-bottom order is persisted via [onReorder] (the account ids, in order). Every surface that + * lists accounts — this list, the drawer switcher, the unified-inbox filter chips — reads the same + * `ORDER BY sortOrder` query, so they all follow the committed order automatically. + * + * Built with `pointerInput` + `Modifier.offset` over a plain [Column] rather than a `LazyColumn` or a + * reorderable-list dependency: the account list is tiny (a handful of rows), it lives inside an + * already-scrolling settings column (a nested lazy list can't share that scroll), and the repo + * prefers no unnecessary dependencies. Only the dragged row moves during the gesture; the list + * commits to the dropped position on release, so no mid-drag recomposition can cancel the gesture. + */ +@Composable +internal fun AccountReorderList( + accounts: List, + onOpenAccount: (String) -> Unit, + onReorder: (List) -> Unit, +) { + // Local working order, re-seeded from upstream whenever the account set changes (add / remove / an + // external reorder) via the remember key. A drag never changes [accounts], so it stays put until + // the drop optimistically sets the new order — which upstream then echoes back identically. + var order by remember(accounts) { mutableStateOf(accounts) } + var draggingId by remember { mutableStateOf(null) } + var dragOffsetY by remember { mutableFloatStateOf(0f) } + var rowHeightPx by remember { mutableIntStateOf(0) } + + Column(Modifier.fillMaxWidth()) { + order.forEach { account -> + val dragging = account.id == draggingId + AccountReorderRow( + id = account.id, + email = account.email, + dragging = dragging, + offsetY = if (dragging) dragOffsetY.roundToInt() else 0, + onMeasured = { rowHeightPx = it }, + onOpen = { onOpenAccount(account.id) }, + onDragStart = { + draggingId = account.id + dragOffsetY = 0f + }, + onDrag = { delta -> dragOffsetY += delta }, + onDragEnd = { + commitDrag(order, account.id, dragOffsetY, rowHeightPx)?.let { moved -> + order = moved + onReorder(moved.map { it.id }) + } + draggingId = null + dragOffsetY = 0f + }, + ) + } + } +} + +/** + * The list [current] reordered by moving the account [id] the number of row-slots its accumulated + * [dragOffsetY] spans (at [rowHeightPx] each), or null when the drop leaves it in place. Pure so the + * index maths is unit-testable without a gesture. + */ +internal fun commitDrag(current: List, id: String, dragOffsetY: Float, rowHeightPx: Int): List? { + if (rowHeightPx <= 0) return null + val from = current.indexOfFirst { it.id == id } + if (from == -1) return null + val to = (from + (dragOffsetY / rowHeightPx).roundToInt()).coerceIn(0, current.lastIndex) + if (to == from) return null + return current.toMutableList().apply { add(to, removeAt(from)) } +} + +@Composable +private fun AccountReorderRow( + id: String, + email: String, + dragging: Boolean, + offsetY: Int, + onMeasured: (Int) -> Unit, + onOpen: () -> Unit, + onDragStart: () -> Unit, + onDrag: (Float) -> Unit, + onDragEnd: () -> Unit, +) { + val elevation by animateDpAsState(if (dragging) 6.dp else 0.dp, label = "accountDragElevation") + Surface( + color = if (dragging) MaterialTheme.colorScheme.surfaceVariant else MaterialTheme.colorScheme.surface, + tonalElevation = elevation, + shadowElevation = elevation, + modifier = Modifier + .fillMaxWidth() + .zIndex(if (dragging) 1f else 0f) + .offset { IntOffset(0, offsetY) } + .onSizeChanged { onMeasured(it.height) }, + ) { + Row( + modifier = Modifier + .fillMaxWidth() + .clickable(onClick = onOpen) + // Keyed on the account id so the gesture restarts only when this slot's account + // changes (an add/remove/re-seed) — never mid-drag, when the order is held stable. + .pointerInput(id) { + detectDragGesturesAfterLongPress( + onDragStart = { onDragStart() }, + onDrag = { change, dragAmount -> + change.consume() + onDrag(dragAmount.y) + }, + onDragEnd = onDragEnd, + onDragCancel = onDragEnd, + ) + } + .padding(horizontal = 16.dp, vertical = 16.dp), + verticalAlignment = Alignment.CenterVertically, + ) { + Text(text = email, style = MaterialTheme.typography.bodyLarge, modifier = Modifier.weight(1f)) + Icon( + imageVector = Icons.Filled.Menu, + contentDescription = stringResource(R.string.account_reorder_handle), + tint = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + } +} diff --git a/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt b/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt index 360acef..626a7c8 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/SettingsScreen.kt @@ -125,9 +125,11 @@ fun SettingsScreen( modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp), ) } else { - accounts.forEach { account -> - ClickRow(title = account.email, onClick = { onOpenAccount(account.id) }) - } + AccountReorderList( + accounts = accounts, + onOpenAccount = onOpenAccount, + onReorder = viewModel::reorderAccounts, + ) } ClickRow(title = stringResource(R.string.settings_add_account), onClick = onAddAccount) HorizontalDivider() diff --git a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt index 043bb80..7ac058b 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt @@ -64,6 +64,9 @@ class SettingsViewModel @Inject constructor( fun toggleAdvanced() = _advancedExpanded.update { !it } + /** Persist the account order the user produced by dragging in Settings (issue #164). */ + fun reorderAccounts(orderedIds: List) = update { accountRepository.reorderAccounts(orderedIds) } + /** Re-read the battery-optimization status; call when the screen resumes (e.g. back from Settings). */ fun refreshBatteryStatus() { _batteryUnrestricted.value = batteryOptimizationManager.isIgnoringBatteryOptimizations() diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index f3a647d..a2383cf 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -260,6 +260,7 @@ New mail notifications Show a notification when new mail arrives No accounts yet + Long-press and drag to reorder account Remove account diff --git a/app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListTest.kt b/app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListTest.kt new file mode 100644 index 0000000..e66aea5 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/ui/settings/AccountReorderListTest.kt @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.settings + +import org.junit.Test +import org.libremail.domain.model.Account +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** The pure reorder-index maths behind the Settings drag-to-reorder list (issue #164). */ +class AccountReorderListTest { + + private val accounts = listOf("a", "b", "c").map { Account.outlook("$it@example.org") } + private val ids get() = accounts.map { it.id } + + @Test + fun `dragging the first row two slots down moves it to the end`() { + val moved = commitDrag(accounts, id = ids[0], dragOffsetY = 200f, rowHeightPx = 100) + assertEquals(listOf(ids[1], ids[2], ids[0]), moved?.map { it.id }) + } + + @Test + fun `dragging the last row two slots up moves it to the front`() { + val moved = commitDrag(accounts, id = ids[2], dragOffsetY = -200f, rowHeightPx = 100) + assertEquals(listOf(ids[2], ids[0], ids[1]), moved?.map { it.id }) + } + + @Test + fun `an offset under half a row height keeps the position`() { + assertNull(commitDrag(accounts, id = ids[0], dragOffsetY = 40f, rowHeightPx = 100)) + } + + @Test + fun `a drop dragged past the ends is clamped to a valid index`() { + val moved = commitDrag(accounts, id = ids[0], dragOffsetY = 900f, rowHeightPx = 100) + assertEquals(listOf(ids[1], ids[2], ids[0]), moved?.map { it.id }) + } + + @Test + fun `an unmeasured row height cannot reorder`() { + assertNull(commitDrag(accounts, id = ids[0], dragOffsetY = 200f, rowHeightPx = 0)) + } + + @Test + fun `an unknown id cannot reorder`() { + assertNull(commitDrag(accounts, id = "missing", dragOffsetY = 200f, rowHeightPx = 100)) + } +} From 53f600369b10d63ade14b48379c3b65a9f1002a7 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 18:45:45 -0500 Subject: [PATCH 2/3] fix(test): stub the new insertAtEnd DAO call in AccountRepositoryImplTest The #164 reorder-accounts feature switched addImapAccount/addOutlookAccount from accountDao.upsert(...) to accountDao.insertAtEnd(...) (a @Transaction default method that stamps sortOrder before delegating to upsert), but the test's mocks/verifies still targeted upsert directly. Since MockK doesn't invoke a mocked interface's default method body, the unstubbed insertAtEnd call threw MockKException. Updated the stub/verify pairs in both add-account happy-path tests and the exactly-0 verifies in both failure-path tests to reference insertAtEnd. Co-Authored-By: Claude Opus 4.8 --- .../data/repository/AccountRepositoryImplTest.kt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt index 96b1b20..cbac6e1 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt @@ -105,7 +105,7 @@ class AccountRepositoryImplTest { coEvery { imapClient.listFolders(capture(used)) } returns listOf( FetchedFolder("INBOX", "INBOX", emptyList(), selectable = true), ) - coEvery { accountDao.upsert(any()) } just Runs + coEvery { accountDao.insertAtEnd(any()) } just Runs val result = repository.addImapAccount(account, "app-password") @@ -114,7 +114,7 @@ class AccountRepositoryImplTest { assertFalse(used.captured.useXoauth2) assertEquals("app-password", used.captured.secret) assertEquals("ada@example.org", used.captured.username) - coVerify { accountDao.upsert(any()) } + coVerify { accountDao.insertAtEnd(any()) } coVerify { accountSettingsRepository.ensureDefaults(account.id) } coVerify { credentialStore.saveSecret(account.id, "app-password") } verify { mailNotifier.ensureAccountChannel(account) } @@ -129,7 +129,7 @@ class AccountRepositoryImplTest { val result = repository.addImapAccount(account(), "wrong") assertTrue(result.isFailure) - coVerify(exactly = 0) { accountDao.upsert(any()) } + coVerify(exactly = 0) { accountDao.insertAtEnd(any()) } coVerify(exactly = 0) { credentialStore.saveSecret(any(), any()) } verify(exactly = 0) { syncScheduler.syncNow() } } @@ -141,7 +141,7 @@ class AccountRepositoryImplTest { FetchedFolder("INBOX", "INBOX", emptyList(), selectable = true), ) val saved = slot() - coEvery { accountDao.upsert(capture(saved)) } just Runs + coEvery { accountDao.insertAtEnd(capture(saved)) } just Runs val result = repository.addOutlookAccount("me@outlook.com", "access-token", "{authstate}") @@ -164,7 +164,7 @@ class AccountRepositoryImplTest { val result = repository.addOutlookAccount("me@outlook.com", "expired", "{authstate}") assertTrue(result.isFailure) - coVerify(exactly = 0) { accountDao.upsert(any()) } + coVerify(exactly = 0) { accountDao.insertAtEnd(any()) } coVerify(exactly = 0) { credentialStore.saveSecret(any(), any()) } } From 64d89d1e2ab38f17fceeb854a6d181398cfb612c Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 20:24:25 -0500 Subject: [PATCH 3/3] fix(accounts): break AccountDao sortOrder ties by email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AccountDaoTest (added by #270's coverage lane) asserts email ordering, but AccountDao.observeAll()/getAll() now order by the user-defined sortOrder (#164) with no tiebreaker. Two accounts inserted via plain upsert() both land on the default sortOrder (0), so they came back in rowid/insertion order instead — failing the test deterministically on CI (API 30 & 31): expected [ada, zed], got [zed, ada]. Add `email` as a secondary ORDER BY key. Real accounts always get distinct sortOrders via insertAtEnd()/reorder(), so drag order is untouched; only equal-sortOrder rows now fall back to a stable, deterministic email order. This also matches the "rank by email" convention already used to seed sortOrder in ACCOUNT_MIGRATION_1_2 and AccountDataMigrator.copyAccountTables, so the existing coverage test passes unchanged. No schema/migration change is needed since this only edits a @Query string, not the entity. Co-Authored-By: Claude Opus 4.8 --- .../kotlin/org/libremail/data/local/dao/AccountDao.kt | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt index af87c1b..dba0662 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt @@ -11,10 +11,17 @@ import org.libremail.data.local.entity.AccountEntity @Dao interface AccountDao { - @Query("SELECT * FROM accounts ORDER BY sortOrder") + /** + * Every listing surface (Settings, the drawer switcher, the unified-inbox filter chips) follows + * the user-defined [AccountEntity.sortOrder] (issue #164); `email` is only a tiebreaker for rows + * that share a sortOrder (e.g. inserted via plain [upsert] rather than [insertAtEnd]/[reorder]), + * so equal-sortOrder accounts still list in a stable, deterministic order. + */ + @Query("SELECT * FROM accounts ORDER BY sortOrder, email") fun observeAll(): Flow> - @Query("SELECT * FROM accounts ORDER BY sortOrder") + /** See [observeAll] — same ordering: sortOrder, then email as a tiebreaker. */ + @Query("SELECT * FROM accounts ORDER BY sortOrder, email") suspend fun getAll(): List @Query("SELECT * FROM accounts WHERE id = :id LIMIT 1")