[GameSelector] Persist quick-filter toolbar edits and repair filter settings migration (#7500)

* [GameSelector] Persist quick-filter toolbar edits and repair filter settings migration

- Save game-filter parameters on toolbar changes, debounced with flush on close
- Repair pre-camelCase gamefilters.ini keys in place on startup
- Map the legacy creator-name filter to hostNameFilter
- Balance begin/endFilterChange in GamesProxyModel::refresh
- Align the hideIgnoredUserGames default between model and settings
- Cover the migration with regression tests

* [Review] Address RickyRister comments on filter persistence

- Rename game-type prefix constants to SCREAMING_SNAKE to match
  MIGRATION_SENTINEL_KEY and project style
- Document why GameSelector destructor flushes a pending debounced
  filter save (toolbar path has no other writer)

---------

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

cockatrice/src/client/settings/cache_settings.cpp

@@ -133,6 +133,10 @@ SettingsCache::SettingsCache()
     // Migrate settings from global.ini to per-class INI files (runs at most once)
     SettingsMigration::migrateSettingsFromGlobalIni(settingsPath);
 
+    // Repair gamefilters.ini keys renamed to camelCase by #7084 for profiles that migrated
+    // before that rename (idempotent, safe to run on every startup, portable builds included).
+    SettingsMigration::migrateGameFiltersToCamelCase(settingsPath);
+
     shortcutsSettings = new ShortcutsSettings(settingsPath, this);
     cardDatabaseSettings = new CardDatabaseSettings(settingsPath, this);
     serversSettings = new ServersSettings(settingsPath, this);

cockatrice/src/interface/widgets/server/game_filter_configs.h

@@ -17,7 +17,9 @@ struct GameFilterConfigs
     static constexpr int DEFAULT_MAX_PLAYERS_MAX = 99;
 
     bool hideBuddiesOnlyGames = false;
-    bool hideIgnoredUserGames = false;
+    // Defaults to true to match GameFiltersSettings::isHideIgnoredUserGames(), so freshly loaded
+    // filters compare equal to a default-constructed config.
+    bool hideIgnoredUserGames = true;
     bool hideFullGames = false;
     bool hideGamesThatStarted = false;
     bool hidePasswordProtectedGames = false;

cockatrice/src/interface/widgets/server/game_selector.cpp

@@ -113,6 +113,17 @@ GameSelector::GameSelector(AbstractClient *_client,
                     quickFilterToolBar->setVisible(
                         SettingsCache::instance().userInterface().getShowGameSelectorFilterToolbar());
                 });
+        // The quick-filter toolbar only updates the in-memory model, so persist every change.
+        // Otherwise each new GameSelector (room switch, reconnect, restart) would revert to stale
+        // saved values and e.g. hide full/started games the user had unchecked. The save is
+        // debounced so typing in the search field does not rewrite the settings file on every
+        // keystroke; a pending save is flushed in the destructor.
+        filterSaveDebounceTimer.setSingleShot(true);
+        filterSaveDebounceTimer.setInterval(500);
+        connect(&filterSaveDebounceTimer, &QTimer::timeout, this,
+                [this] { gameListProxyModel->saveFilterParameters(gameTypeMap); });
+        connect(gameListProxyModel, &GamesProxyModel::filtersChanged, this,
+                [this] { filterSaveDebounceTimer.start(); });
     } else {
         quickFilterToolBar = nullptr;
     }
@@ -187,6 +198,18 @@ GameSelector::GameSelector(AbstractClient *_client,
     connect(client, &AbstractClient::removeFromListEventReceived, this, &GameSelector::processRemoveFromListEvent);
 }
 
