From ce821a3356160749cffa850da66fe5232d4753df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= <44814898+BruebachL@users.noreply.github.com> Date: Sun, 8 Nov 2026 14:54:36 +0100 Subject: [PATCH] Address review findings --- .../widgets/dialogs/dlg_create_game.cpp | 8 +- .../game_hosting/create_game_form_widget.cpp | 23 ++---- tests/game_creation_options_test.cpp | 81 ++++++++++--------- 3 files changed, 52 insertions(+), 60 deletions(-) diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_create_game.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_create_game.cpp index 93ae17718..f5ba5458c 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_create_game.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_create_game.cpp @@ -23,9 +23,11 @@ DlgCreateGame::DlgCreateGame(TabRoom *_room, const QMap &_gameType const bool judge = room && (room->getUserInfo()->user_level() & ServerInfo_User::IsJudge); form = new CreateGameFormWidget(_gameTypes, registered, judge, this); - form->setRememberSettingsChecked(SettingsCache::instance().game().getRememberGameSettings()); - form->setOptions(GameCreationOptions::fromSettings(SettingsCache::instance().game(), _gameTypes, registered)); - if (!form->isRememberSettingsChecked()) { + const bool rememberSettings = SettingsCache::instance().game().getRememberGameSettings(); + form->setRememberSettingsChecked(rememberSettings); + if (rememberSettings) { + form->setOptions(GameCreationOptions::fromSettings(SettingsCache::instance().game(), _gameTypes, registered)); + } else { form->resetToDefaults(); } form->focusDescription(); diff --git a/cockatrice/src/interface/widgets/game_hosting/create_game_form_widget.cpp b/cockatrice/src/interface/widgets/game_hosting/create_game_form_widget.cpp index 64ce30e03..7d9e8e694 100644 --- a/cockatrice/src/interface/widgets/game_hosting/create_game_form_widget.cpp +++ b/cockatrice/src/interface/widgets/game_hosting/create_game_form_widget.cpp @@ -200,26 +200,13 @@ QString CreateGameFormWidget::selectedGameTypesSettingString() const void CreateGameFormWidget::resetToDefaults() { - descriptionEdit->setText(""); - maxPlayersEdit->setValue(2); - - passwordEdit->setText(""); - onlyBuddiesCheckBox->setChecked(false); - onlyRegisteredCheckBox->setChecked(registeredUser); - - spectatorsAllowedCheckBox->setChecked(true); - spectatorsNeedPasswordCheckBox->setChecked(false); - spectatorsCanTalkCheckBox->setChecked(false); - spectatorsSeeEverythingCheckBox->setChecked(false); - createGameAsSpectatorCheckBox->setChecked(false); - - startingLifeTotalEdit->setValue(20); - shareDecklistsOnLoadCheckBox->setChecked(false); - shufflePlayersCheckBox->setChecked(false); - createGameAsJudgeCheckBox->setChecked(false); + GameCreationOptions defaults; + defaults.onlyRegistered = registeredUser; + setOptions(defaults); + // An exclusive button group refuses to uncheck its selected button, which setOptions + // cannot do, so clear the exclusive flag while resetting the game type selection. for (auto *gameTypeCheckBox : gameTypeCheckBoxes) { - // must set auto exclusive to false to be able to set the check to false gameTypeCheckBox->setAutoExclusive(false); gameTypeCheckBox->setChecked(false); gameTypeCheckBox->setAutoExclusive(true); diff --git a/tests/game_creation_options_test.cpp b/tests/game_creation_options_test.cpp index 8e08ef6e7..e4a7996cb 100644 --- a/tests/game_creation_options_test.cpp +++ b/tests/game_creation_options_test.cpp @@ -7,16 +7,27 @@ namespace { +/** + * @brief The remembered settings a test hands to fromSettings. + * + * The fake cannot itself be an aggregate (it derives from a polymorphic interface), so the + * remembered values live in a GameCreationOptions member and the getters read them from + * there. That keeps the test data in one designated initializer. + */ class FakeGameSettings : public IGameSettingsProvider { public: + explicit FakeGameSettings(const GameCreationOptions &_options = {}) : options(_options) + { + } + [[nodiscard]] QString getGameDescription() const override { - return description; + return options.description; } [[nodiscard]] int getMaxPlayers() const override { - return maxPlayers; + return options.maxPlayers; } [[nodiscard]] QString getGameTypes() const override { @@ -24,43 +35,43 @@ public: } [[nodiscard]] bool getOnlyBuddies() const override { - return onlyBuddies; + return options.onlyBuddies; } [[nodiscard]] bool getOnlyRegistered() const override { - return onlyRegistered; + return options.onlyRegistered; } [[nodiscard]] bool getSpectatorsAllowed() const override { - return spectatorsAllowed; + return options.spectatorsAllowed; } [[nodiscard]] bool getSpectatorsNeedPassword() const override { - return spectatorsNeedPassword; + return options.spectatorsNeedPassword; } [[nodiscard]] bool getSpectatorsCanTalk() const override { - return spectatorsCanTalk; + return options.spectatorsCanTalk; } [[nodiscard]] bool getSpectatorsCanSeeEverything() const override { - return spectatorsCanSeeEverything; + return options.spectatorsSeeEverything; } [[nodiscard]] bool getCreateGameAsSpectator() const override { - return createGameAsSpectator; + return options.joinAsSpectator; } [[nodiscard]] int getDefaultStartingLifeTotal() const override { - return defaultStartingLifeTotal; + return options.startingLifeTotal; } [[nodiscard]] bool getShareDecklistsOnLoad() const override { - return shareDecklistsOnLoad; + return options.shareDecklistsOnLoad; } [[nodiscard]] bool getShufflePlayers() const override { - return shufflePlayers; + return options.shufflePlayers; } [[nodiscard]] bool getRememberGameSettings() const override { @@ -79,19 +90,9 @@ public: return 0; } - QString description; - int maxPlayers = 2; + GameCreationOptions options; + /// The legacy "Name, " list; it has no GameCreationOptions counterpart. QString gameTypes; - bool onlyBuddies = false; - bool onlyRegistered = false; - bool spectatorsAllowed = true; - bool spectatorsNeedPassword = false; - bool spectatorsCanTalk = false; - bool spectatorsCanSeeEverything = false; - bool createGameAsSpectator = false; - int defaultStartingLifeTotal = 20; - bool shareDecklistsOnLoad = false; - bool shufflePlayers = false; }; } // namespace @@ -186,19 +187,21 @@ TEST(GameCreationOptions, DescriptionWhitespaceIsSimplified) TEST(GameCreationOptions, FromSettingsCopiesEveryRememberedField) { - FakeGameSettings settings; - settings.description = "Remembered game"; - settings.maxPlayers = 6; - settings.onlyBuddies = true; - settings.onlyRegistered = true; - settings.spectatorsAllowed = false; - settings.spectatorsNeedPassword = true; - settings.spectatorsCanTalk = true; - settings.spectatorsCanSeeEverything = true; - settings.createGameAsSpectator = true; - settings.defaultStartingLifeTotal = 40; - settings.shareDecklistsOnLoad = true; - settings.shufflePlayers = true; + const FakeGameSettings settings{GameCreationOptions{.description = "Remembered game", + .password = {}, + .maxPlayers = 6, + .onlyBuddies = true, + .onlyRegistered = true, + .spectatorsAllowed = false, + .spectatorsNeedPassword = true, + .spectatorsCanTalk = true, + .spectatorsSeeEverything = true, + .joinAsJudge = false, + .joinAsSpectator = true, + .startingLifeTotal = 40, + .shareDecklistsOnLoad = true, + .shufflePlayers = true, + .gameTypeIds = {}}}; const GameCreationOptions options = GameCreationOptions::fromSettings(settings, {}, true); @@ -219,8 +222,8 @@ TEST(GameCreationOptions, FromSettingsCopiesEveryRememberedField) TEST(GameCreationOptions, FromSettingsDropsJoinRestrictionsForUnregisteredHosts) { FakeGameSettings settings; - settings.onlyBuddies = true; - settings.onlyRegistered = true; + settings.options.onlyBuddies = true; + settings.options.onlyRegistered = true; const GameCreationOptions options = GameCreationOptions::fromSettings(settings, {}, false);