[VDS] Decouple tag filter and fix reordered-chips crash (#7242)

* [VDS] Decouple tag filter and fix reordered-chips crash

* [VDS] Address review: dead code, chip reparenting, filter signal and sort fast-path

* [VDS] Address second round of review nits

---------

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

cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_color_identity_filter_widget.cpp

@@ -1,11 +1,10 @@
 #include "deck_preview_color_identity_filter_widget.h"
 
 #include "../../cards/additional_info/mana_symbol_widget.h"
-#include "../visual_deck_storage_widget.h"
 
 #include <QSet>
 
-DeckPreviewColorIdentityFilterWidget::DeckPreviewColorIdentityFilterWidget(VisualDeckStorageWidget *parent)
+DeckPreviewColorIdentityFilterWidget::DeckPreviewColorIdentityFilterWidget(QWidget *parent)
     : QWidget(parent), layout(new QHBoxLayout(this))
 {
     setLayout(layout);

cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_color_identity_filter_widget.h

@@ -14,14 +14,12 @@
 #include <QSet>
 #include <QWidget>
 
-class VisualDeckStorageWidget;
-
 class DeckPreviewColorIdentityFilterWidget : public QWidget
 {
     Q_OBJECT
 
 public:
-    explicit DeckPreviewColorIdentityFilterWidget(VisualDeckStorageWidget *parent);
+    explicit DeckPreviewColorIdentityFilterWidget(QWidget *parent = nullptr);
     void retranslateUi();
 
     /**

cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_tag_display_widget.cpp

@@ -48,6 +48,7 @@ QSize DeckPreviewTagDisplayWidget::sizeHint() const
 
 void DeckPreviewTagDisplayWidget::mousePressEvent(QMouseEvent *event)
 {
+    const TagState previousState = state;
     switch (event->button()) {
         case Qt::LeftButton:
             setState(state != TagState::Selected ? TagState::Selected : TagState::NotSelected);
@@ -62,7 +63,12 @@ void DeckPreviewTagDisplayWidget::mousePressEvent(QMouseEvent *event)
             break;
     }
 
+    // Only announce a change when the state was actually toggled, so a click that falls
+    // through the switch (e.g. a button the widget does not react to) does not drive a
+    // full tag-filter update and layout pass for nothing.
+    if (state != previousState) {
         emit tagClicked();
+    }
     QWidget::mousePressEvent(event);
 }
 

cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_tag_display_widget.h

@@ -48,7 +48,10 @@ public:
 
 signals:
     /**
-     * @brief Emitted when the tag is clicked.
+     * @brief Emitted when a click toggles the chip's selection/exclusion state.
+     *
+     * Not emitted for clicks that leave the state unchanged. Connected handlers use
+     * this as the trigger to update filters built from selectedTags()/excludedTags().
      */
     void tagClicked();
 

cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_tag_filter_widget.cpp

@@ -2,14 +2,10 @@
 
 #include "../general/layout_containers/flow_widget.h"
 #include "deck_preview/deck_preview_tag_display_widget.h"
-#include "visual_deck_storage_model.h"
-#include "visual_deck_storage_sort_filter_proxy_model.h"
-#include "visual_deck_storage_widget.h"
 
 #include <QHBoxLayout>
 
-VisualDeckStorageTagFilterWidget::VisualDeckStorageTagFilterWidget(VisualDeckStorageWidget *_parent)
-    : QWidget(_parent), parent(_parent)
+VisualDeckStorageTagFilterWidget::VisualDeckStorageTagFilterWidget(QWidget *parent) : QWidget(parent)
 {
 
     setSizePolicy(QSizePolicy::Minimum, QSizePolicy::Minimum);
@@ -25,99 +21,72 @@ VisualDeckStorageTagFilterWidget::VisualDeckStorageTagFilterWidget(VisualDeckSto
     layout->addWidget(flowWidget);
 }
 
+void VisualDeckStorageTagFilterWidget::setAllTagsProvider(const std::function<QSet<QString>()> &provider)
+{
+    allTagsProvider = provider;
+}
+
 void VisualDeckStorageTagFilterWidget::showEvent(QShowEvent *event)
 {
     QWidget::showEvent(event);
     refreshTags();
 }
 
-/**
- * @brief The tags of all decks currently accepted by the proxy model.
- */
-QSet<QString> VisualDeckStorageTagFilterWidget::gatherAllTags() const
-{
-    QSet<QString> allTags;
-    auto *proxy = parent->proxyModel();
-
-    for (int proxyRow = 0; proxyRow < proxy->rowCount(); ++proxyRow) {
-        const QModelIndex index = proxy->index(proxyRow, 0);
-        if (!index.data(VisualDeckStorageRoles::FilterMatchRole).toBool()) {
-            continue;
-        }
-        const QStringList deckTags = index.data(VisualDeckStorageRoles::TagsRole).toStringList();
-        for (const QString &tag : deckTags) {
-            allTags.insert(tag);
-        }
-    }
-
-    return allTags;
-}
-
 void VisualDeckStorageTagFilterWidget::refreshTags()
 {
-    QSet<QString> allTags = gatherAllTags();
-    removeTagsNotInList(allTags);
-    addTagsIfNotPresent(allTags);
-    sortTags();
-}
+    const QSet<QString> allTags = allTagsProvider ? allTagsProvider() : QSet<QString>();
 
-void VisualDeckStorageTagFilterWidget::removeTagsNotInList(const QSet<QString> &tags)
-{
+    // Existing chips survive if their tag is still part of the deck set, or if the chip
+    // is currently selected/excluded. Everything else is dropped. Dropped chips must NOT
+    // be re-added to the layout afterwards: they are reparented to nullptr and scheduled
+    // for a deferred delete (QWidget::setParent(nullptr) also hides them), and the flow
+    // layout would keep a dangling reference to them once the deletion runs on the next
+    // event-loop cycle.
+    QList<DeckPreviewTagDisplayWidget *> chips;
     for (DeckPreviewTagDisplayWidget *tagWidget : findChildren<DeckPreviewTagDisplayWidget *>()) {
-        const QString &tagName = tagWidget->getTagName();
-
-        // Keep the tag widget if it is either selected or excluded
-        if (!tags.contains(tagName) && tagWidget->getState() == TagState::NotSelected) {
+        if (tagWidget->getState() != TagState::NotSelected || allTags.contains(tagWidget->getTagName())) {
+            chips.append(tagWidget);
+        } else {
             flowWidget->removeWidget(tagWidget);
+            tagWidget->setParent(nullptr);
             tagWidget->deleteLater();
         }
     }
-}
 
-void VisualDeckStorageTagFilterWidget::addTagsIfNotPresent(const QSet<QString> &tags)
-{
-    for (const QString &tag : tags) {
-        addTagIfNotPresent(tag);
+    // Add chips for tags that are not shown yet.
+    QSet<QString> existingTags;
+    for (DeckPreviewTagDisplayWidget *tagWidget : chips) {
+        existingTags.insert(tagWidget->getTagName());
     }
-}
-
-void VisualDeckStorageTagFilterWidget::addTagIfNotPresent(const QString &tag)
-{
-    // Check if the tag already exists in the flow widget
-    bool tagExists = false;
-    for (DeckPreviewTagDisplayWidget *tagWidget : findChildren<DeckPreviewTagDisplayWidget *>()) {
-        if (tagWidget->getTagName() == tag) {
-            tagExists = true;
-            break;
-        }
-    }
-
-    // If the tag doesn't exist, add a new DeckPreviewTagDisplayWidget
-    if (!tagExists) {
+    for (const QString &tag : allTags) {
+        if (!existingTags.contains(tag)) {
             auto *newTagWidget = new DeckPreviewTagDisplayWidget(this, tag);
-        connect(newTagWidget, &DeckPreviewTagDisplayWidget::tagClicked, parent,
-                &VisualDeckStorageWidget::updateTagFilter);
+            connect(newTagWidget, &DeckPreviewTagDisplayWidget::tagClicked, this,
+                    &VisualDeckStorageTagFilterWidget::filterChanged);
             flowWidget->addWidget(newTagWidget);
+            chips.append(newTagWidget);
         }
     }
 
-void VisualDeckStorageTagFilterWidget::sortTags()
-{
-    // Get all tag widgets
-    QList<DeckPreviewTagDisplayWidget *> tagWidgets = findChildren<DeckPreviewTagDisplayWidget *>();
-
-    // Sort widgets by tag name
-    std::sort(tagWidgets.begin(), tagWidgets.end(), [](DeckPreviewTagDisplayWidget *a, DeckPreviewTagDisplayWidget *b) {
-        return a->getTagName().toLower() < b->getTagName().toLower();
+    // Sort, but skip the full remove/re-add when the order already matches the layout.
+    // FlowWidget inherits QLayout::removeWidget's linear scan, so rebuilding an unchanged
+    // order would be quadratic plus a full relayout on every chip click and load batch.
+    std::sort(chips.begin(), chips.end(), [](DeckPreviewTagDisplayWidget *a, DeckPreviewTagDisplayWidget *b) {
+        const QString aName = a->getTagName();
+        const QString bName = b->getTagName();
+        const int compared = aName.compare(bName, Qt::CaseInsensitive);
+        return compared != 0 ? compared < 0 : aName < bName;
     });
-
-    // Clear and re-add widgets in sorted order
-    for (DeckPreviewTagDisplayWidget *tagWidget : tagWidgets) {
+    if (chips == currentChipOrder) {
+        return;
+    }
+    for (DeckPreviewTagDisplayWidget *tagWidget : chips) {
         flowWidget->removeWidget(tagWidget);
     }
-    for (DeckPreviewTagDisplayWidget *tagWidget : tagWidgets) {
+    for (DeckPreviewTagDisplayWidget *tagWidget : chips) {
         flowWidget->addWidget(tagWidget);
     }
+    currentChipOrder = chips;
 }
 
 QStringList VisualDeckStorageTagFilterWidget::selectedTags() const

cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_tag_filter_widget.h

@@ -9,26 +9,28 @@
 #include <QSet>
 #include <QStringList>
 #include <QWidget>
+#include <functional>
 
+class DeckPreviewTagDisplayWidget;
 class FlowWidget;
-class VisualDeckStorageWidget;
+
 class VisualDeckStorageTagFilterWidget : public QWidget
 {
     Q_OBJECT
 
-    VisualDeckStorageWidget *parent;
     FlowWidget *flowWidget;
-
-    [[nodiscard]] QSet<QString> gatherAllTags() const;
-    void removeTagsNotInList(const QSet<QString> &tags);
-    void addTagsIfNotPresent(const QSet<QString> &tags);
-    void addTagIfNotPresent(const QString &tag);
-    void sortTags();
+    std::function<QSet<QString>()> allTagsProvider;
+    QList<DeckPreviewTagDisplayWidget *> currentChipOrder;
 
 public:
-    explicit VisualDeckStorageTagFilterWidget(VisualDeckStorageWidget *_parent);
+    explicit VisualDeckStorageTagFilterWidget(QWidget *parent = nullptr);
     [[nodiscard]] QStringList getAllKnownTags() const;
 
+    /**
+     * @brief Sets a provider for the full set of tags to draw chips from.
+     */
+    void setAllTagsProvider(const std::function<QSet<QString>()> &provider);
+
     /**
      * @brief The tags currently in "selected" state.
      */
@@ -39,9 +41,18 @@ public:
      */
     [[nodiscard]] QStringList excludedTags() const;
 
+signals:
+    /**
+     * Emitted when a chip's selection or exclusion state changes.
+     *
+     * The chip only emits when its state actually changed, so this fires once per
+     * effective toggle rather than on every click.
+     */
+    void filterChanged();
+
 public slots:
     /**
-     * @brief Rebuilds the tag chips from the tags of the currently visible decks.
+     * @brief Rebuilds the tag chips from the currently available tags.
      */
     void refreshTags();
     void showEvent(QShowEvent *event) override;

cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_widget.cpp

@@ -62,6 +62,9 @@ VisualDeckStorageWidget::VisualDeckStorageWidget(QWidget *parent) : QWidget(pare
 
     // tag filter box
     tagFilterWidget = new VisualDeckStorageTagFilterWidget(this);
+    tagFilterWidget->setAllTagsProvider([this] { return gatherVisibleTags(); });
+    connect(tagFilterWidget, &VisualDeckStorageTagFilterWidget::filterChanged, this,
+            &VisualDeckStorageWidget::updateTagFilter);
     updateTagsVisibility(SettingsCache::instance().visualDeckStorage().getVisualDeckStorageShowTagFilter());
 
     deckPreviewSelectionAnimationEnabled =
@@ -217,6 +220,25 @@ void VisualDeckStorageWidget::updateTagFilter()
     tagFilterWidget->refreshTags();
 }
 
+/**
+ * @brief The tags of all decks currently accepted by the proxy model.
+ */
+QSet<QString> VisualDeckStorageWidget::gatherVisibleTags() const
+{
+    QSet<QString> allTags;
+    for (int proxyRow = 0; proxyRow < storageProxyModel->rowCount(); ++proxyRow) {
+        const QModelIndex index = storageProxyModel->index(proxyRow, 0);
+        if (!index.data(VisualDeckStorageRoles::FilterMatchRole).toBool()) {
+            continue;
+        }
+        const QStringList deckTags = index.data(VisualDeckStorageRoles::TagsRole).toStringList();
+        for (const QString &tag : deckTags) {
+            allTags.insert(tag);
+        }
+    }
+    return allTags;
+}
+
 /**
  * Pushes the color identity filter widget's state into the proxy model.
  */

cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_widget.h

@@ -70,6 +70,7 @@ protected:
 
 private:
     void reapplySortAndFilters();
+    [[nodiscard]] QSet<QString> gatherVisibleTags() const;
 
 private:
     QVBoxLayout *layout;