+GameSelector::~GameSelector()
+{
+    // Nothing else persists quick-filter toolbar edits: the toolbar only updates the in-memory model,
+    // and the debounced timer above is the sole writer for that path (actSetFilter/actClearFilter save
+    // synchronously, but the toolbar does not). Flush a pending debounced save so edits made within
+    // the debounce interval before destruction are not lost on room switch, reconnect, or close.
+    if (filterSaveDebounceTimer.isActive()) {
+        filterSaveDebounceTimer.stop();
+        gameListProxyModel->saveFilterParameters(gameTypeMap);
+    }
+}
+
 void GameSelector::ignoreListReceived(const QList<ServerInfo_User> &)
 {
     gameListProxyModel->refresh();

cockatrice/src/interface/widgets/server/game_selector.h

@@ -4,6 +4,7 @@
 #include "game_type_map.h"
 
 #include <QGroupBox>
+#include <QTimer>
 #include <qtmetamacros.h>
 // IWYU pragma: keep
 // ServerInfo_User appears by value and inside QList in slot signatures, so the
@@ -148,6 +149,9 @@ private:
     const bool showFilters;  /**< Determines whether filter buttons are displayed. */
     GameTypeMap gameTypeMap; /**< Mapping of game types for the current room. */
 
+    QTimer filterSaveDebounceTimer; /**< Delays persisting quick-filter edits so typing does not rewrite the settings
+                                       file on every keystroke. */
+
     /**
      * @brief Updates the widget title to reflect the current number of displayed games.
      *
@@ -211,6 +215,7 @@ public:
                  const bool restoresettings,
                  const bool _showfilters,
                  QWidget *parent = nullptr);
+    ~GameSelector() override;
 
     /**
      * @brief Updates UI text for translation/localization.

cockatrice/src/interface/widgets/server/games_model.cpp

@@ -523,6 +523,9 @@ bool GamesProxyModel::filterAcceptsRow(int sourceRow) const
 
 void GamesProxyModel::refresh()
 {
+#if (QT_VERSION >= QT_VERSION_CHECK(6, 9, 0))
+    beginFilterChange();
+#endif
 #if (QT_VERSION >= QT_VERSION_CHECK(6, 10, 0))
     endFilterChange(QSortFilterProxyModel::Direction::Rows);
 #else

libcockatrice_settings/libcockatrice/settings/settings_migration.cpp

@@ -642,6 +642,34 @@ static void migrateLegacyMessages(const QString &settingsPath)
     legacySetting.endGroup();
 }
 
+/**
+ * gamefilters.ini keys renamed to camelCase by #7084. Profiles that migrated from the legacy
+ * native store before that rename still hold the old snake_case keys, which nothing reads
+ * anymore, so their saved filters silently stopped loading.
+ */
+static const QMap<QString, QString> &renamedGameFilterKeys()
+{
+    static const QMap<QString, QString> keyMap = {
+        {"hide_buddies_only_games", "hideBuddiesOnlyGames"},
+        {"hide_full_games", "hideFullGames"},
+        {"hide_games_that_started", "hideGamesThatStarted"},
+        {"hide_password_protected_games", "hidePasswordProtectedGames"},
+        {"hide_ignored_user_games", "hideIgnoredUserGames"},
+        {"hide_not_buddy_created_games", "hideNotBuddyCreatedGames"},
+        {"hide_open_decklist_games", "hideOpenDecklistGames"},
+        {"game_name_filter", "gameNameFilter"},
+        {"creator_name_filter", "hostNameFilter"},
+        {"min_players", "minPlayers"},
+        {"max_players", "maxPlayers"},
+        {"max_game_age_time", "maxGameAgeTime"},
+        {"show_only_if_spectators_can_watch", "showOnlyIfSpectatorsCanWatch"},
+        {"show_spectator_password_protected", "showSpectatorPasswordProtected"},
+        {"show_only_if_spectators_can_chat", "showOnlyIfSpectatorsCanChat"},
+        {"show_only_if_spectators_can_see_hands", "showOnlyIfSpectatorsCanSeeHands"},
+    };
+    return keyMap;
+}
+
 static void migrateLegacyGameFilters(const QString &settingsPath)
 {
     QSettings legacySetting;
@@ -652,24 +680,7 @@ static void migrateLegacyGameFilters(const QString &settingsPath)
         return;
     }
 
-    const QMap<QString, QString> filterKeyMap = {
-        {"hide_buddies_only_games", "hideBuddiesOnlyGames"},
-        {"hide_full_games", "hideFullGames"},
-        {"hide_games_that_started", "hideGamesThatStarted"},
-        {"hide_password_protected_games", "hidePasswordProtectedGames"},
-        {"hide_ignored_user_games", "hideIgnoredUserGames"},
-        {"hide_not_buddy_created_games", "hideNotBuddyCreatedGames"},
-        {"hide_open_decklist_games", "hideOpenDecklistGames"},
-        {"game_name_filter", "gameNameFilter"},
-        {"creator_name_filter", "creatorNameFilter"},
-        {"min_players", "minPlayers"},
-        {"max_players", "maxPlayers"},
-        {"max_game_age_time", "maxGameAgeTime"},
-        {"show_only_if_spectators_can_watch", "showOnlyIfSpectatorsCanWatch"},
-        {"show_spectator_password_protected", "showSpectatorPasswordProtected"},
-        {"show_only_if_spectators_can_chat", "showOnlyIfSpectatorsCanChat"},
-        {"show_only_if_spectators_can_see_hands", "showOnlyIfSpectatorsCanSeeHands"},
-    };
+    const QMap<QString, QString> &filterKeyMap = renamedGameFilterKeys();
 
     QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat);
     for (auto it = filterKeyMap.constBegin(); it != filterKeyMap.constEnd(); ++it) {
@@ -686,6 +697,68 @@ static void migrateLegacyGameFilters(const QString &settingsPath)
     legacySetting.endGroup();
 }
 
