Fix orphaned arrows when the draw shortcut repeats (#6789)

Holding or spamming the draw-arrow shortcut fires the action again
before the previous local arrow has been placed. Every new arrow takes
the mouse grab from the previous one, so the previous arrow never
receives the release that deletes it. If the cursor had passed over a
card meanwhile, the orphan stayed visible on the board.

Track the in-progress arrow on the CardItem that started it and cancel
it, together with any child arrows, before starting a new one. The
attach-arrow shortcut (Ctrl+Alt+A) uses the same path, so it no longer
leaks arrows either.

Fixes #4329.

Co-authored-by: mb <mbaecker@unforce.de>

cockatrice/src/game_graphics/board/arrow_item.cpp

@@ -88,7 +88,7 @@ void ArrowItem::onTargetDestroyed()
         // Drag and attach arrows are never inserted into the arrow registry and
         // have no server-side counterpart, so no deletion event can clean them
         // up. Delete them locally when either endpoint is destroyed.
-        delArrow();
+        cancel();
         return;
     }
 
@@ -103,6 +103,12 @@ void ArrowItem::delArrow()
     deleteLater();
 }
 
+void ArrowItem::cancel()
+{
+    ungrabMouse();
+    delArrow();
+}
+
 void ArrowItem::updatePath()
 {
     if (!targetItem) {
@@ -350,6 +356,17 @@ void ArrowDragItem::addChildArrow(ArrowDragItem *child)
     childArrows.append(child);
 }
 
+void ArrowDragItem::cancel()
+{
+    for (const auto &child : childArrows) {
+        if (child) {
+            child->cancel();
+        }
+    }
+    childArrows.clear();
+    ArrowItem::cancel();
+}
+
 void ArrowDragItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
 {
     if (targetLocked || !startItem) {
@@ -393,8 +410,10 @@ void ArrowDragItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
     targetItem ? updatePath() : updatePath(endPos);
     update();
 
-    for (auto *child : childArrows) {
-        child->mouseMoveEvent(event);
+    for (const auto &child : childArrows) {
+        if (child) {
+            child->mouseMoveEvent(event);
+        }
     }
 }
 
@@ -403,10 +422,7 @@ void ArrowDragItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
     if (!startItem) {
         // The source card was destroyed while the arrow was being drawn.
         // Clean up the arrow and its children instead of leaking them.
-        delArrow();
-        for (auto *child : childArrows) {
-            child->mouseReleaseEvent(event);
-        }
+        cancel();
         return;
     }
 
@@ -415,7 +431,7 @@ void ArrowDragItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
         // For now, we can safely assume that the start item is always a card.
         // The target item can be a player as well.
         if (!startCard) {
-            delArrow();
+            cancel();
             return;
         }
 
@@ -435,7 +451,7 @@ void ArrowDragItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
         } else if (auto *targetPlayer = qgraphicsitem_cast<PlayerTarget *>(targetItem)) {
             arrow.targetPlayerId = targetPlayer->getOwner()->getPlayerInfo()->getId();
         } else {
-            delArrow();
+            cancel();
             return;
         }
 
@@ -448,9 +464,12 @@ void ArrowDragItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
     }
 
     delArrow();
-    for (auto *child : childArrows) {
-        child->mouseReleaseEvent(event);
+    for (const auto &child : childArrows) {
+        if (child) {
+            child->mouseReleaseEvent(event);
+        }
     }
+    childArrows.clear();
 }
 
 // ArrowAttachItem
@@ -471,6 +490,17 @@ void ArrowAttachItem::addChildArrow(ArrowAttachItem *child)
     childArrows.append(child);
 }
 
