[Client] Keep Commander Spellbook requests bound to the active deck [skip ci]

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.cpp

@@ -26,6 +26,11 @@ quint64 CommanderBracketService::estimateBracket(const DeckList &deck, QObject *
     return CommanderSpellbookApiAccessor::instance().estimateBracket(deck, requester);
 }
 
+void CommanderBracketService::cancelEstimate(quint64 requestId)
+{
+    CommanderSpellbookApiAccessor::instance().cancelEstimate(requestId);
+}
+
 void CommanderBracketService::onEstimateBracketFinished(CommanderSpellbookApiAccessor::RequestId id,
                                                         QObject *requester,
                                                         const EstimateBracketResult &result)

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.h

@@ -30,6 +30,7 @@ public:
     static CommanderBracketService &instance();
 
     quint64 estimateBracket(const DeckList &deck, QObject *requester);
+    void cancelEstimate(quint64 requestId);
 
 signals:
     void estimateFinished(quint64 requestId, QObject *requester, const CommanderBracketEstimate &estimate);

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.cpp

@@ -28,9 +28,18 @@ template <class T> class QSharedPointer;
 
 CommanderBracketWidget::CommanderBracketWidget(QWidget *parent) : QWidget(parent)
 {
+    autoEstimateTimer = new QTimer(this);
+    autoEstimateTimer->setSingleShot(true);
+    connect(autoEstimateTimer, &QTimer::timeout, this, [this]() {
+        if (SettingsCache::instance().deckEditor().getCommanderSpellbookIntegrationEnabled() ==
+            commanderSpellbookIntegrationEnabledIndexAutomatic) {
+            requestBracketEstimate();
+        }
+    });
     bracketLabel = new QLabel(tr("Bracket:"), this);
 
     bracketValueLabel = new QLabel(this);
+    bracketValueLabel->setTextFormat(Qt::PlainText);
     bracketValueLabel->setText("-");
     bracketValueLabel->setObjectName("bracketValueLabel");
 
@@ -65,18 +74,38 @@ CommanderBracketWidget::CommanderBracketWidget(QWidget *parent) : QWidget(parent
             this, &CommanderBracketWidget::maybeAutoEstimateBracket);
     connect(&SettingsCache::instance().deckEditor(),
             &DeckEditorSettings::commanderSpellbookIntegrationUseOfficialBracketNamesChanged, this,
-            &CommanderBracketWidget::maybeAutoEstimateBracket);
+            &CommanderBracketWidget::displayEstimate);
 
     setVisible(false);
 }
 
