Fix for poor performance with large decks (#4347)

* Fix for #4284

-> The menus have the update menu thing emitted when they get triggered.
| -> works surprising well https://youtu.be/KOOmhxvHA2c is a demo on a 10000ish card deck

* changed my comment to make sense

* fix to the issues that @ebbit1q found

what caued them idk

* Revert "fix to the issues that @ebbit1q found"

This reverts commit 20b1ad9f7a675fd3b0d1be7452f71160ce06de71.

* actual fix for the issues @ebbit1q found

* its dirty but works

* fix cards in zoneviews not having a menu

* deleted isempty check as it is updated after the check

* key binds should work now

-> menus updated on zone change/attach/retranslate UI if selected

* clangify

* remove updateCardMenu from carditem entirely

updateCardMenu is done by the player and having it in carditem led to it
often being run multiple times, I've opted to instead run it in the
player and remove the signal entirely

the new logic updates the cardMenu every time a card is set as the
activeCard in the game tab

additionally a cardmenu can change while selected if the selected card:
moves zone, is flipped, or is attached (it receives the unattach action)

this is done in the player instead now, checking if the card is the
activeCard

this however exposes a flaw in the selection management where if you
unselect a card the activeCard is set to nullptr, this was an existing
bug and causes the action on selected cards to suddenly disappear, even
if there are other cards selected!

* revert null test of aCardMenu

Co-authored-by: ebbit1q <ebbit1q@gmail.com>

cockatrice/src/abstractcarditem.cpp

@@ -272,7 +272,6 @@ void AbstractCardItem::setFaceDown(bool _facedown)
 {
     facedown = _facedown;
     update();
-    emit updateCardMenu(this);
 }
 
 void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event)

cockatrice/src/abstractcarditem.h

@@ -36,7 +36,6 @@ signals:
     void hovered(AbstractCardItem *card);
     void showCardInfoPopup(QPoint pos, QString cardName);
     void deleteCardInfoPopup(QString cardName);
-    void updateCardMenu(AbstractCardItem *card);
     void sigPixmapUpdated();
     void cardShiftClicked(QString cardName);
 

cockatrice/src/carditem.cpp

