From 624163916f6a7356653ceaafe35c37e624f4e696 Mon Sep 17 00:00:00 2001 From: Bryce! Date: Tue, 3 Nov 2026 23:49:31 -0500 Subject: [PATCH] [Client] Keep Commander Spellbook requests bound to the active deck (#7722) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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 --- .../commander_bracket_service.cpp | 5 + .../commander_bracket_service.h | 1 + .../commander_bracket_widget.cpp | 101 +++++++++++++++--- .../commander_bracket_widget.h | 8 ++ .../commander_spellbook_api_accessor.cpp | 49 ++++++--- .../commander_spellbook_api_accessor.h | 10 +- .../handle_commander_brackets.cpp | 49 +++++++-- .../handle_commander_brackets.h | 4 +- 8 files changed, 182 insertions(+), 45 deletions(-) diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.cpp b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.cpp index 8e0448faa..7b60ed23c 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.cpp +++ b/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) diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.h b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.h index 22962cc95..00debce86 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_service.h +++ b/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); diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.cpp b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.cpp index 5279165af..9adda008e 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.cpp +++ b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.cpp @@ -28,9 +28,18 @@ template 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 &_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 §ion : lastBracketExplanation.sections) { - tooltip += "" + section.title + "
"; + tooltip += "" + section.title.toHtmlEscaped() + "
"; for (const auto &line : section.bulletPoints) { - tooltip += "• " + line + "
"; + tooltip += "• " + line.toHtmlEscaped() + "
"; } tooltip += "
"; } @@ -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("%1").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")); } diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.h b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.h index f5b2b7958..0ef1eedee 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_bracket_widget.h +++ b/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 @@ -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 &_deck); void retranslateUi(); @@ -35,10 +38,15 @@ private slots: private: bool promptCommanderSpellbookIntegration(); void updateBracketVisibility(bool visible); + void cancelEstimate(); + void displayEstimate(); QSharedPointer deck; bool prompting = false; quint64 requestId = 0; + QTimer *autoEstimateTimer; + bool hasEstimate = false; + CommanderBracketEstimate lastEstimate; QLabel *bracketLabel; QLabel *bracketValueLabel; diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.cpp b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.cpp index bd8d9379d..8456e2e21 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.cpp +++ b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.cpp @@ -9,9 +9,10 @@ #include #include #include +#include #include #include -#include +#include #include 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 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(); - - if (!requester) { - // Requester died — silently drop + const QPointer 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); } diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.h b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.h index 98e7e3710..a1b2936bb 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/commander_spellbook_api_accessor.h +++ b/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 +#include #include +#include #include #include #include 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> pendingRequests; RequestId nextRequestId = 1; }; diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.cpp b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.cpp index 407f53e99..be490ce37 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.cpp +++ b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.cpp @@ -3,11 +3,14 @@ #include "../../../../../client/settings/cache_settings.h" #include +#include #include +#include #include #include #include #include +#include #include #include #include @@ -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(sender()); + auto *finishedReply = qobject_cast(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 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) diff --git a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.h b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.h index 39bddba05..99b15832c 100644 --- a/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.h +++ b/cockatrice/src/interface/widgets/tabs/api/commander_spellbook/handle_commander_brackets.h @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -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 reply; }; #endif // COCKATRICE_HANDLE_COMMANDER_BRACKETS_H