[Refactor] Move the attachment graph and card view teardown onto CardState (#7428)

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/game/board/card_state.cpp

@@ -14,11 +14,17 @@ void CardState::resetState(bool keepAnnotations)
         annotation.clear();
     }
     attachedTo = nullptr;
+    attachedCards.clear();
     setTapped(false, false);
     setDoesntUntap(false);
     emit stateReset();
 }
 
+void CardState::deleteView()
+{
+    emit viewDeleteRequested();
+}
+
 void CardState::setCardRef(const CardRef &_cardRef)
 {
     if (cardRef == _cardRef) {
@@ -189,7 +195,7 @@ void CardState::setDestroyOnZoneChange(bool _destroyOnZoneChange)
     emit stateChanged();
 }
 
-void CardState::setAttachedTo(CardItem *_attachedTo)
+void CardState::setAttachedTo(CardState *_attachedTo)
 {
     if (attachedTo == _attachedTo) {
         return;

cockatrice/src/game/board/card_state.h

@@ -1,6 +1,7 @@
 #ifndef COCKATRICE_CARD_STATE_H
 #define COCKATRICE_CARD_STATE_H
 
+#include <QList>
 #include <QMap>
 #include <QObject>
 #include <QPoint>
@@ -32,7 +33,8 @@ private:
     bool doesntUntap = false;
     bool destroyOnZoneChange = false;
 
-    CardItem *attachedTo = nullptr;
+    CardState *attachedTo = nullptr;
+    QList<CardItem *> attachedCards;
     CardZoneLogic *zone = nullptr;
 
 signals:
@@ -51,9 +53,11 @@ signals:
     void ptChanged(const QString &newPt);
     void doesntUntapChanged(bool newValue);
     void destroyOnZoneChangeChanged(bool newValue);
-    void attachedToChanged(CardItem *newAttachedTo);
+    void attachedToChanged(CardState *newAttachedTo);
     void zoneChanged(CardState *changedCard, CardZoneLogic *newZone);
 
+    void viewDeleteRequested();
+
 public:
     explicit CardState(QObject *parent, CardZoneLogic *_zone = nullptr, const CardRef &_cardRef = {}, int _id = -1)
         : QObject(parent), id(_id), cardRef(_cardRef), zone(_zone)
@@ -62,6 +66,15 @@ public:
 
     void resetState(bool keepAnnotations);
 
+    /**
+     * @brief Asks the card view owning this state to tear itself down.
+     *
+     * The state never holds a card item directly, so it cannot delete one. It asks instead and lets the view run
+     * its own teardown, which detaches attachments, drops it from the active card slot and unregisters its
+     * animations before the graphics object goes away.
+     */
+    void deleteView();
+
     int getId() const
     {
         return id;
@@ -187,12 +200,27 @@ public:
 
     void setDestroyOnZoneChange(bool _destroyOnZoneChange);
 
-    CardItem *getAttachedTo() const
+    CardState *getAttachedTo() const
     {
         return attachedTo;
     }
 
-    void setAttachedTo(CardItem *_attachedTo);
+    void setAttachedTo(CardState *_attachedTo);
+
+    void addAttachedCard(CardItem *card)
+    {
+        attachedCards.append(card);
+    }
+
+    void removeAttachedCard(CardItem *card)
+    {
+        attachedCards.removeOne(card);
+    }
+
+    const QList<CardItem *> &getAttachedCards() const
+    {
+        return attachedCards;
+    }
 };
 
 #endif // COCKATRICE_CARD_STATE_H

cockatrice/src/game/player/player_event_handler.cpp

@@ -356,7 +356,7 @@ void PlayerEventHandler::eventMoveCard(const Event_MoveCard &event, const GameEv
     }
 
     if (card->getAttachedTo() && (startZone != targetZone)) {
-        CardItem *parentCard = card->getAttachedTo();
+        CardState *parentCard = card->getAttachedTo();
         card->setAttachedTo(nullptr);
         parentCard->getZone()->reorganizeCards();
     }
@@ -440,7 +440,7 @@ void PlayerEventHandler::eventDestroyCard(const Event_DestroyCard &event)
 
     emit logDestroyCard(player, card->getName());
     zone->takeCard(-1, event.card_id(), true);
-    card->deleteLater();
+    card->getState()->deleteView();
 }
 
 void PlayerEventHandler::eventAttachCard(const Event_AttachCard &event)
@@ -469,7 +469,7 @@ void PlayerEventHandler::eventAttachCard(const Event_AttachCard &event)
         return;
     }
 
-    CardItem *oldParent = startCard->getAttachedTo();
+    CardState *oldParent = startCard->getAttachedTo();
 
     startCard->setAttachedTo(targetCard);
 

cockatrice/src/game/player/player_logic.cpp

@@ -251,7 +251,7 @@ void PlayerLogic::deleteCard(CardItem *card)
     } else if (dialogSemaphore) {
         cardsToDelete.append(card);
     } else {
-        card->deleteLater();
+        card->getState()->deleteView();
     }
 }
 
