From 80986266b9c0cd910d135bf9be9d4de1fd0188b9 Mon Sep 17 00:00:00 2001 From: unf0rce <37348972+unf0rce@users.noreply.github.com> Date: Sat, 7 Nov 2026 16:53:53 +0100 Subject: [PATCH] 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 --- .../src/game_graphics/board/arrow_item.cpp | 76 ++++++++++++++----- .../src/game_graphics/board/arrow_item.h | 8 +- .../src/game_graphics/board/card_item.cpp | 14 ++++ .../src/game_graphics/board/card_item.h | 3 + 4 files changed, 78 insertions(+), 23 deletions(-) diff --git a/cockatrice/src/game_graphics/board/arrow_item.cpp b/cockatrice/src/game_graphics/board/arrow_item.cpp index f7df98ab5..b07a156bd 100644 --- a/cockatrice/src/game_graphics/board/arrow_item.cpp +++ b/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(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(); } diff --git a/cockatrice/src/game_graphics/board/arrow_item.h b/cockatrice/src/game_graphics/board/arrow_item.h index 64a48cd14..c70a1030b 100644 --- a/cockatrice/src/game_graphics/board/arrow_item.h +++ b/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 childArrows; + QList> 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 childArrows; + QList> childArrows; QMetaObject::Connection positionConnection; public: explicit ArrowAttachItem(ArrowTarget *_startItem); void addChildArrow(ArrowAttachItem *child); + void cancel() override; protected: void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override; diff --git a/cockatrice/src/game_graphics/board/card_item.cpp b/cockatrice/src/game_graphics/board/card_item.cpp index acd0764c9..02c0427d7 100644 --- a/cockatrice/src/game_graphics/board/card_item.cpp +++ b/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()) { diff --git a/cockatrice/src/game_graphics/board/card_item.h b/cockatrice/src/game_graphics/board/card_item.h index 01dacc021..ae22d9493 100644 --- a/cockatrice/src/game_graphics/board/card_item.h +++ b/cockatrice/src/game_graphics/board/card_item.h @@ -13,11 +13,13 @@ #include #include #include +#include #include #include #include #include +class ArrowItem; class CardDragItem; class ServerInfo_Card; class PlayerLogic; @@ -34,6 +36,7 @@ class CardItem : public AbstractCardItem Q_OBJECT private: CardDragItem *dragItem; + QPointer pendingArrow; void prepareDelete(); void handleClickedToPlay(bool shiftHeld);