Rework card menu handling (#6069)

* extract cardMenu from CardItem

* move cardMenu saving to TabGame

* delete TabGame::updateCardMenu

* move checking to updateCardMenu

* unset activeCard when all cards are unselected

cockatrice/src/client/tabs/tab_game.cpp

@@ -261,6 +261,10 @@ void TabGame::retranslateUi()
         sayLabel->setText(tr("&Say:"));
     }
 
+    if (aCardMenu) {
+        aCardMenu->setText(tr("Selected cards"));
+    }
+
     viewMenu->setTitle(tr("&View"));
     cardInfoDockMenu->setTitle(tr("Card Info"));
     messageLayoutDockMenu->setTitle(tr("Messages"));
@@ -576,6 +580,7 @@ Player *TabGame::addPlayer(int playerId, const ServerInfo_User &info)
     scene->addPlayer(newPlayer);
 
     connect(newPlayer, &Player::newCardAdded, this, &TabGame::newCardAdded);
+    connect(newPlayer, &Player::cardMenuUpdated, this, &TabGame::setCardMenu);
     messageLog->connectToPlayer(newPlayer);
 
     if (local && !spectator) {
@@ -1213,22 +1218,17 @@ Player *TabGame::getActiveLocalPlayer() const
 void TabGame::setActiveCard(CardItem *card)
 {
     activeCard = card;
-    updateCardMenu(card);
 }
 
-void TabGame::updateCardMenu(AbstractCardItem *card)
+/**
+ * @param menu The menu to set. Pass in nullptr to set the menu to empty.
+ */
+void TabGame::setCardMenu(QMenu *menu)
 {
-    if (card == nullptr) {
+    if (menu) {
-        return;
+        aCardMenu->setMenu(menu);
-    }
-    Player *player;
-    if ((clients.size() > 1) || !players.contains(localPlayerId)) {
-        player = card->getOwner();
     } else {
-        player = players.value(localPlayerId);
+        aCardMenu->setMenu(new QMenu);
-    }
-    if (player != nullptr) {
-        player->updateCardMenu(static_cast<CardItem *>(card));
     }
 }
 
@@ -1285,6 +1285,11 @@ void TabGame::createMenuItems()
     gameMenu->addAction(aConcede);
     gameMenu->addAction(aFocusChat);
     gameMenu->addAction(aLeaveGame);
+
+    gameMenu->addSeparator();
+
+    aCardMenu = gameMenu->addMenu(new QMenu(this));
+
     addTabMenu(gameMenu);
 }
 

cockatrice/src/client/tabs/tab_game.h

@@ -118,6 +118,7 @@ private:
         *aPlayerListDockVisible, *aPlayerListDockFloating, *aReplayDockVisible, *aReplayDockFloating;
     QAction *aFocusChat;
     QList<QAction *> phaseActions;
+    QAction *aCardMenu;
 
     Player *addPlayer(int playerId, const ServerInfo_User &info);
 
@@ -171,7 +172,7 @@ private slots:
     void incrementGameTime();
     void adminLockChanged(bool lock);
     void newCardAdded(AbstractCardItem *card);
-    void updateCardMenu(AbstractCardItem *card);
+    void setCardMenu(QMenu *menu);
 
     void actGameInfo();
     void actConcede();

cockatrice/src/game/board/card_item.cpp

@@ -24,30 +24,17 @@ CardItem::CardItem(Player *_owner, QGraphicsItem *parent, const CardRef &cardRef
 {
     owner->addCard(this);
 
-    cardMenu = new QMenu;
-    ptMenu = new QMenu;
-    moveMenu = new QMenu;
-
     connect(&SettingsCache::instance().cardCounters(), &CardCounterSettings::colorChanged, this, [this](int counterId) {
         if (counters.contains(counterId))
             update();
     });
-
-    retranslateUi();
-}
-
-CardItem::~CardItem()
-{
-    delete cardMenu;
-    delete ptMenu;
-    delete moveMenu;
 }
 
 void CardItem::prepareDelete()
 {
     if (owner != nullptr) {
-        if (owner->getCardMenu() == cardMenu) {
+        if (owner->getGame()->getActiveCard() == this) {
-            owner->setCardMenu(nullptr);
+            owner->updateCardMenu(nullptr);
             owner->getGame()->setActiveCard(nullptr);
         }
         owner = nullptr;
@@ -79,8 +66,6 @@ void CardItem::setZone(CardZone *_zone)
 
 void CardItem::retranslateUi()
 {
-    moveMenu->setTitle(tr("&Move to"));
-    ptMenu->setTitle(tr("&Power / toughness"));
 }
 
 void CardItem::paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget)
@@ -422,9 +407,13 @@ void CardItem::handleClickedToPlay(bool shiftHeld)
 void CardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
 {
     if (event->button() == Qt::RightButton) {
-        if (cardMenu != nullptr && !cardMenu->isEmpty() && owner != nullptr) {
+
-            cardMenu->popup(event->screenPos());
+        if (owner != nullptr) {
-            return;
+            owner->getGame()->setActiveCard(this);
+            if (QMenu *cardMenu = owner->updateCardMenu(this)) {
+                cardMenu->popup(event->screenPos());
+                return;
+            }
         }
     } else if ((event->modifiers() != Qt::AltModifier) && (event->button() == Qt::LeftButton) &&
                (!SettingsCache::instance().getDoubleClickToPlay())) {
@@ -477,11 +466,11 @@ QVariant CardItem::itemChange(GraphicsItemChange change, const QVariant &value)
 {
     if ((change == ItemSelectedHasChanged) && owner != nullptr) {
         if (value == true) {
-            owner->setCardMenu(cardMenu);
             owner->getGame()->setActiveCard(this);
-        } else if (owner->getCardMenu() == cardMenu) {
+            owner->updateCardMenu(this);
-            owner->setCardMenu(nullptr);
+        } else if (owner->scene()->selectedItems().isEmpty()) {
             owner->getGame()->setActiveCard(nullptr);
+            owner->updateCardMenu(nullptr);
         }
     }
     return AbstractCardItem::itemChange(change, value);

cockatrice/src/game/board/card_item.h

@@ -33,8 +33,6 @@ private:
     CardItem *attachedTo;
     QList<CardItem *> attachedCards;
 
-    QMenu *cardMenu, *ptMenu, *moveMenu;
-
     void prepareDelete();
     void handleClickedToPlay(bool shiftHeld);
 public slots:
@@ -54,7 +52,7 @@ public:
                       const CardRef &cardRef = {},
                       int _cardid = -1,
                       CardZone *_zone = nullptr);
-    ~CardItem() override;
+
     void retranslateUi();
     CardZone *getZone() const
     {
@@ -135,19 +133,6 @@ public:
     void resetState(bool keepAnnotations = false);
     void processCardInfo(const ServerInfo_Card &_info);
 
-    QMenu *getCardMenu() const
-    {
-        return cardMenu;
-    }
-    QMenu *getPTMenu() const
-    {
-        return ptMenu;
-    }
-    QMenu *getMoveMenu() const
-    {
-        return moveMenu;
-    }
-
     bool animationEvent();
     CardDragItem *createDragItem(int _id, const QPointF &_pos, const QPointF &_scenePos, bool faceDown);
     void deleteDragItem();

cockatrice/src/game/player/player.cpp

@@ -449,11 +449,6 @@ Player::Player(const ServerInfo_User &info, int _id, bool _local, bool _judge, T
         initSayMenu();
     }
 
-    aCardMenu = new QAction(this);
-    aCardMenu->setEnabled(false);
-    playerMenu->addSeparator();
-    playerMenu->addAction(aCardMenu);
-
     if (local || judge) {
 
         for (auto &playerList : playerLists) {
@@ -870,8 +865,6 @@ void Player::retranslateUi()
         }
     }
 
-    aCardMenu->setText(tr("Selec&ted cards"));
-
     if (local) {
         sayMenu->setTitle(tr("S&ay"));
     }
@@ -3904,20 +3897,12 @@ void Player::refreshShortcuts()
     }
 }
 
-void Player::updateCardMenu(const CardItem *card)
+QMenu *Player::createCardMenu(const CardItem *card)
 {
-    // If bad card OR is a spectator (as spectators don't need card menus), return
+    if (card == nullptr) {
-    // only update the menu if the card is actually selected
+        return nullptr;
-    if (card == nullptr || (game->isSpectator() && !judge) || game->getActiveCard() != card) {
-        return;
     }
 
-    QMenu *cardMenu = card->getCardMenu();
-    QMenu *ptMenu = card->getPTMenu();
-    QMenu *moveMenu = card->getMoveMenu();
-
-    cardMenu->clear();
-
     bool revealedCard = false;
     bool writeableCard = getLocalOrJudge();
     if (auto *view = qobject_cast<ZoneViewZone *>(card->getZone())) {
@@ -3930,6 +3915,8 @@ void Player::updateCardMenu(const CardItem *card)
         }
     }
 
+    QMenu *cardMenu = new QMenu;
+
     if (revealedCard) {
         cardMenu->addAction(aHide);
         cardMenu->addAction(aClone);
@@ -3940,18 +3927,6 @@ void Player::updateCardMenu(const CardItem *card)
     } else if (writeableCard) {
         bool canModifyCard = judge || card->getOwner() == this;
 
-        if (moveMenu->isEmpty() && canModifyCard) {
-            moveMenu->addAction(aMoveToTopLibrary);
-            moveMenu->addAction(aMoveToXfromTopOfLibrary);
-            moveMenu->addAction(aMoveToBottomLibrary);
-            moveMenu->addSeparator();
-            moveMenu->addAction(aMoveToHand);
-            moveMenu->addSeparator();
-            moveMenu->addAction(aMoveToGraveyard);
-            moveMenu->addSeparator();
-            moveMenu->addAction(aMoveToExile);
-        }
-
         if (card->getZone()) {
             if (card->getZone()->getName() == "table") {
                 // Card is on the battlefield
@@ -3967,23 +3942,7 @@ void Player::updateCardMenu(const CardItem *card)
                     cardMenu->addSeparator();
                     cardMenu->addAction(aSelectAll);
                     cardMenu->addAction(aSelectRow);
-                    return;
+                    return cardMenu;
-                }
-
-                if (ptMenu->isEmpty()) {
-                    ptMenu->addAction(aIncP);
-                    ptMenu->addAction(aDecP);
-                    ptMenu->addAction(aFlowP);
-                    ptMenu->addSeparator();
-                    ptMenu->addAction(aIncT);
-                    ptMenu->addAction(aDecT);
-                    ptMenu->addAction(aFlowT);
-                    ptMenu->addSeparator();
-                    ptMenu->addAction(aIncPT);
-                    ptMenu->addAction(aDecPT);
-                    ptMenu->addSeparator();
-                    ptMenu->addAction(aSetPT);
-                    ptMenu->addAction(aResetPT);
                 }
 
                 cardMenu->addAction(aTap);
@@ -4003,11 +3962,11 @@ void Player::updateCardMenu(const CardItem *card)
                 }
                 cardMenu->addAction(aDrawArrow);
                 cardMenu->addSeparator();
-                cardMenu->addMenu(ptMenu);
+                cardMenu->addMenu(createPtMenu());
                 cardMenu->addAction(aSetAnnotation);
                 cardMenu->addSeparator();
                 cardMenu->addAction(aClone);
-                cardMenu->addMenu(moveMenu);
+                cardMenu->addMenu(createMoveMenu());
                 cardMenu->addSeparator();
                 cardMenu->addAction(aSelectAll);
                 cardMenu->addAction(aSelectRow);
@@ -4031,7 +3990,7 @@ void Player::updateCardMenu(const CardItem *card)
                     cardMenu->addAction(aDrawArrow);
                     cardMenu->addSeparator();
                     cardMenu->addAction(aClone);
-                    cardMenu->addMenu(moveMenu);
+                    cardMenu->addMenu(createMoveMenu());
                     cardMenu->addSeparator();
                     cardMenu->addAction(aSelectAll);
                 } else {
@@ -4052,7 +4011,7 @@ void Player::updateCardMenu(const CardItem *card)
 
                     cardMenu->addSeparator();
                     cardMenu->addAction(aClone);
-                    cardMenu->addMenu(moveMenu);
+                    cardMenu->addMenu(createMoveMenu());
                     cardMenu->addSeparator();
                     cardMenu->addAction(aSelectAll);
                     cardMenu->addAction(aSelectColumn);
@@ -4082,7 +4041,7 @@ void Player::updateCardMenu(const CardItem *card)
 
                 cardMenu->addSeparator();
                 cardMenu->addAction(aClone);
-                cardMenu->addMenu(moveMenu);
+                cardMenu->addMenu(createMoveMenu());
 
                 // actions that are really wonky when done from deck or sideboard
                 if (card->getZone()->getName() == "hand") {
@@ -4103,7 +4062,7 @@ void Player::updateCardMenu(const CardItem *card)
                 }
             }
         } else {
-            cardMenu->addMenu(moveMenu);
+            cardMenu->addMenu(createMoveMenu());
         }
     } else {
         if (card->getZone() && card->getZone()->getName() != "hand") {
@@ -4117,6 +4076,42 @@ void Player::updateCardMenu(const CardItem *card)
             cardMenu->addAction(aSelectAll);
         }
     }
+
+    return cardMenu;
+}
+
+QMenu *Player::createMoveMenu() const
+{
+    QMenu *moveMenu = new QMenu("Move to");
+    moveMenu->addAction(aMoveToTopLibrary);
+    moveMenu->addAction(aMoveToXfromTopOfLibrary);
+    moveMenu->addAction(aMoveToBottomLibrary);
+    moveMenu->addSeparator();
+    moveMenu->addAction(aMoveToHand);
+    moveMenu->addSeparator();
+    moveMenu->addAction(aMoveToGraveyard);
+    moveMenu->addSeparator();
+    moveMenu->addAction(aMoveToExile);
+    return moveMenu;
+}
+
+QMenu *Player::createPtMenu() const
+{
+    QMenu *ptMenu = new QMenu("Power / toughness");
+    ptMenu->addAction(aIncP);
+    ptMenu->addAction(aDecP);
+    ptMenu->addAction(aFlowP);
+    ptMenu->addSeparator();
+    ptMenu->addAction(aIncT);
+    ptMenu->addAction(aDecT);
+    ptMenu->addAction(aFlowT);
+    ptMenu->addSeparator();
+    ptMenu->addAction(aIncPT);
+    ptMenu->addAction(aDecPT);
+    ptMenu->addSeparator();
+    ptMenu->addAction(aSetPT);
+    ptMenu->addAction(aResetPT);
+    return ptMenu;
 }
 
 void Player::addRelatedCardView(const CardItem *card, QMenu *cardMenu)
@@ -4226,23 +4221,30 @@ void Player::addRelatedCardActions(const CardItem *card, QMenu *cardMenu)
     }
 }
 
-void Player::setCardMenu(QMenu *menu)
+/**
+ * Creates a card menu from the given card and sets it as the currently active card menu.
+ * Will first check if the card should have a card menu, and no-ops if not.
+ *
+ * @param card The card to create the menu for. Pass nullptr to disable the card menu.
+ * @return The new card menu, or nullptr if failed.
+ */
+QMenu *Player::updateCardMenu(const CardItem *card)
 {
-    if (aCardMenu != nullptr) {
+    if (!card) {
-        aCardMenu->setEnabled(menu != nullptr);
+        emit cardMenuUpdated(nullptr);
-        if (menu) {
-            aCardMenu->setMenu(menu);
-        }
-    }
-}
-
-QMenu *Player::getCardMenu() const
-{
-    if (aCardMenu != nullptr) {
-        return aCardMenu->menu();
-    } else {
         return nullptr;
     }
+
+    // If is spectator (as spectators don't need card menus), return
+    // only update the menu if the card is actually selected
+    if ((game->isSpectator() && !judge) || game->getActiveCard() != card) {
+        return nullptr;
+    }
+
+    QMenu *menu = createCardMenu(card);
+    emit cardMenuUpdated(menu);
+
+    return menu;
 }
 
 QString Player::getName() const

cockatrice/src/game/player/player.h

@@ -156,6 +156,7 @@ signals:
 
     void sizeChanged();
     void playerCountChanged();
+    void cardMenuUpdated(QMenu *cardMenu);
 public slots:
     void actUntapAll();
     void actRollDie();
@@ -271,7 +272,6 @@ private:
         *aMoveBottomToPlayFaceDown, *aMoveBottomCardToTop, *aMoveBottomCardToGraveyard, *aMoveBottomCardToExile,
         *aMoveBottomCardsToGraveyard, *aMoveBottomCardsToExile, *aDrawBottomCard, *aDrawBottomCards;
 
-    QAction *aCardMenu;
     QList<QAction *> aAddCounter, aSetCounter, aRemoveCounter;
     QAction *aPlay, *aPlayFacedown, *aHide, *aTap, *aDoesntUntap, *aAttach, *aUnattach, *aDrawArrow, *aSetPT, *aResetPT,
         *aIncP, *aDecP, *aIncT, *aDecT, *aIncPT, *aDecPT, *aFlowP, *aFlowT, *aSetAnnotation, *aFlip, *aPeek, *aClone,
@@ -326,6 +326,8 @@ private:
                            const QString &avalue,
                            bool allCards,
                            EventProcessingOptions options);
+    QMenu *createMoveMenu() const;
+    QMenu *createPtMenu() const;
     void addRelatedCardActions(const CardItem *card, QMenu *cardMenu);
     void addRelatedCardView(const CardItem *card, QMenu *cardMenu);
     void createCard(const CardItem *sourceCard,
@@ -484,9 +486,8 @@ public:
     {
         return arrows;
     }
-    void setCardMenu(QMenu *menu);
+    QMenu *updateCardMenu(const CardItem *card);
-    QMenu *getCardMenu() const;
+    QMenu *createCardMenu(const CardItem *card);
-    void updateCardMenu(const CardItem *card);
     bool getActive() const
     {
         return active;