From 24874b10cf527a461cd157d51c9acad472117ab2 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Tue, 13 Oct 2026 07:45:08 +0200 Subject: [PATCH] [GameSelector] Persist quick-filter toolbar edits and repair filter settings migration (#7500) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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 --- .../src/client/settings/cache_settings.cpp | 4 + .../widgets/server/game_filter_configs.h | 4 +- .../widgets/server/game_selector.cpp | 23 ++++ .../interface/widgets/server/game_selector.h | 5 + .../interface/widgets/server/games_model.cpp | 3 + .../settings/settings_migration.cpp | 113 +++++++++++++++--- .../settings/settings_migration.h | 9 ++ tests/settings/settings_migration_test.cpp | 65 ++++++++++ 8 files changed, 207 insertions(+), 19 deletions(-) diff --git a/cockatrice/src/client/settings/cache_settings.cpp b/cockatrice/src/client/settings/cache_settings.cpp index 15a41e7e1..08eccf1e4 100644 --- a/cockatrice/src/client/settings/cache_settings.cpp +++ b/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); diff --git a/cockatrice/src/interface/widgets/server/game_filter_configs.h b/cockatrice/src/interface/widgets/server/game_filter_configs.h index 1b7322c55..93ca010d8 100644 --- a/cockatrice/src/interface/widgets/server/game_filter_configs.h +++ b/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; diff --git a/cockatrice/src/interface/widgets/server/game_selector.cpp b/cockatrice/src/interface/widgets/server/game_selector.cpp index 27464e14e..6922bfe64 100644 --- a/cockatrice/src/interface/widgets/server/game_selector.cpp +++ b/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 &) { gameListProxyModel->refresh(); diff --git a/cockatrice/src/interface/widgets/server/game_selector.h b/cockatrice/src/interface/widgets/server/game_selector.h index 01e093b06..e3407abea 100644 --- a/cockatrice/src/interface/widgets/server/game_selector.h +++ b/cockatrice/src/interface/widgets/server/game_selector.h @@ -4,6 +4,7 @@ #include "game_type_map.h" #include +#include #include // 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. diff --git a/cockatrice/src/interface/widgets/server/games_model.cpp b/cockatrice/src/interface/widgets/server/games_model.cpp index 289dbee44..315d51d9e 100644 --- a/cockatrice/src/interface/widgets/server/games_model.cpp +++ b/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 diff --git a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp b/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp index c3f299f40..d9652cde3 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp +++ b/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 &renamedGameFilterKeys() +{ + static const QMap 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 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 &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 &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 &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; diff --git a/libcockatrice_settings/libcockatrice/settings/settings_migration.h b/libcockatrice_settings/libcockatrice/settings/settings_migration.h index 78f9c8a3f..0a706af98 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.h +++ b/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/) 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. diff --git a/tests/settings/settings_migration_test.cpp b/tests/settings/settings_migration_test.cpp index 440e49b74..2c4dcf7a9 100644 --- a/tests/settings/settings_migration_test.cpp +++ b/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();