+CommanderBracketWidget::~CommanderBracketWidget()
+{
+    cancelEstimate();
+}
+
+void CommanderBracketWidget::cancelEstimate()
+{
+    autoEstimateTimer->stop();
+    const quint64 canceledRequest = requestId;
+    requestId = 0;
+    CommanderBracketService::instance().cancelEstimate(canceledRequest);
+    bracketRefreshButton->setEnabled(true);
+    if (canceledRequest) {
+        bracketValueLabel->setText("-");
+        bracketValueLabel->setToolTip({});
+    }
+}
+
 void CommanderBracketWidget::setDeck(const QSharedPointer<DeckList> &_deck)
 {
+    cancelEstimate();
     deck = _deck;
-    requestId = 0; // invalidate any in-flight estimate for the previous deck
+    hasEstimate = false;
 
     // Reset the displayed bracket
     bracketValueLabel->setText("-");
+    bracketValueLabel->setToolTip({});
     bracketInfoButton->setToolTip({});
     bracketInfoButton->setEnabled(false);
     bracketRefreshButton->setEnabled(true);
@@ -144,7 +173,7 @@ bool CommanderBracketWidget::promptCommanderSpellbookIntegration()
     dialog.exec();
 
     // Persist naming choice (if not disabled)
-    if (clickedButton != disableBtn) {
+    if (clickedButton == enableBtn || clickedButton == automaticBtn) {
         bool useOfficial = namingCombo->currentIndex() == 1;
         SettingsCache::instance().deckEditor().setCommanderSpellbookIntegrationUseOfficialBracketNames(useOfficial);
     }
@@ -176,9 +205,20 @@ void CommanderBracketWidget::updateBracketVisibility(bool visible)
 
 void CommanderBracketWidget::requestBracketEstimate()
 {
+    const int mode = SettingsCache::instance().deckEditor().getCommanderSpellbookIntegrationEnabled();
+    if (!deck || deck->getGameFormat().compare("commander", Qt::CaseInsensitive) != 0 || requestId ||
+        (mode != commanderSpellbookIntegrationEnabledIndexEnabled &&
+         mode != commanderSpellbookIntegrationEnabledIndexAutomatic)) {
+        return;
+    }
+
+    autoEstimateTimer->stop();
+    hasEstimate = false;
     bracketRefreshButton->setEnabled(false);
     bracketInfoButton->setEnabled(false);
+    bracketInfoButton->setToolTip({});
     bracketValueLabel->setText(tr("Calculating…"));
+    bracketValueLabel->setToolTip({});
 
     requestId = CommanderBracketService::instance().estimateBracket(*deck, this);
 }
@@ -191,22 +231,34 @@ void CommanderBracketWidget::onEstimateBracketFinished(quint64 id,
         return;
     }
 
+    requestId = 0;
+    hasEstimate = true;
+    lastEstimate = result;
+    displayEstimate();
+    bracketRefreshButton->setEnabled(true);
+}
+
+void CommanderBracketWidget::displayEstimate()
+{
+    if (!hasEstimate) {
+        return;
+    }
+
     BracketExplainer explainer;
-    lastBracketExplanation = explainer.explain(result.rawResult);
+    lastBracketExplanation = explainer.explain(lastEstimate.rawResult);
 
     // Display bracket
     bracketValueLabel->setText(
         SettingsCache::instance().deckEditor().getCommanderSpellbookIntegrationUseOfficialBracketNames()
-            ? result.officialName
-            : result.displayName);
-    bracketRefreshButton->setEnabled(true);
+            ? lastEstimate.officialName
+            : lastEstimate.displayName);
 
     // Build tooltip
     QString tooltip;
     for (const auto &section : lastBracketExplanation.sections) {
-        tooltip += "<b>" + section.title + "</b><br>";
+        tooltip += "<b>" + section.title.toHtmlEscaped() + "</b><br>";
         for (const auto &line : section.bulletPoints) {
-            tooltip += "• " + line + "<br>";
+            tooltip += "• " + line.toHtmlEscaped() + "<br>";
         }
         tooltip += "<br>";
     }
@@ -215,13 +267,17 @@ void CommanderBracketWidget::onEstimateBracketFinished(quint64 id,
     bracketInfoButton->setEnabled(!tooltip.isEmpty());
 }
 
-void CommanderBracketWidget::onEstimateBracketError(quint64 id, QObject *requester, const QString & /*error*/)
+void CommanderBracketWidget::onEstimateBracketError(quint64 id, QObject *requester, const QString &error)
 {
     if (requester != this || id != requestId) {
         return;
     }
 
+    requestId = 0;
+    hasEstimate = false;
     bracketValueLabel->setText("-");
+    bracketValueLabel->setToolTip(
+        QStringLiteral("<qt>%1</qt>").arg(tr("Unable to estimate bracket: %1").arg(error).toHtmlEscaped()));
     bracketRefreshButton->setEnabled(true);
     bracketInfoButton->setToolTip({});
     bracketInfoButton->setEnabled(false);
@@ -229,6 +285,15 @@ void CommanderBracketWidget::onEstimateBracketError(quint64 id, QObject *request
 
 void CommanderBracketWidget::maybeAutoEstimateBracket()
 {
+    if (prompting) {
+        return;
+    }
+    autoEstimateTimer->stop();
+    if (!deck) {
+        cancelEstimate();
+        updateBracketVisibility(false);
+        return;
+    }
     const QString formatKey = deck->getGameFormat();
 
     const bool isCommander = (formatKey.compare("commander", Qt::CaseInsensitive) == 0);
@@ -236,17 +301,20 @@ void CommanderBracketWidget::maybeAutoEstimateBracket()
     int mode = SettingsCache::instance().deckEditor().getCommanderSpellbookIntegrationEnabled();
 
     if (!isCommander || mode == commanderSpellbookIntegrationEnabledIndexDisabled) {
+        cancelEstimate();
         updateBracketVisibility(false);
         return;
     }
 
     if (mode == commanderSpellbookIntegrationEnabledIndexUnprompted) {
-        if (prompting) {
-            return;
-        }
+        const auto promptedDeck = deck;
         prompting = true;
         const bool accepted = promptCommanderSpellbookIntegration();
         prompting = false;
+        if (deck != promptedDeck || !deck || deck->getGameFormat().compare("commander", Qt::CaseInsensitive) != 0) {
+            maybeAutoEstimateBracket();
+            return;
+        }
         if (!accepted) {
             updateBracketVisibility(false);
             return;
@@ -261,17 +329,17 @@ void CommanderBracketWidget::maybeAutoEstimateBracket()
     }
 
     // Avoid firing if we already have a result or a request in flight
-    if (!bracketRefreshButton->isEnabled()) {
+    if (requestId || hasEstimate) {
         return;
     }
 
     // Defer to avoid races during init / model rebuild
-    QTimer::singleShot(0, this, &CommanderBracketWidget::requestBracketEstimate);
+    autoEstimateTimer->start(0);
 }
 
 void CommanderBracketWidget::retranslateUi()
 {
     bracketLabel->setText(tr("Bracket:"));
-    bracketInfoButton->setToolTip(tr("Why this bracket?"));
+    displayEstimate();
     bracketRefreshButton->setToolTip(tr("Recalculate bracket"));
 }

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.h

@@ -1,6 +1,7 @@
 #ifndef COCKATRICE_COMMANDER_BRACKET_WIDGET_H
 #define COCKATRICE_COMMANDER_BRACKET_WIDGET_H
 
+#include "commander_bracket_service.h"
 #include "commander_spellbook_bracket_explainer.h"
 
 #include <QSharedPointer>
@@ -11,6 +12,7 @@
 
 class QLabel;
 class QToolButton;
+class QTimer;
 struct CommanderBracketEstimate;
 class DeckList;
 class QObject;
@@ -22,6 +24,7 @@ class CommanderBracketWidget : public QWidget
 
 public:
     explicit CommanderBracketWidget(QWidget *parent = nullptr);
+    ~CommanderBracketWidget() override;
 
     void setDeck(const QSharedPointer<DeckList> &_deck);
     void retranslateUi();
@@ -35,10 +38,15 @@ private slots:
 private:
     bool promptCommanderSpellbookIntegration();
     void updateBracketVisibility(bool visible);
+    void cancelEstimate();
+    void displayEstimate();
 
     QSharedPointer<DeckList> deck;
     bool prompting = false;
     quint64 requestId = 0;
+    QTimer *autoEstimateTimer;
+    bool hasEstimate = false;
+    CommanderBracketEstimate lastEstimate;
 
     QLabel *bracketLabel;
     QLabel *bracketValueLabel;

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.cpp

@@ -9,9 +9,10 @@
 #include <QJsonParseError>
 #include <QNetworkReply>
 #include <QNetworkRequest>
+#include <QPointer>
 #include <QStringLiteral>
 #include <QUrl>
-#include <QVariant>
+#include <libcockatrice/network/network_manager.h>
 #include <version_string.h>
 
 class DeckList;
@@ -24,13 +25,18 @@ CommanderSpellbookApiAccessor &CommanderSpellbookApiAccessor::instance()
     return instance;
 }
 
-CommanderSpellbookApiAccessor::CommanderSpellbookApiAccessor(QObject *parent) : QObject(parent)
+CommanderSpellbookApiAccessor::CommanderSpellbookApiAccessor(QObject *parent)
+    : QObject(parent), network(NetworkManagerFactory::create(this))
 {
 }
 
 CommanderSpellbookApiAccessor::RequestId CommanderSpellbookApiAccessor::estimateBracket(const DeckList &deck,
                                                                                         QObject *requester)
 {
+    if (!requester) {
+        return 0;
+    }
+
     CommanderSpellbookDeckRequest deckRequest = CommanderSpellbookDeckRequest::fromDeckList(deck);
 
     QJsonDocument doc(deckRequest.toJson());
@@ -40,30 +46,39 @@ CommanderSpellbookApiAccessor::RequestId CommanderSpellbookApiAccessor::estimate
     req.setHeader(QNetworkRequest::ContentTypeHeader, "application/json");
     req.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING));
 
-    QNetworkReply *reply = network.post(req, body);
+    QNetworkReply *reply = network->post(req, body);
 
     const RequestId id = nextRequestId++;
 
-    reply->setProperty("requestId", QVariant::fromValue(id));
-    reply->setProperty("requester", QVariant::fromValue(requester));
-
-    connect(reply, &QNetworkReply::finished, this, [this, reply]() { onEstimateReplyFinished(reply); });
+    pendingRequests.insert(id, reply);
+    const QPointer<QObject> guardedRequester(requester);
+    connect(requester, &QObject::destroyed, reply, [this, id]() { cancelEstimate(id); });
+    connect(reply, &QNetworkReply::finished, this, [this, reply, id, guardedRequester]() {
+        if (pendingRequests.take(id) != reply) {
+            return;
+        }
+        reply->deleteLater();
+        if (guardedRequester) {
+            onEstimateReplyFinished(reply, id, guardedRequester);
+        }
+    });
 
     return id;
 }
 
-void CommanderSpellbookApiAccessor::onEstimateReplyFinished(QNetworkReply *reply)
+void CommanderSpellbookApiAccessor::cancelEstimate(RequestId id)
 {
-    reply->deleteLater();
-
-    const RequestId id = reply->property("requestId").toULongLong();
-    QObject *requester = reply->property("requester").value<QObject *>();
-
-    if (!requester) {
-        // Requester died — silently drop
+    const QPointer<QNetworkReply> reply = pendingRequests.take(id);
+    if (!reply) {
         return;
     }
+    disconnect(reply, nullptr, this, nullptr);
+    reply->abort();
+    reply->deleteLater();
+}
 
+void CommanderSpellbookApiAccessor::onEstimateReplyFinished(QNetworkReply *reply, RequestId id, QObject *requester)
+{
     if (reply->error() != QNetworkReply::NoError) {
         emit estimateBracketError(id, requester, reply->errorString());
         return;
@@ -78,6 +93,10 @@ void CommanderSpellbookApiAccessor::onEstimateReplyFinished(QNetworkReply *reply
     }
 
     EstimateBracketResult result = EstimateBracketResult::fromJson(doc.object());
+    if (result.bracketTag.trimmed().isEmpty()) {
+        emit estimateBracketError(id, requester, QStringLiteral("Missing bracket in response"));
+        return;
+    }
 
     emit estimateBracketFinished(id, requester, result);
 }

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.h

@@ -1,14 +1,16 @@
 #ifndef COCKATRICE_COMMANDER_SPELLBOOK_API_ACCESSOR_H
 #define COCKATRICE_COMMANDER_SPELLBOOK_API_ACCESSOR_H
 
-#include <QNetworkAccessManager>
+#include <QHash>
 #include <QObject>
+#include <QPointer>
 #include <QString>
 #include <QtGlobal>
 #include <qtmetamacros.h>
 
 class DeckList;
 class QNetworkReply;
+class QNetworkAccessManager;
 struct EstimateBracketResult;
 
 class CommanderSpellbookApiAccessor final : public QObject
@@ -21,6 +23,7 @@ public:
     using RequestId = quint64;
 
     RequestId estimateBracket(const DeckList &deck, QObject *requester);
+    void cancelEstimate(RequestId id);
 
 signals:
     void estimateBracketFinished(RequestId id, QObject *requester, const EstimateBracketResult &result);
@@ -31,9 +34,10 @@ private:
     explicit CommanderSpellbookApiAccessor(QObject *parent = nullptr);
     Q_DISABLE_COPY_MOVE(CommanderSpellbookApiAccessor)
 
-    void onEstimateReplyFinished(QNetworkReply *reply);
+    void onEstimateReplyFinished(QNetworkReply *reply, RequestId id, QObject *requester);
 
-    QNetworkAccessManager network;
+    QNetworkAccessManager *network;
+    QHash<RequestId, QPointer<QNetworkReply>> pendingRequests;
     RequestId nextRequestId = 1;
 };
 

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.cpp

@@ -3,11 +3,14 @@
 #include "../../../../../client/settings/cache_settings.h"
 
 #include <QByteArray>
+#include <QJsonArray>
 #include <QJsonDocument>
+#include <QJsonObject>
 #include <QJsonParseError>
 #include <QMap>
 #include <QNetworkAccessManager>
 #include <QNetworkRequest>
+#include <QSet>
 #include <QString>
 #include <QStringLiteral>
 #include <QUrl>
@@ -22,6 +25,14 @@ HandleCommanderBrackets::HandleCommanderBrackets(QObject *parent)
 {
 }
 
+HandleCommanderBrackets::~HandleCommanderBrackets()
+{
+    if (reply) {
+        disconnect(reply, nullptr, this, nullptr);
+        reply->abort();
+    }
+}
+
 void HandleCommanderBrackets::downloadBracketDefinitions()
 {
     if (reply) {
@@ -35,31 +46,49 @@ void HandleCommanderBrackets::downloadBracketDefinitions()
 
 void HandleCommanderBrackets::actFinishParsingDownloadedData()
 {
-    reply = qobject_cast<QNetworkReply *>(sender());
+    auto *finishedReply = qobject_cast<QNetworkReply *>(sender());
+    if (!finishedReply || finishedReply != reply) {
+        return;
+    }
 
-    if (reply->error() != QNetworkReply::NoError) {
-        emit sigBracketDefinitionsDownloadFailed(reply->error());
+    // Clear the active request before notifying listeners, which may immediately retry.
+    reply = nullptr;
+    finishedReply->deleteLater();
 
-        reply->deleteLater();
+    if (finishedReply->error() != QNetworkReply::NoError) {
+        emit sigBracketDefinitionsDownloadFailed(finishedReply->error());
         return;
     }
 
     QJsonParseError parseError;
 
-    auto document = QJsonDocument::fromJson(reply->readAll(), &parseError);
+    auto document = QJsonDocument::fromJson(finishedReply->readAll(), &parseError);
 
-    if (parseError.error != QJsonParseError::NoError) {
+    if (parseError.error != QJsonParseError::NoError || !document.isObject() ||
+        !document.object().value("brackets").isArray() || document.object().value("brackets").toArray().isEmpty()) {
         emit sigBracketDefinitionsDownloadFailed(QNetworkReply::UnknownContentError);
-
-        reply->deleteLater();
         return;
     }
 
+    // Validate the complete replacement before touching the last usable definitions.
+    // Tags are QSettings group names, so path separators must not create nested groups.
+    QSet<QString> tags;
+    for (const auto &value : document.object().value("brackets").toArray()) {
+        const auto definition = value.toObject();
+        const auto tag = definition.value("tag").toString();
+        if (!value.isObject() || tag.trimmed().isEmpty() || tag.contains('/') || tag.contains('\\') ||
+            tags.contains(tag) || definition.value("officialName").toString().trimmed().isEmpty() ||
+            definition.value("displayName").toString().trimmed().isEmpty() ||
+            !definition.value("explanation").isString()) {
+            emit sigBracketDefinitionsDownloadFailed(QNetworkReply::UnknownContentError);
+            return;
+        }
+        tags.insert(tag);
+    }
+
     updateBracketDefinitions(document.toVariant().toMap());
 
     emit sigBracketDefinitionsDownloaded();
-
-    reply->deleteLater();
 }
 
 void HandleCommanderBrackets::updateBracketDefinitions(const QVariantMap &jsonMap)

cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.h

@@ -3,6 +3,7 @@
 
 #include <QNetworkReply>
 #include <QObject>
+#include <QPointer>
 #include <QVariantMap>
 #include <qtmetamacros.h>
 
@@ -14,6 +15,7 @@ class HandleCommanderBrackets : public QObject
 
 public:
     explicit HandleCommanderBrackets(QObject *parent = nullptr);
+    ~HandleCommanderBrackets() override;
 
     void downloadBracketDefinitions();
 
@@ -28,7 +30,7 @@ private:
     void updateBracketDefinitions(const QVariantMap &jsonMap);
 
     QNetworkAccessManager *nam;
-    QNetworkReply *reply;
+    QPointer<QNetworkReply> reply;
 };
 
 #endif // COCKATRICE_HANDLE_COMMANDER_BRACKETS_H