@@ -332,7 +332,7 @@ bool PlayerLogic::clearCardsToDelete()
 
     for (auto &i : cardsToDelete) {
         if (i != nullptr) {
-            i->deleteLater();
+            i->getState()->deleteView();
         }
     }
     cardsToDelete.clear();

cockatrice/src/game/zones/view_zone_logic.cpp

@@ -147,7 +147,7 @@ void ZoneViewZoneLogic::removeCard(int position, bool toNewZone)
     }
 
     CardItem *card = cards.takeAt(position);
-    card->deleteLater();
+    card->getState()->deleteView();
 
     // The toNewZone check is to prevent the view from auto-closing if the view contains only a single card and that
     // card gets dragged within the view.
@@ -165,7 +165,7 @@ void ZoneViewZoneLogic::removeCard(int position, bool toNewZone)
 void ZoneViewZoneLogic::clearCards()
 {
     for (auto card : cards) {
-        card->deleteLater();
+        card->getState()->deleteView();
     }
     cards.clear();
     emit cardCountChanged();

cockatrice/src/game_graphics/board/card_item.cpp

@@ -65,12 +65,13 @@ CardItem::CardItem(PlayerLogic *_owner,
     });
 
     connect(state, &CardState::stateReset, this, [this] {
-        attachedCards.clear();
         if (scene()) {
             static_cast<GameScene *>(scene())->unregisterAnimationItem(this);
         }
         update();
     });
+
+    connect(state, &CardState::viewDeleteRequested, this, &CardItem::deleteLater);
 }
 
 void CardItem::prepareDelete()
@@ -84,6 +85,7 @@ void CardItem::prepareDelete()
         owner = nullptr;
     }
 
+    const QList<CardItem *> &attachedCards = state->getAttachedCards();
     while (!attachedCards.isEmpty()) {
         attachedCards.first()->setZone(nullptr); // so that it won't try to call reorganizeCards()
         attachedCards.first()->setAttachedTo(nullptr);
@@ -224,7 +226,7 @@ void CardItem::setAttachedTo(CardItem *_attachedTo)
     }
 
     state->setGridPoint(QPoint(-1, state->getGridPoint().y()));
-    state->setAttachedTo(_attachedTo);
+    state->setAttachedTo(_attachedTo == nullptr ? nullptr : _attachedTo->getState());
     if (state->getAttachedTo() != nullptr) {
         // If the zone is being torn down, it might already be null by the time a card tries to un-attach all its
         // attached cards

cockatrice/src/game_graphics/board/card_item.h

@@ -34,7 +34,6 @@ class CardItem : public AbstractCardItem
     Q_OBJECT
 private:
     CardDragItem *dragItem;
-    QList<CardItem *> attachedCards;
 
     void prepareDelete();
     void handleClickedToPlay(bool shiftHeld);
@@ -116,22 +115,22 @@ public:
     {
         state->setDestroyOnZoneChange(_destroy);
     }
-    [[nodiscard]] CardItem *getAttachedTo() const
+    [[nodiscard]] CardState *getAttachedTo() const
     {
         return state->getAttachedTo();
     }
     void setAttachedTo(CardItem *_attachedTo);
     void addAttachedCard(CardItem *card)
     {
-        attachedCards.append(card);
+        state->addAttachedCard(card);
     }
     void removeAttachedCard(CardItem *card)
     {
-        attachedCards.removeOne(card);
+        state->removeAttachedCard(card);
     }
     [[nodiscard]] const QList<CardItem *> &getAttachedCards() const
     {
-        return attachedCards;
+        return state->getAttachedCards();
     }
     void resetState(bool keepAnnotations = false);
     void processCardInfo(const ServerInfo_Card &_info);