Properly delete popup, address comments.

Took 31 minutes

Took 5 seconds

cockatrice/CMakeLists.txt

@@ -145,9 +145,9 @@ set(cockatrice_SOURCES
     src/interface/layouts/overlap_layout.cpp
     src/interface/widgets/utility/card_completer_delegate.cpp
     src/interface/widgets/utility/card_completer_styler.cpp
-    src/interface/widgets/utility/card_completer_utils.cpp
-    src/interface/widgets/utility/card_completer_utils.h
+    src/interface/widgets/utility/completer_utils.cpp
     src/interface/widgets/utility/line_edit_completer.cpp
+    src/interface/widgets/utility/reversed_completer_model.cpp
     src/interface/pixel_map_generator.cpp
     src/interface/theme_config.cpp
     src/interface/theme_manager.cpp

cockatrice/src/interface/widgets/server/user/user_card_settings_dialog.cpp

@@ -1,7 +1,7 @@
 #include "user_card_settings_dialog.h"
 
 #include "../../../card_picture_loader/card_picture_loader.h"
-#include "../../utility/card_completer_utils.h"
+#include "../../utility/completer_utils.h"
 #include "card_database_display_model.h"
 #include "card_database_model.h"
 #include "user_card_art_provider.h"

cockatrice/src/interface/widgets/tabs/api/archidekt/tab_archidekt.cpp

@@ -2,7 +2,7 @@
 
 #include "../../../../../client/settings/cache_settings.h"
 #include "../../../cards/additional_info/mana_symbol_widget.h"
-#include "../../../utility/card_completer_utils.h"
+#include "../../../utility/completer_utils.h"
 #include "../../tab_supervisor.h"
 #include "api_response/archidekt_deck_listing_api_response.h"
 #include "display/archidekt_api_response_deck_display_widget.h"

cockatrice/src/interface/widgets/tabs/api/edhrec/tab_edhrec_main.cpp

@@ -1,7 +1,7 @@
 #include "tab_edhrec_main.h"
 
 #include "../../../../../client/settings/cache_settings.h"
-#include "../../../utility/card_completer_utils.h"
+#include "../../../utility/completer_utils.h"
 #include "../../tab_supervisor.h"
 #include "api_response/average_deck/edhrec_average_deck_api_response.h"
 #include "api_response/commander/edhrec_commander_api_response.h"

cockatrice/src/interface/widgets/tabs/tab_card_art_rules.cpp

@@ -1,6 +1,6 @@
 #include "tab_card_art_rules.h"
 
-#include "../utility/card_completer_utils.h"
+#include "../utility/completer_utils.h"
 #include "libcockatrice/card/database/card_database_manager.h"
 
 #include <QCompleter>

cockatrice/src/interface/widgets/tabs/tab_game.cpp

@@ -21,7 +21,7 @@
 #include "../interface/widgets/cards/card_info_frame_widget.h"
 #include "../interface/widgets/dialogs/dlg_create_game.h"
 #include "../interface/widgets/server/user/user_list_manager.h"
-#include "../interface/widgets/utility/card_completer_utils.h"
+#include "../interface/widgets/utility/completer_utils.h"
 #include "../interface/widgets/utility/line_edit_completer.h"
 #include "../interface/window_main.h"
 #include "../main.h"

cockatrice/src/interface/widgets/tabs/tab_room.cpp

@@ -8,7 +8,7 @@
 #include "../interface/widgets/server/user/user_list_manager.h"
 #include "../interface/widgets/server/user/user_list_widget.h"
 #include "../main.h"
-#include "../utility/card_completer_utils.h"
+#include "../utility/completer_utils.h"
 #include "card/card_completer_proxy_model.h"
 #include "card/card_search_model.h"
 #include "card_database_display_model.h"

cockatrice/src/interface/widgets/utility/card_completer_delegate.cpp

@@ -18,14 +18,14 @@
 namespace
 {
 
-struct ManaColour
+struct ManaColor
 {
     QColor fill;
     QColor rim;
     QColor text;
 };
 
-ManaColour manaColour(QChar symbol)
+ManaColor manaColour(QChar symbol)
 {
     switch (symbol.unicode()) {
         case 'W':
@@ -143,7 +143,7 @@ void CardCompleterDelegate::drawManaSymbol(QPainter *p, QPoint centre, const QSt
 
     const QString label = isNumeric ? QString::number(numVal) : symbol;
 
-    const ManaColour mc =
+    const ManaColor mc =
         (symbol.length() == 1 && QString("WUBRG").contains(symbol)) ? manaColour(symbol[0]) : manaColour(QChar('X'));
 
     QPainterPath circle;

cockatrice/src/interface/widgets/utility/card_completer_styler.cpp

@@ -2,9 +2,10 @@
 
 #include "../cards/card_info_picture_enlarged_widget.h"
 #include "card_completer_delegate.h"
+#include "reversed_completer_model.h"
 
+#include <QAbstractItemModel>
 #include <QAbstractItemView>
-#include <QAbstractProxyModel>
 #include <QCompleter>
 #include <QEvent>
 #include <QKeyEvent>
@@ -25,124 +26,6 @@ const int FadeDuration = 120;
 
 // ---------------------------------------------------------------------------
 
-/**
- * @brief A completer model that can present its rows bottom-to-top.
- *
- * The original row order is kept intact in the source model (row 0 is always the
- * closest match). When enabled, the proxy maps the source rows in reverse so the
- * popup shows the closest match in the row nearest to the text edit. Any change
- * in the source model is forwarded as a full reset, which is all QCompleter
- * needs to rebuild its completion list.
- */
-class ReversedCompleterModel : public QAbstractProxyModel
-{
-public:
-    using QAbstractProxyModel::QAbstractProxyModel;
-
-    void setSourceModel(QAbstractItemModel *sourceModel) override
-    {
-        if (sourceModel == this->sourceModel()) {
-            return;
-        }
-
-        if (QAbstractItemModel *old = this->sourceModel()) {
-            disconnect(old, nullptr, this, nullptr);
-        }
-
-        QAbstractProxyModel::setSourceModel(sourceModel);
-
-        if (sourceModel) {
-            connect(sourceModel, &QAbstractItemModel::modelReset, this, &ReversedCompleterModel::invalidate);
-            connect(sourceModel, &QAbstractItemModel::rowsInserted, this, &ReversedCompleterModel::invalidate);
-            connect(sourceModel, &QAbstractItemModel::rowsRemoved, this, &ReversedCompleterModel::invalidate);
-            connect(sourceModel, &QAbstractItemModel::rowsMoved, this, &ReversedCompleterModel::invalidate);
-            connect(sourceModel, &QAbstractItemModel::columnsInserted, this, &ReversedCompleterModel::invalidate);
-            connect(sourceModel, &QAbstractItemModel::columnsRemoved, this, &ReversedCompleterModel::invalidate);
-            connect(sourceModel, &QAbstractItemModel::dataChanged, this, &ReversedCompleterModel::invalidate);
-            connect(sourceModel, &QAbstractItemModel::layoutChanged, this, &ReversedCompleterModel::invalidate);
-        }
-
-        invalidate();
-    }
-
-    void setEnabled(bool enabled)
-    {
-        if (enabled == isEnabled) {
-            return;
-        }
-        isEnabled = enabled;
-        invalidate();
-    }
-
-    QModelIndex mapToSource(const QModelIndex &proxyIndex) const override
-    {
-        if (!proxyIndex.isValid() || !sourceModel()) {
-            return {};
-        }
-        const int sourceRow = isEnabled ? sourceRowCount() - 1 - proxyIndex.row() : proxyIndex.row();
-        return sourceModel()->index(sourceRow, proxyIndex.column());
-    }
-
-    QModelIndex mapFromSource(const QModelIndex &sourceIndex) const override
-    {
-        if (!sourceIndex.isValid() || !sourceModel()) {
-            return {};
-        }
-        const int proxyRow = isEnabled ? sourceRowCount() - 1 - sourceIndex.row() : sourceIndex.row();
-        return index(proxyRow, sourceIndex.column());
-    }
-
-    QModelIndex index(int row, int column, const QModelIndex &parent = {}) const override
-    {
-        if (parent.isValid() || !sourceModel() || row < 0 || row >= rowCount() || column < 0 ||
-            column >= columnCount()) {
-            return {};
-        }
-        return createIndex(row, column);
-    }
-
-    QModelIndex parent(const QModelIndex &) const override
-    {
-        return {};
-    }
-
-    int rowCount(const QModelIndex &parent = {}) const override
-    {
-        return parent.isValid() || !sourceModel() ? 0 : sourceModel()->rowCount();
-    }
-
-    int columnCount(const QModelIndex &parent = {}) const override
-    {
-        return parent.isValid() || !sourceModel() ? 0 : sourceModel()->columnCount();
-    }
-
-    QVariant data(const QModelIndex &proxyIndex, int role = Qt::DisplayRole) const override
-    {
-        return sourceModel() ? sourceModel()->data(mapToSource(proxyIndex), role) : QVariant();
-    }
-
-    QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override
-    {
-        return sourceModel() ? sourceModel()->headerData(section, orientation, role) : QVariant();
-    }
-
-private:
-    int sourceRowCount() const
-    {
-        return sourceModel() ? sourceModel()->rowCount() : 0;
-    }
-
-    void invalidate()
-    {
-        beginResetModel();
-        endResetModel();
-    }
-
-    bool isEnabled = false;
-};
-
-// ---------------------------------------------------------------------------
-
 void CardCompleterStyler::apply(QCompleter *completer)
 {
     if (!completer) {
@@ -181,6 +64,13 @@ CardCompleterStyler::CardCompleterStyler(QCompleter *completer, QObject *parent)
             &CardCompleterStyler::onCompletionReset);
 }
 
+CardCompleterStyler::~CardCompleterStyler()
+{
+    if (preview) {
+        preview->deleteLater();
+    }
+}
+
 // ---------------------------------------------------------------------------
 
 bool CardCompleterStyler::eventFilter(QObject *obj, QEvent *ev)
@@ -360,6 +250,8 @@ void CardCompleterStyler::showPreview()
         return;
     }
 
+    stopPreviewFade();
+
     preview->show();
     preview->raise();
 
@@ -379,15 +271,23 @@ void CardCompleterStyler::hidePreview()
         return;
     }
 
-    auto *fade = new QPropertyAnimation(preview, "windowOpacity", preview);
+    // Stop any in-flight fade first so a stale one cannot keep the preview visible
+    stopPreviewFade();
+    preview->hide();
+}
 
-    fade->setDuration(FadeDuration);
-    fade->setStartValue(preview->windowOpacity());
-    fade->setEndValue(0.0);
+// ---------------------------------------------------------------------------
 
-    connect(fade, &QPropertyAnimation::finished, preview, &QWidget::hide);
+void CardCompleterStyler::stopPreviewFade()
+{
+    if (!preview) {
+        return;
+    }
 
-    fade->start(QAbstractAnimation::DeleteWhenStopped);
+    const auto animations = preview->findChildren<QPropertyAnimation *>();
+    for (auto *animation : animations) {
+        animation->stop();
+    }
 }
 
 // ---------------------------------------------------------------------------

cockatrice/src/interface/widgets/utility/card_completer_styler.h

@@ -44,9 +44,11 @@ private slots:
 
 private:
     explicit CardCompleterStyler(QCompleter *completer, QObject *parent = nullptr);
+    ~CardCompleterStyler() override;
 
     void showPreview();
     void hidePreview();
+    void stopPreviewFade();
     void reposition();
 
     void updateOrientation();

cockatrice/src/interface/widgets/utility/completer_utils.cpp

@@ -1,4 +1,4 @@
-#include "card_completer_utils.h"
+#include "completer_utils.h"
 
 #include "card_completer_styler.h"
 

cockatrice/src/interface/widgets/utility/completer_utils.h

@@ -1,11 +1,11 @@
 /**
- * @file card_completer_utils.h
+ * @file completer_utils.h
  * @ingroup UtilityWidgets
  */
 //! \todo Document this file.
 
-#ifndef CARD_COMPLETER_UTILS_H
-#define CARD_COMPLETER_UTILS_H
+#ifndef COMPLETER_UTILS_H
+#define COMPLETER_UTILS_H
 
 class CardCompleterProxyModel;
 class CardDatabaseDisplayModel;

cockatrice/src/interface/widgets/utility/line_edit_completer.cpp

@@ -13,7 +13,8 @@ void LineEditCompleter::addCompleter(QCompleter *c, const QString &trigger)
     c->setWidget(this);
     c->setCompletionMode(QCompleter::PopupCompletion);
     c->setCaseSensitivity(Qt::CaseInsensitive);
-    connect(c, qOverload<const QString &>(&QCompleter::activated), this, &LineEditCompleter::insertCompletion);
+    connect(c, qOverload<const QString &>(&QCompleter::activated), this,
+            qOverload<const QString &>(&LineEditCompleter::insertCompletion));
 
     completers.append({c, trigger});
 }
@@ -39,6 +40,18 @@ void LineEditCompleter::focusOutEvent(QFocusEvent *e)
 {
     LineEditUnfocusable::focusOutEvent(e);
 
+    // Commit the highlighted completion so that tabbing away still applies it
+    for (auto &info : completers) {
+        if (!info.completer->popup()->isVisible()) {
+            continue;
+        }
+
+        const QModelIndex currentIndex = info.completer->popup()->currentIndex();
+        if (currentIndex.isValid()) {
+            insertCompletion(info.completer, currentIndex.data().toString());
+        }
+    }
+
     hideCompleterPopups();
 }
 
@@ -46,6 +59,11 @@ void LineEditCompleter::keyPressEvent(QKeyEvent *event)
 {
     LineEditUnfocusable::keyPressEvent(event);
 
+    if (event->key() == Qt::Key_Escape) {
+        hideCompleterPopups();
+        return;
+    }
+
     QString textValue = text();
     int cursorPos = cursorPosition();
 
@@ -89,37 +107,70 @@ void LineEditCompleter::keyPressEvent(QKeyEvent *event)
 }
 
 void LineEditCompleter::insertCompletion(const QString &completion)
+{
+    for (auto &info : completers) {
+        if (info.completer == sender()) {
+            insertCompletion(info.completer, completion);
+            return;
+        }
+    }
+}
+
+void LineEditCompleter::insertCompletion(QCompleter *completer, const QString &completion)
 {
     QString t = text();
     int pos = cursorPosition();
 
-    CompleterInfo *active = nullptr;
-    for (auto &info : completers) {
-        if (info.completer == sender()) {
-            active = &info;
-            break;
+    for (const auto &info : completers) {
+        if (info.completer != completer) {
+            continue;
         }
-    }
-    if (!active) {
-        return;
-    }
 
-    if (active->trigger == "[[") {
-        int triggerPos = t.lastIndexOf("[[", pos - 1);
+        if (info.trigger == "[[") {
+            int triggerPos = t.lastIndexOf("[[", pos - 1);
+            if (triggerPos == -1) {
+                return;
+            }
+
+            // If an earlier "[[" is still open it also encloses the cursor, so
+            // replace from its start. Otherwise completing in text such as
+            // "[[Opt[[Amok" would leave a stray "[[" behind.
+            int startPos = triggerPos;
+            for (int searchFrom = triggerPos; searchFrom > 0;) {
+                const int earlier = t.lastIndexOf("[[", searchFrom - 1);
+                if (earlier == -1) {
+                    break;
+                }
+                const int earlierClose = t.indexOf("]]", earlier + 2);
+                if (earlierClose != -1 && earlierClose < pos) {
+                    break;
+                }
+                startPos = earlier;
+                searchFrom = earlier;
+            }
+
+            // If the cursor sits inside an already-closed [[...]] pair, replace
+            // the whole construct instead of leaving a duplicate closing bracket
+            // behind.
+            int insertEnd = pos;
+            const int closePos = t.indexOf("]]", startPos + 2);
+            if (closePos != -1 && closePos >= pos) {
+                insertEnd = closePos + 2;
+            }
+
+            QString after = t.mid(insertEnd);
+            QString replaced = t.left(startPos + 2) + completion + "]] ";
+            setText(replaced + after);
+            setCursorPosition(replaced.length());
+            return;
+        }
+
+        int triggerPos = t.lastIndexOf("@", pos - 1);
         if (triggerPos == -1) {
             return;
         }
-        QString after = t.mid(pos);
-        QString replaced = t.left(triggerPos + 2) + completion + "]] ";
-        setText(replaced + after);
-        setCursorPosition(replaced.length());
+        setText(t.replace(triggerPos, pos - triggerPos, completion + " "));
+        setCursorPosition(triggerPos + completion.length() + 1);
         return;
     }
-
-    int triggerPos = t.lastIndexOf("@", pos - 1);
-    if (triggerPos == -1) {
-        return;
-    }
-    setText(t.replace(triggerPos, pos - triggerPos, completion + " "));
-    setCursorPosition(triggerPos + completion.length() + 1);
 }

cockatrice/src/interface/widgets/utility/line_edit_completer.h

@@ -32,6 +32,8 @@ signals:
 private:
     QList<CompleterInfo> completers;
 
+    void insertCompletion(QCompleter *completer, const QString &completion);
+
 private slots:
     void insertCompletion(const QString &text);
 

cockatrice/src/interface/widgets/utility/reversed_completer_model.cpp

@@ -0,0 +1,102 @@
+#include "reversed_completer_model.h"
+
+#include <QAbstractItemModel>
+#include <QModelIndex>
+#include <QVariant>
+
+void ReversedCompleterModel::setSourceModel(QAbstractItemModel *sourceModel)
+{
+    if (sourceModel == this->sourceModel()) {
+        return;
+    }
+
+    if (QAbstractItemModel *old = this->sourceModel()) {
+        disconnect(old, nullptr, this, nullptr);
+    }
+
+    QAbstractProxyModel::setSourceModel(sourceModel);
+
+    if (sourceModel) {
+        connect(sourceModel, &QAbstractItemModel::modelReset, this, &ReversedCompleterModel::invalidate);
+        connect(sourceModel, &QAbstractItemModel::rowsInserted, this, &ReversedCompleterModel::invalidate);
+        connect(sourceModel, &QAbstractItemModel::rowsRemoved, this, &ReversedCompleterModel::invalidate);
+        connect(sourceModel, &QAbstractItemModel::rowsMoved, this, &ReversedCompleterModel::invalidate);
+        connect(sourceModel, &QAbstractItemModel::columnsInserted, this, &ReversedCompleterModel::invalidate);
+        connect(sourceModel, &QAbstractItemModel::columnsRemoved, this, &ReversedCompleterModel::invalidate);
+        connect(sourceModel, &QAbstractItemModel::dataChanged, this, &ReversedCompleterModel::invalidate);
+        connect(sourceModel, &QAbstractItemModel::layoutChanged, this, &ReversedCompleterModel::invalidate);
+    }
+
+    invalidate();
+}
+
+void ReversedCompleterModel::setEnabled(bool enabled)
+{
+    if (enabled == isEnabled) {
+        return;
+    }
+    isEnabled = enabled;
+    invalidate();
+}
+
+QModelIndex ReversedCompleterModel::mapToSource(const QModelIndex &proxyIndex) const
+{
+    if (!proxyIndex.isValid() || !sourceModel()) {
+        return {};
+    }
+    const int sourceRow = isEnabled ? sourceRowCount() - 1 - proxyIndex.row() : proxyIndex.row();
+    return sourceModel()->index(sourceRow, proxyIndex.column());
+}
+
+QModelIndex ReversedCompleterModel::mapFromSource(const QModelIndex &sourceIndex) const
+{
+    if (!sourceIndex.isValid() || !sourceModel()) {
+        return {};
+    }
+    const int proxyRow = isEnabled ? sourceRowCount() - 1 - sourceIndex.row() : sourceIndex.row();
+    return index(proxyRow, sourceIndex.column());
+}
+
+QModelIndex ReversedCompleterModel::index(int row, int column, const QModelIndex &parent) const
+{
+    if (parent.isValid() || !sourceModel() || row < 0 || row >= rowCount() || column < 0 || column >= columnCount()) {
+        return {};
+    }
+    return createIndex(row, column);
+}
+
+QModelIndex ReversedCompleterModel::parent(const QModelIndex &) const
+{
+    return {};
+}
+
+int ReversedCompleterModel::rowCount(const QModelIndex &parent) const
+{
+    return parent.isValid() || !sourceModel() ? 0 : sourceModel()->rowCount();
+}
+
+int ReversedCompleterModel::columnCount(const QModelIndex &parent) const
+{
+    return parent.isValid() || !sourceModel() ? 0 : sourceModel()->columnCount();
+}
+
+QVariant ReversedCompleterModel::data(const QModelIndex &proxyIndex, int role) const
+{
+    return sourceModel() ? sourceModel()->data(mapToSource(proxyIndex), role) : QVariant();
+}
+
+QVariant ReversedCompleterModel::headerData(int section, Qt::Orientation orientation, int role) const
+{
+    return sourceModel() ? sourceModel()->headerData(section, orientation, role) : QVariant();
+}
+
+int ReversedCompleterModel::sourceRowCount() const
+{
+    return sourceModel() ? sourceModel()->rowCount() : 0;
+}
+
+void ReversedCompleterModel::invalidate()
+{
+    beginResetModel();
+    endResetModel();
+}

cockatrice/src/interface/widgets/utility/reversed_completer_model.h

@@ -0,0 +1,47 @@
+/**
+ * @file reversed_completer_model.h
+ * @ingroup UtilityWidgets
+ */
+//! \todo Document this file.
+
+#ifndef REVERSED_COMPLETER_MODEL_H
+#define REVERSED_COMPLETER_MODEL_H
+
+#include <QAbstractProxyModel>
+
+class QAbstractItemModel;
+
+/**
+ * @brief A completer model that can present its rows bottom-to-top.
+ *
+ * The original row order is kept intact in the source model (row 0 is always the
+ * closest match). When enabled, the proxy maps the source rows in reverse so the
+ * popup shows the closest match in the row nearest to the text edit. Any change
+ * in the source model is forwarded as a full reset, which is all QCompleter
+ * needs to rebuild its completion list.
+ */
+class ReversedCompleterModel : public QAbstractProxyModel
+{
+public:
+    using QAbstractProxyModel::QAbstractProxyModel;
+
+    void setSourceModel(QAbstractItemModel *sourceModel) override;
+    void setEnabled(bool enabled);
+
+    QModelIndex mapToSource(const QModelIndex &proxyIndex) const override;
+    QModelIndex mapFromSource(const QModelIndex &sourceIndex) const override;
+    QModelIndex index(int row, int column, const QModelIndex &parent = {}) const override;
+    QModelIndex parent(const QModelIndex &) const override;
+    int rowCount(const QModelIndex &parent = {}) const override;
+    int columnCount(const QModelIndex &parent = {}) const override;
+    QVariant data(const QModelIndex &proxyIndex, int role = Qt::DisplayRole) const override;
+    QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
+
+private:
+    int sourceRowCount() const;
+    void invalidate();
+
+    bool isEnabled = false;
+};
+
+#endif // REVERSED_COMPLETER_MODEL_H

cockatrice/src/interface/widgets/visual_deck_editor/visual_deck_editor_widget.cpp

@@ -9,8 +9,8 @@
 #include "../general/layout_containers/flow_widget.h"
 #include "../tabs/visual_deck_editor/tab_deck_editor_visual.h"
 #include "../tabs/visual_deck_editor/tab_deck_editor_visual_tab_widget.h"
-#include "../utility/card_completer_utils.h"
 #include "../utility/compact_push_button.h"
+#include "../utility/completer_utils.h"
 #include "visual_deck_display_options_widget.h"
 
 #include <QCheckBox>