+void ArrowAttachItem::cancel()
+{
+    for (const auto &child : childArrows) {
+        if (child) {
+            child->cancel();
+        }
+    }
+    childArrows.clear();
+    ArrowItem::cancel();
+}
+
 void ArrowAttachItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
 {
     if (targetLocked || !startItem) {
@@ -509,8 +539,10 @@ void ArrowAttachItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
     targetItem ? updatePath() : updatePath(endPos);
     update();
 
-    for (auto *child : childArrows) {
-        child->mouseMoveEvent(event);
+    for (const auto &child : childArrows) {
+        if (child) {
+            child->mouseMoveEvent(event);
+        }
     }
 }
 
@@ -519,17 +551,16 @@ void ArrowAttachItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
     if (!startItem) {
         // The source card was destroyed while the arrow was being drawn.
         // Clean up the arrow and its children instead of leaking them.
-        delArrow();
-        for (auto *child : childArrows) {
-            child->mouseReleaseEvent(event);
-        }
+        cancel();
         return;
     }
 
     // Attaching could move startItem under the current cursor position, causing all children to retarget to it right
     // before they are processed. Prevent that.
-    for (auto *child : childArrows) {
-        child->setTargetLocked(true);
+    for (const auto &child : childArrows) {
+        if (child) {
+            child->setTargetLocked(true);
+        }
     }
 
     if (targetItem && targetItem != startItem) {
@@ -541,7 +572,10 @@ void ArrowAttachItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
     }
 
     delArrow();
-    for (auto *child : childArrows) {
-        child->mouseReleaseEvent(event);
+    for (const auto &child : childArrows) {
+        if (child) {
+            child->mouseReleaseEvent(event);
+        }
     }
+    childArrows.clear();
 }

cockatrice/src/game_graphics/board/arrow_item.h

@@ -70,6 +70,8 @@ public:
 
     void onTargetDestroyed();
     void delArrow();
+    /// Abandons an in-progress drag arrow, discarding any child arrows with it.
+    virtual void cancel();
     void updatePath();
     void updatePath(const QPointF &endPoint);
     void startDrawAnimation();
@@ -112,12 +114,13 @@ class ArrowDragItem : public ArrowItem
 private:
     PlayerLogic *player;
     int deleteInPhase;
-    QList<ArrowDragItem *> childArrows;
+    QList<QPointer<ArrowDragItem>> childArrows;
     QMetaObject::Connection positionConnection;
 
 public:
     ArrowDragItem(PlayerLogic *_owner, ArrowTarget *_startItem, const QColor &_color, int _deleteInPhase);
     void addChildArrow(ArrowDragItem *child);
+    void cancel() override;
 
 protected:
     void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override;
@@ -129,12 +132,13 @@ class ArrowAttachItem : public ArrowItem
     Q_OBJECT
 private:
     PlayerLogic *player;
-    QList<ArrowAttachItem *> childArrows;
+    QList<QPointer<ArrowAttachItem>> childArrows;
     QMetaObject::Connection positionConnection;
 
 public:
     explicit ArrowAttachItem(ArrowTarget *_startItem);
     void addChildArrow(ArrowAttachItem *child);
+    void cancel() override;
 
 protected:
     void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override;

cockatrice/src/game_graphics/board/card_item.cpp

@@ -294,6 +294,13 @@ void CardItem::drawArrow(const QColor &arrowColor)
         return;
     }
 
+    // A held shortcut repeats the action before the previous arrow is placed.
+    // That arrow has lost the mouse grab to the new one, so it would never
+    // receive the release that deletes it; discard it (and its children).
+    if (pendingArrow) {
+        pendingArrow->cancel();
+    }
+
     auto *game = owner->getGame();
     PlayerLogic *arrowOwner = game->getPlayerManager()->getActiveLocalPlayer(game->getGameState()->getActivePlayer());
     int phase = 0; // 0 means to not set the phase
@@ -303,6 +310,7 @@ void CardItem::drawArrow(const QColor &arrowColor)
     }
     ArrowDragItem *arrow = new ArrowDragItem(arrowOwner, this, arrowColor, phase);
     scene()->addItem(arrow);
+    pendingArrow = arrow;
     arrow->grabMouse();
 
     for (const auto &item : scene()->selectedItems()) {
@@ -326,8 +334,14 @@ void CardItem::drawAttachArrow()
         return;
     }
 
+    // See drawArrow(): a repeated trigger must not orphan the previous arrow.
+    if (pendingArrow) {
+        pendingArrow->cancel();
+    }
+
     auto *arrow = new ArrowAttachItem(this);
     scene()->addItem(arrow);
+    pendingArrow = arrow;
     arrow->grabMouse();
 
     for (const auto &item : scene()->selectedItems()) {

cockatrice/src/game_graphics/board/card_item.h

@@ -13,11 +13,13 @@
 #include <QList>
 #include <QPoint>
 #include <QPointF>
+#include <QPointer>
 #include <QString>
 #include <QVariantList>
 #include <libcockatrice/game/board/card_state.h>
 #include <qtmetamacros.h>
 
+class ArrowItem;
 class CardDragItem;
 class ServerInfo_Card;
 class PlayerLogic;
@@ -34,6 +36,7 @@ class CardItem : public AbstractCardItem
     Q_OBJECT
 private:
     CardDragItem *dragItem;
+    QPointer<ArrowItem> pendingArrow;
 
     void prepareDelete();
     void handleClickedToPlay(bool shiftHeld);