[Refactor] Give graphics zones a card item cache and own card placement

cockatrice/src/game/board/card_state.cpp

@@ -13,6 +13,9 @@ void CardState::resetState(bool keepAnnotations)
         annotation.clear();
     }
     attachedTo = nullptr;
+    setTapped(false, false);
+    setDoesntUntap(false);
+    emit stateReset();
 }
 
 void CardState::setCardRef(const CardRef &_cardRef)

cockatrice/src/game/board/card_state.h

@@ -43,6 +43,7 @@ signals:
     void cardPixmapUpdated();
     void tappedChanged(bool newTapped, bool canAnimate);
     void facedownChanged(bool newFaceDown);
+    void stateReset();
 
     void attackingChanged(bool newValue);
     void countersChanged(const QMap<int, int> &newCounters);

cockatrice/src/game/zones/card_zone_algorithms.h

@@ -5,9 +5,9 @@ namespace CardZoneAlgorithms
 {
 
 /**
- * Shared insertion logic for zones where cards become visible on add and follow
+ * Shared insertion logic for zones where cards follow the standard pattern:
- * the standard pattern: clamp index, insert, clear identity if contents unknown,
+ * clamp index, insert, clear identity if contents unknown, reset state.
- * reset state, show card.
+ * Visibility and parenting are handled by the graphics zone's onCardAdded().
  *
  * Zones with different post-add behavior (signal connections, positional resets,
  * hidden cards, or coordinate-based placement) should NOT use this — implement
@@ -18,8 +18,7 @@ namespace CardZoneAlgorithms
  *
  * @tparam CardList Must provide: size() -> int, insert(int, CardType*),
  *                  getContentsKnown() -> bool
- * @tparam CardType Must provide: setId(int), setCardRef(CardRefType),
+ * @tparam CardType Must provide: setId(int), setCardRef(CardRefType), resetState(bool)
- *                  resetState(bool), setVisible(bool)
  * @param keepAnnotations Forwarded to card->resetState(). Stack-like zones preserve
  *                        annotations across zone transitions; hand-like zones clear them.
  */
@@ -37,7 +36,6 @@ void addCardToList(CardList &cards, CardType *card, int x, bool keepAnnotations)
     }
 
     card->resetState(keepAnnotations);
-    card->setVisible(true);
 }
 
 } // namespace CardZoneAlgorithms

cockatrice/src/game/zones/card_zone_logic.h

@@ -53,6 +53,7 @@ public:
     void rawInsertCard(CardItem *card, int index)
     {
         cards.insert(index, card);
+        emit cardCountChanged();
     }
 
     [[nodiscard]] const CardList &getCards() const
@@ -63,6 +64,7 @@ public:
     void sortCards(const QList<CardList::SortOption> &options)
     {
         cards.sortBy(options);
+        emit cardCountChanged();
     }
     [[nodiscard]] QString getName() const
     {

cockatrice/src/game/zones/pile_zone_logic.cpp

@@ -18,13 +18,12 @@ PileZoneLogic::PileZoneLogic(PlayerLogic *_player,
 
 void PileZoneLogic::addCardImpl(CardItem *card, int x, int /*y*/)
 {
-    connect(card, &CardItem::sigPixmapUpdated, this, &PileZoneLogic::callUpdate);
+    connect(card->getState(), &CardState::cardPixmapUpdated, this, &PileZoneLogic::callUpdate);
     // if x is negative set it to add at end
     if (x < 0 || x >= cards.size()) {
         x = cards.size();
     }
     cards.insert(x, card);
-    card->setPos(0, 0);
     if (!contentsKnown()) {
         card->setCardRef({});
         card->setId(-1);
@@ -33,6 +32,5 @@ void PileZoneLogic::addCardImpl(CardItem *card, int x, int /*y*/)
             cards.at(x + 1)->setCardRef({});
         }
     }
-    card->setVisible(false);
     card->resetState();
 }

cockatrice/src/game/zones/table_zone_logic.cpp

@@ -29,7 +29,6 @@ void TableZoneLogic::addCardImpl(CardItem *card, int _x, int _y)
         card->setDoesntUntap(true);
     }
     card->setGridPoint(QPoint(_x, _y));