@@ -23,8 +23,7 @@ CardItem::CardItem(Player *_owner,
                    int _cardid,
                    bool _revealedCard,
                    QGraphicsItem *parent,
-                   CardZone *_zone,
-                   bool updateMenu)
+                   CardZone *_zone)
     : AbstractCardItem(_name, _owner, _cardid, parent), zone(_zone), revealedCard(_revealedCard), attacking(false),
       destroyOnZoneChange(false), doesntUntap(false), dragItem(nullptr), attachedTo(nullptr)
 {
@@ -35,9 +34,6 @@ CardItem::CardItem(Player *_owner,
     moveMenu = new QMenu;
 
     retranslateUi();
-    if (updateMenu) { // avoid updating card menu too often
-        emit updateCardMenu(this);
-    }
 }
 
 CardItem::~CardItem()
@@ -84,7 +80,6 @@ void CardItem::deleteLater()
 void CardItem::setZone(CardZone *_zone)
 {
     zone = _zone;
-    emit updateCardMenu(this);
 }
 
 void CardItem::retranslateUi()
@@ -203,23 +198,25 @@ void CardItem::setPT(const QString &_pt)
 
 void CardItem::setAttachedTo(CardItem *_attachedTo)
 {
-    if (attachedTo)
+    if (attachedTo != nullptr) {
         attachedTo->removeAttachedCard(this);
+    }
 
     gridPoint.setX(-1);
     attachedTo = _attachedTo;
-    if (attachedTo) {
+    if (attachedTo != nullptr) {
         setParentItem(attachedTo->getZone());
         attachedTo->addAttachedCard(this);
-        if (zone != attachedTo->getZone())
+        if (zone != attachedTo->getZone()) {
             attachedTo->getZone()->reorganizeCards();
-    } else
+        }
+    } else {
         setParentItem(zone);
+    }
 
-    if (zone)
+    if (zone != nullptr) {
         zone->reorganizeCards();
-
-    emit updateCardMenu(this);
+    }
 }
 
 void CardItem::resetState()
@@ -371,8 +368,7 @@ void CardItem::playCard(bool faceDown)
 void CardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
 {
     if (event->button() == Qt::RightButton) {
-        if (cardMenu && !cardMenu->isEmpty() && owner != nullptr) {
-            owner->updateCardMenu(this);
+        if (cardMenu != nullptr && !cardMenu->isEmpty() && owner != nullptr) {
             cardMenu->exec(event->screenPos());
         }
     } else if ((event->modifiers() != Qt::AltModifier) && (event->button() == Qt::LeftButton) &&
@@ -437,13 +433,13 @@ bool CardItem::animationEvent()
 
 QVariant CardItem::itemChange(GraphicsItemChange change, const QVariant &value)
 {
-    if ((change == ItemSelectedHasChanged) && owner) {
+    if ((change == ItemSelectedHasChanged) && owner != nullptr) {
         if (value == true) {
             owner->setCardMenu(cardMenu);
             owner->getGame()->setActiveCard(this);
         } else if (owner->getCardMenu() == cardMenu) {
-            owner->setCardMenu(0);
-            owner->getGame()->setActiveCard(0);
+            owner->setCardMenu(nullptr);
+            owner->getGame()->setActiveCard(nullptr);
         }
     }
     return QGraphicsItem::itemChange(change, value);

cockatrice/src/carditem.h

@@ -53,8 +53,7 @@ public:
              int _cardid = -1,
              bool revealedCard = false,
              QGraphicsItem *parent = nullptr,
-             CardZone *_zone = nullptr,
-             bool updateMenu = false);
+             CardZone *_zone = nullptr);
     ~CardItem();
     void retranslateUi();
     CardZone *getZone() const

cockatrice/src/player.cpp

@@ -1945,7 +1945,6 @@ void Player::eventSetCardCounter(const Event_SetCardCounter &event)
 
     int oldValue = card->getCounters().value(event.counter_id(), 0);
     card->setCounter(event.counter_id(), event.counter_value());
-    updateCardMenu(card);
     emit logSetCardCounter(this, card->getName(), event.counter_id(), event.counter_value(), oldValue);
 }
 
@@ -2014,7 +2013,7 @@ void Player::eventMoveCard(const Event_MoveCard &event, const GameEventContext &
         x = 0;
     }
     CardItem *card = startZone->takeCard(position, event.card_id(), startZone != targetZone);
-    if (!card) {
+    if (card == nullptr) {
         return;
     }
     if (startZone != targetZone) {
@@ -2080,6 +2079,7 @@ void Player::eventMoveCard(const Event_MoveCard &event, const GameEventContext &
             i->delArrow();
         }
     }
+    updateCardMenu(card);
 }
 
 void Player::eventFlipCard(const Event_FlipCard &event)
@@ -2094,6 +2094,7 @@ void Player::eventFlipCard(const Event_FlipCard &event)
     }
     emit logFlipCard(this, card->getName(), event.face_down());
     card->setFaceDown(event.face_down());
+    updateCardMenu(card);
 }
 
 void Player::eventDestroyCard(const Event_DestroyCard &event)
@@ -2111,7 +2112,7 @@ void Player::eventDestroyCard(const Event_DestroyCard &event)
     QList<CardItem *> attachedCards = card->getAttachedCards();
     // This list is always empty except for buggy server implementations.
     for (auto &attachedCard : attachedCards) {
-        attachedCard->setAttachedTo(0);
+        attachedCard->setAttachedTo(nullptr);
     }
 
     emit logDestroyCard(this, card->getName());
@@ -2162,6 +2163,7 @@ void Player::eventAttachCard(const Event_AttachCard &event)
     } else {
         emit logUnattachCard(this, startCard->getName());
     }
+    updateCardMenu(startCard);
 }
 
 void Player::eventDrawCards(const Event_DrawCards &event)
