Address comments, fix windows CI

Took 8 minutes

cockatrice/src/client/settings/cache_settings.cpp

@@ -107,7 +107,7 @@ SettingsCache::SettingsCache()
     settings = new QSettings(settingsPath + "global.ini", QSettings::IniFormat, this);
 
     // Migrate from legacy NativeFormat settings to per-class INI files (runs at most once)
-    if (!isPortableBuild && !QFile(settingsPath + "global.ini").exists()) {
+    if (!isPortableBuild) {
         SettingsMigration::migrateLegacySettings(settingsPath);
     }
 

libcockatrice_settings/CMakeLists.txt

@@ -67,3 +67,7 @@ target_include_directories(
 )
 
 target_link_libraries(libcockatrice_settings PUBLIC libcockatrice_card libcockatrice_utility ${QT_CORE_MODULE})
+
+if(TEST)
+  target_compile_definitions(libcockatrice_settings PRIVATE SETTINGS_UNIT_TEST)
+endif()

libcockatrice_settings/libcockatrice/settings/cache_storage_settings.cpp

@@ -27,7 +27,8 @@ int CacheStorageSettings::getCardPictureLoaderCacheMethod() const
 
 int CacheStorageSettings::getLocalCardImageStorageNamingScheme() const
 {
-    return getValue("localCardImageStorageNamingScheme", QString(), QString(), 0).toInt();
+    return getValue("localCardImageStorageNamingScheme", QString(), QString(), LOCAL_CARD_IMAGE_NAMING_SCHEME_DEFAULT)
+        .toInt();
 }
 
 void CacheStorageSettings::setPixmapCacheSize(int _pixmapCacheSize)

libcockatrice_settings/libcockatrice/settings/cache_storage_settings.h

@@ -15,6 +15,8 @@ constexpr int NETWORK_CACHE_SIZE_DEFAULT = 1024 * 4; // 4 GB
 constexpr int NETWORK_CACHE_SIZE_MIN = 1;            // 1 MB
 constexpr int NETWORK_CACHE_SIZE_MAX = 1024 * 1024;  // 1 TB
 
+#define LOCAL_CARD_IMAGE_NAMING_SCHEME_DEFAULT 6
+
 // In Days
 #define NETWORK_REDIRECT_CACHE_TTL_DEFAULT 30
 #define NETWORK_REDIRECT_CACHE_TTL_MIN 1

libcockatrice_settings/libcockatrice/settings/personal_settings.cpp

@@ -7,7 +7,7 @@ PersonalSettings::PersonalSettings(const QString &settingPath, QObject *parent)
 
 QString PersonalSettings::getLang() const
 {
-    return getValue("lang", QString(), QString(), "English").toString();
+    return getValue("lang", QString(), QString(), QString()).toString();
 }
 
 QString PersonalSettings::getClientID()

libcockatrice_settings/libcockatrice/settings/settings_migration.cpp

@@ -61,6 +61,9 @@ static void migrateGameSettings(const QString &settingsPath, QSettings &globalIn
 
     QSettings gameIni(settingsPath + "game.ini", QSettings::IniFormat);
     for (const auto &key : gameKeys) {
+        if (key == "maxfontsize") {
+            continue;
+        }
         gameIni.setValue("game/" + key, globalIni.value("game/" + key));
     }
     for (const auto &key : localGameKeys) {
@@ -460,10 +463,18 @@ static void migrateLegacyGameFilters(const QString &settingsPath)
 
 bool SettingsMigration::migrateLegacySettings(const QString &settingsPath)
 {
+    QSettings personalIni(settingsPath + "personal.ini", QSettings::IniFormat);
+    if (personalIni.value("migration/legacy_complete", false).toBool()) {
+        return false;
+    }
+
     migrateLegacySets(settingsPath);
     migrateLegacyServers(settingsPath);
     migrateLegacyMessages(settingsPath);
     migrateLegacyGameFilters(settingsPath);
+
+    personalIni.setValue("migration/legacy_complete", true);
+    personalIni.sync();
     return true;
 }
 
@@ -475,7 +486,14 @@ bool SettingsMigration::migrateSettingsFromGlobalIni(const QString &settingsPath
 
     QSettings globalIni(settingsPath + "global.ini", QSettings::IniFormat);
     if (globalIni.value(MIGRATION_SENTINEL_KEY, false).toBool()) {
-        return false;
+        // If a user runs an older Cockatrice build that writes to global.ini (non-sentinel keys)
+        // and then upgrades back, those new keys will be silently ignored. Re-migrate them.
+        globalIni.sync();
+        auto allKeys = globalIni.allKeys();
+        allKeys.removeAll(MIGRATION_SENTINEL_KEY);
+        if (allKeys.isEmpty()) {
+            return false;
+        }
     }
 
     migrateTabsSettings(settingsPath, globalIni);
@@ -490,9 +508,8 @@ bool SettingsMigration::migrateSettingsFromGlobalIni(const QString &settingsPath
     migratePathsSettings(settingsPath, globalIni);
     migrateVisualDeckStorageSettings(settingsPath, globalIni);
 
-    if (!QFile::rename(settingsPath + "global.ini", settingsPath + "global.ini.old")) {
+    QFile::remove(settingsPath + "global.ini.old");
-        return false;
+    QFile::rename(settingsPath + "global.ini", settingsPath + "global.ini.old");
-    }
 
     QSettings newGlobalIni(settingsPath + "global.ini", QSettings::IniFormat);
     newGlobalIni.setValue(MIGRATION_SENTINEL_KEY, true);

tests/settings/settings_defaults_test.cpp

@@ -61,7 +61,7 @@ TEST_F(SettingsDefaultsTest, CacheStorage_CardPictureLoaderCacheMethod_Default)
 TEST_F(SettingsDefaultsTest, CacheStorage_LocalCardImageStorageNamingScheme_Default)
 {
     CacheStorageSettings s(settingsPath, nullptr);
-    ASSERT_EQ(s.getLocalCardImageStorageNamingScheme(), 0);
+    ASSERT_EQ(s.getLocalCardImageStorageNamingScheme(), LOCAL_CARD_IMAGE_NAMING_SCHEME_DEFAULT);
 }
 
 // --- GameSettings ---
@@ -255,7 +255,7 @@ TEST_F(SettingsDefaultsTest, Chat_RoomHistory_Default)
 TEST_F(SettingsDefaultsTest, Personal_Lang_Default)
 {
     PersonalSettings s(settingsPath, nullptr);
-    ASSERT_EQ(s.getLang(), QString("English"));
+    ASSERT_EQ(s.getLang(), QString(""));
 }
 
 TEST_F(SettingsDefaultsTest, Personal_ClientID_Default)

tests/settings/settings_migration_test.cpp

@@ -1,4 +1,5 @@
 #include "gtest/gtest.h"
+#include <QCoreApplication>
 #include <QDir>
 #include <QFile>
 #include <QSettings>
@@ -18,6 +19,9 @@ protected:
     void SetUp() override
     {
         settingsPath = tempDir.path() + "/";
+        QSettings nativeSettings;
+        nativeSettings.clear();
+        nativeSettings.sync();
     }
 
     bool fileExists(const QString &name) const
@@ -312,10 +316,139 @@ TEST_F(SettingsMigrationTest, CardsKeysKeepGroupPrefix)
     ASSERT_EQ(readFromIni("cards.ini", "interface/deckeditorbannercardcomboboxvisible"), QVariant(true));
 }
 
+// --- migrateLegacySettings tests ---
+
+TEST_F(SettingsMigrationTest, LegacyMigratesServersMessagesAndFilters)
+{
+    {
+        QSettings nativeSettings;
+        nativeSettings.setValue("server/previoushostlogin", "test_user");
+        nativeSettings.setValue("server/auto_connect", true);
+        nativeSettings.beginGroup("messages");
+        nativeSettings.setValue("count", 1);
+        nativeSettings.setValue("message0", "hello");
+        nativeSettings.endGroup();
+        nativeSettings.beginGroup("filter_games");
+        nativeSettings.setValue("show_empty", true);
+        nativeSettings.endGroup();
+        nativeSettings.sync();
+    }
+
+    ASSERT_TRUE(SettingsMigration::migrateLegacySettings(settingsPath));
+
+    ASSERT_TRUE(fileExists("servers.ini"));
+    ASSERT_EQ(readFromIni("servers.ini", "server/previoushostlogin"), QVariant("test_user"));
+    ASSERT_EQ(readFromIni("servers.ini", "server/auto_connect"), QVariant(true));
+
+    ASSERT_TRUE(fileExists("messages.ini"));
+    ASSERT_EQ(readFromIni("messages.ini", "messages/count"), QVariant(1));
+    ASSERT_EQ(readFromIni("messages.ini", "messages/message0"), QVariant("hello"));
+
+    ASSERT_TRUE(fileExists("gamefilters.ini"));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/show_empty"), QVariant(true));
+}
+
+TEST_F(SettingsMigrationTest, LegacyMigratesSets)
+{
+    {
+        QSettings nativeSettings;
+        nativeSettings.beginGroup("sets");
+        nativeSettings.beginGroup("set1");
+        nativeSettings.setValue("sortkey", "001");
+        nativeSettings.setValue("enabled", true);
+        nativeSettings.setValue("isknown", false);
+        nativeSettings.endGroup();
+        nativeSettings.endGroup();
+        nativeSettings.sync();
+    }
+
+    ASSERT_TRUE(SettingsMigration::migrateLegacySettings(settingsPath));
+
+    ASSERT_TRUE(fileExists("cardDatabase.ini"));
+    ASSERT_EQ(readFromIni("cardDatabase.ini", "sets/set1/sortkey"), QVariant("001"));
+    ASSERT_EQ(readFromIni("cardDatabase.ini", "sets/set1/enabled"), QVariant(true));
+    ASSERT_EQ(readFromIni("cardDatabase.ini", "sets/set1/isknown"), QVariant(false));
+}
+
+TEST_F(SettingsMigrationTest, LegacyMigrationIsIdempotent)
+{
+    {
+        QSettings nativeSettings;
+        nativeSettings.setValue("server/previoushostlogin", "test_user");
+        nativeSettings.setValue("server/auto_connect", true);
+        nativeSettings.sync();
+    }
+
+    ASSERT_TRUE(SettingsMigration::migrateLegacySettings(settingsPath));
+
+    // Change the migrated value
+    {
+        QSettings serversIni(settingsPath + "servers.ini", QSettings::IniFormat);
+        serversIni.setValue("server/previoushostlogin", "modified_user");
+        serversIni.sync();
+    }
+
+    // Second migration should NOT overwrite the change
+    ASSERT_FALSE(SettingsMigration::migrateLegacySettings(settingsPath));
+    ASSERT_EQ(readFromIni("servers.ini", "server/previoushostlogin"), QVariant("modified_user"));
+}
+
+TEST_F(SettingsMigrationTest, LegacyMigrationEmptyNativeFormatWritesSentinel)
+{
+    ASSERT_TRUE(SettingsMigration::migrateLegacySettings(settingsPath));
+    ASSERT_TRUE(fileExists("personal.ini"));
+    ASSERT_EQ(readFromIni("personal.ini", "migration/legacy_complete"), QVariant(true));
+    ASSERT_FALSE(fileExists("messages.ini"));
+    ASSERT_FALSE(fileExists("gamefilters.ini"));
+    ASSERT_FALSE(fileExists("cardDatabase.ini"));
+
+    // Second call should be a no-op
+    ASSERT_FALSE(SettingsMigration::migrateLegacySettings(settingsPath));
+}
+
+// --- migrateSettingsFromGlobalIni rename-failure test ---
+
+TEST_F(SettingsMigrationTest, RenameFailureWithExistingBackup)
+{
+    {
+        QSettings g(settingsPath + "global.ini", QSettings::IniFormat);
+        g.setValue("tabs/visualDeckStorage", true);
+        g.setValue("sound/enabled", true);
+        g.sync();
+    }
+
+    // Create a stale backup
+    {
+        QFile oldBackup(settingsPath + "global.ini.old");
+        ASSERT_TRUE(oldBackup.open(QIODevice::WriteOnly));
+        oldBackup.close();
+    }
+
+    // Migration should still succeed
+    ASSERT_TRUE(SettingsMigration::migrateSettingsFromGlobalIni(settingsPath));
+
+    // Per-file INIs should be created
+    ASSERT_TRUE(fileExists("tabs.ini"));
+    ASSERT_TRUE(fileExists("sound.ini"));
+
+    // Sentinel should be set in the new global.ini
+    ASSERT_EQ(readFromIni("global.ini", "migration/perfile_complete"), QVariant(true));
+
+    // Original data should be backed up (stale backup was replaced)
+    ASSERT_TRUE(fileExists("global.ini.old"));
+    ASSERT_EQ(readFromIni("global.ini.old", "tabs/visualDeckStorage"), QVariant(true));
+
+    // Second call should be a no-op
+    ASSERT_FALSE(SettingsMigration::migrateSettingsFromGlobalIni(settingsPath));
+}
+
 } // namespace
 
 int main(int argc, char **argv)
 {
     ::testing::InitGoogleTest(&argc, argv);
+    QCoreApplication app(argc, argv);
+    app.setOrganizationName("CockatriceTest");
+    app.setApplicationName("SettingsMigrationTest");
     return RUN_ALL_TESTS();
 }