[Refactor] Hold zone views and card view teardown outside game graphics (#7429)

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

cockatrice/src/game/player/player_event_handler.cpp

@@ -1,7 +1,6 @@
 #include "player_event_handler.h"
 
 #include "../../game_graphics/board/card_item.h"
-#include "../../game_graphics/zones/view_zone.h"
 #include "../abstract_game.h"
 #include "../board/arrow_data.h"
 #include "../board/card_list.h"
@@ -9,6 +8,7 @@
 #include "../zones/card_zone_logic.h"
 #include "../zones/hand_zone_logic.h"
 #include "../zones/pile_zone_logic.h"
+#include "../zones/view_zone_logic.h"
 #include "libcockatrice/card/card_info.h"
 #include "libcockatrice/card/printing/exact_card.h"
 #include "libcockatrice/protocol/pb/card_attributes.pb.h"
@@ -76,12 +76,12 @@ void PlayerEventHandler::eventShuffle(const Event_Shuffle &event)
     }
 
     // close all views that contain shuffled cards
-    for (ZoneViewZone *view : zone->getViews()) {
+    for (auto *viewLogic : zone->getViews()) {
-        if (view != nullptr) {
+        if (viewLogic != nullptr) {
-            int length = view->getLogic()->getCards().length();
+            int length = viewLogic->getCards().length();
             // we want to close empty views as well
             if (length == 0 || length > absStart) { // note this assumes views always start at the top of the library
-                view->close();
+                viewLogic->requestClose();
             }
         } else {
             qWarning() << zone->getName() << "of" << player->getPlayerInfo()->getName() << "holds empty zoneview!";
@@ -345,9 +345,9 @@ void PlayerEventHandler::eventMoveCard(const Event_MoveCard &event, const GameEv
     if (card == nullptr) {
         return;
     }
-    if (startZone != targetZone) {
+    const bool zoneChanged = startZone != targetZone;
-        card->deleteCardInfoPopup();
+    const bool ownerChanged = zoneChanged && startZone->getPlayer() != targetZone->getPlayer();
-    }
+    emit cardViewRefreshRequested(card->getState(), zoneChanged, ownerChanged ? targetZone->getPlayer() : nullptr);
     if (event.has_card_name()) {
         QString name = QString::fromStdString(event.card_name());
         QString providerId =
@@ -361,22 +361,13 @@ void PlayerEventHandler::eventMoveCard(const Event_MoveCard &event, const GameEv
         parentCard->getZone()->reorganizeCards();
     }
 
-    card->deleteDragItem();
-
     card->setId(event.new_card_id());
     card->setFaceDown(event.face_down());
     if (startZone != targetZone) {
-        card->setBeingPointedAt(false);
-        card->setHovered(false);
-
         const QList<CardItem *> &attachedCards = card->getAttachedCards();
         for (auto attachedCard : attachedCards) {
             emit targetZone->cardAdded(attachedCard);
         }
-
-        if (startZone->getPlayer() != targetZone->getPlayer()) {
-            card->setOwner(targetZone->getPlayer());
-        }
     }
 
     // The log event has to be sent before the card is added to the target zone

cockatrice/src/game/player/player_event_handler.h

@@ -28,6 +28,7 @@
 #include <sys/types.h>
 
 class CardItem;
+class CardState;
 class CardZoneLogic;
 class PlayerLogic;
 class Event_AttachCard;
@@ -268,6 +269,14 @@ signals:
 
     void cardZoneChanged(CardItem *card, bool sameZone);
     void requestCardMenuUpdate(CardItem *card);
+    /**
+     * @brief A card was just moved by the server and its view needs refreshing.
+     *
+     * @param card The card that moved.
+     * @param zoneChanged Whether the card crossed a zone boundary.
+     * @param newOwner The player the card now belongs to, or nullptr to leave the owner alone.
+     */
+    void cardViewRefreshRequested(CardState *card, bool zoneChanged, PlayerLogic *newOwner);
 
 private:
     /** Owning player instance. */

cockatrice/src/game/zones/card_zone_logic.cpp

@@ -2,9 +2,6 @@
 
 #include "../../client/translation.h"
 #include "../../game_graphics/board/card_item.h"
-#include "../../game_graphics/zones/card_zone.h"
-#include "../../game_graphics/zones/view_zone.h"
-#include "../board/card_list.h"
 #include "../player/player_actions.h"
 #include "../player/player_info.h"
 #include "../player/player_logic.h"
@@ -47,16 +44,16 @@ CardZoneLogic::CardZoneLogic(PlayerLogic *_player,
 void CardZoneLogic::addCard(CardItem *card, const bool reorganize, const int x, const int y)
 {
     if (!card) {
-        qCWarning(CardZoneLog) << "CardZoneLogic::addCard() card is null; this shouldn't normally happen";
+        qCWarning(CardZoneLogicLog) << "CardZoneLogic::addCard() card is null; this shouldn't normally happen";
         return;
     }
 
-    for (auto *view : views) {
+    for (auto *viewLogic : views) {
-        if (qobject_cast<ZoneViewZoneLogic *>(view->getLogic())->prepareAddCard(x)) {
+        if (viewLogic->prepareAddCard(x)) {
             auto copy = new CardItem(player, nullptr, card->getCardRef(), card->getId());
             copy->setFaceDown(card->getFaceDown());
 
-            view->getLogic()->addCard(copy, reorganize, x, y);
+            viewLogic->addCard(copy, reorganize, x, y);
         }
     }
 
@@ -90,8 +87,8 @@ CardItem *CardZoneLogic::takeCard(int position, int cardId, bool toNewZone)
         return nullptr;
     }
 
-    for (auto *view : views) {
+    for (auto *viewLogic : views) {
-        qobject_cast<ZoneViewZoneLogic *>(view->getLogic())->removeCard(position, toNewZone);
+        viewLogic->removeCard(position, toNewZone);
     }
 
     CardItem *c = cards.takeAt(position);
@@ -107,7 +104,7 @@ CardItem *CardZoneLogic::getCard(int cardId)
 {
     CardItem *c = cards.findCard(cardId);
     if (!c) {
-        qCWarning(CardZoneLog) << "CardZoneLogic::getCard: card id=" << cardId << "not found";
+        qCWarning(CardZoneLogicLog) << "CardZoneLogic::getCard: card id=" << cardId << "not found";
         return nullptr;
     }
     // If the card's id is -1, this zone is invisible,
@@ -122,7 +119,7 @@ CardItem *CardZoneLogic::getCard(int cardId)
 void CardZoneLogic::removeCard(CardItem *card)
 {
     if (!card) {
-        qCWarning(CardZoneLog) << "CardZoneLogic::removeCard: card is null, this shouldn't normally happen";
+        qCWarning(CardZoneLogicLog) << "CardZoneLogic::removeCard: card is null, this shouldn't normally happen";
         return;
     }
 

cockatrice/src/game/zones/card_zone_logic.h

@@ -21,7 +21,11 @@ class CardItem;
 inline Q_LOGGING_CATEGORY(CardZoneLogicLog, "card_zone_logic");
 
 class PlayerLogic;
-class ZoneViewZone;
+class ZoneViewZoneLogic;
+class QMenu;
+class QAction;
+class QPainter;
+class CardDragItem;
 
 class CardZoneLogic : public QObject
 {
@@ -79,7 +83,7 @@ public:
     {
         return cards.getContentsKnown();
     }
-    QList<ZoneViewZone *> &getViews()
+    QList<ZoneViewZoneLogic *> &getViews()
     {
         return views;
     }
@@ -111,7 +115,7 @@ protected:
     PlayerLogic *player;
     QString name;
     CardList cards;
-    QList<ZoneViewZone *> views;
+    QList<ZoneViewZoneLogic *> views;
     bool hasCardAttr;
     bool isShufflable;
     bool alwaysRevealTopCard;

cockatrice/src/game/zones/view_zone_logic.cpp

@@ -162,6 +162,11 @@ void ZoneViewZoneLogic::removeCard(int position, bool toNewZone)
     reorganizeCards();
 }
 
+void ZoneViewZoneLogic::requestClose()
+{
+    emit closeView();
+}
+
 void ZoneViewZoneLogic::clearCards()
 {
     for (auto card : cards) {

cockatrice/src/game/zones/view_zone_logic.h

@@ -48,6 +48,13 @@ public:
     void removeCard(int position, bool toNewZone);
     void updateCardIds(CardAction action);
 
+    /**
+     * @brief Asks the graphics view showing this zone to close itself.
+     *
+     * Logic code has no handle on the view, so it goes through the logic object the view was built from.
+     */
+    void requestClose();
+
     /** @brief Removes all cards from the view, without touching anything else. Used in replay rewind */
     void clearCards();
 

cockatrice/src/game_graphics/game_scene.cpp

@@ -291,6 +291,8 @@ void GameScene::addPlayer(PlayerLogic *player)
             [this, id = player->getPlayerInfo()->getId()]() { clearArrowsForPlayerLocally(id); });
 
     connect(player->getPlayerEventHandler(), &PlayerEventHandler::cardZoneChanged, this, &GameScene::onCardZoneChanged);
+    connect(player->getPlayerEventHandler(), &PlayerEventHandler::cardViewRefreshRequested, this,
+            &GameScene::onCardViewRefreshRequested);
 
     rearrange();
 }
@@ -650,6 +652,25 @@ void GameScene::onCardZoneChanged(CardItem *card, bool sameZone)
     }
 }
 
+void GameScene::onCardViewRefreshRequested(CardState *card, bool zoneChanged, PlayerLogic *newOwner)
+{
+    auto *item = qobject_cast<CardItem *>(card->parent());
+    if (!item) {
+        return;
+    }
+    if (zoneChanged) {
+        item->deleteCardInfoPopup();
+    }
+    item->deleteDragItem();
+    if (zoneChanged) {
+        item->setBeingPointedAt(false);
+        item->setHovered(false);
+    }
+    if (newOwner) {
+        item->setOwner(newOwner);
+    }
+}
+
 void GameScene::clearArrowsForPlayer(int playerId)
 {
     for (int arrowId : arrowRegistry.idsForPlayer(playerId)) {

cockatrice/src/game_graphics/game_scene.h

@@ -43,6 +43,7 @@ class ZoneViewWidget;
 class CardZone;
 class AbstractCardItem;
 class CardItem;
+class CardState;
 class ServerInfo_Card;
 class PhasesToolbar;
 class QBasicTimer;
@@ -282,6 +283,14 @@ public slots:
     void requestArrowDeletion(int playerId, int arrowId);
 
     void onCardZoneChanged(CardItem *card, bool sameZone);
+    /**
+     * @brief Refreshes the view of a card that the server just moved.
+     *
+     * @param card The card that moved.
+     * @param zoneChanged Whether the card crossed a zone boundary.
+     * @param newOwner The player the card now belongs to, or nullptr to leave the owner alone.
+     */
+    void onCardViewRefreshRequested(CardState *card, bool zoneChanged, PlayerLogic *newOwner);
 
 protected:
     /** @brief Handles hover updates. */

cockatrice/src/game_graphics/zones/view_zone.cpp

@@ -40,21 +40,23 @@ class QGraphicsItem;
 ZoneViewZone::ZoneViewZone(ZoneViewZoneLogic *_logic, QGraphicsItem *parent)
     : SelectZone(_logic, parent), bRect(QRectF()), minRows(0), groupBy(CardList::NoSort), sortBy(CardList::NoSort)
 {
-    if (!(qobject_cast<ZoneViewZoneLogic *>(getLogic())->getRevealZone() &&
+    auto *viewLogic = qobject_cast<ZoneViewZoneLogic *>(getLogic());
-          !qobject_cast<ZoneViewZoneLogic *>(getLogic())->getWriteableRevealZone())) {
+    if (!(viewLogic->getRevealZone() && !viewLogic->getWriteableRevealZone())) {
-        qobject_cast<ZoneViewZoneLogic *>(getLogic())->getOriginalZone()->getViews().append(this);
+        viewLogic->getOriginalZone()->getViews().append(viewLogic);
     }
     connect(_logic, &ZoneViewZoneLogic::closeView, this, &ZoneViewZone::close);
 }
 
 void ZoneViewZone::addToViews()
 {
-    qobject_cast<ZoneViewZoneLogic *>(getLogic())->getOriginalZone()->getViews().append(this);
+    auto *viewLogic = qobject_cast<ZoneViewZoneLogic *>(getLogic());
+    viewLogic->getOriginalZone()->getViews().append(viewLogic);
 }
 
 void ZoneViewZone::removeFromViews()
 {
-    qobject_cast<ZoneViewZoneLogic *>(getLogic())->getOriginalZone()->getViews().removeOne(this);
+    auto *viewLogic = qobject_cast<ZoneViewZoneLogic *>(getLogic());
+    viewLogic->getOriginalZone()->getViews().removeOne(viewLogic);
 }
 
 /**
@@ -64,9 +66,9 @@ void ZoneViewZone::removeFromViews()
 void ZoneViewZone::close()
 {
     emit closed();
-    if (!(qobject_cast<ZoneViewZoneLogic *>(getLogic())->getRevealZone() &&
+    auto *viewLogic = qobject_cast<ZoneViewZoneLogic *>(getLogic());
-          !qobject_cast<ZoneViewZoneLogic *>(getLogic())->getWriteableRevealZone())) {
+    if (!(viewLogic->getRevealZone() && !viewLogic->getWriteableRevealZone())) {
-        qobject_cast<ZoneViewZoneLogic *>(getLogic())->getOriginalZone()->getViews().removeOne(this);
+        viewLogic->getOriginalZone()->getViews().removeOne(viewLogic);
     }
     deleteLater();
 }