From e3820c3f34ad7aaf62e4aa27fa9995ca8aea4980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:52:25 +0200 Subject: [PATCH 01/13] [PictureLoader] Seed per-host allowances on demand and skip hosts in 429 backoff The quota reset re-filled every host's remaining allowance to a full MAX_REQUESTS_PER_SEC as soon as the queue had a request for it. A server that was just rate limited could therefore be hammered again at full speed immediately after (or even during) recovery. Only seed a host's allowance the first time it is dispatched in the current second, seeded from its reduced sustained quota, and skip hosts still inside their 429 backoff window entirely. This makes the pacing commit's burst-free behavior hold per host too, instead of just smoothing the global aggregate. --- .../card_picture_loader_worker.cpp | 22 +++++++++++++------ .../card_picture_loader_worker_work.cpp | 5 +++++ .../card_picture_loader_worker_work.h | 3 +++ 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 4a2caaab4..3add23bfb 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -138,6 +138,9 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { requestQuota = MAX_REQUESTS_PER_SEC; + // Allowances are seeded per host on demand in processSingleRequest(), so a + // rate-limited host never gets a fresh full quota mid-second. + hostQuotaRemaining.clear(); QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { @@ -146,11 +149,6 @@ void CardPictureLoaderWorker::resetRequestQuota() } } - for (const auto &request : requestLoadQueue) { - const QString host = request.first.host(); - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); - } - processQueuedRequests(); } @@ -184,10 +182,20 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() bool CardPictureLoaderWorker::processSingleRequest() { + QDateTime now = QDateTime::currentDateTime(); for (int i = 0; i < requestLoadQueue.size(); ++i) { const auto &request = requestLoadQueue.at(i); - QString host = request.first.host(); - int allowance = hostQuotaRemaining.value(host, MAX_REQUESTS_PER_SEC); + const QString host = request.first.host(); + // Don't dispatch requests to a host that is currently in its 429 backoff. + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + continue; + } + // Seed the allowance only now, so a host that was rate limited last second + // doesn't get a fresh full quota the moment it is queried mid-second. + if (!hostQuotaRemaining.contains(host)) { + hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + } + int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { hostQuotaRemaining.insert(host, allowance - 1); makeRequest(request.first, request.second); diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index 66c56337c..072a919d7 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp @@ -22,6 +22,11 @@ static const QStringList MD5_BLACKLIST = { "fbc7d763c08771c260b39e2115414eeb" // Current card back hash }; +ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() +{ + return s_rateLimiter; +} + CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index 1e56a4373..8490cb3ac 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -43,6 +43,9 @@ public: CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading + /** @brief Shared per-server 429 backoff state. */ + static ServerRateLimiter &rateLimiter(); + public slots: /** * @brief Handles a finished network reply for the card image. From da307a82b3c63e0b90cef4197d8203fb79133db6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 16:58:10 +0200 Subject: [PATCH 02/13] [PictureLoader] Add user-configurable per-host request caps Picture downloads were throttled to a uniform 10 requests/second per host with no way to tune a specific server. A rate-limited API host (Scryfall caps at 10 req/s) can trip 429s during bursts, and CDN hosts with no rate limit were throttled needlessly. Introduce developer-owned per-host caps that users can only ever lower, never raise, exposed in the download settings page: - DownloadSettings::DEVELOPER_HOST_CAPS sets the ceiling per host (api.scryfall.com 9, cards.scryfall.io unlimited, others 10). - A new hostRequestLimits setting stores user overrides in downloads.ini; clampHostRequestLimit() bounds them to [1, devCap] so a user can reduce api.scryfall.com to 5 but never raise it above 9. - The picture worker seeds, halves on 429, and recovers its sustained per-host allowance against the effective ceiling instead of the global maximum, and skips per-host accounting entirely for unlocked hosts (cards.scryfall.io) while global pacing and 429 backoff still apply. - The deck editor settings page gains one spinbox per known host, each clamped to its developer cap. --- .../card_picture_loader_worker.cpp | 41 ++++++++-- .../card_picture_loader_worker.h | 9 +++ .../deck_editor_settings_page.cpp | 74 +++++++++++++++++++ .../settings_page/deck_editor_settings_page.h | 7 ++ .../settings/download_settings.cpp | 45 +++++++++++ .../settings/download_settings.h | 34 +++++++++ tests/settings/settings_defaults_test.cpp | 32 ++++++++ 7 files changed, 236 insertions(+), 6 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 3add23bfb..84c5a02c5 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -16,15 +16,16 @@ #include #include -static constexpr int MAX_REQUESTS_PER_SEC = 10; -static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance +static constexpr int MAX_REQUESTS_PER_SEC = DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT; +static constexpr int MIN_HOST_QUOTA = DownloadSettings::MIN_HOST_REQUEST_LIMIT; static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between individual network requests static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), - requestQuota(MAX_REQUESTS_PER_SEC) + requestQuota(MAX_REQUESTS_PER_SEC), + hostRequestLimits(SettingsCache::instance().downloads().getHostRequestLimits()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -74,6 +75,9 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest); dispatchTimer.setInterval(DISPATCH_INTERVAL_MS); + + connect(&SettingsCache::instance().downloads(), &DownloadSettings::hostRequestLimitsChanged, this, + [this] { hostRequestLimits = SettingsCache::instance().downloads().getHostRequestLimits(); }); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -145,7 +149,9 @@ void CardPictureLoaderWorker::resetRequestQuota() QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { - it.value() = qMin(MAX_REQUESTS_PER_SEC, it.value() + 1); + // Recover towards the host's effective allowance ceiling, which may be + // lowered by the user's per-host request limits. + it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); } } @@ -190,10 +196,18 @@ bool CardPictureLoaderWorker::processSingleRequest() if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { continue; } + const int ceiling = hostAllowanceCeiling(host); + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the per-host allowance + // entirely; only the global quota and request pacing still apply. + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); + return true; + } // Seed the allowance only now, so a host that was rate limited last second // doesn't get a fresh full quota the moment it is queried mid-second. if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + hostQuotaRemaining.insert(host, hostRequestQuota.value(host, ceiling)); } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { @@ -206,9 +220,24 @@ bool CardPictureLoaderWorker::processSingleRequest() return false; } +int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const +{ + const int devCap = DownloadSettings::getDeveloperHostCaps().value(host, MAX_REQUESTS_PER_SEC); + if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestLimits.contains(host)) { + return DownloadSettings::UNLIMITED_HOST_QUOTA; + } + const int requested = hostRequestLimits.value(host, devCap); + return SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); + if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // Unlocked hosts have no per-host allowance to halve; the shared backoff + // window tracked by the rate limiter still paces them. + return; + } + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, hostAllowanceCeiling(host)) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 9f7fd9437..70bc36419 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -125,12 +125,21 @@ private: QTimer requestTimer; ///< Timer to reset the request quota QTimer dispatchTimer; ///< Timer pacing individual network requests QHash hostRequestQuota; ///< Sustained per-host request allowance + QHash hostRequestLimits; ///< User-set per-host request allowances QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited CardPictureLoaderLocal *localLoader; ///< Loader for local images QSet currentlyLoading; ///< Deduplication: contains pixmapCacheKey currently being loaded + /** + * @brief Effective per-host allowance ceiling for a host. + * @param host The host to look up + * @return The allowance ceiling in requests/second, or DownloadSettings::UNLIMITED_HOST_QUOTA + * when the developer unlocked the host and no user limit is set for it. + */ + [[nodiscard]] int hostAllowanceCeiling(const QString &host) const; + /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index f3eac05b8..4337bf0f3 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,12 +10,17 @@ #include #include #include +#include #include +#include +#include #include #include #include #include +static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for hosts unlocked by the developer + DeckEditorSettingsPage::DeckEditorSettingsPage() { picDownloadCheckBox.setChecked(SettingsCache::instance().downloads().getPicDownload()); @@ -96,6 +101,53 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() &DownloadSettings::setDownloadSpoilerStatus); connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, this, &DeckEditorSettingsPage::setSpoilersEnabled); + // Per-host request limit group: one spinbox per known picture host. A spinbox at its + // lower bound (0 for unlocked hosts, the developer cap for capped hosts) means "follow + // the developer default"; the worker clamps any explicit value against the developer cap. + mpRequestLimitGroupBox = new QGroupBox; + auto *requestLimitLayout = new QGridLayout; + + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + const QHash userLimits = SettingsCache::instance().downloads().getHostRequestLimits(); + + QSet hosts; + for (const QString &urlTemplate : SettingsCache::instance().downloads().getAllURLs()) { + hosts.insert(QUrl(urlTemplate).host()); + } + const QList devHosts = devCaps.keys(); + for (const QString &devHost : devHosts) { + hosts.insert(devHost); + } + + QList sortedHosts(hosts.cbegin(), hosts.cend()); + std::sort(sortedHosts.begin(), sortedHosts.end(), + [](const QString &a, const QString &b) { return a.localeAwareCompare(b) < 0; }); + + int hostRow = 1; + for (const QString &host : sortedHosts) { + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; + + auto *hostLabel = new QLabel(host); + auto *spinBox = new QSpinBox; + if (unlocked) { + spinBox->setRange(0, UNLOCKED_HOST_LIMIT_MAX); // 0 means "unlimited" + spinBox->setValue(userLimits.value(host, 0)); + } else { + spinBox->setRange(DownloadSettings::MIN_HOST_REQUEST_LIMIT, devCap); + spinBox->setValue(userLimits.value(host, devCap)); + } + connect(spinBox, &QSpinBox::valueChanged, this, &DeckEditorSettingsPage::storeRequestLimits); + + requestLimitLayout->addWidget(hostLabel, hostRow, 0); + requestLimitLayout->addWidget(spinBox, hostRow, 1); + requestLimitSpinBoxes.insert(host, spinBox); + ++hostRow; + } + + requestLimitLayout->addWidget(&requestLimitHelpLabel, hostRow, 0, 1, 2); + mpRequestLimitGroupBox->setLayout(requestLimitLayout); + mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -104,6 +156,7 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() auto *lpMainLayout = new QVBoxLayout; lpMainLayout->addWidget(mpGeneralGroupBox); + lpMainLayout->addWidget(mpRequestLimitGroupBox); lpMainLayout->addWidget(mpSpoilerGroupBox); setLayout(lpMainLayout); @@ -164,6 +217,24 @@ void DeckEditorSettingsPage::storeSettings() SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); } +void DeckEditorSettingsPage::storeRequestLimits() +{ + QHash stored; + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + for (auto it = requestLimitSpinBoxes.cbegin(); it != requestLimitSpinBoxes.cend(); ++it) { + const QString host = it.key(); + const int value = it.value()->value(); + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + // Only values that differ from the developer default are persisted; the worker + // treats a missing entry as "follow the developer default". + const int developerDefault = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? 0 : devCap; + if (value != developerDefault) { + stored.insert(host, value); + } + } + SettingsCache::instance().downloads().setHostRequestLimits(stored); +} + void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) { storeSettings(); @@ -230,6 +301,9 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput) void DeckEditorSettingsPage::retranslateUi() { mpGeneralGroupBox->setTitle(tr("URL Download Priority")); + mpRequestLimitGroupBox->setTitle(tr("Per-Host Request Limit")); + requestLimitHelpLabel.setText(tr("Pictures per second per host. Hosts can be lowered below their developer " + "limit but never raised above it; 0 means the host is not throttled per host.")); mpSpoilerGroupBox->setTitle(tr("Spoilers")); mcDownloadSpoilersCheckBox.setText(tr("Download Spoilers Automatically")); mcSpoilerSaveLabel.setText(tr("Spoiler Location:")); diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index 5db009c8a..b3745def4 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -5,9 +5,11 @@ #include #include +#include #include #include #include +#include class DeckEditorSettingsPage : public AbstractSettingsPage { @@ -19,6 +21,7 @@ public: private slots: void storeSettings(); + void storeRequestLimits(); void urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int); void setSpoilersEnabled(bool); void spoilerPathButtonClicked(); @@ -40,6 +43,10 @@ private: QGroupBox *mpGeneralGroupBox; QGroupBox *mpSpoilerGroupBox; + QGroupBox *mpRequestLimitGroupBox; + QLabel requestLimitHelpLabel; + QHash requestLimitSpinBoxes; + QLineEdit *mpSpoilerSavePathLineEdit; QLabel mcSpoilerSaveLabel; QLabel lastUpdatedLabel; diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index cfa1c054e..c510d8863 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -9,6 +9,22 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { "https://gatherer.wizards.com/Handlers/Image.ashx?multiverseid=!set:muid!&type=card", "https://gatherer.wizards.com/Handlers/Image.ashx?name=!name!&type=card"}; +// Developer-set ceilings for the per-host request allowance. Users may lower a host's +// allowance via the download settings, but can never raise it above these values. Hosts +// not listed default to DEFAULT_HOST_REQUEST_LIMIT. A cap of UNLIMITED_HOST_QUOTA marks a +// host that is never throttled per host (request pacing and 429 backoff still apply). +const QHash DownloadSettings::DEVELOPER_HOST_CAPS = { + // The Scryfall API enforces 10 requests/second; stay one under so a burst can't trip 429s. + {"api.scryfall.com", 9}, + // The Scryfall image CDN has no documented per-client rate limit. + {"cards.scryfall.io", UNLIMITED_HOST_QUOTA}, +}; + +const QHash &DownloadSettings::getDeveloperHostCaps() +{ + return DEVELOPER_HOST_CAPS; +} + DownloadSettings::DownloadSettings(const QString &settingPath, QObject *parent = nullptr) : SettingsManager(settingPath + "downloads.ini", "downloads", QString(), parent) { @@ -50,3 +66,32 @@ void DownloadSettings::setDownloadSpoilerStatus(bool _spoilerStatus) setValue(_spoilerStatus, "downloadSpoilers"); emit downloadSpoilerStatusChanged(); } + +QHash DownloadSettings::getHostRequestLimits() const +{ + const QVariantMap stored = getValue("hostRequestLimits").toMap(); + QHash hostRequestLimits; + for (auto it = stored.cbegin(); it != stored.cend(); ++it) { + hostRequestLimits.insert(it.key(), it.value().toInt()); + } + return hostRequestLimits; +} + +void DownloadSettings::setHostRequestLimits(const QHash &hostRequestLimits) +{ + QVariantMap stored; + for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { + stored.insert(it.key(), it.value()); + } + setValue(stored, "hostRequestLimits"); + emit hostRequestLimitsChanged(); +} + +int DownloadSettings::clampHostRequestLimit(const QString &host, int requested) const +{ + const int devCap = DEVELOPER_HOST_CAPS.value(host, DEFAULT_HOST_REQUEST_LIMIT); + if (devCap == UNLIMITED_HOST_QUOTA) { + return qMax(MIN_HOST_REQUEST_LIMIT, requested); + } + return qBound(MIN_HOST_REQUEST_LIMIT, requested, devCap); +} diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index a3a6f4ca9..e075a3c07 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -9,14 +9,34 @@ #include "settings_manager.h" +#include + class DownloadSettings : public SettingsManager { Q_OBJECT friend class SettingsCache; static const QStringList DEFAULT_DOWNLOAD_URLS; + static const QHash DEVELOPER_HOST_CAPS; public: + /** @brief Per-host request allowance (requests/second) when no developer cap applies. */ + static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; + /** @brief Floor for any per-host request allowance. */ + static constexpr int MIN_HOST_REQUEST_LIMIT = 1; + /** @brief Developer cap marking a host as never throttled per host (pacing still applies). */ + static constexpr int UNLIMITED_HOST_QUOTA = -1; + + /** + * @brief Developer-set per-host allowance ceilings (requests/second), keyed by host. + * + * Hosts not present default to DEFAULT_HOST_REQUEST_LIMIT. An entry of + * UNLIMITED_HOST_QUOTA marks a host that users may still lower, but that is never + * throttled per host by default. Users can never raise a host's allowance above its + * developer cap. + */ + static const QHash &getDeveloperHostCaps(); + explicit DownloadSettings(const QString &, QObject *); QStringList getAllURLs() const; @@ -27,9 +47,23 @@ public: [[nodiscard]] bool getDownloadSpoilersStatus() const; void setDownloadSpoilerStatus(bool _spoilerStatus); + /** @brief User-set per-host request allowances (requests/second). Missing hosts use the developer default. */ + QHash getHostRequestLimits() const; + void setHostRequestLimits(const QHash &hostRequestLimits); + + /** + * @brief Clamps the user's requested allowance for a host against its developer cap. + * @param host The host to clamp for + * @param requested The user-requested allowance in requests/second + * @return The effective allowance. Users may lower a host's allowance but never raise it + * above the developer cap; hosts with UNLIMITED_HOST_QUOTA have no upper bound. + */ + [[nodiscard]] int clampHostRequestLimit(const QString &host, int requested) const; + signals: void picDownloadChanged(); void downloadSpoilerStatusChanged(); + void hostRequestLimitsChanged(); }; #endif // COCKATRICE_DOWNLOADSETTINGS_H diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index eda778ae9..932ddb99b 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -334,6 +334,38 @@ TEST_F(SettingsDefaultsTest, Download_DownloadSpoilersStatus_Default) ASSERT_EQ(s.getDownloadSpoilersStatus(), false); } +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_Default) +{ + DownloadSettings s(settingsPath, nullptr); + ASSERT_TRUE(s.getHostRequestLimits().isEmpty()); +} + +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_SetAndGet) +{ + DownloadSettings s(settingsPath, nullptr); + s.setHostRequestLimits({{"api.scryfall.com", 5}}); + const QHash limits = s.getHostRequestLimits(); + ASSERT_EQ(limits.size(), 1); + ASSERT_EQ(limits.value("api.scryfall.com"), 5); +} + +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_StackedHostCaps) +{ + DownloadSettings s(settingsPath, nullptr); + // The developer cap for the Scryfall API lowers the ceiling to 9; a user can + // reduce it further but can never raise it above the cap. + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 9), 9); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 20), 9); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 5), 5); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 0), 1); + // Hosts without a developer cap fall back to the global default ceiling. + ASSERT_EQ(s.clampHostRequestLimit("gatherer.wizards.com", 10), 10); + ASSERT_EQ(s.clampHostRequestLimit("gatherer.wizards.com", 20), 10); + // The Scryfall CDN is unlocked: no upper bound (values are only floored). + ASSERT_EQ(s.clampHostRequestLimit("cards.scryfall.io", 20), 20); + ASSERT_EQ(s.clampHostRequestLimit("cards.scryfall.io", 0), 1); +} + // --- AppearanceSettings --- TEST_F(SettingsDefaultsTest, Appearance_ThemeName_Default) From 7b82ca08daa18fa5d188c4570c69db2b2f6706e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 17:32:30 +0200 Subject: [PATCH 03/13] [PictureLoader] Let unlocked hosts skip dispatch pacing; adjust limits per URL Two refinements to the per-host request caps: - Unlocked hosts (UNLIMITED_HOST_QUOTA, e.g. cards.scryfall.io) no longer wait on the 100ms dispatch pacing or consume the global per-second quota. dispatchQueuedRequest fires their queued requests back-to-back, bounded only by their 429 backoff window and Qt's per-host connection pool, so an unthrottled CDN is not artificially slowed. - The deck editor download settings page replaces the static grid of one spinbox per known host with an "Adjust Rate Limit" toolbar action on the URL list. It picks the host out of the selected URL and clamps the entry against the developer cap table (including for user-added URLs). Also fixes a review finding: resetRequestQuota could write the UNLIMITED_HOST_QUOTA sentinel (-1) into the sustained per-host quota when a host became unlocked mid-run, permanently poisoning its allowance. Stale entries for unlocked hosts are now dropped, and the per-second seed is clamped against the effective ceiling so a lowered limit applies immediately. --- .../card_picture_loader_worker.cpp | 47 +++++-- .../deck_editor_settings_page.cpp | 121 ++++++++---------- .../settings_page/deck_editor_settings_page.h | 10 +- .../settings/download_settings.cpp | 2 +- .../settings/download_settings.h | 2 +- 5 files changed, 91 insertions(+), 91 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 84c5a02c5..fab651d72 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -147,12 +147,19 @@ void CardPictureLoaderWorker::resetRequestQuota() hostQuotaRemaining.clear(); QDateTime now = QDateTime::currentDateTime(); - for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { + for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end();) { + // An unlocked host has no per-host allowance; drop any stale entry instead of + // recovering it towards the UNLIMITED_HOST_QUOTA sentinel, which would poison it. + if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + it = hostRequestQuota.erase(it); + continue; + } if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { // Recover towards the host's effective allowance ceiling, which may be // lowered by the user's per-host request limits. it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); } + ++it; } processQueuedRequests(); @@ -173,6 +180,27 @@ void CardPictureLoaderWorker::processQueuedRequests() void CardPictureLoaderWorker::dispatchQueuedRequest() { + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + return; + } + + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the dispatch pacing and the + // global per-second quota: dispatch every queued request for them back-to-back, bounded + // only by their 429 backoff window and Qt's per-host connection pool. + QDateTime now = QDateTime::currentDateTime(); + for (int i = 0; i < requestLoadQueue.size();) { + const auto &request = requestLoadQueue.at(i); + const QString host = request.first.host(); + if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && + !CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); + } else { + ++i; + } + } + if (requestLoadQueue.isEmpty() || requestQuota <= 0) { dispatchTimer.stop(); return; @@ -197,17 +225,11 @@ bool CardPictureLoaderWorker::processSingleRequest() continue; } const int ceiling = hostAllowanceCeiling(host); - // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the per-host allowance - // entirely; only the global quota and request pacing still apply. - if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { - makeRequest(request.first, request.second); - requestLoadQueue.removeAt(i); - return true; - } // Seed the allowance only now, so a host that was rate limited last second - // doesn't get a fresh full quota the moment it is queried mid-second. + // doesn't get a fresh full quota the moment it is queried mid-second. Clamp + // against the ceiling so a lowered user cap applies from this second onward. if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, ceiling)); + hostQuotaRemaining.insert(host, qMin(ceiling, hostRequestQuota.value(host, ceiling))); } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { @@ -232,12 +254,13 @@ int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + const int ceiling = hostAllowanceCeiling(host); + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { // Unlocked hosts have no per-host allowance to halve; the shared backoff // window tracked by the rate limiter still paces them. return; } - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, hostAllowanceCeiling(host)) / 2)); + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, ceiling) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index 4337bf0f3..6223cd480 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,16 +10,14 @@ #include #include #include -#include #include #include -#include #include #include #include #include -static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for hosts unlocked by the developer +static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for rate limits on hosts unlocked by the developer DeckEditorSettingsPage::DeckEditorSettingsPage() { @@ -70,11 +68,16 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() aRemove->setIcon(themePixmap(QStringLiteral("icons/decrement"))); connect(aRemove, &QAction::triggered, this, &DeckEditorSettingsPage::actRemoveURL); + aRateLimit = new QAction(this); + aRateLimit->setIcon(themePixmap(QStringLiteral("icons/cogwheel"))); + connect(aRateLimit, &QAction::triggered, this, &DeckEditorSettingsPage::actAdjustRateLimit); + auto *urlToolBar = new QToolBar; urlToolBar->setOrientation(Qt::Vertical); urlToolBar->addAction(aAdd); urlToolBar->addAction(aRemove); urlToolBar->addAction(aEdit); + urlToolBar->addAction(aRateLimit); urlToolBar->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::MinimumExpanding); auto *urlListLayout = new QHBoxLayout; @@ -101,53 +104,6 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() &DownloadSettings::setDownloadSpoilerStatus); connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, this, &DeckEditorSettingsPage::setSpoilersEnabled); - // Per-host request limit group: one spinbox per known picture host. A spinbox at its - // lower bound (0 for unlocked hosts, the developer cap for capped hosts) means "follow - // the developer default"; the worker clamps any explicit value against the developer cap. - mpRequestLimitGroupBox = new QGroupBox; - auto *requestLimitLayout = new QGridLayout; - - const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); - const QHash userLimits = SettingsCache::instance().downloads().getHostRequestLimits(); - - QSet hosts; - for (const QString &urlTemplate : SettingsCache::instance().downloads().getAllURLs()) { - hosts.insert(QUrl(urlTemplate).host()); - } - const QList devHosts = devCaps.keys(); - for (const QString &devHost : devHosts) { - hosts.insert(devHost); - } - - QList sortedHosts(hosts.cbegin(), hosts.cend()); - std::sort(sortedHosts.begin(), sortedHosts.end(), - [](const QString &a, const QString &b) { return a.localeAwareCompare(b) < 0; }); - - int hostRow = 1; - for (const QString &host : sortedHosts) { - const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; - - auto *hostLabel = new QLabel(host); - auto *spinBox = new QSpinBox; - if (unlocked) { - spinBox->setRange(0, UNLOCKED_HOST_LIMIT_MAX); // 0 means "unlimited" - spinBox->setValue(userLimits.value(host, 0)); - } else { - spinBox->setRange(DownloadSettings::MIN_HOST_REQUEST_LIMIT, devCap); - spinBox->setValue(userLimits.value(host, devCap)); - } - connect(spinBox, &QSpinBox::valueChanged, this, &DeckEditorSettingsPage::storeRequestLimits); - - requestLimitLayout->addWidget(hostLabel, hostRow, 0); - requestLimitLayout->addWidget(spinBox, hostRow, 1); - requestLimitSpinBoxes.insert(host, spinBox); - ++hostRow; - } - - requestLimitLayout->addWidget(&requestLimitHelpLabel, hostRow, 0, 1, 2); - mpRequestLimitGroupBox->setLayout(requestLimitLayout); - mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -156,7 +112,6 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() auto *lpMainLayout = new QVBoxLayout; lpMainLayout->addWidget(mpGeneralGroupBox); - lpMainLayout->addWidget(mpRequestLimitGroupBox); lpMainLayout->addWidget(mpSpoilerGroupBox); setLayout(lpMainLayout); @@ -217,22 +172,52 @@ void DeckEditorSettingsPage::storeSettings() SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); } -void DeckEditorSettingsPage::storeRequestLimits() +void DeckEditorSettingsPage::actAdjustRateLimit() { - QHash stored; - const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); - for (auto it = requestLimitSpinBoxes.cbegin(); it != requestLimitSpinBoxes.cend(); ++it) { - const QString host = it.key(); - const int value = it.value()->value(); - const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - // Only values that differ from the developer default are persisted; the worker - // treats a missing entry as "follow the developer default". - const int developerDefault = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? 0 : devCap; - if (value != developerDefault) { - stored.insert(host, value); - } + if (urlList->currentItem() == nullptr) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("Select a URL in the list first.")); + return; } - SettingsCache::instance().downloads().setHostRequestLimits(stored); + + const QString host = QUrl(urlList->currentItem()->text()).host(); + if (host.isEmpty()) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("The selected URL does not have a valid host.")); + return; + } + + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + const QHash currentLimits = SettingsCache::instance().downloads().getHostRequestLimits(); + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; + + bool ok = false; + int minimum; + int maximum; + int defaultValue; + if (unlocked) { + minimum = 0; // 0 means "unlimited" + maximum = UNLOCKED_HOST_LIMIT_MAX; + defaultValue = currentLimits.value(host, 0); + } else { + minimum = DownloadSettings::MIN_HOST_REQUEST_LIMIT; + maximum = devCap; + defaultValue = currentLimits.value(host, devCap); + } + + const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), + tr("Requests per second (developer maximum is %1):").arg(maximum), + defaultValue, minimum, maximum, 1, &ok); + if (!ok) { + return; + } + + QHash limits = currentLimits; + if (unlocked ? value == 0 : value == devCap) { + limits.remove(host); + } else { + limits.insert(host, value); + } + SettingsCache::instance().downloads().setHostRequestLimits(limits); } void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) @@ -301,9 +286,6 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput) void DeckEditorSettingsPage::retranslateUi() { mpGeneralGroupBox->setTitle(tr("URL Download Priority")); - mpRequestLimitGroupBox->setTitle(tr("Per-Host Request Limit")); - requestLimitHelpLabel.setText(tr("Pictures per second per host. Hosts can be lowered below their developer " - "limit but never raised above it; 0 means the host is not throttled per host.")); mpSpoilerGroupBox->setTitle(tr("Spoilers")); mcDownloadSpoilersCheckBox.setText(tr("Download Spoilers Automatically")); mcSpoilerSaveLabel.setText(tr("Spoiler Location:")); @@ -318,4 +300,5 @@ void DeckEditorSettingsPage::retranslateUi() aAdd->setText(tr("Add New URL")); aEdit->setText(tr("Edit URL")); aRemove->setText(tr("Remove URL")); -} \ No newline at end of file + aRateLimit->setText(tr("Adjust Rate Limit")); +} diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index b3745def4..57de5699e 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -5,11 +5,9 @@ #include #include -#include #include #include #include -#include class DeckEditorSettingsPage : public AbstractSettingsPage { @@ -21,7 +19,6 @@ public: private slots: void storeSettings(); - void storeRequestLimits(); void urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int); void setSpoilersEnabled(bool); void spoilerPathButtonClicked(); @@ -30,6 +27,7 @@ private slots: void actAddURL(); void actRemoveURL(); void actEditURL(); + void actAdjustRateLimit(); void resetDownloadedURLsButtonClicked(); private: @@ -37,16 +35,12 @@ private: QLabel urlLinkLabel; QCheckBox picDownloadCheckBox; QListWidget *urlList; - QAction *aAdd, *aEdit, *aRemove; + QAction *aAdd, *aEdit, *aRemove, *aRateLimit; QCheckBox mcDownloadSpoilersCheckBox; QLabel msDownloadSpoilersLabel; QGroupBox *mpGeneralGroupBox; QGroupBox *mpSpoilerGroupBox; - QGroupBox *mpRequestLimitGroupBox; - QLabel requestLimitHelpLabel; - QHash requestLimitSpinBoxes; - QLineEdit *mpSpoilerSavePathLineEdit; QLabel mcSpoilerSaveLabel; QLabel lastUpdatedLabel; diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index c510d8863..5293e390b 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -12,7 +12,7 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { // Developer-set ceilings for the per-host request allowance. Users may lower a host's // allowance via the download settings, but can never raise it above these values. Hosts // not listed default to DEFAULT_HOST_REQUEST_LIMIT. A cap of UNLIMITED_HOST_QUOTA marks a -// host that is never throttled per host (request pacing and 429 backoff still apply). +// host that is never throttled per host and skips the dispatch pacing (429 backoff still applies). const QHash DownloadSettings::DEVELOPER_HOST_CAPS = { // The Scryfall API enforces 10 requests/second; stay one under so a burst can't trip 429s. {"api.scryfall.com", 9}, diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index e075a3c07..9fcf9e61d 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -24,7 +24,7 @@ public: static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; /** @brief Floor for any per-host request allowance. */ static constexpr int MIN_HOST_REQUEST_LIMIT = 1; - /** @brief Developer cap marking a host as never throttled per host (pacing still applies). */ + /** @brief Developer cap marking a host as never throttled per host or by the dispatch pacing. */ static constexpr int UNLIMITED_HOST_QUOTA = -1; /** From 77ded1da97adb24b81eb73778e54e8cda393b0bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 22:50:19 +0200 Subject: [PATCH 04/13] [PictureLoader] Fix worker thread shutdown and cross-thread cache clearing clearNetworkCache() ran directly on the UI thread while the worker thread owned the disk cache and redirect cache, racing cache reads/writes. Make it a worker-thread slot invoked via a blocking queued call when the thread is running, so the 'Cached card pictures have been reset.' message is truthful. The worker thread was also never quit()/wait()ed: both destructors only deleteLater'd their objects, so Qt warned 'QThread: Destroyed while thread is still running' and leaked a running loop at exit. Wire the worker's finished() signal to its own deleteLater() (canonical worker-object pattern), add shutdownThread() to stop the loop, and let CardPictureLoader destroy the QThread only after wait() has returned. --- .../card_picture_loader.cpp | 20 ++++++++++-- .../card_picture_loader_worker.cpp | 26 +++++++++++++++- .../card_picture_loader_worker.h | 31 +++++++++++++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index 8c81d641d..e7e31f5a2 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -55,7 +56,14 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) CardPictureLoader::~CardPictureLoader() { - worker->deleteLater(); + if (worker) { + // Capture the thread first: shutdownThread() blocks until the worker has been freed by the + // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. + QThread *pictureLoaderThread = worker->workerThread(); + worker->shutdownThread(); + worker = nullptr; + delete pictureLoaderThread; + } } void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size) @@ -295,7 +303,15 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - getInstance().worker->clearNetworkCache(); + auto &worker = *getInstance().worker; + // The disk cache and redirect cache are owned by the worker thread; clearing them from the + // UI thread would race with the worker's cache reads/writes. Block until the worker thread + // has executed the clear so the "Cached card pictures have been reset." message is truthful. + if (worker.isRunning()) { + QMetaObject::invokeMethod(&worker, "clearNetworkCache", Qt::BlockingQueuedConnection); + } else { + worker.clearNetworkCache(); + } } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index fab651d72..daac1bff5 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -60,6 +60,9 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() localLoader = new CardPictureLoaderLocal(this); pictureLoaderThread = new QThread; + // The worker object frees itself once its thread finishes, so no event loop is left + // running and the QThread is never destroyed while still executing. + connect(pictureLoaderThread, &QThread::finished, this, &QObject::deleteLater); pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); @@ -83,7 +86,28 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() CardPictureLoaderWorker::~CardPictureLoaderWorker() { saveRedirectCache(); - pictureLoaderThread->deleteLater(); +} + +void CardPictureLoaderWorker::shutdownThread() +{ + // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as + // its event loop exits, so nothing - not even a member read - may run once wait() returns. + // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. + QThread *thread = pictureLoaderThread; + if (thread) { + thread->quit(); + thread->wait(); + } +} + +QThread *CardPictureLoaderWorker::workerThread() const +{ + return pictureLoaderThread; +} + +bool CardPictureLoaderWorker::isRunning() const +{ + return pictureLoaderThread != nullptr && pictureLoaderThread->isRunning(); } void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 70bc36419..96405252f 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -74,10 +74,37 @@ public: */ void onHostRateLimited(const QString &host); - /** @brief Clears the network cache and redirect cache. */ - void clearNetworkCache(); + /** + * @brief Stops the worker thread and releases it. + * + * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an + * exit request to the worker's event loop and QThread::wait() blocks until the loop has + * returned and the thread finished. Only QThread members are touched here, so this method is + * safe to call from the owning thread. The worker object itself is freed by the finished() -> + * deleteLater chain (see the constructor); the QThread object is deleted afterwards by the + * owner (CardPictureLoader::~CardPictureLoader), not by this method. + */ + void shutdownThread(); + + /** @return Whether the worker's thread is currently running. */ + bool isRunning() const; + + /** + * @brief Returns the worker's QThread. + * @return The worker thread + * + * Only meaningful while the worker object is alive; capture it before calling shutdownThread(). + */ + QThread *workerThread() const; public slots: + /** + * @brief Clears the network cache and redirect cache. + * + * Runs on the worker thread; invoke it via a queued call when coming from another thread, + * since both caches are owned by the worker thread. + */ + void clearNetworkCache(); /** * @brief Makes a network request for the given URL using the specified worker. * @param url URL to load From 91519166f9de65db162908ca81a7e93667b32bae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 03:55:45 +0200 Subject: [PATCH 05/13] [PictureLoader] Guard dispatch timer restarts and drop dead request quota --- .../card_picture_loader_worker.cpp | 22 +++++++++++-------- .../card_picture_loader_worker.h | 3 +-- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 4a2caaab4..b29e838ff 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -23,8 +23,7 @@ static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between indi static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets CardPictureLoaderWorker::CardPictureLoaderWorker() - : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), - requestQuota(MAX_REQUESTS_PER_SEC) + : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -137,8 +136,6 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { - requestQuota = MAX_REQUESTS_PER_SEC; - QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { @@ -156,27 +153,34 @@ void CardPictureLoaderWorker::resetRequestQuota() void CardPictureLoaderWorker::processQueuedRequests() { + Q_ASSERT(thread() == QThread::currentThread()); + if (requestLoadQueue.isEmpty()) { dispatchTimer.stop(); + requestTimer.stop(); return; } // Start lazily from the worker's own thread: QTimer must be started in the thread it lives in. if (!requestTimer.isActive()) { requestTimer.start(); } - dispatchTimer.start(); + // Restarting an active timer would reset the pacing countdown, so a burst of enqueues could + // keep starving the dispatcher; only start it when it has actually stopped. + if (!dispatchTimer.isActive()) { + dispatchTimer.start(); + } } void CardPictureLoaderWorker::dispatchQueuedRequest() { - if (requestLoadQueue.isEmpty() || requestQuota <= 0) { + if (requestLoadQueue.isEmpty()) { + // All queued requests have been dispatched; stop the pacing and quota-reset timers. dispatchTimer.stop(); + requestTimer.stop(); return; } - if (processSingleRequest()) { - --requestQuota; - } else { + if (!processSingleRequest()) { // No queued host currently has allowance left in this second; wait for the quota reset. dispatchTimer.stop(); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 9f7fd9437..f0fe17976 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -86,7 +86,7 @@ public slots: */ QNetworkReply *makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *workThread); - /** @brief Processes all queued requests respecting the request quota. */ + /** @brief Starts the pacing timers if there is queued work, stops them when the queue is empty. */ void processQueuedRequests(); /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ @@ -121,7 +121,6 @@ private: bool picDownload; ///< Whether downloading images from network is enabled QQueue> requestLoadQueue; ///< Queue of pending network requests - int requestQuota; ///< Remaining requests allowed per second QTimer requestTimer; ///< Timer to reset the request quota QTimer dispatchTimer; ///< Timer pacing individual network requests QHash hostRequestQuota; ///< Sustained per-host request allowance From 5956dcab8315b832d09c13e8dc312e364fb1d8df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:52:25 +0200 Subject: [PATCH 06/13] [PictureLoader] Seed per-host allowances on demand and skip hosts in 429 backoff The quota reset re-filled every host's remaining allowance to a full MAX_REQUESTS_PER_SEC as soon as the queue had a request for it. A server that was just rate limited could therefore be hammered again at full speed immediately after (or even during) recovery. Only seed a host's allowance the first time it is dispatched in the current second, seeded from its reduced sustained quota, and skip hosts still inside their 429 backoff window entirely. This makes the pacing commit's burst-free behavior hold per host too, instead of just smoothing the global aggregate. --- .../card_picture_loader_worker.cpp | 23 +++++++++++++------ .../card_picture_loader_worker_work.cpp | 5 ++++ .../card_picture_loader_worker_work.h | 3 +++ 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index b29e838ff..91a984dea 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -136,6 +136,10 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { + // Allowances are seeded per host on demand in processSingleRequest(), so a + // rate-limited host never gets a fresh full quota mid-second. + hostQuotaRemaining.clear(); + QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { @@ -143,11 +147,6 @@ void CardPictureLoaderWorker::resetRequestQuota() } } - for (const auto &request : requestLoadQueue) { - const QString host = request.first.host(); - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); - } - processQueuedRequests(); } @@ -188,10 +187,20 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() bool CardPictureLoaderWorker::processSingleRequest() { + QDateTime now = QDateTime::currentDateTime(); for (int i = 0; i < requestLoadQueue.size(); ++i) { const auto &request = requestLoadQueue.at(i); - QString host = request.first.host(); - int allowance = hostQuotaRemaining.value(host, MAX_REQUESTS_PER_SEC); + const QString host = request.first.host(); + // Don't dispatch requests to a host that is currently in its 429 backoff. + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + continue; + } + // Seed the allowance only now, so a host that was rate limited last second + // doesn't get a fresh full quota the moment it is queried mid-second. + if (!hostQuotaRemaining.contains(host)) { + hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + } + int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { hostQuotaRemaining.insert(host, allowance - 1); makeRequest(request.first, request.second); diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index 66c56337c..072a919d7 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp @@ -22,6 +22,11 @@ static const QStringList MD5_BLACKLIST = { "fbc7d763c08771c260b39e2115414eeb" // Current card back hash }; +ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() +{ + return s_rateLimiter; +} + CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index 1e56a4373..8490cb3ac 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -43,6 +43,9 @@ public: CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading + /** @brief Shared per-server 429 backoff state. */ + static ServerRateLimiter &rateLimiter(); + public slots: /** * @brief Handles a finished network reply for the card image. From 310caa7dc0694f3cddc4a73f8d53b532440fc827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 04:21:09 +0200 Subject: [PATCH 07/13] [PictureLoader] Hand backed-off requests back to their worker instead of parking them --- .../card_picture_loader_worker.cpp | 25 +++++++++---- .../card_picture_loader_worker_work.cpp | 2 +- .../card_picture_loader_worker_work.h | 35 +++++++++++-------- 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 91a984dea..2925b3af9 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -108,6 +108,14 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture QUrl cachedRedirect = getCachedRedirect(url); if (!cachedRedirect.isEmpty()) { emit imageRequestSucceeded(url); + // The redirect target is a different host, which may itself be in 429 backoff; hand the + // entry back to its worker so it waits the backoff out instead of dispatching straight + // onto the backed-off host. + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(cachedRedirect.host(), + QDateTime::currentDateTime())) { + worker->scheduleDeferredRetry(); + return nullptr; + } return makeRequest(cachedRedirect, worker); } @@ -136,8 +144,9 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { - // Allowances are seeded per host on demand in processSingleRequest(), so a - // rate-limited host never gets a fresh full quota mid-second. + // Allowances are seeded lazily per host in processSingleRequest() when a request is first + // looked at in a new second, so a host that enters the queue mid-second now gets its reduced + // per-host allowance instead of falling through to the full per-second default. hostQuotaRemaining.clear(); QDateTime now = QDateTime::currentDateTime(); @@ -191,12 +200,16 @@ bool CardPictureLoaderWorker::processSingleRequest() for (int i = 0; i < requestLoadQueue.size(); ++i) { const auto &request = requestLoadQueue.at(i); const QString host = request.first.host(); - // Don't dispatch requests to a host that is currently in its 429 backoff. + // Don't dispatch requests to a host that is currently in its 429 backoff; hand the entry + // back to its worker so it can wait the backoff out or fall through to another source, + // instead of leaving it parked in the queue with no reply pending. if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { - continue; + requestLoadQueue.removeAt(i); + request.second->startNextPicDownload(); + return true; } - // Seed the allowance only now, so a host that was rate limited last second - // doesn't get a fresh full quota the moment it is queried mid-second. + // Seed the allowance now so a host that was rate limited gets its reduced + // allowance instead of a fresh full quota mid-second. if (!hostQuotaRemaining.contains(host)) { hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index 072a919d7..e4a36ab8c 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp @@ -22,7 +22,7 @@ static const QStringList MD5_BLACKLIST = { "fbc7d763c08771c260b39e2115414eeb" // Current card back hash }; -ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() +const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() { return s_rateLimiter; } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index 8490cb3ac..f05d727de 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -44,7 +44,27 @@ public: CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading /** @brief Shared per-server 429 backoff state. */ - static ServerRateLimiter &rateLimiter(); + static const ServerRateLimiter &rateLimiter(); + + /** + * @brief Starts downloading the next URL for this card. + * + * Skips URLs whose server is currently in 429 backoff, either waiting the + * backoff out or falling through to the other configured sources. Also used by + * the dispatch machinery to hand an entry back after it was removed from the + * request queue when its host turned out to be backed off. + */ + void startNextPicDownload(); + + /** + * @brief Schedules a deferred retry after the relevant server backoff expires. + * + * Waits on the current URL's server when it is the reason we are blocked, + * otherwise on the earliest active backoff. If no servers are in backoff, + * concludes with failure. Otherwise resets the CardPictureToLoad indices and + * retries after the backoff period. + */ + void scheduleDeferredRetry(); public slots: /** @@ -58,9 +78,6 @@ private: static ServerRateLimiter s_rateLimiter; ///< Shared per-server 429 backoff state - /** @brief Starts downloading the next URL for this card. */ - void startNextPicDownload(); - /** @brief Called when all URLs have been exhausted or download failed. */ void picDownloadFailed(); @@ -85,16 +102,6 @@ private: */ void concludeImageLoad(const QImage &image); - /** - * @brief Schedules a deferred retry after the relevant server backoff expires. - * - * Waits on the current URL's server when it is the reason we are blocked, - * otherwise on the earliest active backoff. If no servers are in backoff, - * concludes with failure. Otherwise resets the CardPictureToLoad indices and - * retries after the backoff period. - */ - void scheduleDeferredRetry(); - private slots: /** @brief Updates the picDownload setting when it changes. */ void picDownloadChanged(); From ab72ba813267a6d1411b4128ab2a8e79285cc6c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 16:58:10 +0200 Subject: [PATCH 08/13] [PictureLoader] Add user-configurable per-host request caps Picture downloads were throttled to a uniform 10 requests/second per host with no way to tune a specific server. A rate-limited API host (Scryfall caps at 10 req/s) can trip 429s during bursts, and CDN hosts with no rate limit were throttled needlessly. Introduce developer-owned per-host caps that users can only ever lower, never raise, exposed in the download settings page: - DownloadSettings::DEVELOPER_HOST_CAPS sets the ceiling per host (api.scryfall.com 9, cards.scryfall.io unlimited, others 10). - A new hostRequestLimits setting stores user overrides in downloads.ini; clampHostRequestLimit() bounds them to [1, devCap] so a user can reduce api.scryfall.com to 5 but never raise it above 9. - The picture worker seeds, halves on 429, and recovers its sustained per-host allowance against the effective ceiling instead of the global maximum, and skips per-host accounting entirely for unlocked hosts (cards.scryfall.io) while global pacing and 429 backoff still apply. - The deck editor settings page gains one spinbox per known host, each clamped to its developer cap. --- .../card_picture_loader_worker.cpp | 41 ++++++++-- .../card_picture_loader_worker.h | 9 +++ .../deck_editor_settings_page.cpp | 74 +++++++++++++++++++ .../settings_page/deck_editor_settings_page.h | 7 ++ .../settings/download_settings.cpp | 45 +++++++++++ .../settings/download_settings.h | 34 +++++++++ tests/settings/settings_defaults_test.cpp | 32 ++++++++ 7 files changed, 236 insertions(+), 6 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 2925b3af9..9c3069bd9 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -16,14 +16,15 @@ #include #include -static constexpr int MAX_REQUESTS_PER_SEC = 10; -static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance +static constexpr int MAX_REQUESTS_PER_SEC = DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT; +static constexpr int MIN_HOST_QUOTA = DownloadSettings::MIN_HOST_REQUEST_LIMIT; static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between individual network requests static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets CardPictureLoaderWorker::CardPictureLoaderWorker() - : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()) + : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), + hostRequestLimits(SettingsCache::instance().downloads().getHostRequestLimits()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -73,6 +74,9 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest); dispatchTimer.setInterval(DISPATCH_INTERVAL_MS); + + connect(&SettingsCache::instance().downloads(), &DownloadSettings::hostRequestLimitsChanged, this, + [this] { hostRequestLimits = SettingsCache::instance().downloads().getHostRequestLimits(); }); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -152,7 +156,9 @@ void CardPictureLoaderWorker::resetRequestQuota() QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { - it.value() = qMin(MAX_REQUESTS_PER_SEC, it.value() + 1); + // Recover towards the host's effective allowance ceiling, which may be + // lowered by the user's per-host request limits. + it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); } } @@ -208,10 +214,18 @@ bool CardPictureLoaderWorker::processSingleRequest() request.second->startNextPicDownload(); return true; } +const int ceiling = hostAllowanceCeiling(host); + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the per-host allowance + // entirely; only the request pacing still applies. + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); + return true; + } // Seed the allowance now so a host that was rate limited gets its reduced // allowance instead of a fresh full quota mid-second. if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + hostQuotaRemaining.insert(host, hostRequestQuota.value(host, ceiling)); } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { @@ -224,9 +238,24 @@ bool CardPictureLoaderWorker::processSingleRequest() return false; } +int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const +{ + const int devCap = DownloadSettings::getDeveloperHostCaps().value(host, MAX_REQUESTS_PER_SEC); + if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestLimits.contains(host)) { + return DownloadSettings::UNLIMITED_HOST_QUOTA; + } + const int requested = hostRequestLimits.value(host, devCap); + return SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); + if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // Unlocked hosts have no per-host allowance to halve; the shared backoff + // window tracked by the rate limiter still paces them. + return; + } + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, hostAllowanceCeiling(host)) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index f0fe17976..d066d224a 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -124,12 +124,21 @@ private: QTimer requestTimer; ///< Timer to reset the request quota QTimer dispatchTimer; ///< Timer pacing individual network requests QHash hostRequestQuota; ///< Sustained per-host request allowance + QHash hostRequestLimits; ///< User-set per-host request allowances QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited CardPictureLoaderLocal *localLoader; ///< Loader for local images QSet currentlyLoading; ///< Deduplication: contains pixmapCacheKey currently being loaded + /** + * @brief Effective per-host allowance ceiling for a host. + * @param host The host to look up + * @return The allowance ceiling in requests/second, or DownloadSettings::UNLIMITED_HOST_QUOTA + * when the developer unlocked the host and no user limit is set for it. + */ + [[nodiscard]] int hostAllowanceCeiling(const QString &host) const; + /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index f3eac05b8..4337bf0f3 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,12 +10,17 @@ #include #include #include +#include #include +#include +#include #include #include #include #include +static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for hosts unlocked by the developer + DeckEditorSettingsPage::DeckEditorSettingsPage() { picDownloadCheckBox.setChecked(SettingsCache::instance().downloads().getPicDownload()); @@ -96,6 +101,53 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() &DownloadSettings::setDownloadSpoilerStatus); connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, this, &DeckEditorSettingsPage::setSpoilersEnabled); + // Per-host request limit group: one spinbox per known picture host. A spinbox at its + // lower bound (0 for unlocked hosts, the developer cap for capped hosts) means "follow + // the developer default"; the worker clamps any explicit value against the developer cap. + mpRequestLimitGroupBox = new QGroupBox; + auto *requestLimitLayout = new QGridLayout; + + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + const QHash userLimits = SettingsCache::instance().downloads().getHostRequestLimits(); + + QSet hosts; + for (const QString &urlTemplate : SettingsCache::instance().downloads().getAllURLs()) { + hosts.insert(QUrl(urlTemplate).host()); + } + const QList devHosts = devCaps.keys(); + for (const QString &devHost : devHosts) { + hosts.insert(devHost); + } + + QList sortedHosts(hosts.cbegin(), hosts.cend()); + std::sort(sortedHosts.begin(), sortedHosts.end(), + [](const QString &a, const QString &b) { return a.localeAwareCompare(b) < 0; }); + + int hostRow = 1; + for (const QString &host : sortedHosts) { + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; + + auto *hostLabel = new QLabel(host); + auto *spinBox = new QSpinBox; + if (unlocked) { + spinBox->setRange(0, UNLOCKED_HOST_LIMIT_MAX); // 0 means "unlimited" + spinBox->setValue(userLimits.value(host, 0)); + } else { + spinBox->setRange(DownloadSettings::MIN_HOST_REQUEST_LIMIT, devCap); + spinBox->setValue(userLimits.value(host, devCap)); + } + connect(spinBox, &QSpinBox::valueChanged, this, &DeckEditorSettingsPage::storeRequestLimits); + + requestLimitLayout->addWidget(hostLabel, hostRow, 0); + requestLimitLayout->addWidget(spinBox, hostRow, 1); + requestLimitSpinBoxes.insert(host, spinBox); + ++hostRow; + } + + requestLimitLayout->addWidget(&requestLimitHelpLabel, hostRow, 0, 1, 2); + mpRequestLimitGroupBox->setLayout(requestLimitLayout); + mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -104,6 +156,7 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() auto *lpMainLayout = new QVBoxLayout; lpMainLayout->addWidget(mpGeneralGroupBox); + lpMainLayout->addWidget(mpRequestLimitGroupBox); lpMainLayout->addWidget(mpSpoilerGroupBox); setLayout(lpMainLayout); @@ -164,6 +217,24 @@ void DeckEditorSettingsPage::storeSettings() SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); } +void DeckEditorSettingsPage::storeRequestLimits() +{ + QHash stored; + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + for (auto it = requestLimitSpinBoxes.cbegin(); it != requestLimitSpinBoxes.cend(); ++it) { + const QString host = it.key(); + const int value = it.value()->value(); + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + // Only values that differ from the developer default are persisted; the worker + // treats a missing entry as "follow the developer default". + const int developerDefault = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? 0 : devCap; + if (value != developerDefault) { + stored.insert(host, value); + } + } + SettingsCache::instance().downloads().setHostRequestLimits(stored); +} + void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) { storeSettings(); @@ -230,6 +301,9 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput) void DeckEditorSettingsPage::retranslateUi() { mpGeneralGroupBox->setTitle(tr("URL Download Priority")); + mpRequestLimitGroupBox->setTitle(tr("Per-Host Request Limit")); + requestLimitHelpLabel.setText(tr("Pictures per second per host. Hosts can be lowered below their developer " + "limit but never raised above it; 0 means the host is not throttled per host.")); mpSpoilerGroupBox->setTitle(tr("Spoilers")); mcDownloadSpoilersCheckBox.setText(tr("Download Spoilers Automatically")); mcSpoilerSaveLabel.setText(tr("Spoiler Location:")); diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index 5db009c8a..b3745def4 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -5,9 +5,11 @@ #include #include +#include #include #include #include +#include class DeckEditorSettingsPage : public AbstractSettingsPage { @@ -19,6 +21,7 @@ public: private slots: void storeSettings(); + void storeRequestLimits(); void urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int); void setSpoilersEnabled(bool); void spoilerPathButtonClicked(); @@ -40,6 +43,10 @@ private: QGroupBox *mpGeneralGroupBox; QGroupBox *mpSpoilerGroupBox; + QGroupBox *mpRequestLimitGroupBox; + QLabel requestLimitHelpLabel; + QHash requestLimitSpinBoxes; + QLineEdit *mpSpoilerSavePathLineEdit; QLabel mcSpoilerSaveLabel; QLabel lastUpdatedLabel; diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index cfa1c054e..c510d8863 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -9,6 +9,22 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { "https://gatherer.wizards.com/Handlers/Image.ashx?multiverseid=!set:muid!&type=card", "https://gatherer.wizards.com/Handlers/Image.ashx?name=!name!&type=card"}; +// Developer-set ceilings for the per-host request allowance. Users may lower a host's +// allowance via the download settings, but can never raise it above these values. Hosts +// not listed default to DEFAULT_HOST_REQUEST_LIMIT. A cap of UNLIMITED_HOST_QUOTA marks a +// host that is never throttled per host (request pacing and 429 backoff still apply). +const QHash DownloadSettings::DEVELOPER_HOST_CAPS = { + // The Scryfall API enforces 10 requests/second; stay one under so a burst can't trip 429s. + {"api.scryfall.com", 9}, + // The Scryfall image CDN has no documented per-client rate limit. + {"cards.scryfall.io", UNLIMITED_HOST_QUOTA}, +}; + +const QHash &DownloadSettings::getDeveloperHostCaps() +{ + return DEVELOPER_HOST_CAPS; +} + DownloadSettings::DownloadSettings(const QString &settingPath, QObject *parent = nullptr) : SettingsManager(settingPath + "downloads.ini", "downloads", QString(), parent) { @@ -50,3 +66,32 @@ void DownloadSettings::setDownloadSpoilerStatus(bool _spoilerStatus) setValue(_spoilerStatus, "downloadSpoilers"); emit downloadSpoilerStatusChanged(); } + +QHash DownloadSettings::getHostRequestLimits() const +{ + const QVariantMap stored = getValue("hostRequestLimits").toMap(); + QHash hostRequestLimits; + for (auto it = stored.cbegin(); it != stored.cend(); ++it) { + hostRequestLimits.insert(it.key(), it.value().toInt()); + } + return hostRequestLimits; +} + +void DownloadSettings::setHostRequestLimits(const QHash &hostRequestLimits) +{ + QVariantMap stored; + for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { + stored.insert(it.key(), it.value()); + } + setValue(stored, "hostRequestLimits"); + emit hostRequestLimitsChanged(); +} + +int DownloadSettings::clampHostRequestLimit(const QString &host, int requested) const +{ + const int devCap = DEVELOPER_HOST_CAPS.value(host, DEFAULT_HOST_REQUEST_LIMIT); + if (devCap == UNLIMITED_HOST_QUOTA) { + return qMax(MIN_HOST_REQUEST_LIMIT, requested); + } + return qBound(MIN_HOST_REQUEST_LIMIT, requested, devCap); +} diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index a3a6f4ca9..e075a3c07 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -9,14 +9,34 @@ #include "settings_manager.h" +#include + class DownloadSettings : public SettingsManager { Q_OBJECT friend class SettingsCache; static const QStringList DEFAULT_DOWNLOAD_URLS; + static const QHash DEVELOPER_HOST_CAPS; public: + /** @brief Per-host request allowance (requests/second) when no developer cap applies. */ + static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; + /** @brief Floor for any per-host request allowance. */ + static constexpr int MIN_HOST_REQUEST_LIMIT = 1; + /** @brief Developer cap marking a host as never throttled per host (pacing still applies). */ + static constexpr int UNLIMITED_HOST_QUOTA = -1; + + /** + * @brief Developer-set per-host allowance ceilings (requests/second), keyed by host. + * + * Hosts not present default to DEFAULT_HOST_REQUEST_LIMIT. An entry of + * UNLIMITED_HOST_QUOTA marks a host that users may still lower, but that is never + * throttled per host by default. Users can never raise a host's allowance above its + * developer cap. + */ + static const QHash &getDeveloperHostCaps(); + explicit DownloadSettings(const QString &, QObject *); QStringList getAllURLs() const; @@ -27,9 +47,23 @@ public: [[nodiscard]] bool getDownloadSpoilersStatus() const; void setDownloadSpoilerStatus(bool _spoilerStatus); + /** @brief User-set per-host request allowances (requests/second). Missing hosts use the developer default. */ + QHash getHostRequestLimits() const; + void setHostRequestLimits(const QHash &hostRequestLimits); + + /** + * @brief Clamps the user's requested allowance for a host against its developer cap. + * @param host The host to clamp for + * @param requested The user-requested allowance in requests/second + * @return The effective allowance. Users may lower a host's allowance but never raise it + * above the developer cap; hosts with UNLIMITED_HOST_QUOTA have no upper bound. + */ + [[nodiscard]] int clampHostRequestLimit(const QString &host, int requested) const; + signals: void picDownloadChanged(); void downloadSpoilerStatusChanged(); + void hostRequestLimitsChanged(); }; #endif // COCKATRICE_DOWNLOADSETTINGS_H diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index eda778ae9..932ddb99b 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -334,6 +334,38 @@ TEST_F(SettingsDefaultsTest, Download_DownloadSpoilersStatus_Default) ASSERT_EQ(s.getDownloadSpoilersStatus(), false); } +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_Default) +{ + DownloadSettings s(settingsPath, nullptr); + ASSERT_TRUE(s.getHostRequestLimits().isEmpty()); +} + +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_SetAndGet) +{ + DownloadSettings s(settingsPath, nullptr); + s.setHostRequestLimits({{"api.scryfall.com", 5}}); + const QHash limits = s.getHostRequestLimits(); + ASSERT_EQ(limits.size(), 1); + ASSERT_EQ(limits.value("api.scryfall.com"), 5); +} + +TEST_F(SettingsDefaultsTest, Download_HostRequestLimits_StackedHostCaps) +{ + DownloadSettings s(settingsPath, nullptr); + // The developer cap for the Scryfall API lowers the ceiling to 9; a user can + // reduce it further but can never raise it above the cap. + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 9), 9); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 20), 9); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 5), 5); + ASSERT_EQ(s.clampHostRequestLimit("api.scryfall.com", 0), 1); + // Hosts without a developer cap fall back to the global default ceiling. + ASSERT_EQ(s.clampHostRequestLimit("gatherer.wizards.com", 10), 10); + ASSERT_EQ(s.clampHostRequestLimit("gatherer.wizards.com", 20), 10); + // The Scryfall CDN is unlocked: no upper bound (values are only floored). + ASSERT_EQ(s.clampHostRequestLimit("cards.scryfall.io", 20), 20); + ASSERT_EQ(s.clampHostRequestLimit("cards.scryfall.io", 0), 1); +} + // --- AppearanceSettings --- TEST_F(SettingsDefaultsTest, Appearance_ThemeName_Default) From d6dea73be52865843d221f6a9ee955ce267b58f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 17:32:30 +0200 Subject: [PATCH 09/13] [PictureLoader] Let unlocked hosts skip dispatch pacing; adjust limits per URL Two refinements to the per-host request caps: - Unlocked hosts (UNLIMITED_HOST_QUOTA, e.g. cards.scryfall.io) no longer wait on the 100ms dispatch pacing or consume the global per-second quota. dispatchQueuedRequest fires their queued requests back-to-back, bounded only by their 429 backoff window and Qt's per-host connection pool, so an unthrottled CDN is not artificially slowed. - The deck editor download settings page replaces the static grid of one spinbox per known host with an "Adjust Rate Limit" toolbar action on the URL list. It picks the host out of the selected URL and clamps the entry against the developer cap table (including for user-added URLs). Also fixes a review finding: resetRequestQuota could write the UNLIMITED_HOST_QUOTA sentinel (-1) into the sustained per-host quota when a host became unlocked mid-run, permanently poisoning its allowance. Stale entries for unlocked hosts are now dropped, and the per-second seed is clamped against the effective ceiling so a lowered limit applies immediately. --- .../card_picture_loader_worker.cpp | 52 ++++++-- .../deck_editor_settings_page.cpp | 121 ++++++++---------- .../settings_page/deck_editor_settings_page.h | 10 +- .../settings/download_settings.cpp | 2 +- .../settings/download_settings.h | 2 +- 5 files changed, 94 insertions(+), 93 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 9c3069bd9..151996505 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -154,12 +154,19 @@ void CardPictureLoaderWorker::resetRequestQuota() hostQuotaRemaining.clear(); QDateTime now = QDateTime::currentDateTime(); - for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { + for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end();) { + // An unlocked host has no per-host allowance; drop any stale entry instead of + // recovering it towards the UNLIMITED_HOST_QUOTA sentinel, which would poison it. + if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + it = hostRequestQuota.erase(it); + continue; + } if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { // Recover towards the host's effective allowance ceiling, which may be // lowered by the user's per-host request limits. it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); } + ++it; } processQueuedRequests(); @@ -194,6 +201,28 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() return; } + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the dispatch pacing: dispatch every + // queued request for them back-to-back, bounded only by their 429 backoff window and Qt's + // per-host connection pool. + QDateTime now = QDateTime::currentDateTime(); + for (int i = 0; i < requestLoadQueue.size();) { + const auto &request = requestLoadQueue.at(i); + const QString host = request.first.host(); + if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && + !CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); + } else { + ++i; + } + } + + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + requestTimer.stop(); + return; + } + if (!processSingleRequest()) { // No queued host currently has allowance left in this second; wait for the quota reset. dispatchTimer.stop(); @@ -214,18 +243,12 @@ bool CardPictureLoaderWorker::processSingleRequest() request.second->startNextPicDownload(); return true; } -const int ceiling = hostAllowanceCeiling(host); - // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the per-host allowance - // entirely; only the request pacing still applies. - if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { - makeRequest(request.first, request.second); - requestLoadQueue.removeAt(i); - return true; - } - // Seed the allowance now so a host that was rate limited gets its reduced - // allowance instead of a fresh full quota mid-second. + const int ceiling = hostAllowanceCeiling(host); + // Seed the allowance lazily so a host that enters the queue mid-second gets its reduced + // per-host allowance, clamped against the ceiling so a lowered user cap applies from this + // second onward. if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, hostRequestQuota.value(host, ceiling)); + hostQuotaRemaining.insert(host, qMin(ceiling, hostRequestQuota.value(host, ceiling))); } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { @@ -250,12 +273,13 @@ int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + const int ceiling = hostAllowanceCeiling(host); + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { // Unlocked hosts have no per-host allowance to halve; the shared backoff // window tracked by the rate limiter still paces them. return; } - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, hostAllowanceCeiling(host)) / 2)); + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, ceiling) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index 4337bf0f3..6223cd480 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,16 +10,14 @@ #include #include #include -#include #include #include -#include #include #include #include #include -static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for hosts unlocked by the developer +static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for rate limits on hosts unlocked by the developer DeckEditorSettingsPage::DeckEditorSettingsPage() { @@ -70,11 +68,16 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() aRemove->setIcon(themePixmap(QStringLiteral("icons/decrement"))); connect(aRemove, &QAction::triggered, this, &DeckEditorSettingsPage::actRemoveURL); + aRateLimit = new QAction(this); + aRateLimit->setIcon(themePixmap(QStringLiteral("icons/cogwheel"))); + connect(aRateLimit, &QAction::triggered, this, &DeckEditorSettingsPage::actAdjustRateLimit); + auto *urlToolBar = new QToolBar; urlToolBar->setOrientation(Qt::Vertical); urlToolBar->addAction(aAdd); urlToolBar->addAction(aRemove); urlToolBar->addAction(aEdit); + urlToolBar->addAction(aRateLimit); urlToolBar->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::MinimumExpanding); auto *urlListLayout = new QHBoxLayout; @@ -101,53 +104,6 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() &DownloadSettings::setDownloadSpoilerStatus); connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, this, &DeckEditorSettingsPage::setSpoilersEnabled); - // Per-host request limit group: one spinbox per known picture host. A spinbox at its - // lower bound (0 for unlocked hosts, the developer cap for capped hosts) means "follow - // the developer default"; the worker clamps any explicit value against the developer cap. - mpRequestLimitGroupBox = new QGroupBox; - auto *requestLimitLayout = new QGridLayout; - - const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); - const QHash userLimits = SettingsCache::instance().downloads().getHostRequestLimits(); - - QSet hosts; - for (const QString &urlTemplate : SettingsCache::instance().downloads().getAllURLs()) { - hosts.insert(QUrl(urlTemplate).host()); - } - const QList devHosts = devCaps.keys(); - for (const QString &devHost : devHosts) { - hosts.insert(devHost); - } - - QList sortedHosts(hosts.cbegin(), hosts.cend()); - std::sort(sortedHosts.begin(), sortedHosts.end(), - [](const QString &a, const QString &b) { return a.localeAwareCompare(b) < 0; }); - - int hostRow = 1; - for (const QString &host : sortedHosts) { - const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; - - auto *hostLabel = new QLabel(host); - auto *spinBox = new QSpinBox; - if (unlocked) { - spinBox->setRange(0, UNLOCKED_HOST_LIMIT_MAX); // 0 means "unlimited" - spinBox->setValue(userLimits.value(host, 0)); - } else { - spinBox->setRange(DownloadSettings::MIN_HOST_REQUEST_LIMIT, devCap); - spinBox->setValue(userLimits.value(host, devCap)); - } - connect(spinBox, &QSpinBox::valueChanged, this, &DeckEditorSettingsPage::storeRequestLimits); - - requestLimitLayout->addWidget(hostLabel, hostRow, 0); - requestLimitLayout->addWidget(spinBox, hostRow, 1); - requestLimitSpinBoxes.insert(host, spinBox); - ++hostRow; - } - - requestLimitLayout->addWidget(&requestLimitHelpLabel, hostRow, 0, 1, 2); - mpRequestLimitGroupBox->setLayout(requestLimitLayout); - mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -156,7 +112,6 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() auto *lpMainLayout = new QVBoxLayout; lpMainLayout->addWidget(mpGeneralGroupBox); - lpMainLayout->addWidget(mpRequestLimitGroupBox); lpMainLayout->addWidget(mpSpoilerGroupBox); setLayout(lpMainLayout); @@ -217,22 +172,52 @@ void DeckEditorSettingsPage::storeSettings() SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); } -void DeckEditorSettingsPage::storeRequestLimits() +void DeckEditorSettingsPage::actAdjustRateLimit() { - QHash stored; - const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); - for (auto it = requestLimitSpinBoxes.cbegin(); it != requestLimitSpinBoxes.cend(); ++it) { - const QString host = it.key(); - const int value = it.value()->value(); - const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - // Only values that differ from the developer default are persisted; the worker - // treats a missing entry as "follow the developer default". - const int developerDefault = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? 0 : devCap; - if (value != developerDefault) { - stored.insert(host, value); - } + if (urlList->currentItem() == nullptr) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("Select a URL in the list first.")); + return; } - SettingsCache::instance().downloads().setHostRequestLimits(stored); + + const QString host = QUrl(urlList->currentItem()->text()).host(); + if (host.isEmpty()) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("The selected URL does not have a valid host.")); + return; + } + + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + const QHash currentLimits = SettingsCache::instance().downloads().getHostRequestLimits(); + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; + + bool ok = false; + int minimum; + int maximum; + int defaultValue; + if (unlocked) { + minimum = 0; // 0 means "unlimited" + maximum = UNLOCKED_HOST_LIMIT_MAX; + defaultValue = currentLimits.value(host, 0); + } else { + minimum = DownloadSettings::MIN_HOST_REQUEST_LIMIT; + maximum = devCap; + defaultValue = currentLimits.value(host, devCap); + } + + const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), + tr("Requests per second (developer maximum is %1):").arg(maximum), + defaultValue, minimum, maximum, 1, &ok); + if (!ok) { + return; + } + + QHash limits = currentLimits; + if (unlocked ? value == 0 : value == devCap) { + limits.remove(host); + } else { + limits.insert(host, value); + } + SettingsCache::instance().downloads().setHostRequestLimits(limits); } void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) @@ -301,9 +286,6 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput) void DeckEditorSettingsPage::retranslateUi() { mpGeneralGroupBox->setTitle(tr("URL Download Priority")); - mpRequestLimitGroupBox->setTitle(tr("Per-Host Request Limit")); - requestLimitHelpLabel.setText(tr("Pictures per second per host. Hosts can be lowered below their developer " - "limit but never raised above it; 0 means the host is not throttled per host.")); mpSpoilerGroupBox->setTitle(tr("Spoilers")); mcDownloadSpoilersCheckBox.setText(tr("Download Spoilers Automatically")); mcSpoilerSaveLabel.setText(tr("Spoiler Location:")); @@ -318,4 +300,5 @@ void DeckEditorSettingsPage::retranslateUi() aAdd->setText(tr("Add New URL")); aEdit->setText(tr("Edit URL")); aRemove->setText(tr("Remove URL")); -} \ No newline at end of file + aRateLimit->setText(tr("Adjust Rate Limit")); +} diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index b3745def4..57de5699e 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -5,11 +5,9 @@ #include #include -#include #include #include #include -#include class DeckEditorSettingsPage : public AbstractSettingsPage { @@ -21,7 +19,6 @@ public: private slots: void storeSettings(); - void storeRequestLimits(); void urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int); void setSpoilersEnabled(bool); void spoilerPathButtonClicked(); @@ -30,6 +27,7 @@ private slots: void actAddURL(); void actRemoveURL(); void actEditURL(); + void actAdjustRateLimit(); void resetDownloadedURLsButtonClicked(); private: @@ -37,16 +35,12 @@ private: QLabel urlLinkLabel; QCheckBox picDownloadCheckBox; QListWidget *urlList; - QAction *aAdd, *aEdit, *aRemove; + QAction *aAdd, *aEdit, *aRemove, *aRateLimit; QCheckBox mcDownloadSpoilersCheckBox; QLabel msDownloadSpoilersLabel; QGroupBox *mpGeneralGroupBox; QGroupBox *mpSpoilerGroupBox; - QGroupBox *mpRequestLimitGroupBox; - QLabel requestLimitHelpLabel; - QHash requestLimitSpinBoxes; - QLineEdit *mpSpoilerSavePathLineEdit; QLabel mcSpoilerSaveLabel; QLabel lastUpdatedLabel; diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index c510d8863..5293e390b 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -12,7 +12,7 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { // Developer-set ceilings for the per-host request allowance. Users may lower a host's // allowance via the download settings, but can never raise it above these values. Hosts // not listed default to DEFAULT_HOST_REQUEST_LIMIT. A cap of UNLIMITED_HOST_QUOTA marks a -// host that is never throttled per host (request pacing and 429 backoff still apply). +// host that is never throttled per host and skips the dispatch pacing (429 backoff still applies). const QHash DownloadSettings::DEVELOPER_HOST_CAPS = { // The Scryfall API enforces 10 requests/second; stay one under so a burst can't trip 429s. {"api.scryfall.com", 9}, diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index e075a3c07..9fcf9e61d 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -24,7 +24,7 @@ public: static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; /** @brief Floor for any per-host request allowance. */ static constexpr int MIN_HOST_REQUEST_LIMIT = 1; - /** @brief Developer cap marking a host as never throttled per host (pacing still applies). */ + /** @brief Developer cap marking a host as never throttled per host or by the dispatch pacing. */ static constexpr int UNLIMITED_HOST_QUOTA = -1; /** From 21ca53962d04827059b156356be8335d4aa5b7cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 04:35:12 +0200 Subject: [PATCH 10/13] [PictureLoader] Cap unlocked host bursts and adapt them to 429s --- .../card_picture_loader_worker.cpp | 102 +++++++++++++----- .../card_picture_loader_worker.h | 12 +++ 2 files changed, 86 insertions(+), 28 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 151996505..5ab097bc4 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -140,8 +140,16 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture QNetworkReply *reply = networkManager->get(req); + // Track in-flight replies per host so the unlocked fast path can bound how many requests it + // issues at once, instead of creating replies that time out before Qt opens a connection. + const QString host = url.host(); + hostInFlight.insert(host, hostInFlight.value(host) + 1); + // Connect reply handling - connect(reply, &QNetworkReply::finished, worker, [reply, worker] { worker->handleNetworkReply(reply); }); + connect(reply, &QNetworkReply::finished, worker, [this, reply, worker, host] { + hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1)); + worker->handleNetworkReply(reply); + }); return reply; } @@ -155,16 +163,20 @@ void CardPictureLoaderWorker::resetRequestQuota() QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end();) { - // An unlocked host has no per-host allowance; drop any stale entry instead of - // recovering it towards the UNLIMITED_HOST_QUOTA sentinel, which would poison it. - if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { - it = hostRequestQuota.erase(it); - continue; - } if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { - // Recover towards the host's effective allowance ceiling, which may be - // lowered by the user's per-host request limits. - it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); + if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // A developer-unlocked host that fell back after a 429 recovers towards the default + // allowance; once it gets there it becomes unlocked (fast-path) again. + if (it.value() + 1 >= DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT) { + it = hostRequestQuota.erase(it); + continue; + } + it.value() += 1; + } else { + // Recover towards the host's effective allowance ceiling, which may be + // lowered by the user's per-host request limits. + it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); + } } ++it; } @@ -201,20 +213,34 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() return; } - // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the dispatch pacing: dispatch every - // queued request for them back-to-back, bounded only by their 429 backoff window and Qt's - // per-host connection pool. QDateTime now = QDateTime::currentDateTime(); + bool dispatched = false; + // Set while an unlocked host still has queued work blocked only by the in-flight cap; the + // timer must keep running so it gets another try as soon as a slot frees. A host blocked by + // its 429 backoff instead waits for the next quota-reset tick to restart the dispatcher. + bool unlockedCapped = false; + + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the pacing and the per-host + // allowance: dispatch their queued requests back-to-back, bounded by their 429 backoff and the + // per-host in-flight cap so a large burst can't queue replies that time out before Qt opens a + // connection for them. for (int i = 0; i < requestLoadQueue.size();) { const auto &request = requestLoadQueue.at(i); const QString host = request.first.host(); - if (hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && - !CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { - makeRequest(request.first, request.second); - requestLoadQueue.removeAt(i); - } else { - ++i; + if (isUnlockedHost(host)) { + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + ++i; + continue; + } + if (hostInFlight.value(host) < MAX_IN_FLIGHT_PER_HOST) { + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); + dispatched = true; + continue; + } + unlockedCapped = true; } + ++i; } if (requestLoadQueue.isEmpty()) { @@ -223,8 +249,13 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() return; } - if (!processSingleRequest()) { - // No queued host currently has allowance left in this second; wait for the quota reset. + if (processSingleRequest()) { + dispatched = true; + } + + // Keep the timer running while there is progress to make or unlocked work waiting on a free + // in-flight slot; otherwise no host has allowance left this second, so wait for the quota reset. + if (!dispatched && !unlockedCapped) { dispatchTimer.stop(); } } @@ -243,7 +274,17 @@ bool CardPictureLoaderWorker::processSingleRequest() request.second->startNextPicDownload(); return true; } - const int ceiling = hostAllowanceCeiling(host); + // Unlocked hosts are handled by dispatchQueuedRequest's fast path, bounded by the in-flight + // cap; they must not fall through to the per-host allowance arithmetic below. + if (isUnlockedHost(host)) { + continue; + } + int ceiling = hostAllowanceCeiling(host); + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // A 429 dropped this unlocked host out of the fast path and installed a concrete + // allowance; pace it against that allowance until the recovery loop unlocks it again. + ceiling = hostRequestQuota.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + } // Seed the allowance lazily so a host that enters the queue mid-second gets its reduced // per-host allowance, clamped against the ceiling so a lowered user cap applies from this // second onward. @@ -271,15 +312,20 @@ int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const return SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); } +bool CardPictureLoaderWorker::isUnlockedHost(const QString &host) const +{ + return hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestQuota.contains(host); +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { const int ceiling = hostAllowanceCeiling(host); - if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { - // Unlocked hosts have no per-host allowance to halve; the shared backoff - // window tracked by the rate limiter still paces them. - return; - } - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, ceiling) / 2)); + // An unlocked host has no per-host allowance to halve. Install one instead so it drops out of + // the unlocked fast path and is paced like a throttled host; the recovery loop in + // resetRequestQuota() then walks it back up and unlocks it again. + const int base = + ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : ceiling; + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, base) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index d066d224a..1f4bebb53 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -127,6 +127,10 @@ private: QHash hostRequestLimits; ///< User-set per-host request allowances QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited + QHash hostInFlight; ///< Network replies currently in flight, per host + + /** @brief Maximum concurrent in-flight network replies per host. */ + static constexpr int MAX_IN_FLIGHT_PER_HOST = 6; CardPictureLoaderLocal *localLoader; ///< Loader for local images QSet currentlyLoading; ///< Deduplication: contains pixmapCacheKey currently being loaded @@ -139,6 +143,14 @@ private: */ [[nodiscard]] int hostAllowanceCeiling(const QString &host) const; + /** + * @brief Whether a host may skip dispatch pacing and per-host allowance entirely. + * + * A host is unlocked while it has no user limit and no reduced allowance installed by a 429. + * A 429 drops it out of the fast path until resetRequestQuota() walks the allowance back up. + */ + [[nodiscard]] bool isUnlockedHost(const QString &host) const; + /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; From b3b930587b7d0628b27c8bf942da3d4674da02b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 04:35:16 +0200 Subject: [PATCH 11/13] [PictureLoader] Store per-host limits readably and show them per URL --- .../deck_editor_settings_page.cpp | 108 +++++++++++++++--- .../settings_page/deck_editor_settings_page.h | 12 ++ .../settings/download_settings.cpp | 39 +++++-- 3 files changed, 137 insertions(+), 22 deletions(-) diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index 6223cd480..8628aa5a8 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -17,8 +18,6 @@ #include #include -static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; ///< Upper bound for rate limits on hosts unlocked by the developer - DeckEditorSettingsPage::DeckEditorSettingsPage() { picDownloadCheckBox.setChecked(SettingsCache::instance().downloads().getPicDownload()); @@ -54,7 +53,9 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() urlList->setDragDropMode(QAbstractItemView::InternalMove); connect(urlList->model(), &QAbstractItemModel::rowsMoved, this, &DeckEditorSettingsPage::urlListChanged); - urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); + for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { + addUrlItem(url); + } aAdd = new QAction(this); aAdd->setIcon(themePixmap(QStringLiteral("icons/increment"))); @@ -125,7 +126,9 @@ void DeckEditorSettingsPage::resetDownloadedURLsButtonClicked() { SettingsCache::instance().downloads().resetToDefaultURLs(); urlList->clear(); - urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); + for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { + addUrlItem(url); + } QMessageBox::information(this, tr("Success"), tr("Download URLs have been reset.")); } @@ -134,7 +137,7 @@ void DeckEditorSettingsPage::actAddURL() bool ok; QString msg = QInputDialog::getText(this, tr("Add URL"), tr("URL:"), QLineEdit::Normal, QString(), &ok); if (ok) { - urlList->addItem(msg); + addUrlItem(msg); storeSettings(); } } @@ -149,12 +152,14 @@ void DeckEditorSettingsPage::actRemoveURL() void DeckEditorSettingsPage::actEditURL() { - if (urlList->currentItem()) { - QString oldText = urlList->currentItem()->text(); + QListWidgetItem *item = urlList->currentItem(); + if (item) { + const QString oldText = urlForItem(item); bool ok; QString msg = QInputDialog::getText(this, tr("Edit URL"), tr("URL:"), QLineEdit::Normal, oldText, &ok); if (ok) { - urlList->currentItem()->setText(msg); + item->setData(Qt::UserRole, msg); + item->setText(urlLabel(msg)); storeSettings(); } } @@ -166,10 +171,77 @@ void DeckEditorSettingsPage::storeSettings() QStringList downloadUrls; for (int i = 0; i < urlList->count(); i++) { - qInfo() << "Priority" << i << ":" << urlList->item(i)->text(); - downloadUrls << urlList->item(i)->text(); + const QString url = urlForItem(urlList->item(i)); + qInfo() << "Priority" << i << ":" << url; + downloadUrls << url; } SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); + + // Drop per-host limits whose host is no longer referenced by any configured URL, so removing + // a URL doesn't leave a stale throttle behind that reactivates if the host is re-added. + QSet usedHosts; + for (const QString &url : downloadUrls) { + const QString host = QUrl(url).host(); + if (!host.isEmpty()) { + usedHosts.insert(host); + } + } + QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); + bool limitsChanged = false; + for (auto it = limits.begin(); it != limits.end();) { + if (!usedHosts.contains(it.key())) { + it = limits.erase(it); + limitsChanged = true; + } else { + ++it; + } + } + if (limitsChanged) { + SettingsCache::instance().downloads().setHostRequestLimits(limits); + } + + refreshUrlItems(); +} + +QListWidgetItem *DeckEditorSettingsPage::addUrlItem(const QString &url) +{ + auto *item = new QListWidgetItem(urlLabel(url)); + item->setData(Qt::UserRole, url); + urlList->addItem(item); + return item; +} + +QString DeckEditorSettingsPage::urlForItem(const QListWidgetItem *item) const +{ + return item->data(Qt::UserRole).toString(); +} + +QString DeckEditorSettingsPage::urlLabel(const QString &url) const +{ + const QString host = QUrl(url).host(); + if (host.isEmpty()) { + return url; + } + + const QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); + const int devCap = + DownloadSettings::getDeveloperHostCaps().value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !limits.contains(host)) { + return tr("%1 (unlimited)").arg(url); + } + + const int requested = limits.value( + host, devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : devCap); + const int effective = SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); + return tr("%1 (%2/s)").arg(url).arg(effective); +} + +void DeckEditorSettingsPage::refreshUrlItems() +{ + for (int i = 0; i < urlList->count(); ++i) { + QListWidgetItem *item = urlList->item(i); + item->setText(urlLabel(urlForItem(item))); + } } void DeckEditorSettingsPage::actAdjustRateLimit() @@ -179,7 +251,7 @@ void DeckEditorSettingsPage::actAdjustRateLimit() return; } - const QString host = QUrl(urlList->currentItem()->text()).host(); + const QString host = QUrl(urlForItem(urlList->currentItem())).host(); if (host.isEmpty()) { QMessageBox::information(this, tr("Adjust Rate Limit"), tr("The selected URL does not have a valid host.")); return; @@ -194,19 +266,21 @@ void DeckEditorSettingsPage::actAdjustRateLimit() int minimum; int maximum; int defaultValue; + QString prompt; if (unlocked) { minimum = 0; // 0 means "unlimited" - maximum = UNLOCKED_HOST_LIMIT_MAX; + maximum = DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT; defaultValue = currentLimits.value(host, 0); + prompt = tr("Requests per second (0 = unlimited, fastest; up to %1):").arg(maximum); } else { minimum = DownloadSettings::MIN_HOST_REQUEST_LIMIT; maximum = devCap; defaultValue = currentLimits.value(host, devCap); + prompt = tr("Requests per second (developer maximum is %1):").arg(maximum); } - const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), - tr("Requests per second (developer maximum is %1):").arg(maximum), - defaultValue, minimum, maximum, 1, &ok); + const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), prompt, defaultValue, + minimum, maximum, 1, &ok); if (!ok) { return; } @@ -218,6 +292,7 @@ void DeckEditorSettingsPage::actAdjustRateLimit() limits.insert(host, value); } SettingsCache::instance().downloads().setHostRequestLimits(limits); + refreshUrlItems(); } void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) @@ -301,4 +376,7 @@ void DeckEditorSettingsPage::retranslateUi() aEdit->setText(tr("Edit URL")); aRemove->setText(tr("Remove URL")); aRateLimit->setText(tr("Adjust Rate Limit")); + + // The per-URL rate limit suffixes are translated, so refresh them when the language changes. + refreshUrlItems(); } diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index 57de5699e..23e33fa20 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -47,6 +47,18 @@ private: QLabel infoOnSpoilersLabel; QPushButton *mpSpoilerPathButton; QPushButton *updateNowButton; + + /** @brief Adds a list item for the given URL, storing the raw URL alongside its displayed label. */ + QListWidgetItem *addUrlItem(const QString &url); + + /** @brief Returns the raw URL stored on a list item. */ + [[nodiscard]] QString urlForItem(const QListWidgetItem *item) const; + + /** @brief Returns the display label for a URL, including its current effective rate limit. */ + [[nodiscard]] QString urlLabel(const QString &url) const; + + /** @brief Refreshes the displayed label of every URL item after limits or settings change. */ + void refreshUrlItems(); }; #endif // COCKATRICE_DECK_EDITOR_SETTINGS_PAGE_H diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index 5293e390b..a321f0bd0 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -69,21 +69,46 @@ void DownloadSettings::setDownloadSpoilerStatus(bool _spoilerStatus) QHash DownloadSettings::getHostRequestLimits() const { - const QVariantMap stored = getValue("hostRequestLimits").toMap(); + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { + settings.beginGroup(defaultGroup); + } + settings.beginGroup("hostRequestLimits"); + QHash hostRequestLimits; - for (auto it = stored.cbegin(); it != stored.cend(); ++it) { - hostRequestLimits.insert(it.key(), it.value().toInt()); + const QStringList hosts = settings.childKeys(); + for (const QString &host : hosts) { + hostRequestLimits.insert(host, settings.value(host).toInt()); + } + + settings.endGroup(); + if (!defaultGroup.isEmpty()) { + settings.endGroup(); } return hostRequestLimits; } void DownloadSettings::setHostRequestLimits(const QHash &hostRequestLimits) { - QVariantMap stored; - for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { - stored.insert(it.key(), it.value()); + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { + settings.beginGroup(defaultGroup); } - setValue(stored, "hostRequestLimits"); + + // Drop the legacy single-key form (an opaque @Variant blob) written by earlier builds so each + // host is stored as a plain, hand-editable key in its own subgroup. + settings.remove("hostRequestLimits"); + settings.beginGroup("hostRequestLimits"); + settings.remove(QString()); + for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { + settings.setValue(it.key(), it.value()); + } + settings.endGroup(); + + if (!defaultGroup.isEmpty()) { + settings.endGroup(); + } + settings.sync(); emit hostRequestLimitsChanged(); } From ad75153e0f4a219710e39b0918de2ca8cc3a0e65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 22:50:19 +0200 Subject: [PATCH 12/13] [PictureLoader] Fix worker thread shutdown and cross-thread cache clearing clearNetworkCache() ran directly on the UI thread while the worker thread owned the disk cache and redirect cache, racing cache reads/writes. Make it a worker-thread slot invoked via a blocking queued call when the thread is running, so the 'Cached card pictures have been reset.' message is truthful. The worker thread was also never quit()/wait()ed: both destructors only deleteLater'd their objects, so Qt warned 'QThread: Destroyed while thread is still running' and leaked a running loop at exit. Wire the worker's finished() signal to its own deleteLater() (canonical worker-object pattern), add shutdownThread() to stop the loop, and let CardPictureLoader destroy the QThread only after wait() has returned. --- .../card_picture_loader.cpp | 20 ++++++++++-- .../card_picture_loader_worker.cpp | 26 +++++++++++++++- .../card_picture_loader_worker.h | 31 +++++++++++++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index 8c81d641d..e7e31f5a2 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -55,7 +56,14 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) CardPictureLoader::~CardPictureLoader() { - worker->deleteLater(); + if (worker) { + // Capture the thread first: shutdownThread() blocks until the worker has been freed by the + // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. + QThread *pictureLoaderThread = worker->workerThread(); + worker->shutdownThread(); + worker = nullptr; + delete pictureLoaderThread; + } } void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size) @@ -295,7 +303,15 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - getInstance().worker->clearNetworkCache(); + auto &worker = *getInstance().worker; + // The disk cache and redirect cache are owned by the worker thread; clearing them from the + // UI thread would race with the worker's cache reads/writes. Block until the worker thread + // has executed the clear so the "Cached card pictures have been reset." message is truthful. + if (worker.isRunning()) { + QMetaObject::invokeMethod(&worker, "clearNetworkCache", Qt::BlockingQueuedConnection); + } else { + worker.clearNetworkCache(); + } } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 5ab097bc4..a5965017e 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -59,6 +59,9 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() localLoader = new CardPictureLoaderLocal(this); pictureLoaderThread = new QThread; + // The worker object frees itself once its thread finishes, so no event loop is left + // running and the QThread is never destroyed while still executing. + connect(pictureLoaderThread, &QThread::finished, this, &QObject::deleteLater); pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); @@ -82,7 +85,28 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() CardPictureLoaderWorker::~CardPictureLoaderWorker() { saveRedirectCache(); - pictureLoaderThread->deleteLater(); +} + +void CardPictureLoaderWorker::shutdownThread() +{ + // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as + // its event loop exits, so nothing - not even a member read - may run once wait() returns. + // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. + QThread *thread = pictureLoaderThread; + if (thread) { + thread->quit(); + thread->wait(); + } +} + +QThread *CardPictureLoaderWorker::workerThread() const +{ + return pictureLoaderThread; +} + +bool CardPictureLoaderWorker::isRunning() const +{ + return pictureLoaderThread != nullptr && pictureLoaderThread->isRunning(); } void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 1f4bebb53..d2a444ac9 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -74,10 +74,37 @@ public: */ void onHostRateLimited(const QString &host); - /** @brief Clears the network cache and redirect cache. */ - void clearNetworkCache(); + /** + * @brief Stops the worker thread and releases it. + * + * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an + * exit request to the worker's event loop and QThread::wait() blocks until the loop has + * returned and the thread finished. Only QThread members are touched here, so this method is + * safe to call from the owning thread. The worker object itself is freed by the finished() -> + * deleteLater chain (see the constructor); the QThread object is deleted afterwards by the + * owner (CardPictureLoader::~CardPictureLoader), not by this method. + */ + void shutdownThread(); + + /** @return Whether the worker's thread is currently running. */ + bool isRunning() const; + + /** + * @brief Returns the worker's QThread. + * @return The worker thread + * + * Only meaningful while the worker object is alive; capture it before calling shutdownThread(). + */ + QThread *workerThread() const; public slots: + /** + * @brief Clears the network cache and redirect cache. + * + * Runs on the worker thread; invoke it via a queued call when coming from another thread, + * since both caches are owned by the worker thread. + */ + void clearNetworkCache(); /** * @brief Makes a network request for the given URL using the specified worker. * @param url URL to load From e960471c6967b1a660aae16b760cf56ee5657871 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 04:42:31 +0200 Subject: [PATCH 13/13] [PictureLoader] Guard cache teardown and stop blocking the UI thread --- .../card_picture_loader.cpp | 27 ++++++++++++------- .../card_picture_loader/card_picture_loader.h | 7 +++++ .../card_picture_loader_worker.cpp | 25 ++++++++++------- .../card_picture_loader_worker.h | 21 ++++++++------- .../card_picture_loader_worker_work.cpp | 4 +-- .../card_picture_loader_worker_work.h | 5 ++-- .../settings_page/storage_settings_page.cpp | 9 +++++-- 7 files changed, 64 insertions(+), 34 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index e7e31f5a2..0d464a3f7 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -41,6 +41,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) qRegisterMetaType(); connect(worker, &CardPictureLoaderWorker::imageLoaded, this, &CardPictureLoader::imageLoaded); + connect(worker, &CardPictureLoaderWorker::networkCacheCleared, this, &CardPictureLoader::networkCacheCleared); statusBar = new CardPictureLoaderStatusBar(nullptr); QMainWindow *mainWindow = qobject_cast(QApplication::activeWindow()); @@ -60,9 +61,13 @@ CardPictureLoader::~CardPictureLoader() // Capture the thread first: shutdownThread() blocks until the worker has been freed by the // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. QThread *pictureLoaderThread = worker->workerThread(); - worker->shutdownThread(); + const bool stopped = worker->shutdownThread(); worker = nullptr; - delete pictureLoaderThread; + // Deleting a QThread that is still running is undefined behaviour, so only free it once the + // bounded wait in shutdownThread() confirmed that it stopped. + if (stopped) { + delete pictureLoaderThread; + } } } @@ -303,15 +308,17 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - auto &worker = *getInstance().worker; - // The disk cache and redirect cache are owned by the worker thread; clearing them from the - // UI thread would race with the worker's cache reads/writes. Block until the worker thread - // has executed the clear so the "Cached card pictures have been reset." message is truthful. - if (worker.isRunning()) { - QMetaObject::invokeMethod(&worker, "clearNetworkCache", Qt::BlockingQueuedConnection); - } else { - worker.clearNetworkCache(); + // During teardown the worker is released before this singleton, so a queued clear may still + // arrive with no worker left to run it. + CardPictureLoaderWorker *worker = getInstance().worker; + if (!worker) { + return; } + // The disk cache and redirect cache are owned by the worker thread, so the clear has to run + // there. Invoke it asynchronously to keep the GUI responsive while the worker may be walking + // the user's picture directories or recursively deleting the cache directory; callers that + // need to know when it is done can listen for networkCacheCleared(). + QMetaObject::invokeMethod(worker, &CardPictureLoaderWorker::clearNetworkCache, Qt::QueuedConnection); } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h index 5c3ac84a3..c53fdfcc4 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h @@ -110,6 +110,9 @@ public: public slots: /** * @brief Clears the network disk cache of the worker. + * + * The clear runs on the worker thread, so this returns before it has completed; connect to + * networkCacheCleared() to act once it is done. */ static void clearNetworkCache(); @@ -122,6 +125,10 @@ public slots: void imageLoaded(const ExactCard &card, const QImage &image); void saveCardImageToLocalStorage(const ExactCard &card, const QPixmap &pixmap); +signals: + /** @brief Emitted after the worker has finished clearing the network and redirect caches. */ + void networkCacheCleared(); + private slots: /** * @brief Triggered when the user changes the picture download settings. diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index a5965017e..13407aab9 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -21,6 +21,7 @@ static constexpr int MIN_HOST_QUOTA = DownloadSettings::MIN_HOST_REQUEST_LIMIT; static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between individual network requests static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets +static constexpr int THREAD_SHUTDOWN_WAIT_MS = 5000; ///< Bounded wait for the worker thread to stop at exit CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), @@ -87,16 +88,26 @@ CardPictureLoaderWorker::~CardPictureLoaderWorker() saveRedirectCache(); } -void CardPictureLoaderWorker::shutdownThread() +bool CardPictureLoaderWorker::shutdownThread() { // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as // its event loop exits, so nothing - not even a member read - may run once wait() returns. // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. QThread *thread = pictureLoaderThread; - if (thread) { - thread->quit(); - thread->wait(); + if (!thread) { + return true; } + thread->quit(); + // Only an unbounded wait() would guarantee the thread stops, but this runs from a function-local + // static destructor after main() has returned, with no UI left to interrupt a worker stuck in a + // slow slot or on a stalled filesystem. Bound the wait and leave such a thread to the OS rather + // than hanging the process forever. + if (!thread->wait(THREAD_SHUTDOWN_WAIT_MS)) { + qCWarning(CardPictureLoaderWorkerLog) << "Picture loader worker thread did not stop within" + << THREAD_SHUTDOWN_WAIT_MS << "ms; leaving it to be torn down by the OS"; + return false; + } + return true; } QThread *CardPictureLoaderWorker::workerThread() const @@ -104,11 +115,6 @@ QThread *CardPictureLoaderWorker::workerThread() const return pictureLoaderThread; } -bool CardPictureLoaderWorker::isRunning() const -{ - return pictureLoaderThread != nullptr && pictureLoaderThread->isRunning(); -} - void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) { QUrl cachedRedirect = getCachedRedirect(url); @@ -459,4 +465,5 @@ void CardPictureLoaderWorker::clearNetworkCache() { networkManager->cache()->clear(); redirectCache.clear(); + emit networkCacheCleared(); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index d2a444ac9..8ae4ffc3b 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -75,19 +75,19 @@ public: void onHostRateLimited(const QString &host); /** - * @brief Stops the worker thread and releases it. + * @brief Stops the worker thread and reports whether it stopped. * * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an - * exit request to the worker's event loop and QThread::wait() blocks until the loop has - * returned and the thread finished. Only QThread members are touched here, so this method is - * safe to call from the owning thread. The worker object itself is freed by the finished() -> - * deleteLater chain (see the constructor); the QThread object is deleted afterwards by the + * exit request to the worker's event loop and QThread::wait() blocks (bounded) until the loop + * has returned and the thread finished. Only QThread members are touched here, so this method + * is safe to call from the owning thread. The worker object itself is freed by the finished() + * -> deleteLater chain (see the constructor); the QThread object is deleted afterwards by the * owner (CardPictureLoader::~CardPictureLoader), not by this method. + * + * @return true if the thread stopped within the timeout, false if it is still running (in + * which case the owner must not delete the QThread). */ - void shutdownThread(); - - /** @return Whether the worker's thread is currently running. */ - bool isRunning() const; + [[nodiscard]] bool shutdownThread(); /** * @brief Returns the worker's QThread. @@ -209,6 +209,9 @@ signals: /** @brief Emitted when a network request successfully completes. */ void imageRequestSucceeded(const QUrl &url); + + /** @brief Emitted after clearNetworkCache() has finished clearing both caches. */ + void networkCacheCleared(); }; #endif // PICTURE_LOADER_WORKER_H diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index e4a36ab8c..6cedc9e8b 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp @@ -27,8 +27,8 @@ const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() return s_rateLimiter; } -CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) - : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), +CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad) + : QObject(worker), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) { // Hook up signals to the orchestrator diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index f05d727de..da5049ca7 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -36,10 +36,11 @@ class CardPictureLoaderWorkerWork : public QObject public: /** * @brief Constructs a worker for downloading a specific card image. - * @param worker The orchestrating CardPictureLoaderWorker + * @param worker The orchestrating CardPictureLoaderWorker; the work object becomes its child so + * it is destroyed with the worker even if it never reaches concludeImageLoad(). * @param toLoad The ExactCard to download */ - explicit CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad); + explicit CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad); CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading diff --git a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp index 17838e501..4aad91115 100644 --- a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp @@ -181,9 +181,14 @@ StorageSettingsPage::StorageSettingsPage() void StorageSettingsPage::clearDownloadedPicsButtonClicked() { - CardPictureLoader::clearNetworkCache(); + // The network cache is cleared asynchronously on the worker thread, so wait for the completion + // signal before confirming; the in-memory pixmap cache is cleared synchronously right away. + connect( + &CardPictureLoader::getInstance(), &CardPictureLoader::networkCacheCleared, this, + [this] { QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); }, + Qt::SingleShotConnection); CardPictureLoader::clearPixmapCache(); - QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); + CardPictureLoader::clearNetworkCache(); } void StorageSettingsPage::clearImageBackupsButtonClicked()