+static const QString LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX = QStringLiteral("filter_games/game_type/");
+static const QString GAME_FILTERS_GAME_TYPE_PREFIX = QStringLiteral("filter_games/gameType/");
+
+static bool gameFiltersCamelCaseMigrationPending(const QString &settingsPath)
+{
+    if (!QFile::exists(settingsPath + "gamefilters.ini")) {
+        return false;
+    }
+
+    QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat);
+    const QMap<QString, QString> &keyMap = renamedGameFilterKeys();
+    for (auto it = keyMap.constBegin(); it != keyMap.constEnd(); ++it) {
+        if (filtersIni.contains("filter_games/" + it.key()) && !filtersIni.contains("filter_games/" + it.value())) {
+            return true;
+        }
+    }
+    // Orphaned by #7084, which migrated the legacy host-name filter to a key nothing reads.
+    if (filtersIni.contains("filter_games/creatorNameFilter") && !filtersIni.contains("filter_games/hostNameFilter")) {
+        return true;
+    }
+    for (const auto &key : filtersIni.allKeys()) {
+        if (key.startsWith(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX) &&
+            !filtersIni.contains(GAME_FILTERS_GAME_TYPE_PREFIX +
+                                 key.mid(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX.size()))) {
+            return true;
+        }
+    }
+    return false;
+}
+
+bool SettingsMigration::migrateGameFiltersToCamelCase(const QString &settingsPath)
+{
+    if (!gameFiltersCamelCaseMigrationPending(settingsPath)) {
+        return false;
+    }
+
+    QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat);
+    const QMap<QString, QString> &keyMap = renamedGameFilterKeys();
+    for (auto it = keyMap.constBegin(); it != keyMap.constEnd(); ++it) {
+        const QString oldKey = "filter_games/" + it.key();
+        const QString newKey = "filter_games/" + it.value();
+        if (filtersIni.contains(oldKey) && !filtersIni.contains(newKey)) {
+            filtersIni.setValue(newKey, filtersIni.value(oldKey));
+        }
+    }
+    if (filtersIni.contains("filter_games/creatorNameFilter") && !filtersIni.contains("filter_games/hostNameFilter")) {
+        filtersIni.setValue("filter_games/hostNameFilter", filtersIni.value("filter_games/creatorNameFilter"));
+    }
+    for (const auto &key : filtersIni.allKeys()) {
+        if (key.startsWith(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX)) {
+            const QString newKey = GAME_FILTERS_GAME_TYPE_PREFIX + key.mid(LEGACY_GAME_FILTERS_GAME_TYPE_PREFIX.size());
+            if (!filtersIni.contains(newKey)) {
+                filtersIni.setValue(newKey, filtersIni.value(key));
+            }
+        }
+    }
+    // The old keys are deliberately kept: pre-#7084 builds still read them, so removing them
+    // would break downgrades, and nothing in the current code reads them anymore.
+    filtersIni.sync();
+    return true;
+}
+
 bool SettingsMigration::migrateLegacySettings(const QString &settingsPath)
 {
     QSettings personalIni(settingsPath + "personal.ini", QSettings::IniFormat);
@@ -744,6 +817,10 @@ bool SettingsMigration::isMigrationPending(const QString &settingsPath)
         return true;
     }
 