-    card->setVisible(true);
 }
 
 CardItem *TableZoneLogic::takeCard(int position, int cardId, bool toNewZone)

cockatrice/src/game/zones/view_zone_logic.cpp

@@ -168,6 +168,7 @@ void ZoneViewZoneLogic::clearCards()
         card->deleteLater();
     }
     cards.clear();
+    emit cardCountChanged();
 }
 
 void ZoneViewZoneLogic::setWriteableRevealZone(bool _writeableRevealZone)

cockatrice/src/game_graphics/board/card_item.cpp

@@ -63,6 +63,14 @@ CardItem::CardItem(PlayerLogic *_owner,
             update();
         }
     });
+
+    connect(state, &CardState::stateReset, this, [this] {
+        attachedCards.clear();
+        if (scene()) {
+            static_cast<GameScene *>(scene())->unregisterAnimationItem(this);
+        }
+        update();
+    });
 }
 
 void CardItem::prepareDelete()
@@ -250,13 +258,6 @@ void CardItem::setAttachedTo(CardItem *_attachedTo)
 void CardItem::resetState(bool keepAnnotations)
 {
     state->resetState(keepAnnotations);
-    attachedCards.clear();
-    setTapped(false, false);
-    setDoesntUntap(false);
-    if (scene()) {
-        static_cast<GameScene *>(scene())->unregisterAnimationItem(this);
-    }
-    update();
 }
 
 void CardItem::processCardInfo(const ServerInfo_Card &_info)

cockatrice/src/game_graphics/zones/card_zone.cpp

@@ -18,12 +18,31 @@ CardZone::CardZone(CardZoneLogic *_logic, QGraphicsItem *parent)
     : AbstractGraphicsItem(parent), menu(nullptr), doubleClickAction(0), logic(_logic)
 {
     connect(logic, &CardZoneLogic::retranslateUi, this, &CardZone::retranslateUi);
+    // Invalidation is connected before any consumer so a handler that reads cardItems()
+    // during the same signal emission always sees a stale flag and rebuilds.
+    connect(logic, &CardZoneLogic::cardAdded, this, &CardZone::invalidateItems);
+    connect(logic, &CardZoneLogic::cardCountChanged, this, &CardZone::invalidateItems);
+    connect(logic, &CardZoneLogic::reorganizeCards, this, &CardZone::invalidateItems);
     connect(logic, &CardZoneLogic::cardAdded, this, &CardZone::onCardAdded);
     connect(logic, &CardZoneLogic::setGraphicsVisibility, this, [this](bool v) { this->setVisible(v); });
     connect(logic, &CardZoneLogic::updateGraphics, this, [this]() { update(); });
     connect(logic, &CardZoneLogic::reorganizeCards, this, &CardZone::reorganizeCards);
 }
 
+void CardZone::invalidateItems()
+{
+    itemsDirty = true;
+}
+
+const QList<CardItem *> &CardZone::cardItems() const
+{
+    if (itemsDirty) {
+        items = getLogic()->getCards();
+        itemsDirty = false;
+    }
+    return items;
+}
+
 void CardZone::onCardAdded(CardItem *addedCard)
 {
     addedCard->setParentItem(this);
@@ -33,8 +52,8 @@ void CardZone::onCardAdded(CardItem *addedCard)
 
 void CardZone::retranslateUi()
 {
-    for (int i = 0; i < getLogic()->getCards().size(); ++i) {
+    for (auto *card : cardItems()) {
-        getLogic()->getCards()[i]->retranslateUi();
+        card->retranslateUi();
     }
 }
 

cockatrice/src/game_graphics/zones/card_zone.h

@@ -76,6 +76,15 @@ public:
         return logic;
     }
 
+    /**
+     * @brief The CardItems backing getLogic()->getCards(), in zone order.
+     *
+     * Game logic talks about cards as states; this is the graphics side's view of the same list.
+     * It is rebuilt on demand after any content change, so views never reach into the logic's
+     * container themselves and never observe a stale one.
+     */
+    [[nodiscard]] const QList<CardItem *> &cardItems() const;
+
     void setMenu(QMenu *_menu, QAction *_doubleClickAction = 0)
     {
         menu = _menu;
@@ -83,7 +92,12 @@ public:
     }
 
 private:
+    /** @brief Marks the cached card items stale. Connected to every content-changing signal. */
+    void invalidateItems();
+
     CardZoneLogic *logic;
+    mutable QList<CardItem *> items;
+    mutable bool itemsDirty = true;
 };
 
 #endif

