From beea819b896e0e1afd675d0135b04e7b6fd3fe70 Mon Sep 17 00:00:00 2001 From: DawnFire42 Date: Tue, 9 Jun 2026 15:29:22 -0400 Subject: [PATCH] Code cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add missing isCommandZoneCounterBlocked check to cmdSetCounterActive - Revert accidental deck view layout change from b4057a86 - Fix duplicate @param in playSelectedCardsImpl doc - Add null check for PlayerLogic in CommandZoneMenu constructor - Add index bounds check in CommandZone handleDropEvent - Add index bounds check in StackZone handleDropEvent - Add ownership comment for tax counter widget creation - Add command zone to zoneGraphicsItems map - Conditionally show command zone menu item based on server support - Remove layer-violating includes from player_logic.cpp - Fix tax counter increment (1 per cast, not 2) - Add getTaxCounterIfActive() helper to PlayerGraphicsItem --- cockatrice/src/game/board/counter_state.cpp | 2 +- cockatrice/src/game/player/player_actions.cpp | 5 ++-- cockatrice/src/game/player/player_actions.h | 5 ++-- cockatrice/src/game/player/player_logic.cpp | 2 -- .../game_graphics/player/menu/card_menu.cpp | 6 ++--- .../player/menu/command_zone_menu.cpp | 26 +++++++++---------- .../game_graphics/player/menu/move_menu.cpp | 5 ++++ .../player/player_graphics_item.cpp | 9 +++++++ .../player/player_graphics_item.h | 2 ++ .../src/game_graphics/zones/command_zone.cpp | 3 ++- .../src/game_graphics/zones/stack_zone.cpp | 3 ++- .../src/interface/widgets/tabs/tab_game.cpp | 2 +- .../server/remote/game/server_player.cpp | 4 +++ 13 files changed, 45 insertions(+), 29 deletions(-) diff --git a/cockatrice/src/game/board/counter_state.cpp b/cockatrice/src/game/board/counter_state.cpp index b8377102f..116de4a8d 100644 --- a/cockatrice/src/game/board/counter_state.cpp +++ b/cockatrice/src/game/board/counter_state.cpp @@ -37,4 +37,4 @@ void CounterState::setActive(bool newActive) } active = newActive; emit activeChanged(newActive); -} \ No newline at end of file +} diff --git a/cockatrice/src/game/player/player_actions.cpp b/cockatrice/src/game/player/player_actions.cpp index 681641093..7c62696a1 100644 --- a/cockatrice/src/game/player/player_actions.cpp +++ b/cockatrice/src/game/player/player_actions.cpp @@ -1657,7 +1657,7 @@ void PlayerActions::actPlayAndIncreaseTax(QList selectedCards) if (originalZone == ZoneNames::COMMAND) { CounterState *state = player->getCounters().value(CounterIds::CommanderTax, nullptr); if (state && state->isActive()) { - sendIncCounter(CounterIds::CommanderTax, 2); + sendIncCounter(CounterIds::CommanderTax, 1); } } }); @@ -1669,7 +1669,7 @@ void PlayerActions::actPlayAndIncreasePartnerTax(QList selectedCards if (originalZone == ZoneNames::COMMAND) { CounterState *state = player->getCounters().value(CounterIds::PartnerTax, nullptr); if (state && state->isActive()) { - sendIncCounter(CounterIds::PartnerTax, 2); + sendIncCounter(CounterIds::PartnerTax, 1); } } }); @@ -1695,6 +1695,7 @@ void PlayerActions::actModifyTaxCounter(int counterId, int delta) void PlayerActions::actToggleTaxCounter(int counterId) { CounterState *state = player->getCounters().value(counterId, nullptr); + // Prevent disabling a counter with tax accumulated; player must reset to 0 first if (!state || (state->isActive() && state->getValue() != 0)) { return; } diff --git a/cockatrice/src/game/player/player_actions.h b/cockatrice/src/game/player/player_actions.h index c8ef1d359..282a80a5c 100644 --- a/cockatrice/src/game/player/player_actions.h +++ b/cockatrice/src/game/player/player_actions.h @@ -257,9 +257,8 @@ private: /** * @brief Shared implementation for playing selected cards with an optional post-play callback. - * @param selectedCards - * @param selectedCards - * @param selectedCards + * @param selectedCards Cards to play + * @param faceDown Whether to play cards face-down * @param postPlayCallback Called after each card is played, receiving the card and its *original* zone name * (captured before playCard, since playCard sends a move command that may change the card's zone). */ diff --git a/cockatrice/src/game/player/player_logic.cpp b/cockatrice/src/game/player/player_logic.cpp index aabc014b3..9e88f7ee3 100644 --- a/cockatrice/src/game/player/player_logic.cpp +++ b/cockatrice/src/game/player/player_logic.cpp @@ -2,11 +2,9 @@ #include "../../game_graphics/board/arrow_item.h" #include "../../game_graphics/board/card_item.h" -#include "../../game_graphics/board/commander_tax_counter.h" #include "../../game_graphics/board/counter_general.h" #include "../../game_graphics/game_scene.h" #include "../../game_graphics/player/player_target.h" -#include "../../game_graphics/zones/command_zone.h" #include "../../game_graphics/zones/hand_zone.h" #include "../../game_graphics/zones/pile_zone.h" #include "../../game_graphics/zones/stack_zone.h" diff --git a/cockatrice/src/game_graphics/player/menu/card_menu.cpp b/cockatrice/src/game_graphics/player/menu/card_menu.cpp index 67c5ad716..736af8353 100644 --- a/cockatrice/src/game_graphics/player/menu/card_menu.cpp +++ b/cockatrice/src/game_graphics/player/menu/card_menu.cpp @@ -165,13 +165,11 @@ CardMenu::CardMenu(PlayerGraphicsItem *_player, const CardItem *_card, bool _sho if (writeableCard) { addAction(aPlay); - AbstractCounter *cmdTax = player->getCounterWidget(CounterIds::CommanderTax); - if (cmdTax && cmdTax->isActive()) { + if (player->getTaxCounterIfActive(CounterIds::CommanderTax)) { addAction(aPlayAndIncreaseTax); } - AbstractCounter *partnerTax = player->getCounterWidget(CounterIds::PartnerTax); - if (partnerTax && partnerTax->isActive()) { + if (player->getTaxCounterIfActive(CounterIds::PartnerTax)) { addAction(aPlayAndIncreasePartnerTax); } diff --git a/cockatrice/src/game_graphics/player/menu/command_zone_menu.cpp b/cockatrice/src/game_graphics/player/menu/command_zone_menu.cpp index dd2aa621f..ee9bf219a 100644 --- a/cockatrice/src/game_graphics/player/menu/command_zone_menu.cpp +++ b/cockatrice/src/game_graphics/player/menu/command_zone_menu.cpp @@ -26,7 +26,8 @@ CommandZoneMenu::CommandZoneMenu(PlayerGraphicsItem *_player, QMenu *playerMenu) } }); - if (player->getLogic()->getPlayerInfo()->getLocalOrJudge()) { + PlayerLogic *logic = player->getLogic(); + if (logic && logic->getPlayerInfo()->getLocalOrJudge()) { addAction(aViewZone); addSeparator(); @@ -117,32 +118,29 @@ void CommandZoneMenu::actToggleMinimized() void CommandZoneMenu::updateTaxCounterActionStates() { - AbstractCounter *cmdTax = player->getCounterWidget(CounterIds::CommanderTax); - bool cmdActive = cmdTax != nullptr && cmdTax->isActive(); - - AbstractCounter *partnerTax = player->getCounterWidget(CounterIds::PartnerTax); - bool partnerActive = partnerTax != nullptr && partnerTax->isActive(); + AbstractCounter *cmdTax = player->getTaxCounterIfActive(CounterIds::CommanderTax); + AbstractCounter *partnerTax = player->getTaxCounterIfActive(CounterIds::PartnerTax); if (aIncreaseCommanderTax) { - aIncreaseCommanderTax->setVisible(cmdActive); + aIncreaseCommanderTax->setVisible(cmdTax != nullptr); } if (aDecreaseCommanderTax) { - aDecreaseCommanderTax->setVisible(cmdActive); + aDecreaseCommanderTax->setVisible(cmdTax != nullptr); } if (aToggleCommanderTaxCounter) { - aToggleCommanderTaxCounter->setText(cmdActive ? tr("&Remove Commander Tax") : tr("&Add Commander Tax")); - aToggleCommanderTaxCounter->setVisible(!cmdActive || (cmdTax && cmdTax->getValue() == 0)); + aToggleCommanderTaxCounter->setText(cmdTax ? tr("&Remove Commander Tax") : tr("&Add Commander Tax")); + aToggleCommanderTaxCounter->setVisible(!cmdTax || cmdTax->getValue() == 0); } if (aIncreasePartnerTax) { - aIncreasePartnerTax->setVisible(partnerActive); + aIncreasePartnerTax->setVisible(partnerTax != nullptr); } if (aDecreasePartnerTax) { - aDecreasePartnerTax->setVisible(partnerActive); + aDecreasePartnerTax->setVisible(partnerTax != nullptr); } if (aTogglePartnerTaxCounter) { - aTogglePartnerTaxCounter->setText(partnerActive ? tr("R&emove Partner Tax") : tr("&Add Partner Tax")); - aTogglePartnerTaxCounter->setVisible(!partnerActive || (partnerTax && partnerTax->getValue() == 0)); + aTogglePartnerTaxCounter->setText(partnerTax ? tr("R&emove Partner Tax") : tr("&Add Partner Tax")); + aTogglePartnerTaxCounter->setVisible(!partnerTax || partnerTax->getValue() == 0); } } diff --git a/cockatrice/src/game_graphics/player/menu/move_menu.cpp b/cockatrice/src/game_graphics/player/menu/move_menu.cpp index 2c345a81c..becbc3625 100644 --- a/cockatrice/src/game_graphics/player/menu/move_menu.cpp +++ b/cockatrice/src/game_graphics/player/menu/move_menu.cpp @@ -54,6 +54,11 @@ MoveMenu::MoveMenu(PlayerGraphicsItem *player) : QMenu(tr("Move to")) addSeparator(); addAction(aMoveToCommandZone); + auto *playerLogic = player->getLogic(); + auto updateCommandZoneVisibility = [this](bool has) { aMoveToCommandZone->setVisible(has); }; + connect(playerLogic, &PlayerLogic::commandZoneSupportChanged, this, updateCommandZoneVisibility); + updateCommandZoneVisibility(playerLogic->hasServerCommandZone()); + setShortcutsActive(); retranslateUi(); diff --git a/cockatrice/src/game_graphics/player/player_graphics_item.cpp b/cockatrice/src/game_graphics/player/player_graphics_item.cpp index 2f9ac91a6..b586ab386 100644 --- a/cockatrice/src/game_graphics/player/player_graphics_item.cpp +++ b/cockatrice/src/game_graphics/player/player_graphics_item.cpp @@ -143,6 +143,7 @@ void PlayerGraphicsItem::initializeZones() zoneGraphicsItems.insert(player->getTableZone()->getName(), tableZoneGraphicsItem); zoneGraphicsItems.insert(player->getStackZone()->getName(), stackZoneGraphicsItem); zoneGraphicsItems.insert(player->getHandZone()->getName(), handZoneGraphicsItem); + zoneGraphicsItems.insert(player->getCommandZone()->getName(), commandZoneGraphicsItem); } void PlayerGraphicsItem::onCustomZoneAdded(QString customZoneName) @@ -203,6 +204,8 @@ void PlayerGraphicsItem::onCounterAdded(CounterState *state) qWarning() << "Cannot create tax counter" << state->getName() << "- command zone not available"; return; } + // Qt parent (commandZoneGraphicsItem) owns widget; counterWidgets map holds reference + // for lookup; CommandZone::registerTaxCounter connects QObject::destroyed for cleanup widget = new CommanderTaxCounter(state, player, commandZoneGraphicsItem); widget->setActive(state->isActive()); commandZoneGraphicsItem->registerTaxCounter(widget); @@ -267,6 +270,12 @@ QList PlayerGraphicsItem::getTaxCounterWidgets() const return result; } +AbstractCounter *PlayerGraphicsItem::getTaxCounterIfActive(int counterId) const +{ + AbstractCounter *counter = getCounterWidget(counterId); + return (counter && counter->isActive()) ? counter : nullptr; +} + void PlayerGraphicsItem::rearrangeZones() { auto base = QPointF(CardDimensions::HEIGHT_F + counterAreaWidth + 15, 0); diff --git a/cockatrice/src/game_graphics/player/player_graphics_item.h b/cockatrice/src/game_graphics/player/player_graphics_item.h index 47c5fafba..87fb94e26 100644 --- a/cockatrice/src/game_graphics/player/player_graphics_item.h +++ b/cockatrice/src/game_graphics/player/player_graphics_item.h @@ -125,6 +125,8 @@ public: } /** @brief Returns all tax counter widgets (commander tax and partner tax). */ [[nodiscard]] QList getTaxCounterWidgets() const; + /** @brief Returns the tax counter if it exists and is active, or nullptr otherwise. */ + [[nodiscard]] AbstractCounter *getTaxCounterIfActive(int counterId) const; public slots: void onPlayerActiveChanged(bool _active); diff --git a/cockatrice/src/game_graphics/zones/command_zone.cpp b/cockatrice/src/game_graphics/zones/command_zone.cpp index 69824499a..72451e5c3 100644 --- a/cockatrice/src/game_graphics/zones/command_zone.cpp +++ b/cockatrice/src/game_graphics/zones/command_zone.cpp @@ -96,7 +96,8 @@ void CommandZone::handleDropEvent(const QList &dragItems, // Same-zone no-op: don't move a card onto itself const auto &cards = getLogic()->getCards(); - if (!cards.isEmpty() && startZone == getLogic() && cards.at(index)->getId() == dragItems.at(0)->getId()) { + if (!cards.isEmpty() && index < cards.size() && startZone == getLogic() && + cards.at(index)->getId() == dragItems.at(0)->getId()) { return; } diff --git a/cockatrice/src/game_graphics/zones/stack_zone.cpp b/cockatrice/src/game_graphics/zones/stack_zone.cpp index c3015496c..877312232 100644 --- a/cockatrice/src/game_graphics/zones/stack_zone.cpp +++ b/cockatrice/src/game_graphics/zones/stack_zone.cpp @@ -51,7 +51,8 @@ void StackZone::handleDropEvent(const QList &dragItems, // Same-zone no-op: don't move a card onto itself const auto &cards = getLogic()->getCards(); - if (!cards.isEmpty() && startZone == getLogic() && cards.at(index)->getId() == dragItems.at(0)->getId()) { + if (!cards.isEmpty() && index < cards.size() && startZone == getLogic() && + cards.at(index)->getId() == dragItems.at(0)->getId()) { return; } diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.cpp b/cockatrice/src/interface/widgets/tabs/tab_game.cpp index a6fc3f151..a81161e83 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_game.cpp @@ -690,7 +690,7 @@ void TabGame::addLocalPlayer(PlayerLogic *newPlayer, int playerId) auto *deckView = new TabbedDeckViewContainer(playerId, this); connect(deckView->playerDeckView, &DeckViewContainer::newCardAdded, this, &TabGame::newCardAdded); deckViewContainers.insert(playerId, deckView); - deckViewContainerLayout->insertWidget(0, deckView, 1); + deckViewContainerLayout->addWidget(deckView); // auto load deck for player if that debug setting is enabled QString deckPath = SettingsCache::instance().debug().getDeckPathForPlayer(newPlayer->getPlayerInfo()->getName()); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp index 017b7e79b..9d5ba3de5 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_player.cpp @@ -578,6 +578,10 @@ Response::ResponseCode Server_Player::cmdSetCounterActive(const Command_SetCount } const int counterId = cmd.counter_id(); + if (isCommandZoneCounterBlocked(counterId)) { + return Response::RespContextError; + } + Server_Counter *c = counters.value(counterId, nullptr); if (!c) { return Response::RespNameNotFound;