+    if (gameFiltersCamelCaseMigrationPending(settingsPath)) {
+        return true;
+    }
+
     QSettings personalIni(settingsPath + "personal.ini", QSettings::IniFormat);
     if (personalIni.value("migration/legacy_complete", false).toBool()) {
         return false;

libcockatrice_settings/libcockatrice/settings/settings_migration.h

@@ -9,6 +9,15 @@ public:
     static bool migrateSettingsFromGlobalIni(const QString &settingsPath);
     static bool migrateLegacySettings(const QString &settingsPath);
 
+    /**
+     * Moves pre-camelCase gamefilters.ini keys (snake_case, game_type/<hash>) to the camelCase
+     * keys used since #7084, for profiles that already migrated before that rename and therefore
+     * never pass through migrateLegacySettings again. Copies values only where the new key is
+     * absent, never deletes the old keys, and returns true when anything was copied. Idempotent,
+     * so it runs on every startup.
+     */
+    static bool migrateGameFiltersToCamelCase(const QString &settingsPath);
+
     /**
      * Returns true when at least one of the two migrations would still do work, i.e. when a
      * backup of the settings directory is worth taking before they run.

tests/settings/settings_migration_test.cpp

@@ -705,6 +705,71 @@ TEST_F(SettingsMigrationTest, LegacyMigrationCamelCasesKeys)
     ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/gameType/deadbeef"), QVariant(true));
 }
 
+TEST_F(SettingsMigrationTest, LegacyMigrationMapsCreatorNameFilterToHostNameFilter)
+{
+    {
+        QSettings nativeSettings;
+        nativeSettings.setValue("filter_games/creator_name_filter", QStringList({"Alice", "Bob"}));
+        nativeSettings.sync();
+    }
+
+    ASSERT_TRUE(SettingsMigration::migrateLegacySettings(settingsPath));
+
+    ASSERT_TRUE(fileExists("gamefilters.ini"));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hostNameFilter"), QVariant(QStringList({"Alice", "Bob"})));
+}
+
+TEST_F(SettingsMigrationTest, GameFiltersIniSnakeCaseMigratesToCamelCase)
+{
+    {
+        QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat);
+        filtersIni.setValue("filter_games/hide_full_games", true);
+        filtersIni.setValue("filter_games/hide_games_that_started", true);
+        filtersIni.setValue("filter_games/game_name_filter", "test game");
+        filtersIni.setValue("filter_games/creator_name_filter", QStringList({"Alice"}));
+        filtersIni.setValue("filter_games/game_type/deadbeef", true);
+        filtersIni.sync();
+    }
+
+    ASSERT_TRUE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath));
+
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hideFullGames"), QVariant(true));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hideGamesThatStarted"), QVariant(true));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/gameNameFilter"), QVariant("test game"));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hostNameFilter"), QVariant(QStringList({"Alice"})));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/gameType/deadbeef"), QVariant(true));
+
+    // Idempotent: a second run has nothing left to do
+    ASSERT_FALSE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath));
+}
+
+TEST_F(SettingsMigrationTest, GameFiltersIniOrphanCreatorNameFilterMigratesToHostName)
+{
+    {
+        QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat);
+        filtersIni.setValue("filter_games/creatorNameFilter", QStringList({"Alice"}));
+        filtersIni.sync();
+    }
+
+    ASSERT_TRUE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hostNameFilter"), QVariant(QStringList({"Alice"})));
+    ASSERT_FALSE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath));
+}
+
+TEST_F(SettingsMigrationTest, GameFiltersIniKeepsNewerCamelCaseValues)
+{
+    {
+        QSettings filtersIni(settingsPath + "gamefilters.ini", QSettings::IniFormat);
+        filtersIni.setValue("filter_games/hide_full_games", true);
+        filtersIni.setValue("filter_games/hideFullGames", false);
+        filtersIni.sync();
+    }
+
+    // Nothing to do: the current-format key already exists and must win
+    ASSERT_FALSE(SettingsMigration::migrateGameFiltersToCamelCase(settingsPath));
+    ASSERT_EQ(readFromIni("gamefilters.ini", "filter_games/hideFullGames"), QVariant(false));
+}
+
 TEST_F(SettingsMigrationTest, LegacyMigrationEmptyNativeFormatWritesSentinel)
 {
     QSettings().clear();