cockatrice/src/game_graphics/zones/hand_zone.cpp

@@ -47,8 +47,8 @@ void HandZone::handleDropEvent(const QList<CardDragItem *> &dragItems,
     QPoint point = dropPoint + scenePos().toPoint();
     int x = -1;
     if (SettingsCache::instance().userInterface().getHorizontalHand()) {
-        for (x = 0; x < getLogic()->getCards().size(); x++) {
+        for (x = 0; x < cardItems().size(); x++) {
-            if (point.x() < static_cast<CardItem *>(getLogic()->getCards().at(x))->scenePos().x()) {
+            if (point.x() < cardItems().at(x)->scenePos().x()) {
                 break;
             }
         }
@@ -83,23 +83,23 @@ void HandZone::paint(QPainter *painter, const QStyleOptionGraphicsItem * /*optio
 
 void HandZone::reorganizeCards()
 {
-    if (!getLogic()->getCards().isEmpty()) {
+    if (!cardItems().isEmpty()) {
-        const int cardCount = getLogic()->getCards().size();
+        const int cardCount = cardItems().size();
         if (SettingsCache::instance().userInterface().getHorizontalHand()) {
             bool leftJustified = SettingsCache::instance().userInterface().getLeftJustified();
-            qreal cardWidth = getLogic()->getCards().at(0)->boundingRect().width();
+            qreal cardWidth = cardItems().at(0)->boundingRect().width();
             const int xPadding = leftJustified ? cardWidth * 1.4 : 5;
             qreal totalWidth =
                 leftJustified ? boundingRect().width() - (1 * xPadding) - 5 : boundingRect().width() - 2 * xPadding;
 
             if (cardCount == 1) {
-                CardItem *c = getLogic()->getCards().at(0);
+                CardItem *c = cardItems().at(0);
                 qreal xPosition = leftJustified ? xPadding : xPadding + (totalWidth - cardWidth) / 2;
                 c->setPos(xPosition, 5);
                 c->setRealZValue(0);
             } else {
                 for (int i = 0; i < cardCount; i++) {
-                    CardItem *c = getLogic()->getCards().at(i);
+                    CardItem *c = cardItems().at(i);
                     // If the total width of the cards is smaller than the available width,
                     // the cards do not need to overlap and are displayed in the center of the area.
                     if (cardWidth * cardCount > totalWidth) {
@@ -124,7 +124,7 @@ void HandZone::reorganizeCards()
 
 void HandZone::sortHand(const QList<CardList::SortOption> &options)
 {
-    if (getLogic()->getCards().isEmpty()) {
+    if (cardItems().isEmpty()) {
         return;
     }
 

cockatrice/src/game_graphics/zones/pile_zone.cpp

@@ -66,15 +66,14 @@ void PileZone::paint(QPainter *painter, const QStyleOptionGraphicsItem * /*optio
 {
     painter->drawPath(shape());
 
-    if (!getLogic()->getCards().isEmpty()) {
+    if (!cardItems().isEmpty()) {
-        getLogic()->getCards().at(0)->paintPicture(painter, getLogic()->getCards().at(0)->getTranslatedSize(painter),
+        cardItems().at(0)->paintPicture(painter, cardItems().at(0)->getTranslatedSize(painter), 90);
-                                                   90);
     }
 
     painter->translate(CardDimensions::WIDTH_HALF_F, CardDimensions::HEIGHT_HALF_F);
     painter->rotate(-90);
     painter->translate(-CardDimensions::WIDTH_HALF_F, -CardDimensions::HEIGHT_HALF_F);
-    paintNumberEllipse(getLogic()->getCards().size(), 28, Qt::white, -1, -1, painter);
+    paintNumberEllipse(cardItems().size(), 28, Qt::white, -1, -1, painter);
 }
 
 void PileZone::handleDropEvent(const QList<CardDragItem *> &dragItems, CardZoneLogic *startZone, const QPoint &)
@@ -88,6 +87,15 @@ void PileZone::handleDropEvent(const QList<CardDragItem *> &dragItems, CardZoneL
     getLogic()->getPlayer()->getPlayerActions()->moveCards(startZone, getLogic(), 0, 0, cards);
 }
 
+void PileZone::onCardAdded(CardItem *addedCard)
+{
+    CardZone::onCardAdded(addedCard);
+    if (addedCard) {
+        addedCard->setPos(0, 0);
+        addedCard->setVisible(false);
+    }
+}
+
 void PileZone::reorganizeCards()
 {
     update();
@@ -115,15 +123,14 @@ void PileZone::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
         return;
     }
 
-    if (getLogic()->getCards().isEmpty()) {
+    if (cardItems().isEmpty()) {
         return;
     }
 
     bool forceFaceDown = event->modifiers().testFlag(Qt::ShiftModifier);
     bool bottomCard = event->modifiers().testFlag(Qt::ControlModifier);
-    CardItem *card = bottomCard ? getLogic()->getCards().last() : getLogic()->getCards().first();
+    CardItem *card = bottomCard ? cardItems().last() : cardItems().first();
-    const int cardid =
+    const int cardid = getLogic()->contentsKnown() ? card->getId() : (bottomCard ? cardItems().size() - 1 : 0);
-        getLogic()->contentsKnown() ? card->getId() : (bottomCard ? getLogic()->getCards().size() - 1 : 0);
     CardDragItem *drag = card->createDragItem(cardid, event->pos(), event->scenePos(), forceFaceDown);
     drag->grabMouse();
     setCursor(Qt::OpenHandCursor);
@@ -136,8 +143,8 @@ void PileZone::mouseReleaseEvent(QGraphicsSceneMouseEvent * /*event*/)
 
 void PileZone::hoverEnterEvent(QGraphicsSceneHoverEvent *event)
 {
-    if (!getLogic()->getCards().isEmpty()) {
+    if (!cardItems().isEmpty()) {
-        getLogic()->getCards()[0]->processHoverEvent();
+        cardItems()[0]->processHoverEvent();
     }
     QGraphicsItem::hoverEnterEvent(event);
 }

cockatrice/src/game_graphics/zones/pile_zone.h

@@ -33,6 +33,7 @@ public:
     [[nodiscard]] QPainterPath shape() const override;
     void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget) override;
     void reorganizeCards() override;
+    void onCardAdded(CardItem *addedCard) override;
     void
     handleDropEvent(const QList<CardDragItem *> &dragItems, CardZoneLogic *startZone, const QPoint &dropPoint) override;
 

cockatrice/src/game_graphics/zones/select_zone.cpp

@@ -85,7 +85,7 @@ SelectZone *SelectZone::findOwningSelectZone(const QGraphicsItem *card)
 
 SelectZone::StackLayoutParams SelectZone::buildStackParams(qreal minOffset) const
 {
-    const auto &cards = getLogic()->getCards();
+    const auto &cards = cardItems();
     if (cards.isEmpty()) {
         return {0, boundingRect().height(), 0.0, 0.0, minOffset};
     }
@@ -97,7 +97,7 @@ SelectZone::StackLayoutParams SelectZone::buildStackParams(qreal minOffset) cons
 
 int SelectZone::calcDropIndexFromY(qreal dropY, bool allowCountExpand, qreal minOffset) const
 {
-    const auto &cards = getLogic()->getCards();
+    const auto &cards = cardItems();
     if (cards.isEmpty()) {
         return 0;
     }
@@ -115,7 +115,7 @@ void SelectZone::restoreStaleEscapedCards()
     if (!cardClipContainer) {
         return;
     }
-    for (auto *card : getLogic()->getCards()) {
+    for (auto *card : cardItems()) {
         // A card parented to the zone (instead of the clip container) should
         // only occur while it is actively hovered. If hover cleanup was
         // missed, reparent it back so clipping resumes.
@@ -127,7 +127,7 @@ void SelectZone::restoreStaleEscapedCards()
 
 void SelectZone::layoutCardsVertically(const StackLayoutParams &params)
 {
-    const auto &cards = getLogic()->getCards();
+    const auto &cards = cardItems();
     if (cards.isEmpty() || params.cardCount <= 0) {
         return;
     }
@@ -162,9 +162,9 @@ SelectZone::~SelectZone()
     if (cardClipContainer) {
         // Reparent any hover-escaped cards back to the clip container so Qt's
         // parent-child tree is consistent for destruction. setParentItem() does
-        // not invalidate getLogic()->getCards() (it modifies the graphics tree,
+        // not invalidate cardItems() — it only moves the item in the graphics
-        // not the zone's logical card list).
+        // tree, it never changes the zone's contents.
-        for (auto *card : getLogic()->getCards()) {
+        for (auto *card : cardItems()) {
             if (card && card->parentItem() == this) {
                 card->setParentItem(cardClipContainer);
             }
@@ -247,7 +247,7 @@ void SelectZone::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
         }
 
         QRectF selectionRect = QRectF(selectionOrigin, pos).normalized();
-        for (auto card : getLogic()->getCards()) {
+        for (auto card : cardItems()) {
             if (card->getAttachedTo() && card->getAttachedTo()->getZone() != getLogic()) {
                 continue;
             }

cockatrice/src/game_graphics/zones/stack_zone.cpp

@@ -72,7 +72,7 @@ void StackZone::handleDropEvent(const QList<CardDragItem *> &dragItems,
     int index = calcDropIndexFromY(dropPoint.y(), !sameZone, MIN_CARD_VISIBLE);
     if (sameZone) {
         // Same-zone no-op: don't move a card onto itself
-        const auto &cards = getLogic()->getCards();
+        const auto &cards = cardItems();
         if (!cards.isEmpty() && cards.at(index)->getId() == dragItems.at(0)->getId()) {
             return;
         }
@@ -102,7 +102,7 @@ void StackZone::setHeight(qreal newHeight)
 
 void StackZone::reorganizeCards()
 {
-    if (!getLogic()->getCards().isEmpty()) {
+    if (!cardItems().isEmpty()) {
         const auto params = buildStackParams(MIN_CARD_VISIBLE);
         layoutCardsVertically(params);
     }

cockatrice/src/game_graphics/zones/table_zone.cpp

@@ -228,8 +228,8 @@ void TableZone::reorganizeCards()
     // Calculate card stack widths so mapping functions work properly
     computeCardStackWidths();
 
-    for (int i = 0; i < getLogic()->getCards().size(); ++i) {
+    for (int i = 0; i < cardItems().size(); ++i) {
-        QPoint gridPoint = getLogic()->getCards()[i]->getGridPos();
+        QPoint gridPoint = cardItems()[i]->getGridPos();
         if (gridPoint.x() == -1) {
             continue;
         }
@@ -238,17 +238,17 @@ void TableZone::reorganizeCards()
         qreal x = mapPoint.x();
         qreal y = mapPoint.y();
 
-        int numberAttachedCards = getLogic()->getCards()[i]->getAttachedCards().size();
+        int numberAttachedCards = cardItems()[i]->getAttachedCards().size();
         qreal actualX = x + numberAttachedCards * STACKED_CARD_OFFSET_X;
         qreal actualY = y;
         if (numberAttachedCards) {
             actualY += 15;
         }
 
-        getLogic()->getCards()[i]->setPos(actualX, actualY);
+        cardItems()[i]->setPos(actualX, actualY);
-        getLogic()->getCards()[i]->setRealZValue(ZValues::tableCardZValue(actualX, actualY));
+        cardItems()[i]->setRealZValue(ZValues::tableCardZValue(actualX, actualY));
 
-        QListIterator<CardItem *> attachedCardIterator(getLogic()->getCards()[i]->getAttachedCards());
+        QListIterator<CardItem *> attachedCardIterator(cardItems()[i]->getAttachedCards());
         int j = 0;
         while (attachedCardIterator.hasNext()) {
             ++j;
@@ -285,9 +285,9 @@ void TableZone::resizeToContents()
     int xMax = 0;
 
     // Find rightmost card position, which includes the left margin amount.
-    for (int i = 0; i < getLogic()->getCards().size(); ++i) {
+    for (int i = 0; i < cardItems().size(); ++i) {
-        if (getLogic()->getCards()[i]->pos().x() > xMax) {
+        if (cardItems()[i]->pos().x() > xMax) {
-            xMax = (int)getLogic()->getCards()[i]->pos().x();
+            xMax = (int)cardItems()[i]->pos().x();
         }
     }
 
@@ -308,9 +308,9 @@ void TableZone::resizeToContents()
 
 CardItem *TableZone::getCardFromGrid(const QPoint &gridPoint) const
 {
-    for (int i = 0; i < getLogic()->getCards().size(); i++) {
+    for (int i = 0; i < cardItems().size(); i++) {
-        if (getLogic()->getCards().at(i)->getGridPoint() == gridPoint) {
+        if (cardItems().at(i)->getGridPoint() == gridPoint) {
-            return getLogic()->getCards().at(i);
+            return cardItems().at(i);
         }
     }
     return 0;
@@ -327,8 +327,8 @@ void TableZone::computeCardStackWidths()
     // Each card stack is three grid points worth of card locations.
     // First pass: compute the number of cards at each card stack.
     QMap<int, int> cardStackCount;
-    for (int i = 0; i < getLogic()->getCards().size(); ++i) {
+    for (int i = 0; i < cardItems().size(); ++i) {
-        const QPoint &gridPoint = getLogic()->getCards()[i]->getGridPos();
+        const QPoint &gridPoint = cardItems()[i]->getGridPos();
         if (gridPoint.x() == -1) {
             continue;
         }
@@ -339,8 +339,8 @@ void TableZone::computeCardStackWidths()
 
     // Second pass: compute the width at each card stack.
     cardStackWidth.clear();
-    for (int i = 0; i < getLogic()->getCards().size(); ++i) {
+    for (int i = 0; i < cardItems().size(); ++i) {
-        const QPoint &gridPoint = getLogic()->getCards()[i]->getGridPos();
+        const QPoint &gridPoint = cardItems()[i]->getGridPos();
         if (gridPoint.x() == -1) {
             continue;
         }
@@ -348,8 +348,8 @@ void TableZone::computeCardStackWidths()
         const int key = getCardStackMapKey(gridPoint.x() / 3, gridPoint.y());
         const int stackCount = cardStackCount.value(key, 0);
         if (stackCount == 1) {
-            cardStackWidth.insert(key, CardDimensions::WIDTH + getLogic()->getCards()[i]->getAttachedCards().size() *
+            cardStackWidth.insert(key, CardDimensions::WIDTH +
-                                                                   STACKED_CARD_OFFSET_X);
+                                           cardItems()[i]->getAttachedCards().size() * STACKED_CARD_OFFSET_X);
         } else {
             cardStackWidth.insert(key, CardDimensions::WIDTH + (stackCount - 1) * STACKED_CARD_OFFSET_X);
         }

cockatrice/src/game_graphics/zones/view_zone.cpp

@@ -139,8 +139,8 @@ void ZoneViewZone::zoneDumpReceived(const Response &r)
 void ZoneViewZone::reorganizeCards()
 {
     // filter cards
-    CardList cardsToDisplay = CardList(getLogic()->getCards().getContentsKnown());
+    CardList cardsToDisplay = CardList(getLogic()->contentsKnown());
-    for (auto card : getLogic()->getCards()) {
+    for (auto card : cardItems()) {
         if (filterString.check(card->getCard().getCardPtr())) {
             card->show();
             cardsToDisplay.append(card);

tests/card_zone_algorithms/card_zone_algorithms_test.cpp

@@ -16,7 +16,6 @@ struct MockCard
     bool cardRefWasCalled = false;
     bool resetStateCalled = false;
     bool resetStateKeepAnnotations = false;
-    bool visibleSet = false;
 
     void setId(int id)
     {
@@ -35,11 +34,6 @@ struct MockCard
         resetStateCalled = true;
         resetStateKeepAnnotations = keepAnnotations;
     }
-
-    void setVisible(bool visible)
-    {
-        visibleSet = visible;
-    }
 };
 
 class MockCardList
@@ -109,7 +103,6 @@ TEST_F(AddCardAlgorithmTest, ContentsKnownPreservesIdentity)
 
     EXPECT_FALSE(card.idWasCalled);
     EXPECT_FALSE(card.cardRefWasCalled);
-    EXPECT_TRUE(card.visibleSet);
 }
 
 TEST_F(AddCardAlgorithmTest, ContentsUnknownClearsIdentity)