From d4e4c55f4cde5cdd7d9607365ad7a6ca1ec21e25 Mon Sep 17 00:00:00 2001 From: cketti Date: Tue, 7 May 2024 18:20:46 +0200 Subject: [PATCH 1/4] Change nullability of `SettingsImporter.importSettings()` parameter We never use an `accountUuids` value of `null` anywhere in the code. --- .../java/com/fsck/k9/preferences/SettingsImporter.kt | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt index 9081d830db9..38fa5b5efc2 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt @@ -88,7 +88,7 @@ class SettingsImporter internal constructor( fun importSettings( inputStream: InputStream, globalSettings: Boolean, - accountUuids: List?, + accountUuids: List, ): ImportResults { try { var globalSettingsImported = false @@ -103,11 +103,7 @@ class SettingsImporter internal constructor( null } - val filteredAccounts = if (accountUuids == null) { - settings.accounts - } else { - settings.accounts?.filterKeys { it in accountUuids } - } + val filteredAccounts = settings.accounts?.filterKeys { it in accountUuids } val imported = settings.copy( globalSettings = filteredGlobalSettings, @@ -136,7 +132,7 @@ class SettingsImporter internal constructor( } } - if (!accountUuids.isNullOrEmpty()) { + if (accountUuids.isNotEmpty()) { if (imported.accounts != null) { for (accountUuid in accountUuids) { val account = imported.accounts[accountUuid] From ebbdb046ee3c261c95283ba91682cc4c5154ab73 Mon Sep 17 00:00:00 2001 From: cketti Date: Tue, 7 May 2024 18:31:25 +0200 Subject: [PATCH 2/4] Change `SettingsFile.Contents.accounts` to be non-nullable --- .../com/fsck/k9/preferences/SettingsFile.kt | 2 +- .../fsck/k9/preferences/SettingsFileParser.kt | 6 +- .../fsck/k9/preferences/SettingsImporter.kt | 114 +++++++++--------- 3 files changed, 59 insertions(+), 63 deletions(-) diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt index 91c51265357..1cedc2e7f67 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt @@ -8,7 +8,7 @@ internal interface SettingsFile { data class Contents( val contentVersion: Int, val globalSettings: SettingsMap?, - val accounts: Map?, + val accounts: Map, ) data class Account( diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt index cb9aa260c94..216bd4fe814 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt @@ -99,7 +99,7 @@ private class XmlSettingsParser( } } - return Contents(contentVersion, generalSettings, accounts) + return Contents(contentVersion, generalSettings, accounts.orEmpty()) } private fun readFileFormatVersion(): Int { @@ -144,7 +144,7 @@ private class XmlSettingsParser( return settings.takeIf { it.isNotEmpty() } } - private fun readAccounts(): Map? { + private fun readAccounts(): Map { val accounts = mutableMapOf() readElement { eventType -> @@ -168,7 +168,7 @@ private class XmlSettingsParser( } } - return accounts.takeIf { it.isNotEmpty() } + return accounts } private fun readAccount(): Account? { diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt index 38fa5b5efc2..bc12ca0ef9f 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt @@ -103,7 +103,7 @@ class SettingsImporter internal constructor( null } - val filteredAccounts = settings.accounts?.filterKeys { it in accountUuids } + val filteredAccounts = settings.accounts.filterKeys { it in accountUuids } val imported = settings.copy( globalSettings = filteredGlobalSettings, @@ -133,74 +133,70 @@ class SettingsImporter internal constructor( } if (accountUuids.isNotEmpty()) { - if (imported.accounts != null) { - for (accountUuid in accountUuids) { - val account = imported.accounts[accountUuid] - if (account != null) { - try { - var editor = preferences.createStorageEditor() - - val importResult = importAccount(editor, imported.contentVersion, account) - - if (editor.commit()) { - Timber.v( - "Committed settings for account \"%s\" to the settings database.", - importResult.imported.name, - ) - - // Add UUID of the account we just imported to the list of account UUIDs - editor = preferences.createStorageEditor() - - val newUuid = importResult.imported.uuid - val oldAccountUuids = preferences.storage.getString("accountUuids", "") - val newAccountUuids = if (oldAccountUuids.isNotEmpty()) { - "$oldAccountUuids,$newUuid" - } else { - newUuid - } - - putString(editor, "accountUuids", newAccountUuids) - - if (!editor.commit()) { - throw SettingsImportExportException("Failed to set account UUID list") - } - - // Reload accounts - preferences.loadAccounts() - - importedAccounts.add(importResult) + for (accountUuid in accountUuids) { + val account = imported.accounts[accountUuid] + if (account != null) { + try { + var editor = preferences.createStorageEditor() + + val importResult = importAccount(editor, imported.contentVersion, account) + + if (editor.commit()) { + Timber.v( + "Committed settings for account \"%s\" to the settings database.", + importResult.imported.name, + ) + + // Add UUID of the account we just imported to the list of account UUIDs + editor = preferences.createStorageEditor() + + val newUuid = importResult.imported.uuid + val oldAccountUuids = preferences.storage.getString("accountUuids", "") + val newAccountUuids = if (oldAccountUuids.isNotEmpty()) { + "$oldAccountUuids,$newUuid" } else { - Timber.w( - "Error while committing settings for account \"%s\" to the settings database.", - importResult.original.name, - ) + newUuid + } + + putString(editor, "accountUuids", newAccountUuids) - erroneousAccounts.add(importResult.original) + if (!editor.commit()) { + throw SettingsImportExportException("Failed to set account UUID list") } - } catch (e: InvalidSettingValueException) { - Timber.e(e, "Encountered invalid setting while importing account \"%s\"", account.name) - erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) - } catch (e: Exception) { - Timber.e(e, "Exception while importing account \"%s\"", account.name) + // Reload accounts + preferences.loadAccounts() + + importedAccounts.add(importResult) + } else { + Timber.w( + "Error while committing settings for account \"%s\" to the settings database.", + importResult.original.name, + ) - erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) + erroneousAccounts.add(importResult.original) } - } else { - Timber.w( - "Was asked to import account with UUID %s. But this account wasn't found.", - accountUuid, - ) + } catch (e: InvalidSettingValueException) { + Timber.e(e, "Encountered invalid setting while importing account \"%s\"", account.name) + + erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) + } catch (e: Exception) { + Timber.e(e, "Exception while importing account \"%s\"", account.name) + + erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) } + } else { + Timber.w( + "Was asked to import account with UUID %s. But this account wasn't found.", + accountUuid, + ) } + } - val editor = preferences.createStorageEditor() + val editor = preferences.createStorageEditor() - if (!editor.commit()) { - throw SettingsImportExportException("Failed to set default account") - } - } else { - Timber.w("Was asked to import at least one account but none found.") + if (!editor.commit()) { + throw SettingsImportExportException("Failed to set default account") } } From 7d5d607e8d6d2ed8ded2a10d2d0829aa9f26cf30 Mon Sep 17 00:00:00 2001 From: cketti Date: Tue, 7 May 2024 20:01:59 +0200 Subject: [PATCH 3/4] Change the way `SettingsImporter` iterates over accounts --- .../fsck/k9/preferences/SettingsImporter.kt | 92 +++++++++---------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt index bc12ca0ef9f..ba9a09dfac0 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt @@ -133,63 +133,63 @@ class SettingsImporter internal constructor( } if (accountUuids.isNotEmpty()) { - for (accountUuid in accountUuids) { - val account = imported.accounts[accountUuid] - if (account != null) { - try { - var editor = preferences.createStorageEditor() - - val importResult = importAccount(editor, imported.contentVersion, account) - - if (editor.commit()) { - Timber.v( - "Committed settings for account \"%s\" to the settings database.", - importResult.imported.name, - ) - - // Add UUID of the account we just imported to the list of account UUIDs - editor = preferences.createStorageEditor() + val foundAccountUuids = imported.accounts.map { it.value.uuid }.toSet() + val missingAccountUuids = accountUuids.toSet() - foundAccountUuids + if (missingAccountUuids.isNotEmpty()) { + for (accountUuid in missingAccountUuids) { + Timber.w("Was asked to import account %s. But this account wasn't found.", accountUuid) + } + } - val newUuid = importResult.imported.uuid - val oldAccountUuids = preferences.storage.getString("accountUuids", "") - val newAccountUuids = if (oldAccountUuids.isNotEmpty()) { - "$oldAccountUuids,$newUuid" - } else { - newUuid - } + for (account in imported.accounts.values) { + try { + var editor = preferences.createStorageEditor() - putString(editor, "accountUuids", newAccountUuids) + val importResult = importAccount(editor, imported.contentVersion, account) - if (!editor.commit()) { - throw SettingsImportExportException("Failed to set account UUID list") - } + if (editor.commit()) { + Timber.v( + "Committed settings for account \"%s\" to the settings database.", + importResult.imported.name, + ) - // Reload accounts - preferences.loadAccounts() + // Add UUID of the account we just imported to the list of account UUIDs + editor = preferences.createStorageEditor() - importedAccounts.add(importResult) + val newUuid = importResult.imported.uuid + val oldAccountUuids = preferences.storage.getString("accountUuids", "") + val newAccountUuids = if (oldAccountUuids.isNotEmpty()) { + "$oldAccountUuids,$newUuid" } else { - Timber.w( - "Error while committing settings for account \"%s\" to the settings database.", - importResult.original.name, - ) + newUuid + } - erroneousAccounts.add(importResult.original) + putString(editor, "accountUuids", newAccountUuids) + + if (!editor.commit()) { + throw SettingsImportExportException("Failed to set account UUID list") } - } catch (e: InvalidSettingValueException) { - Timber.e(e, "Encountered invalid setting while importing account \"%s\"", account.name) - erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) - } catch (e: Exception) { - Timber.e(e, "Exception while importing account \"%s\"", account.name) + // Reload accounts + preferences.loadAccounts() + + importedAccounts.add(importResult) + } else { + Timber.w( + "Error while committing settings for account \"%s\" to the settings database.", + importResult.original.name, + ) - erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) + erroneousAccounts.add(importResult.original) } - } else { - Timber.w( - "Was asked to import account with UUID %s. But this account wasn't found.", - accountUuid, - ) + } catch (e: InvalidSettingValueException) { + Timber.e(e, "Encountered invalid setting while importing account \"%s\"", account.name) + + erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) + } catch (e: Exception) { + Timber.e(e, "Exception while importing account \"%s\"", account.name) + + erroneousAccounts.add(AccountDescription(account.name!!, account.uuid)) } } From a4a10cebf8cab9a0b5dcf28dae30566745b377b1 Mon Sep 17 00:00:00 2001 From: cketti Date: Tue, 7 May 2024 20:42:32 +0200 Subject: [PATCH 4/4] Change `SettingsFile.Contents.accounts` to a list --- .../java/com/fsck/k9/preferences/SettingsFile.kt | 2 +- .../com/fsck/k9/preferences/SettingsFileParser.kt | 12 ++++++------ .../java/com/fsck/k9/preferences/SettingsImporter.kt | 8 ++++---- .../fsck/k9/preferences/SettingsFileParserTest.kt | 8 ++++---- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt index 1cedc2e7f67..7c5e29b5cbe 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFile.kt @@ -8,7 +8,7 @@ internal interface SettingsFile { data class Contents( val contentVersion: Int, val globalSettings: SettingsMap?, - val accounts: Map, + val accounts: List, ) data class Account( diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt index 216bd4fe814..982ee3d2194 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsFileParser.kt @@ -64,7 +64,7 @@ private class XmlSettingsParser( private fun readRoot(): Contents { var generalSettings: SettingsMap? = null - var accounts: Map? = null + var accounts: List? = null val fileFormatVersion = readFileFormatVersion() if (fileFormatVersion != SettingsExporter.FILE_FORMAT_VERSION) { @@ -144,8 +144,8 @@ private class XmlSettingsParser( return settings.takeIf { it.isNotEmpty() } } - private fun readAccounts(): Map { - val accounts = mutableMapOf() + private fun readAccounts(): List { + val accounts = mutableListOf() readElement { eventType -> if (eventType == XmlPullParser.START_TAG) { @@ -155,10 +155,10 @@ private class XmlSettingsParser( if (account == null) { // Do nothing - readAccount() already logged a message - } else if (!accounts.containsKey(account.uuid)) { - accounts[account.uuid] = account - } else { + } else if (accounts.any { it.uuid == account.uuid }) { Timber.w("Duplicate account entries with UUID %s. Ignoring!", account.uuid) + } else { + accounts.add(account) } } else -> { diff --git a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt index ba9a09dfac0..d98505efd7f 100644 --- a/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt +++ b/app/core/src/main/java/com/fsck/k9/preferences/SettingsImporter.kt @@ -56,7 +56,7 @@ class SettingsImporter internal constructor( // If the stream contains global settings the "globalSettings" member will not be null val globalSettings = (imported.globalSettings != null) - val accounts = imported.accounts?.values.orEmpty().map { importedAccount -> + val accounts = imported.accounts.map { importedAccount -> AccountDescription( name = getAccountDisplayName(importedAccount), uuid = importedAccount.uuid, @@ -103,7 +103,7 @@ class SettingsImporter internal constructor( null } - val filteredAccounts = settings.accounts.filterKeys { it in accountUuids } + val filteredAccounts = settings.accounts.filter { it.uuid in accountUuids } val imported = settings.copy( globalSettings = filteredGlobalSettings, @@ -133,7 +133,7 @@ class SettingsImporter internal constructor( } if (accountUuids.isNotEmpty()) { - val foundAccountUuids = imported.accounts.map { it.value.uuid }.toSet() + val foundAccountUuids = imported.accounts.map { it.uuid }.toSet() val missingAccountUuids = accountUuids.toSet() - foundAccountUuids if (missingAccountUuids.isNotEmpty()) { for (accountUuid in missingAccountUuids) { @@ -141,7 +141,7 @@ class SettingsImporter internal constructor( } } - for (account in imported.accounts.values) { + for (account in imported.accounts) { try { var editor = preferences.createStorageEditor() diff --git a/app/core/src/test/java/com/fsck/k9/preferences/SettingsFileParserTest.kt b/app/core/src/test/java/com/fsck/k9/preferences/SettingsFileParserTest.kt index 688651e94ec..01a3aa9c098 100644 --- a/app/core/src/test/java/com/fsck/k9/preferences/SettingsFileParserTest.kt +++ b/app/core/src/test/java/com/fsck/k9/preferences/SettingsFileParserTest.kt @@ -6,9 +6,9 @@ import assertk.assertThat import assertk.assertions.containsExactly import assertk.assertions.extracting import assertk.assertions.hasSize +import assertk.assertions.index import assertk.assertions.isEqualTo import assertk.assertions.isNotNull -import assertk.assertions.key import assertk.assertions.prop import com.fsck.k9.mail.AuthType import com.fsck.k9.preferences.SettingsFile.Account @@ -38,7 +38,7 @@ class SettingsFileParserTest : RobolectricTest() { assertThat(results.accounts).isNotNull().all { hasSize(1) - key(accountUuid).all { + index(0).all { prop(Account::uuid).isEqualTo(accountUuid) prop(Account::name).isEqualTo("Account") } @@ -68,7 +68,7 @@ class SettingsFileParserTest : RobolectricTest() { assertThat(results.accounts).isNotNull().all { hasSize(1) - key(accountUuid).all { + index(0).all { prop(Account::uuid).isEqualTo(accountUuid) prop(Account::identities).isNotNull() .extracting(Identity::email).containsExactly("user@gmail.com") @@ -97,7 +97,7 @@ class SettingsFileParserTest : RobolectricTest() { assertThat(results.accounts) .isNotNull() - .key(accountUuid) + .index(0) .prop(Account::incoming) .isNotNull() .prop(Server::authenticationType)