Merge pull request #7853 from thunderbird/clean_up_SettingsFileParser_2

Refactor `SettingsImporter` [4/x]

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<String, Account>?,
+ val accounts: List<Account>,
  )
 
  data class Account(

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<String, Account>? = null
+ var accounts: List<Account>? = null
 
  val fileFormatVersion = readFileFormatVersion()
  if (fileFormatVersion != SettingsExporter.FILE_FORMAT_VERSION) {
@@ -99,7 +99,7 @@ private class XmlSettingsParser(
  }
  }
 
- return Contents(contentVersion, generalSettings, accounts)
+ return Contents(contentVersion, generalSettings, accounts.orEmpty())
  }
 
  private fun readFileFormatVersion(): Int {
@@ -144,8 +144,8 @@ private class XmlSettingsParser(
  return settings.takeIf { it.isNotEmpty() }
  }
 
- private fun readAccounts(): Map<String, Account>? {
- val accounts = mutableMapOf<String, Account>()
+ private fun readAccounts(): List<Account> {
+ val accounts = mutableListOf<Account>()
 
  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 -> {
@@ -168,7 +168,7 @@ private class XmlSettingsParser(
  }
  }
 
- return accounts.takeIf { it.isNotEmpty() }
+ return accounts
  }
 
  private fun readAccount(): Account? {

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,
@@ -88,7 +88,7 @@ class SettingsImporter internal constructor(
  fun importSettings(
  inputStream: InputStream,
  globalSettings: Boolean,
- accountUuids: List<String>?,
+ accountUuids: List<String>,
  ): 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.filter { it.uuid in accountUuids }
 
  val imported = settings.copy(
  globalSettings = filteredGlobalSettings,
@@ -136,75 +132,71 @@ class SettingsImporter internal constructor(
  }
  }
 
- if (!accountUuids.isNullOrEmpty()) {
- 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)
- } else {
- Timber.w(
- "Error while committing settings for account \"%s\" to the settings database.",
- importResult.original.name,
- )
-
- erroneousAccounts.add(importResult.original)
- }
- } 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))
+ if (accountUuids.isNotEmpty()) {
+ val foundAccountUuids = imported.accounts.map { it.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)
+ }
+ }
+
+ for (account in imported.accounts) {
+ 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)
  } else {
  Timber.w(
- "Was asked to import account with UUID %s. But this account wasn't found.",
- accountUuid,
+ "Error while committing settings for account \"%s\" to the settings database.",
+ importResult.original.name,
  )
+
+ erroneousAccounts.add(importResult.original)
  }
- }
+ } catch (e: InvalidSettingValueException) {
+ Timber.e(e, "Encountered invalid setting while importing account \"%s\"", account.name)
 
- val editor = preferences.createStorageEditor()
+ erroneousAccounts.add(AccountDescription(account.name!!, account.uuid))
+ } catch (e: Exception) {
+ Timber.e(e, "Exception while importing account \"%s\"", account.name)
 
- if (!editor.commit()) {
- throw SettingsImportExportException("Failed to set default account")
+ erroneousAccounts.add(AccountDescription(account.name!!, account.uuid))
  }
- } else {
- Timber.w("Was asked to import at least one account but none found.")
+ }
+
+ val editor = preferences.createStorageEditor()
+
+ if (!editor.commit()) {
+ throw SettingsImportExportException("Failed to set default account")
  }
  }
 

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("[email protected]")
@@ -97,7 +97,7 @@ class SettingsFileParserTest : RobolectricTest() {
 
  assertThat(results.accounts)
  .isNotNull()
- .key(accountUuid)
+ .index(0)
  .prop(Account::incoming)
  .isNotNull()
  .prop(Server::authenticationType)