@@ -3278,17 +3280,14 @@ void Player::refreshShortcuts()
 {
     if (shortcutsActive) {
         setShortcutsActive();
-
-        for (const CardItem *cardItem : table->getCards()) {
-            updateCardMenu(cardItem);
-        }
     }
 }
 
 void Player::updateCardMenu(const CardItem *card)
 {
     // If bad card OR is a spectator (as spectators don't need card menus), return
-    if (card == nullptr || (game->isSpectator() && !judge)) {
+    // only update the menu if the card is actually selected
+    if (card == nullptr || (game->isSpectator() && !judge) || game->getActiveCard() != card) {
         return;
     }
 
@@ -3521,7 +3520,7 @@ void Player::addRelatedCardActions(const CardItem *card, QMenu *cardMenu)
 
 void Player::setCardMenu(QMenu *menu)
 {
-    if (aCardMenu) {
+    if (aCardMenu != nullptr) {
         aCardMenu->setEnabled(menu != nullptr);
         aCardMenu->setMenu(menu);
     }
@@ -3529,10 +3528,11 @@ void Player::setCardMenu(QMenu *menu)
 
 QMenu *Player::getCardMenu() const
 {
-    if (aCardMenu) {
+    if (aCardMenu != nullptr) {
         return aCardMenu->menu();
+    } else {
+        return nullptr;
     }
-    return nullptr;
 }
 
 QString Player::getName() const

cockatrice/src/tab_game.cpp

@@ -1349,7 +1349,6 @@ void TabGame::newCardAdded(AbstractCardItem *card)
     connect(card, SIGNAL(hovered(AbstractCardItem *)), cardInfo, SLOT(setCard(AbstractCardItem *)));
     connect(card, SIGNAL(showCardInfoPopup(QPoint, QString)), this, SLOT(showCardInfoPopup(QPoint, QString)));
     connect(card, SIGNAL(deleteCardInfoPopup(QString)), this, SLOT(deleteCardInfoPopup(QString)));
-    connect(card, SIGNAL(updateCardMenu(AbstractCardItem *)), this, SLOT(updateCardMenu(AbstractCardItem *)));
     connect(card, SIGNAL(cardShiftClicked(QString)), this, SLOT(linkCardToChat(QString)));
 }
 
@@ -1413,6 +1412,12 @@ Player *TabGame::getActiveLocalPlayer() const
     return nullptr;
 }
 
+void TabGame::setActiveCard(CardItem *card)
+{
+    activeCard = card;
+    updateCardMenu(card);
+}
+
 void TabGame::updateCardMenu(AbstractCardItem *card)
 {
     Player *player;

cockatrice/src/tab_game.h

@@ -298,10 +298,7 @@ public:
     Player *getActiveLocalPlayer() const;
     AbstractClient *getClientForPlayer(int playerId) const;
 
-    void setActiveCard(CardItem *_card)
-    {
-        activeCard = _card;
-    }
+    void setActiveCard(CardItem *card);
     CardItem *getActiveCard() const
     {
         return activeCard;

cockatrice/src/zoneviewzone.cpp

@@ -83,15 +83,12 @@ void ZoneViewZone::initializeCards(const QList<const ServerInfo_Card *> &cardLis
 
 void ZoneViewZone::zoneDumpReceived(const Response &r)
 {
-    static constexpr int MAX_VIEW_MENU = 300;
     const Response_DumpZone &resp = r.GetExtension(Response_DumpZone::ext);
     const int respCardListSize = resp.zone_info().card_list_size();
     for (int i = 0; i < respCardListSize; ++i) {
         const ServerInfo_Card &cardInfo = resp.zone_info().card_list(i);
         auto cardName = QString::fromStdString(cardInfo.name());
-        // stop updating card menus after MAX_VIEW_MENU cards
-        // this means only the first cards in the menu have actions but others can still be dragged
-        auto *card = new CardItem(player, cardName, cardInfo.id(), revealZone, this, this, i < MAX_VIEW_MENU);
+        auto *card = new CardItem(player, cardName, cardInfo.id(), revealZone, this, this);
         cards.insert(i, card);
     }
     reorganizeCards();