[Settings] Store server profiles with QSettings arrays (#7629)

The server list used hand-numbered saveName%1/server%1 keys and an O(n)
shift loop to delete entries. Store the profiles with beginReadArray and
beginWriteArray instead, and migrate the legacy keys once in the
constructor so existing profiles and saved passwords survive.

SettingsManager::batchWrite no longer disables atomic sync, so a batch is
still written through QSettings' atomic save. Add tests for add, update,
remove, password updates and the legacy migration.

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/interface/intents/intent_login.cpp

@@ -27,9 +27,9 @@ void IntentGetLoginCredentials::onPreconditionSatisfied()
     ServersSettings &servers = SettingsCache::instance().servers();
     const int index = servers.findServerIndex(context->hostname, context->port);
 
-    if (index >= 0) {
-        context->username = servers.getValue(QString("username%1").arg(index), "server", "server_details").toString();
-        context->password = servers.getValue(QString("password%1").arg(index), "server", "server_details").toString();
+    if (const auto server = servers.getServer(index)) {
+        context->username = server->username;
+        context->password = server->password;
         emitFinished();
     } else {
         emitFailed(tr("No saved credentials for this server"));
@@ -69,9 +69,8 @@ void IntentGetLoginCredentials::onPreconditionNotSatisfied()
         // it with the raw hostname when addNewServer updates the entry in place.
         QString saveName = context->hostname;
         const int existingIndex = servers.findServerIndex(context->hostname, context->port);
-        if (existingIndex >= 0) {
-            saveName =
-                servers.getValue(QString("saveName%1").arg(existingIndex), "server", "server_details").toString();
+        if (const auto existing = servers.getServer(existingIndex)) {
+            saveName = existing->saveName;
             if (saveName.isEmpty()) {
                 saveName = context->hostname;
             }

cockatrice/src/interface/intents/url_parser.cpp

@@ -418,12 +418,10 @@ void IntentUrlParser::restoreToPreviousServer(const PendingIntentChain &chain)
 
         ServersSettings &servers = SettingsCache::instance().servers();
         const int index = servers.findServerIndex(chain.previousServerHost, chain.previousServerPort);
-        if (index >= 0 && servers.hasLoginData(chain.previousServerHost, chain.previousServerPort)) {
-            const QString username =
-                servers.getValue(QString("username%1").arg(index), "server", "server_details").toString();
-            const QString password =
-                servers.getValue(QString("password%1").arg(index), "server", "server_details").toString();
-            client->connectToServer(chain.previousServerHost, chain.previousServerPort.toUInt(), username, password);
+        const auto server = servers.getServer(index);
+        if (server && servers.hasLoginData(chain.previousServerHost, chain.previousServerPort)) {
+            client->connectToServer(chain.previousServerHost, chain.previousServerPort.toUInt(), server->username,
+                                    server->password);
             return;
         }
         client->disconnectFromServer();
@@ -438,11 +436,9 @@ void IntentUrlParser::restoreToPreviousServer(const PendingIntentChain &chain)
     // when credentials are saved, otherwise stay offline.
     ServersSettings &servers = SettingsCache::instance().servers();
     const int index = servers.findServerIndex(chain.previousServerHost, chain.previousServerPort);
-    if (index >= 0 && servers.hasLoginData(chain.previousServerHost, chain.previousServerPort)) {
-        const QString username =
-            servers.getValue(QString("username%1").arg(index), "server", "server_details").toString();
-        const QString password =
-            servers.getValue(QString("password%1").arg(index), "server", "server_details").toString();
-        client->connectToServer(chain.previousServerHost, chain.previousServerPort.toUInt(), username, password);
+    const auto server = servers.getServer(index);
+    if (server && servers.hasLoginData(chain.previousServerHost, chain.previousServerPort)) {
+        client->connectToServer(chain.previousServerHost, chain.previousServerPort.toUInt(), server->username,
+                                server->password);
     }
 }

cockatrice/src/interface/widgets/server/user/user_info_connection.cpp

@@ -29,19 +29,10 @@ QMap<QString, std::pair<QString, UserConnection_Information>> UserConnection_Inf
 
     ServersSettings &servers = SettingsCache::instance().servers();
 
-    int size = servers.getValue("totalServers", "server", "server_details").toInt() + 1;
-
-    for (int i = 0; i < size; i++) {
-        QString _saveName = servers.getValue(QString("saveName%1").arg(i), "server", "server_details").toString();
-        QString serverName = servers.getValue(QString("server%1").arg(i), "server", "server_details").toString();
-        QString portNum = servers.getValue(QString("port%1").arg(i), "server", "server_details").toString();
-        QString userName = servers.getValue(QString("username%1").arg(i), "server", "server_details").toString();
-        QString pass = servers.getValue(QString("password%1").arg(i), "server", "server_details").toString();
-        bool savePass = servers.getValue(QString("savePassword%1").arg(i), "server", "server_details").toBool();
-        QString _site = servers.getValue(QString("site%1").arg(i), "server", "server_details").toString();
-
-        UserConnection_Information userInfo(_saveName, serverName, portNum, userName, pass, savePass, _site);
-        serverList.insert(_saveName, std::make_pair(serverName, userInfo));
+    for (const auto &entry : servers.getServers()) {
+        UserConnection_Information userInfo(entry.saveName, entry.host, entry.port, entry.username, entry.password,
+                                            entry.savePassword, entry.site);
+        serverList.insert(entry.saveName, std::make_pair(entry.host, userInfo));
     }
 
     return serverList;
@@ -53,28 +44,18 @@ QStringList UserConnection_Information::getServerInfo(const QString &find)
 
     ServersSettings &servers = SettingsCache::instance().servers();
 
-    int size = servers.getValue("totalServers", "server", "server_details").toInt() + 1;
-    for (int i = 0; i < size; i++) {
-        QString _saveName = servers.getValue(QString("saveName%1").arg(i), "server", "server_details").toString();
-
-        if (find != _saveName) {
+    for (const auto &entry : servers.getServers()) {
+        if (find != entry.saveName) {
             continue;
         }
 
-        QString serverName = servers.getValue(QString("server%1").arg(i), "server", "server_details").toString();
-        QString portNum = servers.getValue(QString("port%1").arg(i), "server", "server_details").toString();
-        QString userName = servers.getValue(QString("username%1").arg(i), "server", "server_details").toString();
-        QString pass = servers.getValue(QString("password%1").arg(i), "server", "server_details").toString();
-        bool savePass = servers.getValue(QString("savePassword%1").arg(i), "server", "server_details").toBool();
-        QString _site = servers.getValue(QString("site%1").arg(i), "server", "server_details").toString();
-
-        _server.append(_saveName);
-        _server.append(serverName);
-        _server.append(portNum);
-        _server.append(userName);
-        _server.append(pass);
-        _server.append(savePass ? "1" : "0");
-        _server.append(_site);
+        _server.append(entry.saveName);
+        _server.append(entry.host);
+        _server.append(entry.port);
+        _server.append(entry.username);
+        _server.append(entry.password);
+        _server.append(entry.savePassword ? "1" : "0");
+        _server.append(entry.site);
         break;
     }
 

libcockatrice_settings/libcockatrice/settings/servers_settings.cpp

@@ -5,16 +5,118 @@
 #include <QDebug>
 #include <QList>
 #include <QMessageLogger>
-#include <QSharedPointer>
+#include <QSettings>
+#include <QString>
 #include <QVariant>
-#include <qnamespace.h>
+#include <algorithm>
+#include <optional>
 #include <utility>
 
-class QObject;
+namespace
+{
+constexpr auto SERVER_GROUP = "server";
+constexpr auto SERVER_DETAILS_GROUP = "server_details";
+constexpr auto SERVERS_ARRAY = "servers";
+
+QString legacyKey(const char *field, int index)
+{
+    return QStringLiteral("%1/%2%3")
+        .arg(QString::fromLatin1(SERVER_DETAILS_GROUP), QString::fromLatin1(field))
+        .arg(index);
+}
+
+//! Expects \a settings to be positioned on the server group.
+QList<ServersSettings::ServerEntry> readServerArray(QSettings &settings)
+{
+    QList<ServersSettings::ServerEntry> servers;
+
+    settings.beginGroup(SERVER_DETAILS_GROUP);
+    const int size = settings.beginReadArray(SERVERS_ARRAY);
+    servers.reserve(size);
+    for (int i = 0; i < size; ++i) {
+        settings.setArrayIndex(i);
+        ServersSettings::ServerEntry entry;
+        entry.saveName = settings.value("saveName").toString();
+        entry.host = settings.value("server").toString();
+        entry.port = settings.value("port").toString();
+        entry.username = settings.value("username").toString();
+        entry.password = settings.value("password").toString();
+        entry.savePassword = settings.value("savePassword").toBool();
+        entry.site = settings.value("site").toString();
+        servers.append(entry);
+    }
+    settings.endArray();
+    settings.endGroup();
+
+    return servers;
+}
+
+//! Expects \a settings to be positioned on the server group.
+QList<ServersSettings::ServerEntry> readLegacyServers(QSettings &settings, int lastIndex)
+{
+    QList<ServersSettings::ServerEntry> servers;
+
+    for (int i = 0; i <= lastIndex; ++i) {
+        ServersSettings::ServerEntry entry;
+        entry.saveName = settings.value(legacyKey("saveName", i)).toString();
+        entry.host = settings.value(legacyKey("server", i)).toString();
+        entry.port = settings.value(legacyKey("port", i)).toString();
+        entry.username = settings.value(legacyKey("username", i)).toString();
+        entry.password = settings.value(legacyKey("password", i)).toString();
+        entry.savePassword = settings.value(legacyKey("savePassword", i)).toBool();
+        entry.site = settings.value(legacyKey("site", i)).toString();
+
+        // Older builds could leave the unused index 0 behind.
+        if (!entry.saveName.isEmpty() || !entry.host.isEmpty()) {
+            servers.append(entry);
+        }
+    }
+
+    return servers;
+}
+
+void writeServerArray(QSettings &settings, const QList<ServersSettings::ServerEntry> &servers)
+{
+    settings.beginGroup(SERVER_DETAILS_GROUP);
+    // beginWriteArray() never drops entries past the new size, so without this
+    // a removed profile would keep its slot - including its saved password -
+    // in servers.ini forever. The legacy keys live beside the array, not in it.
+    settings.remove(SERVERS_ARRAY);
+    settings.beginWriteArray(SERVERS_ARRAY, servers.size());
+    for (int i = 0; i < servers.size(); ++i) {
+        settings.setArrayIndex(i);
+        const ServersSettings::ServerEntry &entry = servers.at(i);
+        settings.setValue("saveName", entry.saveName);
+        settings.setValue("server", entry.host);
+        settings.setValue("port", entry.port);
+        settings.setValue("username", entry.username);
+        settings.setValue("password", entry.password);
+        settings.setValue("savePassword", entry.savePassword);
+        settings.setValue("site", entry.site);
+    }
+    settings.endArray();
+    settings.endGroup();
+}
+} // namespace
 
 ServersSettings::ServersSettings(const QString &settingPath, QObject *parent)
-    : SettingsManager(settingPath + "servers.ini", "server", QString(), parent)
+    : SettingsManager(settingPath + "servers.ini", SERVER_GROUP, QString(), parent)
 {
+    migrateLegacyServers();
+}
+
+QList<ServersSettings::ServerEntry> ServersSettings::getServers() const
+{
+    return loadServers();
+}
+
+std::optional<ServersSettings::ServerEntry> ServersSettings::getServer(int index) const
+{
+    const QList<ServerEntry> servers = loadServers();
+    if (index < 0 || index >= servers.size()) {
+        return std::nullopt;
+    }
+    return servers.at(index);
 }
 
 void ServersSettings::setPreviousHostLogin(int previous)
@@ -45,16 +147,14 @@ void ServersSettings::setPrevioushostName(const QString &name)
 
 QString ServersSettings::getSaveName(QString defaultname)
 {
-    int index = getPrevioushostindex(getPrevioushostName());
-    QVariant saveName = getValue(QString("saveName%1").arg(index), "server", "server_details");
-    return saveName == QVariant() ? std::move(defaultname) : saveName.toString();
+    const auto server = getServer(getPrevioushostindex(getPrevioushostName()));
+    return server ? server->saveName : std::move(defaultname);
 }
 
 QString ServersSettings::getSite(QString defaultSite)
 {
-    int index = getPrevioushostindex(getPrevioushostName());
-    QVariant site = getValue(QString("site%1").arg(index), "server", "server_details");
-    return site == QVariant() ? std::move(defaultSite) : site.toString();
+    const auto server = getServer(getPrevioushostindex(getPrevioushostName()));
+    return server ? server->site : std::move(defaultSite);
 }
 
 QString ServersSettings::getPrevioushostName() const
@@ -65,10 +165,9 @@ QString ServersSettings::getPrevioushostName() const
 
 int ServersSettings::getPrevioushostindex(const QString &saveName) const
 {
-    int size = getValue("totalServers", "server", "server_details").toInt();
-
-    for (int i = 0; i <= size; ++i) {
-        if (saveName == getValue(QString("saveName%1").arg(i), "server", "server_details").toString()) {
+    const QList<ServerEntry> servers = loadServers();
+    for (int i = 0; i < servers.size(); ++i) {
+        if (servers.at(i).saveName == saveName) {
             return i;
         }
     }
@@ -78,43 +177,38 @@ int ServersSettings::getPrevioushostindex(const QString &saveName) const
 
 QString ServersSettings::getHostname(QString defaultHost) const
 {
-    int index = getPrevioushostindex(getPrevioushostName());
-    QVariant hostname = getValue(QString("server%1").arg(index), "server", "server_details");
-    return hostname == QVariant() ? std::move(defaultHost) : hostname.toString();
+    const auto server = getServer(getPrevioushostindex(getPrevioushostName()));
+    return server ? server->host : std::move(defaultHost);
 }
 
 QString ServersSettings::getPort(QString defaultPort) const
 {
-    int index = getPrevioushostindex(getPrevioushostName());
-    QVariant port = getValue(QString("port%1").arg(index), "server", "server_details");
-    qCDebug(ServersSettingsLog) << "getPort() index = " << index << " port.val = " << port.toString();
-    return port == QVariant() ? std::move(defaultPort) : port.toString();
+    const auto server = getServer(getPrevioushostindex(getPrevioushostName()));
+    qCDebug(ServersSettingsLog) << "getPort() found server = " << server.has_value();
+    return server ? server->port : std::move(defaultPort);
 }
 
 QString ServersSettings::getPlayerName(QString defaultName) const
 {
-    int index = getPrevioushostindex(getPrevioushostName());
-    QVariant name = getValue(QString("username%1").arg(index), "server", "server_details");
-    qCDebug(ServersSettingsLog) << "getPlayerName() index = " << index << " name.val = " << name.toString();
-    return name == QVariant() ? std::move(defaultName) : name.toString();
+    const auto server = getServer(getPrevioushostindex(getPrevioushostName()));
+    qCDebug(ServersSettingsLog) << "getPlayerName() found server = " << server.has_value();
+    return server ? server->username : std::move(defaultName);
 }
 
 QString ServersSettings::getPassword()
 {
-    int index = getPrevioushostindex(getPrevioushostName());
-
-    if (getSavePassword()) {
-        return getValue(QString("password%1").arg(index), "server", "server_details").toString();
+    const auto server = getServer(getPrevioushostindex(getPrevioushostName()));
+    if (!server || !server->savePassword) {
+        return QString();
     }
 
-    return QString();
+    return server->password;
 }
 
 bool ServersSettings::getSavePassword() const
 {
-    int index = getPrevioushostindex(getPrevioushostName());
-    bool save = getValue(QString("savePassword%1").arg(index), "server", "server_details").toBool();
-    return save;
+    const auto server = getServer(getPrevioushostindex(getPrevioushostName()));
+    return server && server->savePassword;
 }
 
 void ServersSettings::setAutoConnect(int autoconnect)
@@ -157,16 +251,20 @@ void ServersSettings::setFPPlayerName(QString playerName)
 
 void ServersSettings::setServerPassword(const QString &saveName, const QString &password)
 {
-    const int index = getPrevioushostindex(saveName);
-    if (index >= 0) {
-        setValue(password, QString("password%1").arg(index), "server", "server_details");
-    } else {
-        // A credential write that silently no-ops is hard to diagnose from a
-        // bug report; surface a mismatched profile name instead.
+    QList<ServerEntry> servers = loadServers();
+    for (ServerEntry &server : servers) {
+        if (server.saveName == saveName) {
+            server.password = password;
+            storeServers(servers);
+            return;
+        }
+    }
+
+    // A credential write that silently no-ops is hard to diagnose from a bug
+    // report; surface a mismatched profile name instead.
     qCWarning(ServersSettingsLog) << "setServerPassword() could not find profile:" << saveName
                                   << "- password not saved";
 }
-}
 
 QString ServersSettings::getFPPlayerName(QString defaultName) const
 {
@@ -202,61 +300,28 @@ void ServersSettings::addNewServer(const QString &saveName,
         return;
     }
 
-    int index = getValue("totalServers", "server", "server_details").toInt() + 1;
-
-    setValue(saveName, QString("saveName%1").arg(index), "server", "server_details");
-    setValue(serv, QString("server%1").arg(index), "server", "server_details");
-    setValue(port, QString("port%1").arg(index), "server", "server_details");
-    setValue(username, QString("username%1").arg(index), "server", "server_details");
-    setValue(savePassword, QString("savePassword%1").arg(index), "server", "server_details");
-    setValue(index, "totalServers", "server", "server_details");
-    setValue(password, QString("password%1").arg(index), "server", "server_details");
-    setValue(site, QString("site%1").arg(index), "server", "server_details");
+    QList<ServerEntry> servers = loadServers();
+    ServerEntry entry;
+    entry.saveName = saveName;
+    entry.host = serv;
+    entry.port = port;
+    entry.username = username;
+    entry.password = password;
+    entry.savePassword = savePassword;
+    entry.site = site;
+    servers.append(entry);
+    storeServers(servers);
 }
 
 void ServersSettings::removeServer(QString servAddr)
 {
-    int size = getValue("totalServers", "server", "server_details").toInt();
-
-    bool found = false;
-    for (int i = 0; i <= size; ++i) {
-        if (!found) {
-            // find entry and overwrite it
-            if (servAddr == getValue(QString("server%1").arg(i), "server", "server_details").toString()) {
-                found = true;
+    QList<ServerEntry> servers = loadServers();
+    for (int i = 0; i < servers.size(); ++i) {
+        if (servers.at(i).host == servAddr) {
+            servers.removeAt(i);
+            storeServers(servers);
+            return;
         }
-        } else {
-            // move all other entries after it one back, overwriting the previous one
-            int previous = i - 1; // we delete only one entry
-            setValue(getValue(QString("server%1").arg(i), "server", "server_details"),
-                     QString("server%1").arg(previous), "server", "server_details");
-            setValue(getValue(QString("port%1").arg(i), "server", "server_details"), QString("port%1").arg(previous),
-                     "server", "server_details");
-            setValue(getValue(QString("username%1").arg(i), "server", "server_details"),
-                     QString("username%1").arg(previous), "server", "server_details");
-            setValue(getValue(QString("savePassword%1").arg(i), "server", "server_details"),
-                     QString("savePassword%1").arg(previous), "server", "server_details");
-            setValue(getValue(QString("password%1").arg(i), "server", "server_details"),
-                     QString("password%1").arg(previous), "server", "server_details");
-            setValue(getValue(QString("saveName%1").arg(i), "server", "server_details"),
-                     QString("saveName%1").arg(previous), "server", "server_details");
-            setValue(getValue(QString("site%1").arg(i), "server", "server_details"), QString("site%1").arg(previous),
-                     "server", "server_details");
-        }
-    }
-
-    // if we have deleted an entry, adjust the total
-    if (found) {
-        setValue(size - 1, "totalServers", "server", "server_details");
-
-        // delete last value
-        deleteValue(QString("server%1").arg(size), "server", "server_details");
-        deleteValue(QString("port%1").arg(size), "server", "server_details");
-        deleteValue(QString("username%1").arg(size), "server", "server_details");
-        deleteValue(QString("savePassword%1").arg(size), "server", "server_details");
-        deleteValue(QString("password%1").arg(size), "server", "server_details");
-        deleteValue(QString("saveName%1").arg(size), "server", "server_details");
-        deleteValue(QString("site%1").arg(size), "server", "server_details");
     }
 }
 
@@ -265,23 +330,23 @@ void ServersSettings::removeServer(QString servAddr)
  */
 bool ServersSettings::updateExistingServerWithoutLoss(QString saveName, QString serv, QString port, QString site)
 {
-    int size = getValue("totalServers", "server", "server_details").toInt();
+    QList<ServerEntry> servers = loadServers();
+    for (ServerEntry &server : servers) {
+        if (server.host != serv) {
+            continue;
+        }
 
-    for (int i = 0; i <= size; ++i) {
-        if (serv == getValue(QString("server%1").arg(i), "server", "server_details").toString()) {
         if (!port.isEmpty()) {
-                setValue(port, QString("port%1").arg(i), "server", "server_details");
+            server.port = port;
         }
-
         if (!site.isEmpty()) {
-                setValue(site, QString("site%1").arg(i), "server", "server_details");
+            server.site = site;
         }
-
-            setValue(saveName, QString("saveName%1").arg(i), "server", "server_details");
-
+        server.saveName = saveName;
+        storeServers(servers);
         return true;
     }
-    }
+
     return false;
 }
 
@@ -293,19 +358,15 @@ bool ServersSettings::updateExistingServer(QString saveName,
                                            bool savePassword,
                                            QString site)
 {
-    int size = getValue("totalServers", "server", "server_details").toInt();
-
-    for (int i = 0; i <= size; ++i) {
-        if (serv == getValue(QString("server%1").arg(i), "server", "server_details").toString()) {
-            setValue(port, QString("port%1").arg(i), "server", "server_details");
-            updateServerFields(i, saveName, username, password, savePassword, site);
-
-            return true;
-        }
-    }
+    const int index = findServerIndex(serv, port);
+    if (index < 0) {
         return false;
     }
 
+    updateServerFields(index, saveName, username, password, savePassword, site);
+    return true;
+}
+
 void ServersSettings::updateServerFields(int index,
                                          const QString &saveName,
                                          const QString &username,
@@ -313,33 +374,33 @@ void ServersSettings::updateServerFields(int index,
                                          bool savePassword,
                                          const QString &site)
 {
+    QList<ServerEntry> servers = loadServers();
+    if (index < 0 || index >= servers.size()) {
+        return;
+    }
+
+    ServerEntry &server = servers[index];
     if (!username.isEmpty()) {
-        setValue(username, QString("username%1").arg(index), "server", "server_details");
+        server.username = username;
     }
-
     if (savePassword && !password.isEmpty()) {
-        setValue(password, QString("password%1").arg(index), "server", "server_details");
+        server.password = password;
     } else {
-        setValue(QString(), QString("password%1").arg(index), "server", "server_details");
+        server.password.clear();
     }
-
     if (!site.isEmpty()) {
-        setValue(site, QString("site%1").arg(index), "server", "server_details");
+        server.site = site;
     }
-
-    setValue(savePassword, QString("savePassword%1").arg(index), "server", "server_details");
-    setValue(saveName, QString("saveName%1").arg(index), "server", "server_details");
+    server.savePassword = savePassword;
+    server.saveName = saveName;
+    storeServers(servers);
 }
 
 int ServersSettings::findServerIndex(const QString &host, const QString &port) const
 {
-    int size = getValue("totalServers", "server", "server_details").toInt();
-
-    for (int i = 0; i <= size; ++i) {
-        QString storedHost = getValue(QString("server%1").arg(i), "server", "server_details").toString();
-        QString storedPort = getValue(QString("port%1").arg(i), "server", "server_details").toString();
-
-        if (storedHost == host && storedPort == port) {
+    const QList<ServerEntry> servers = loadServers();
+    for (int i = 0; i < servers.size(); ++i) {
+        if (servers.at(i).host == host && servers.at(i).port == port) {
             return i;
         }
     }
@@ -349,12 +410,9 @@ int ServersSettings::findServerIndex(const QString &host, const QString &port) c
 
 int ServersSettings::findHostIndex(const QString &host) const
 {
-    int size = getValue("totalServers", "server", "server_details").toInt();
-
-    for (int i = 0; i <= size; ++i) {
-        QString storedHost = getValue(QString("server%1").arg(i), "server", "server_details").toString();
-
-        if (storedHost.compare(host, Qt::CaseInsensitive) == 0) {
+    const QList<ServerEntry> servers = loadServers();
+    for (int i = 0; i < servers.size(); ++i) {
+        if (servers.at(i).host.compare(host, Qt::CaseInsensitive) == 0) {
             return i;
         }
     }
@@ -364,29 +422,100 @@ int ServersSettings::findHostIndex(const QString &host) const
 
 bool ServersSettings::hasUsername(const QString &host, const QString &port) const
 {
-    int index = findServerIndex(host, port);
-    if (index < 0) {
-        return false;
-    }
-
-    QString user = getValue(QString("username%1").arg(index), "server", "server_details").toString();
-    return !user.isEmpty();
+    const auto server = getServer(findServerIndex(host, port));
+    return server && !server->username.isEmpty();
 }
 
 bool ServersSettings::hasCredentials(const QString &host, const QString &port) const
 {
-    int index = findServerIndex(host, port);
-    if (index < 0) {
-        return false;
-    }
-
-    bool save = getValue(QString("savePassword%1").arg(index), "server", "server_details").toBool();
-    QString password = getValue(QString("password%1").arg(index), "server", "server_details").toString();
-
-    return save && !password.isEmpty();
+    const auto server = getServer(findServerIndex(host, port));
+    return server && server->savePassword && !server->password.isEmpty();
 }
 
 bool ServersSettings::hasLoginData(const QString &host, const QString &port) const
 {
     return hasUsername(host, port) && hasCredentials(host, port);
 }
+
+QList<ServersSettings::ServerEntry> ServersSettings::loadServers() const
+{
+    QSettings settings = getSettings();
+    settings.beginGroup(SERVER_GROUP);
+    const QList<ServerEntry> servers = readServerArray(settings);
+    settings.endGroup();
+
+    return servers;
+}
+
+void ServersSettings::storeServers(const QList<ServerEntry> &servers)
+{
+    batchWrite([&servers](QSettings &settings) {
+        settings.beginGroup(SERVER_GROUP);
+        writeServerArray(settings, servers);
+        settings.endGroup();
+    });
+}
+
+void ServersSettings::migrateLegacyServers()
+{
+    {
+        QSettings settings = getSettings();
+        settings.beginGroup(SERVER_GROUP);
+        // Only builds that predate the array format write this marker. Once it
+        // is gone there is nothing left to import and no reason to touch the
+        // file (a sync on every start is a needless open-and-write).
+        const bool hasLegacy = settings.contains(QStringLiteral("%1/totalServers").arg(SERVER_DETAILS_GROUP));
+        settings.endGroup();
+
+        if (!hasLegacy) {
+            return;
+        }
+    }
+
+    const QSettings::Status status = batchWrite([](QSettings &settings) {
+        settings.beginGroup(SERVER_GROUP);
+
+        const int lastLegacyIndex =
+            settings.value(QStringLiteral("%1/totalServers").arg(SERVER_DETAILS_GROUP), -1).toInt();
+
+        QList<ServersSettings::ServerEntry> servers = readServerArray(settings);
+
+        // An older build can write the legacy keys again after the array
+        // migration, so the array is not automatically complete: append every
+        // legacy profile the array does not already hold instead of dropping it.
+        int imported = 0;
+        const QList<ServersSettings::ServerEntry> legacyServers = readLegacyServers(settings, lastLegacyIndex);
+        for (const ServersSettings::ServerEntry &entry : legacyServers) {
+            const bool known =
+                std::any_of(servers.cbegin(), servers.cend(), [&entry](const ServersSettings::ServerEntry &existing) {
+                    return existing.host == entry.host && existing.port == entry.port;
+                });
+            if (!known) {
+                servers.append(entry);
+                ++imported;
+            }
+        }
+
+        writeServerArray(settings, servers);
+        if (imported > 0) {
+            qCDebug(ServersSettingsLog) << "Migrated" << imported << "server profiles to the array format";
+        }
+
+        // The array (freshly written or already present) is authoritative now.
+        for (int i = 0; i <= lastLegacyIndex; ++i) {
+            settings.remove(legacyKey("saveName", i));
+            settings.remove(legacyKey("server", i));
+            settings.remove(legacyKey("port", i));
+            settings.remove(legacyKey("username", i));
+            settings.remove(legacyKey("password", i));
+            settings.remove(legacyKey("savePassword", i));
+            settings.remove(legacyKey("site", i));
+        }
+        settings.remove(QStringLiteral("%1/totalServers").arg(SERVER_DETAILS_GROUP));
+        settings.endGroup();
+    });
+
+    if (status != QSettings::NoError) {
+        qCWarning(ServersSettingsLog) << "Failed to migrate server profiles:" << status;
+    }
+}

libcockatrice_settings/libcockatrice/settings/servers_settings.h

@@ -9,9 +9,11 @@
 
 #include "settings_manager.h"
 
+#include <QList>
 #include <QLoggingCategory>
 #include <QString>
 #include <QStringList>
+#include <optional>
 #include <qtmetamacros.h>
 
 class QObject;
@@ -27,6 +29,22 @@ class ServersSettings : public SettingsManager
     friend class SettingsCache;
 
 public:
+    struct ServerEntry
+    {
+        QString saveName;
+        QString host;
+        QString port;
+        QString username;
+        QString password;
+        bool savePassword = false;
+        QString site;
+    };
+
+    explicit ServersSettings(const QString &settingPath, QObject *parent = nullptr);
+
+    QList<ServerEntry> getServers() const;
+    std::optional<ServerEntry> getServer(int index) const;
+
     int getPreviousHostLogin() const;
     int getPrevioushostindex(const QString &) const;
     QStringList getPreviousHostList() const;
@@ -88,8 +106,11 @@ public:
     bool getClearDebugLogStatus(bool abDefaultValue) const;
 
 private:
-    explicit ServersSettings(const QString &settingPath, QObject *parent = nullptr);
     ServersSettings(const ServersSettings & /*other*/);
+
+    QList<ServerEntry> loadServers() const;
+    void storeServers(const QList<ServerEntry> &servers);
+    void migrateLegacyServers();
 };
 
 #endif // SERVERSSETTINGS_H

libcockatrice_settings/libcockatrice/settings/settings_manager.cpp

@@ -193,13 +193,12 @@ QVariant SettingsManager::getValue(const QString &name,
     return value;
 }
 
-void SettingsManager::batchWrite(std::function<void(QSettings &)> batchWriteFunction)
+QSettings::Status SettingsManager::batchWrite(std::function<void(QSettings &)> batchWriteFunction)
 {
     auto settings = getSettings();
-    settings.setAtomicSyncRequired(false);
     batchWriteFunction(settings);
     settings.sync(); // single flush
-    settings.setAtomicSyncRequired(true);
+    return settings.status();
 }
 
 /**

libcockatrice_settings/libcockatrice/settings/settings_manager.h

@@ -28,7 +28,9 @@ public:
     QVariant getValue(const QString &name, const QString &group, const QString &subGroup = QString()) const;
     QVariant
     getValue(const QString &name, const QString &group, const QString &subGroup, const QVariant &defaultValue) const;
-    void batchWrite(std::function<void(QSettings &)> batchWriteFunction);
+    //! \brief Runs \a batchWriteFunction on a temporary QSettings and syncs once.
+    //! \return the status of the underlying file after the final sync.
+    QSettings::Status batchWrite(std::function<void(QSettings &)> batchWriteFunction);
 
     void sync();
 

tests/settings/CMakeLists.txt

@@ -1,5 +1,7 @@
 cockatrice_add_test(NAME settings_manager_test SOURCES settings_manager_test.cpp LIBS libcockatrice_settings)
 
+cockatrice_add_test(NAME servers_settings_test SOURCES servers_settings_test.cpp LIBS libcockatrice_settings)
+
 cockatrice_add_test(
   NAME
   settings_defaults_test

tests/settings/servers_settings_test.cpp

@@ -0,0 +1,228 @@
+#include "gtest/gtest.h"
+#include <QSettings>
+#include <QString>
+#include <QTemporaryDir>
+#include <libcockatrice/settings/servers_settings.h>
+
+namespace
+{
+class ServersSettingsTest : public ::testing::Test
+{
+protected:
+    QTemporaryDir tempDir;
+    QString settingsPath;
+
+    void SetUp() override
+    {
+        settingsPath = tempDir.path() + "/";
+    }
+
+    QString settingsFile() const
+    {
+        return settingsPath + "servers.ini";
+    }
+
+    void writeLegacyServer(int index,
+                           const QString &saveName,
+                           const QString &host,
+                           const QString &port,
+                           const QString &username,
+                           const QString &password,
+                           bool savePassword,
+                           const QString &site)
+    {
+        QSettings settings(settingsFile(), QSettings::IniFormat);
+        const QString prefix = QStringLiteral("server/server_details/");
+        settings.setValue(prefix + QStringLiteral("saveName%1").arg(index), saveName);
+        settings.setValue(prefix + QStringLiteral("server%1").arg(index), host);
+        settings.setValue(prefix + QStringLiteral("port%1").arg(index), port);
+        settings.setValue(prefix + QStringLiteral("username%1").arg(index), username);
+        settings.setValue(prefix + QStringLiteral("password%1").arg(index), password);
+        settings.setValue(prefix + QStringLiteral("savePassword%1").arg(index), savePassword);
+        settings.setValue(prefix + QStringLiteral("site%1").arg(index), site);
+    }
+};
+
+TEST_F(ServersSettingsTest, AddServerStoresAllFields)
+{
+    ServersSettings settings(settingsPath, nullptr);
+    settings.addNewServer("Alpha", "alpha.example", "4748", "alice", "secret", true, "https://alpha.example");
+
+    const auto servers = settings.getServers();
+    ASSERT_EQ(servers.size(), 1);
+    EXPECT_EQ(servers.first().saveName, QString("Alpha"));
+    EXPECT_EQ(servers.first().host, QString("alpha.example"));
+    EXPECT_EQ(servers.first().port, QString("4748"));
+    EXPECT_EQ(servers.first().username, QString("alice"));
+    EXPECT_EQ(servers.first().password, QString("secret"));
+    EXPECT_TRUE(servers.first().savePassword);
+    EXPECT_EQ(servers.first().site, QString("https://alpha.example"));
+}
+
+TEST_F(ServersSettingsTest, AddExistingHostAndPortUpdatesInsteadOfDuplicating)
+{
+    ServersSettings settings(settingsPath, nullptr);
+    settings.addNewServer("Alpha", "alpha.example", "4748", "alice", "secret", true, "site-a");
+    settings.addNewServer("Renamed", "alpha.example", "4748", "bob", "new-secret", true, "site-b");
+
+    const auto servers = settings.getServers();
+    ASSERT_EQ(servers.size(), 1);
+    EXPECT_EQ(servers.first().saveName, QString("Renamed"));
+    EXPECT_EQ(servers.first().username, QString("bob"));
+    EXPECT_EQ(servers.first().password, QString("new-secret"));
+    EXPECT_EQ(servers.first().site, QString("site-b"));
+}
+
+TEST_F(ServersSettingsTest, RemoveServerRemovesOnlyMatchingHostAndKeepsOrder)
+{
+    ServersSettings settings(settingsPath, nullptr);
+    settings.addNewServer("Alpha", "alpha.example", "4748", "alice", "a", true, "");
+    settings.addNewServer("Beta", "beta.example", "4748", "bob", "b", true, "");
+    settings.addNewServer("Gamma", "gamma.example", "4748", "carol", "c", true, "");
+
+    settings.removeServer("beta.example");
+
+    const auto servers = settings.getServers();
+    ASSERT_EQ(servers.size(), 2);
+    EXPECT_EQ(servers.at(0).saveName, QString("Alpha"));
+    EXPECT_EQ(servers.at(1).saveName, QString("Gamma"));
+}
+
+TEST_F(ServersSettingsTest, RemovingLastServerDropsItsStaleArraySlot)
+{
+    ServersSettings settings(settingsPath, nullptr);
+    settings.addNewServer("Alpha", "alpha.example", "4748", "alice", "a", true, "");
+    settings.addNewServer("Beta", "beta.example", "4748", "bob", "b", true, "");
+    settings.addNewServer("Gamma", "gamma.example", "4748", "carol", "gamma-secret", true, "");
+
+    settings.removeServer("gamma.example");
+
+    QSettings raw(settingsFile(), QSettings::IniFormat);
+    EXPECT_FALSE(raw.contains("server/server_details/servers/3/password"));
+    EXPECT_FALSE(raw.contains("server/server_details/servers/3/saveName"));
+    ASSERT_EQ(settings.getServers().size(), 2);
+}
+
+TEST_F(ServersSettingsTest, UpdateWithoutLossKeepsCredentials)
+{
+    ServersSettings settings(settingsPath, nullptr);
+    settings.addNewServer("Alpha", "alpha.example", "4748", "alice", "secret", true, "");
+
+    EXPECT_TRUE(settings.updateExistingServerWithoutLoss("Renamed", "alpha.example", "1234", "site"));
+
+    const auto server = settings.getServer(settings.findServerIndex("alpha.example", "1234"));
+    ASSERT_TRUE(server.has_value());
+    EXPECT_EQ(server->saveName, QString("Renamed"));
+    EXPECT_EQ(server->username, QString("alice"));
+    EXPECT_EQ(server->password, QString("secret"));
+    EXPECT_TRUE(server->savePassword);
+    EXPECT_EQ(server->site, QString("site"));
+}
+
+TEST_F(ServersSettingsTest, SetServerPasswordUpdatesOnlyTheNamedProfile)
+{
+    ServersSettings settings(settingsPath, nullptr);
+    settings.addNewServer("Alpha", "alpha.example", "4748", "alice", "old", true, "");
+    settings.addNewServer("Beta", "beta.example", "4748", "bob", "keep", true, "");
+
+    settings.setServerPassword("Beta", "new");
+
+    EXPECT_EQ(settings.getServer(0)->password, QString("old"));
+    EXPECT_EQ(settings.getServer(1)->password, QString("new"));
+}
+
+TEST_F(ServersSettingsTest, LegacyProfilesAreMigrated)
+{
+    {
+        QSettings settings(settingsFile(), QSettings::IniFormat);
+        settings.setValue("server/server_details/totalServers", 2);
+    }
+    writeLegacyServer(1, "Alpha", "alpha.example", "4748", "alice", "secret", true, "site-a");
+    writeLegacyServer(2, "Beta", "beta.example", "4748", "bob", "other", false, "site-b");
+
+    ServersSettings settings(settingsPath, nullptr);
+
+    const auto servers = settings.getServers();
+    ASSERT_EQ(servers.size(), 2);
+    EXPECT_EQ(servers.at(0).saveName, QString("Alpha"));
+    EXPECT_EQ(servers.at(0).host, QString("alpha.example"));
+    EXPECT_TRUE(servers.at(0).savePassword);
+    EXPECT_EQ(servers.at(1).saveName, QString("Beta"));
+    EXPECT_FALSE(servers.at(1).savePassword);
+
+    QSettings raw(settingsFile(), QSettings::IniFormat);
+    EXPECT_FALSE(raw.contains("server/server_details/totalServers"));
+    EXPECT_FALSE(raw.contains("server/server_details/saveName1"));
+    EXPECT_FALSE(raw.contains("server/server_details/server1"));
+}
+
+TEST_F(ServersSettingsTest, MigrationRunsOnce)
+{
+    writeLegacyServer(1, "Alpha", "alpha.example", "4748", "alice", "secret", true, "");
+    {
+        QSettings settings(settingsFile(), QSettings::IniFormat);
+        settings.setValue("server/server_details/totalServers", 1);
+    }
+
+    {
+        ServersSettings first(settingsPath, nullptr);
+        ASSERT_EQ(first.getServers().size(), 1);
+    }
+
+    ServersSettings second(settingsPath, nullptr);
+    ASSERT_EQ(second.getServers().size(), 1);
+}
+
+TEST_F(ServersSettingsTest, LegacyProfileAddedAfterMigrationIsMerged)
+{
+    {
+        ServersSettings first(settingsPath, nullptr);
+        first.addNewServer("Alpha", "alpha.example", "4748", "alice", "secret", true, "");
+    }
+
+    // An older build writes its own profile back in the legacy format after the
+    // array migration already happened.
+    writeLegacyServer(1, "Beta", "beta.example", "4748", "bob", "other", true, "site-b");
+    // A duplicate of a profile the array already holds must not be added twice.
+    writeLegacyServer(2, "Alpha duplicate", "alpha.example", "4748", "eve", "dupe", true, "");
+    {
+        QSettings settings(settingsFile(), QSettings::IniFormat);
+        settings.setValue("server/server_details/totalServers", 2);
+    }
+
+    ServersSettings settings(settingsPath, nullptr);
+
+    const auto servers = settings.getServers();
+    ASSERT_EQ(servers.size(), 2);
+    EXPECT_EQ(servers.at(0).saveName, QString("Alpha"));
+    EXPECT_EQ(servers.at(0).password, QString("secret"));
+    EXPECT_EQ(servers.at(1).saveName, QString("Beta"));
+    EXPECT_EQ(servers.at(1).host, QString("beta.example"));
+    EXPECT_EQ(servers.at(1).username, QString("bob"));
+    EXPECT_EQ(servers.at(1).password, QString("other"));
+    EXPECT_TRUE(servers.at(1).savePassword);
+    EXPECT_EQ(servers.at(1).site, QString("site-b"));
+
+    QSettings raw(settingsFile(), QSettings::IniFormat);
+    EXPECT_FALSE(raw.contains("server/server_details/totalServers"));
+    EXPECT_FALSE(raw.contains("server/server_details/saveName1"));
+    EXPECT_FALSE(raw.contains("server/server_details/server2"));
+
+    // The merge is persisted, so a further start adds nothing.
+    ServersSettings again(settingsPath, nullptr);
+    ASSERT_EQ(again.getServers().size(), 2);
+}
+
+TEST_F(ServersSettingsTest, OutOfRangeIndexReturnsNullopt)
+{
+    ServersSettings settings(settingsPath, nullptr);
+    EXPECT_FALSE(settings.getServer(-1).has_value());
+    EXPECT_FALSE(settings.getServer(0).has_value());
+}
+} // namespace
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}