[Client] Keep Commander Spellbook requests bound to the active deck (#7722)

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

* [Client] Restore the bracket info tooltip when idle

retranslateUi() used to install the static "Why this bracket?" tooltip, but it now calls displayEstimate(), which returned early while no estimate existed. That left the info button without its idle tooltip.

Set the fallback where displayEstimate() owns the button's tooltip. An existing estimate still wins, and a language change refreshes the text.

---------

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

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,35 @@ void CommanderBracketWidget::onEstimateBracketFinished(quint64 id,
         return;
     }
 
+    requestId = 0;
+    hasEstimate = true;
+    lastEstimate = result;
+    displayEstimate();
+    bracketRefreshButton->setEnabled(true);
+}
+
+void CommanderBracketWidget::displayEstimate()
+{
+    if (!hasEstimate) {
+        bracketInfoButton->setToolTip(tr("Why this bracket?"));
+        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 +268,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 +286,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 +302,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 +330,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