Refactor loading/saving settings

Author: TheOneRing

src/gui/accountmanager.cpp

@@ -94,91 +94,87 @@ AccountManager *AccountManager::create(QQmlEngine *qmlEngine, QJSEngine *)
 
 bool AccountManager::restore()
 {
-    auto settings = ConfigFile::settingsWithGroup(accountsC());
-    if (settings->status() != QSettings::NoError) {
-        qCWarning(lcAccountManager) << "Could not read settings from" << settings->fileName()
-                                    << settings->status();
+    auto settings = ConfigFile::makeQSettings();
+    if (settings.status() != QSettings::NoError) {
+        qCWarning(lcAccountManager) << "Could not read settings from" << settings.fileName() << settings.status();
         return false;
     }
 
-    // If there are no accounts, check the old format.
-    const auto &childGroups = settings->childGroups();
-
-    for (const auto &accountId : childGroups) {
-        settings->beginGroup(accountId);
-        if (auto acc = loadAccountHelper(*settings)) {
-            acc->_id = accountId;
-            if (auto accState = AccountState::loadFromSettings(acc, *settings)) {
+    const auto size = settings.beginReadArray(accountsC());
+    for (auto i = 0; i < size; ++i) {
+        settings.setArrayIndex(i);
+        if (auto acc = loadAccountHelper(settings)) {
+            if (auto accState = AccountState::loadFromSettings(acc, settings)) {
                 addAccountState(std::move(accState));
             }
         }
-        settings->endGroup();
     }
+    settings.endArray();
 
     return true;
 }
 
 void AccountManager::save(bool saveCredentials)
 {
-    for (const auto &acc : std::as_const(_accounts)) {
-        saveAccount(acc->account().data(), saveCredentials);
-    }
+    auto settings = ConfigFile::makeQSettings();
+    settings.remove(accountsC());
+    settings.beginWriteArray(accountsC(), _accounts.size());
 
-    qCInfo(lcAccountManager) << "Saved all account settings";
-}
+    int i = 0;
+    for (const auto &accountState : std::as_const(_accounts)) {
+        settings.setArrayIndex(i++);
+        auto account = accountState->account();
+        qCDebug(lcAccountManager) << "Saving account" << account->url().toString();
+        settings.setValue(urlC(), account->_url.toString());
+        settings.setValue(davUserDisplyNameC(), account->_displayName);
+        settings.setValue(userUUIDC(), account->uuid());
+        if (account->hasCapabilities()) {
+            settings.setValue(capabilitesC(), account->capabilities().raw());
+        }
+        if (account->hasDefaultSyncRoot()) {
+            settings.setValue(defaultSyncRootC(), account->defaultSyncRoot());
+        }
+        if (account->_credentials) {
+            if (saveCredentials) {
+                // Only persist the credentials if the parameter is set, on migration from 1.8.x
+                // we want to save the accounts but not overwrite the credentials
+                // (This is easier than asynchronously fetching the credentials from keychain and then
+                // re-persisting them)
+                account->_credentials->persist();
+            }
 
-void AccountManager::saveAccount(Account *account, bool saveCredentials)
-{
-    qCDebug(lcAccountManager) << "Saving account" << account->url().toString();
-    auto settings = ConfigFile::settingsWithGroup(accountsC());
-    settings->beginGroup(account->id());
-
-    settings->setValue(urlC(), account->_url.toString());
-    settings->setValue(davUserDisplyNameC(), account->_displayName);
-    settings->setValue(userUUIDC(), account->uuid());
-    if (account->hasCapabilities()) {
-        settings->setValue(capabilitesC(), account->capabilities().raw());
-    }
-    if (account->hasDefaultSyncRoot()) {
-        settings->setValue(defaultSyncRootC(), account->defaultSyncRoot());
-    }
-    if (account->_credentials) {
-        if (saveCredentials) {
-            // Only persist the credentials if the parameter is set, on migration from 1.8.x
-            // we want to save the accounts but not overwrite the credentials
-            // (This is easier than asynchronously fetching the credentials from keychain and then
-            // re-persisting them)
-            account->_credentials->persist();
+            for (auto it = account->_settingsMap.constBegin(); it != account->_settingsMap.constEnd(); ++it) {
+                settings.setValue(it.key(), it.value());
+            }
+
+            // HACK: Save http_user also as user
+            if (account->_settingsMap.contains(httpUserC()))
+                settings.setValue(userC(), account->_settingsMap.value(httpUserC()));
         }
 
-        for (auto it = account->_settingsMap.constBegin(); it != account->_settingsMap.constEnd(); ++it) {
-            settings->setValue(it.key(), it.value());
+        // Save accepted certificates.
+        settings.beginGroup("General");
+        qCInfo(lcAccountManager) << "Saving " << account->approvedCerts().count() << " unknown certs.";
+        const auto approvedCerts = account->approvedCerts();
+        QByteArray certs;
+        for (const auto &cert : approvedCerts) {
+            certs += cert.toPem() + '\n';
         }
+        if (!certs.isEmpty()) {
+            settings.setValue(caCertsKeyC(), certs);
+        }
+        settings.endGroup();
 
-        // HACK: Save http_user also as user
-        if (account->_settingsMap.contains(httpUserC()))
-            settings->setValue(userC(), account->_settingsMap.value(httpUserC()));
-    }
+        // save the account state
+        this->account(account->uuid())->writeToSettings(settings);
+        settings.endGroup();
 
-    // Save accepted certificates.
-    settings->beginGroup(QStringLiteral("General"));
-    qCInfo(lcAccountManager) << "Saving " << account->approvedCerts().count() << " unknown certs.";
-    const auto approvedCerts = account->approvedCerts();
-    QByteArray certs;
-    for (const auto &cert : approvedCerts) {
-        certs += cert.toPem() + '\n';
+        settings.sync();
+        qCDebug(lcAccountManager) << "Saved account settings, status:" << settings.status();
     }
-    if (!certs.isEmpty()) {
-        settings->setValue(caCertsKeyC(), certs);
-    }
-    settings->endGroup();
-
-    // save the account state
-    this->account(account->uuid())->writeToSettings(*settings);
-    settings->endGroup();
+    settings.endArray();
 
-    settings->sync();
-    qCDebug(lcAccountManager) << "Saved account settings, status:" << settings->status();
+    qCInfo(lcAccountManager) << "Saved all account settings";
 }
 
 QStringList AccountManager::accountNames() const
@@ -240,28 +236,13 @@ AccountPtr AccountManager::loadAccountHelper(QSettings &settings)
     return acc;
 }
 
-AccountStatePtr AccountManager::account(const QString &name)
+AccountStatePtr AccountManager::account(const QUuid uuid)
 {
-    for (const auto &acc : std::as_const(_accounts)) {
-        if (acc->account()->displayNameWithHost() == name) {
-            return acc;
-        }
-    }
-    return AccountStatePtr();
-}
-
-AccountStatePtr AccountManager::account(const QUuid uuid) {
     return _accounts.value(uuid);
 }
 
 AccountStatePtr AccountManager::addAccount(const AccountPtr &newAccount)
 {
-    auto id = newAccount->id();
-    if (id.isEmpty() || !isAccountIdAvailable(id)) {
-        id = generateFreeAccountId();
-    }
-    newAccount->_id = id;
-
     return addAccountState(AccountState::fromNewAccount(newAccount));
 }
 
@@ -283,12 +264,10 @@ void AccountManager::deleteAccount(AccountStatePtr account)
     account->account()->credentials()->forgetSensitiveData();
     account->account()->credentialManager()->clear();
 
-    auto settings = ConfigFile::settingsWithGroup(accountsC());
-    settings->remove(account->account()->id());
-
     Q_EMIT accountRemoved(account);
     Q_EMIT accountsChanged();
     account->deleteLater();
+    save(false);
 }
 
 AccountPtr AccountManager::createAccount(const QUuid &uuid)
@@ -305,36 +284,12 @@ void AccountManager::shutdown()
     }
 }
 
-bool AccountManager::isAccountIdAvailable(const QString &id) const
-{
-    for (const auto &acc : _accounts) {
-        if (acc->account()->id() == id) {
-            return false;
-        }
-    }
-    if (_additionalBlockedAccountIds.contains(id))
-        return false;
-    return true;
-}
-
-QString AccountManager::generateFreeAccountId() const
-{
-    int i = 0;
-    while (true) {
-        QString id = QString::number(i);
-        if (isAccountIdAvailable(id)) {
-            return id;
-        }
-        ++i;
-    }
-}
-
 AccountStatePtr AccountManager::addAccountState(std::unique_ptr<AccountState> &&accountState)
 {
     auto *rawAccount = accountState->account().data();
-    connect(rawAccount, &Account::wantsAccountSaved, this, [rawAccount, this] {
+    connect(rawAccount, &Account::wantsAccountSaved, this, [this] {
         // persis the account, not the credentials, we don't know whether they are ready yet
-        saveAccount(rawAccount, false);
+        save(false);
     });
 
     AccountStatePtr statePtr = accountState.release();

src/gui/accountmanager.h

@@ -71,11 +71,6 @@ class OPENCLOUD_GUI_EXPORT AccountManager : public QObject
      */
     const QList<AccountStatePtr> accounts() { return _accounts.values(); }
 
-    /**
-     * Return the account state pointer for an account identified by its display name
-     */
-    Q_DECL_DEPRECATED_X("Please use the uuid to specify the account") AccountStatePtr account(const QString &name);
-
     /**
      * Return the account state pointer for an account identified by its display name
      */
@@ -106,23 +101,17 @@ class OPENCLOUD_GUI_EXPORT AccountManager : public QObject
     void saveAccountHelper(Account *account, QSettings &settings, bool saveCredentials = true);
     AccountPtr loadAccountHelper(QSettings &settings);
 
-    bool isAccountIdAvailable(const QString &id) const;
-    QString generateFreeAccountId() const;
-
     // Adds an account to the tracked list, emitting accountAdded()
     AccountStatePtr addAccountState(std::unique_ptr<AccountState> &&accountState);
 
-public Q_SLOTS:
-    /// Saves account data, not including the credentials
-    void saveAccount(Account *account, bool saveCredentials);
-
 Q_SIGNALS:
     void accountAdded(AccountStatePtr account);
     void accountRemoved(AccountStatePtr account);
     void accountsChanged();
 
 private:
     AccountManager() {}
+
     QMap<QUuid, AccountStatePtr> _accounts;
     /// Account ids from settings that weren't read
     QSet<QString> _additionalBlockedAccountIds;

src/gui/accountstate.cpp

@@ -583,13 +583,6 @@ Account *AccountState::accountForQml() const
     return _account.data();
 }
 
-std::unique_ptr<QSettings> AccountState::settings()
-{
-    auto s = ConfigFile::settingsWithGroup(QStringLiteral("Accounts"));
-    s->beginGroup(_account->id());
-    return s;
-}
-
 bool AccountState::isSettingUp() const
 {
     return _settingUp;

src/gui/accountstate.h

@@ -129,9 +129,6 @@ class OPENCLOUD_GUI_EXPORT AccountState : public QObject
 
     bool isConnected() const;
 
-    /** Returns a new settings object for this account, already in the right groups. */
-    std::unique_ptr<QSettings> settings();
-
     /** Mark the timestamp when the last successful ETag check happened for
      *  this account.
      *  The checkConnectivity() method uses the timestamp to save a call to