From 61035f8ae0f13bbaff6504c79bbbc835be033789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:50:15 +0200 Subject: [PATCH 01/11] [PictureLoader] Serve cached pictures from the disk cache instead of re-fetching them With picture downloads enabled, requests were issued with AlwaysNetwork cache control, which per Qt never consults the disk cache. A picture that had already been downloaded was therefore fetched from the network again on every session start, with the queue bypass letting those re-fetches skip the rate limit entirely. Treat the network cache as the intent of the 'Network Cache' storage method suggests: if the URL is already cached, serve it with AlwaysCache (no network, no quota); only a genuine miss goes to the network, and only when downloads are enabled. Cache hits skip the queue for free since they never consume the per-second request allowance. --- .../card_picture_loader_worker.cpp | 15 +++++++++------ 1 file changed, 9 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 d288236d2..34092f361 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 @@ -84,8 +84,8 @@ void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWor SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && cache->metaData(url).isValid()) { - // If we hit a cached url, we get to make the request for free, since it won't contribute towards the - // rate-limit + // A request that will be served from the disk cache never touches the network and therefore + // doesn't use up any of the rate limit, so it gets to skip the queue. makeRequest(url, worker); return; } @@ -107,10 +107,13 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture req.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING)); req.setRawHeader("Accept", "image/avif,image/webp,image/apng,image/,/*;q=0.8"); - bool useNetworkCache = - !picDownload && static_cast( - SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == - CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE; + // Cached entries are served straight from the disk cache even when picture downloads are + // enabled: re-fetching an already-cached image would burn the rate limit for nothing. Only a + // genuine cache miss goes to the network, and only when downloads are enabled. + bool useNetworkCache = static_cast( + SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == + CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && + (cache->metaData(url).isValid() || !picDownload); req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork); From 64b3b7e0b470911f4450b7e8baa2b87be5a0f8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:51:07 +0200 Subject: [PATCH 02/11] [PictureLoader] Pace requests and run the throttle timers on the worker thread Previously the whole backed-up queue was drained in a burst as soon as a request was enqueued, sending up to 10 requests back-to-back and then immediately re-filling the quota one second later. That hard-bursts a rate-limited API like Scryfall's (10 requests/second) into a 30 second lockout. Introduce a pacing timer that dispatches a single queue entry every 100 ms, so the per-second allowance is used smoothly instead of in spikes, and keep the quota timer at 1 second. Also fix both timers' thread affinity: they are QTimer value members and so are not QObject children, meaning moveToThread() on the worker left them on the main thread while the slot code started them from the picture thread, which was a no-op that also warned. They are moved to the worker thread explicitly and started lazily from there. --- .../card_picture_loader_worker.cpp | 40 ++++++++++++++++--- .../card_picture_loader_worker.h | 4 ++ 2 files changed, 39 insertions(+), 5 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 34092f361..4a2caaab4 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 @@ -17,8 +17,10 @@ #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 qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering +static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance +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()), @@ -60,11 +62,18 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); + // QTimer value members are not QObject children, so moveToThread on the worker doesn't move + // them. They must live in the worker's thread to be started from the slot code that runs there. + requestTimer.moveToThread(pictureLoaderThread); + dispatchTimer.moveToThread(pictureLoaderThread); + connect(this, &CardPictureLoaderWorker::imageLoadEnqueued, this, &CardPictureLoaderWorker::handleImageLoadEnqueued); connect(&requestTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::resetRequestQuota); - requestTimer.setInterval(1000); - requestTimer.start(); + requestTimer.setInterval(static_cast(QUOTA_RESET_INTERVAL_MS)); + + connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest); + dispatchTimer.setInterval(DISPATCH_INTERVAL_MS); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -147,8 +156,29 @@ void CardPictureLoaderWorker::resetRequestQuota() void CardPictureLoaderWorker::processQueuedRequests() { - while (requestQuota > 0 && processSingleRequest()) { + if (requestLoadQueue.isEmpty()) { + dispatchTimer.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(); +} + +void CardPictureLoaderWorker::dispatchQueuedRequest() +{ + if (requestLoadQueue.isEmpty() || requestQuota <= 0) { + dispatchTimer.stop(); + return; + } + + if (processSingleRequest()) { --requestQuota; + } else { + // 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 d1c519b7a..9f7fd9437 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 @@ -89,6 +89,9 @@ public slots: /** @brief Processes all queued requests respecting the request quota. */ void processQueuedRequests(); + /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ + void dispatchQueuedRequest(); + /** * @brief Processes a single queued request. * @return true if a request was processed, false if queue is empty. @@ -120,6 +123,7 @@ private: 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 QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited From 5a6db206cebd00447dc9c5b8b60a19e4b1d75b68 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 03/11] [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 41b49ed4bce98ac2f0ba12c627e9a944a7541b93 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 04/11] [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 5d025ca0bde16f8ab88ffffa3c37e20d0e200b8f Mon Sep 17 00:00:00 2001 From: tooomm Date: Sat, 12 Sep 2026 17:30:46 +0200 Subject: [PATCH 05/11] Use capitalized app names (#7255) * use capitalized app name * update urls * app description * Update main.cpp --- cockatrice/src/client/sound_engine.cpp | 4 ++-- cockatrice/src/interface/deck_loader/deck_file_format.h | 2 +- cockatrice/src/interface/deck_loader/deck_loader.cpp | 2 +- cockatrice/src/interface/deck_loader/deck_loader.h | 2 +- cockatrice/src/interface/theme_manager.cpp | 2 +- cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp | 4 ++-- cockatrice/src/interface/window_main.cpp | 6 +++--- cockatrice/src/main.cpp | 8 ++++---- oracle/src/main.cpp | 6 +++--- oracle/src/pages.cpp | 2 +- oracle/src/parsehelpers.cpp | 2 +- oracle/src/raw_json_scanner.h | 2 +- servatrice/src/servatrice_database_interface.cpp | 7 ++++--- 13 files changed, 25 insertions(+), 24 deletions(-) diff --git a/cockatrice/src/client/sound_engine.cpp b/cockatrice/src/client/sound_engine.cpp index 18de2264d..96cafa3d3 100644 --- a/cockatrice/src/client/sound_engine.cpp +++ b/cockatrice/src/client/sound_engine.cpp @@ -94,7 +94,7 @@ QStringMap &SoundEngine::getAvailableThemes() QDir dir; availableThemes.clear(); - // load themes from user profile dir + // Load themes from user profile dir dir.setPath(SettingsCache::instance().getDataPath() + "/sounds"); @@ -104,7 +104,7 @@ QStringMap &SoundEngine::getAvailableThemes() } } - // load themes from cockatrice system dir + // Load themes from Cockatrice system dir dir.setPath(qApp->applicationDirPath() + #ifdef Q_OS_MAC "/../Resources/sounds" diff --git a/cockatrice/src/interface/deck_loader/deck_file_format.h b/cockatrice/src/interface/deck_loader/deck_file_format.h index 995de32c0..3a25797ec 100644 --- a/cockatrice/src/interface/deck_loader/deck_file_format.h +++ b/cockatrice/src/interface/deck_loader/deck_file_format.h @@ -17,7 +17,7 @@ enum Format PlainText, /** - * This is cockatrice's native deck file format, and supports deck metadata such as banner cards and tags. + * This is Cockatrice's native deck file format, and supports deck metadata such as banner cards and tags. * Stored as .cod files. */ Cockatrice diff --git a/cockatrice/src/interface/deck_loader/deck_loader.cpp b/cockatrice/src/interface/deck_loader/deck_loader.cpp index f03339da8..f29b4eed2 100644 --- a/cockatrice/src/interface/deck_loader/deck_loader.cpp +++ b/cockatrice/src/interface/deck_loader/deck_loader.cpp @@ -50,7 +50,7 @@ DeckLoader::loadFromFile(const QString &fileName, DeckFileFormat::Format fmt, bo result = deckList.loadFromFile_Native(&file); if (!result) { qCInfo(DeckLoaderLog) << "Failed to load " << fileName - << "as cockatrice format; retrying as plain format"; + << "as Cockatrice format; retrying as plain format"; file.seek(0); result = deckList.loadFromFile_Plain(&file, CardNameNormalizer()); fmt = DeckFileFormat::PlainText; diff --git a/cockatrice/src/interface/deck_loader/deck_loader.h b/cockatrice/src/interface/deck_loader/deck_loader.h index b851c6895..be0df311d 100644 --- a/cockatrice/src/interface/deck_loader/deck_loader.h +++ b/cockatrice/src/interface/deck_loader/deck_loader.h @@ -131,7 +131,7 @@ public: static void printDeckList(QPrinter *printer, const DeckList &deckList); /** - * Converts the given deck's file to the cockatrice file format. + * Converts the given deck's file to the Cockatrice file format. * Uses the lastLoadInfo in the LoadedDeck to determine the current name of the file and where to save to. * @param deck The deck to convert. Should have valid lastLoadInfo. Will update the lastLoadInfo. * @return Whether the conversion succeeded. diff --git a/cockatrice/src/interface/theme_manager.cpp b/cockatrice/src/interface/theme_manager.cpp index cc6be175a..12c8fad2c 100644 --- a/cockatrice/src/interface/theme_manager.cpp +++ b/cockatrice/src/interface/theme_manager.cpp @@ -224,7 +224,7 @@ QStringMap &ThemeManager::getAvailableThemes() } } - // load themes from cockatrice system dir + // Load themes from Cockatrice system dir dir.setPath(systemThemesBasePath()); for (QString themeName : dir.entryList(QDir::AllDirs | QDir::NoDotAndDotDot, QDir::Name)) { diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp index de7dd3e97..fb559fc4b 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_settings.cpp @@ -478,13 +478,13 @@ void DlgSettings::closeEvent(QCloseEvent *event) case Invalid: loadErrorMessage = tr("Your card database is invalid.\n\n" "Cockatrice may not function correctly with an invalid database\n\n" - "You may need to rerun oracle to update your card database.\n\n" + "You may need to rerun Oracle to update your card database.\n\n" "Would you like to change your database location setting?"); break; case VersionTooOld: loadErrorMessage = tr("Your card database version is too old.\n\n" "This can cause problems loading card information or images\n\n" - "Usually this can be fixed by rerunning oracle to to update your card database.\n\n" + "Usually this can be fixed by rerunning Oracle to to update your card database.\n\n" "Would you like to change your database location setting?"); break; case NotLoaded: diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index ce28fc24a..3daaeb8d3 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -92,8 +92,8 @@ #include #define GITHUB_PAGES_URL "https://cockatrice.github.io" -#define GITHUB_CONTRIBUTORS_URL "https://github.com/Cockatrice/Cockatrice/graphs/contributors?type=c" -#define GITHUB_CONTRIBUTE_URL "https://github.com/Cockatrice/Cockatrice#cockatrice" +#define GITHUB_CONTRIBUTORS_URL "https://github.com/Cockatrice/Cockatrice/graphs/contributors" +#define GITHUB_CONTRIBUTE_URL "https://github.com/Cockatrice/Cockatrice#" #define GITHUB_TRANSIFEX_TRANSLATORS_URL "https://github.com/Cockatrice/Cockatrice/wiki/Translator-Hall-of-Fame" #define GITHUB_TRANSLATOR_FAQ_URL "https://github.com/Cockatrice/Cockatrice/wiki/Translation-FAQ" #define GITHUB_ISSUES_URL "https://github.com/Cockatrice/Cockatrice/issues" @@ -1050,7 +1050,7 @@ void MainWindow::createCardUpdateProcess(bool background) if (dir.exists(binaryName)) { updaterCmd = dir.absoluteFilePath(binaryName); - } else { // try and find the directory oracle is stored in the build directory + } else { // try and find the directory Oracle is stored in the build directory QDir findLocalDir(dir); findLocalDir.cdUp(); findLocalDir.cd(getCardUpdaterBinaryName()); diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index ac77b4241..829e08742 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -231,7 +231,7 @@ int main(int argc, char *argv[]) // These values are only used by the settings loader/saver // Wrong or outdated values are kept to not break things QCoreApplication::setOrganizationName("Cockatrice"); - QCoreApplication::setOrganizationDomain("cockatrice.de"); + QCoreApplication::setOrganizationDomain("cockatrice.github.io"); QCoreApplication::setApplicationName("Cockatrice"); QCoreApplication::setApplicationVersion(VERSION_STRING); @@ -250,7 +250,7 @@ int main(int argc, char *argv[]) // Command-line parser QCommandLineParser parser; - parser.setApplicationDescription("Cockatrice"); + parser.setApplicationDescription("Cockatrice Client"); parser.addHelpOption(); parser.addVersionOption(); @@ -350,8 +350,8 @@ int main(int argc, char *argv[]) qCInfo(MainLog) << "MainWindow constructor finished"; ui.setWindowIcon(themePixmap(QStringLiteral("cockatrice"))); - // set name of the app desktop file; used by wayland to load the window icon - QGuiApplication::setDesktopFileName("cockatrice"); + // Set name of the app desktop file; used by wayland to load the window icon + QGuiApplication::setDesktopFileName("Cockatrice"); SettingsCache::instance().network().setClientID(generateClientID()); diff --git a/oracle/src/main.cpp b/oracle/src/main.cpp index bb88153b1..0f962f9be 100644 --- a/oracle/src/main.cpp +++ b/oracle/src/main.cpp @@ -50,8 +50,8 @@ int main(int argc, char *argv[]) QApplication app(argc, argv); QCoreApplication::setOrganizationName("Cockatrice"); - QCoreApplication::setOrganizationDomain("cockatrice"); - // this can't be changed, as it influences the default save path for cards.xml + QCoreApplication::setOrganizationDomain("Cockatrice"); + // This can't be changed, as it influences the default save path for cards.xml QCoreApplication::setApplicationName("Cockatrice"); // If the program is opened with the -s flag, it will only do spoilers. Otherwise it will do MTGJSON/Tokens @@ -83,7 +83,7 @@ int main(int argc, char *argv[]) QIcon icon("theme:appicon.svg"); wizard.setWindowIcon(icon); // set name of the app desktop file; used by wayland to load the window icon - QGuiApplication::setDesktopFileName("oracle"); + QGuiApplication::setDesktopFileName("Oracle"); wizard.show(); diff --git a/oracle/src/pages.cpp b/oracle/src/pages.cpp index a10daadb3..df0c3f51d 100644 --- a/oracle/src/pages.cpp +++ b/oracle/src/pages.cpp @@ -796,7 +796,7 @@ void SaveSetsPage::retranslateUi() { setTitle(tr("Sets imported")); if (wizard()->downloadedPlainXml) { - setSubTitle(tr("A cockatrice database file of %1 MB has been downloaded.") + setSubTitle(tr("A Cockatrice card database file of %1 MB has been downloaded.") .arg(qRound(wizard()->xmlData.size() / 1000000.0))); } else { setSubTitle(tr("The following sets have been found:")); diff --git a/oracle/src/parsehelpers.cpp b/oracle/src/parsehelpers.cpp index a97bf67c9..ee7c4eb2e 100644 --- a/oracle/src/parsehelpers.cpp +++ b/oracle/src/parsehelpers.cpp @@ -20,7 +20,7 @@ * Note that "...enters tapped unless..." returns false. * * @param name The name of the card - * @param text The oracle text of the card + * @param text The Oracle text of the card */ bool parseCipt(const QString &name, const QString &text) { diff --git a/oracle/src/raw_json_scanner.h b/oracle/src/raw_json_scanner.h index 086f2b3e3..04fcc52e4 100644 --- a/oracle/src/raw_json_scanner.h +++ b/oracle/src/raw_json_scanner.h @@ -54,7 +54,7 @@ using ScanProgressCallback = std::function expectedversion) { - qCCritical(DatabaseInterfaceLog) << poolStr << "Error opening database: the database schema version" - << dbversion << "is too new, you need to update servatrice" - << "(this servatrice actually uses version" << expectedversion << ")"; + qCCritical(DatabaseInterfaceLog) + << poolStr << "Error opening database: the database schema version" << dbversion + << "is too new, you need to update Servatrice" << "(Currently running Servatrice actually uses version" + << expectedversion << ")"; return false; } } else { From 30f5142b595f90e6672cfedd1704a82f6c1726c9 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 06/11] [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 fd82b140a85a38145c1a1c59eaf510e329dcc503 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 13 Sep 2026 03:36:29 +0200 Subject: [PATCH 07/11] [PictureLoader] Serve cached pictures from the disk cache instead of re-fetching them (#7284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With picture downloads enabled, requests were issued with AlwaysNetwork cache control, which per Qt never consults the disk cache. A picture that had already been downloaded was therefore fetched from the network again on every session start, with the queue bypass letting those re-fetches skip the rate limit entirely. Treat the network cache as the intent of the 'Network Cache' storage method suggests: if the URL is already cached, serve it with AlwaysCache (no network, no quota); only a genuine miss goes to the network, and only when downloads are enabled. Cache hits skip the queue for free since they never consume the per-second request allowance. Co-authored-by: Lukas Brübach --- .../card_picture_loader_worker.cpp | 15 +++++++++------ 1 file changed, 9 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 d288236d2..34092f361 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 @@ -84,8 +84,8 @@ void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWor SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && cache->metaData(url).isValid()) { - // If we hit a cached url, we get to make the request for free, since it won't contribute towards the - // rate-limit + // A request that will be served from the disk cache never touches the network and therefore + // doesn't use up any of the rate limit, so it gets to skip the queue. makeRequest(url, worker); return; } @@ -107,10 +107,13 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture req.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING)); req.setRawHeader("Accept", "image/avif,image/webp,image/apng,image/,/*;q=0.8"); - bool useNetworkCache = - !picDownload && static_cast( - SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == - CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE; + // Cached entries are served straight from the disk cache even when picture downloads are + // enabled: re-fetching an already-cached image would burn the rate limit for nothing. Only a + // genuine cache miss goes to the network, and only when downloads are enabled. + bool useNetworkCache = static_cast( + SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == + CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && + (cache->metaData(url).isValid() || !picDownload); req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork); From 1c6ee62393d7495343c09a501ecdd86d19877369 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:51:07 +0200 Subject: [PATCH 08/11] [PictureLoader] Pace requests and run the throttle timers on the worker thread Previously the whole backed-up queue was drained in a burst as soon as a request was enqueued, sending up to 10 requests back-to-back and then immediately re-filling the quota one second later. That hard-bursts a rate-limited API like Scryfall's (10 requests/second) into a 30 second lockout. Introduce a pacing timer that dispatches a single queue entry every 100 ms, so the per-second allowance is used smoothly instead of in spikes, and keep the quota timer at 1 second. Also fix both timers' thread affinity: they are QTimer value members and so are not QObject children, meaning moveToThread() on the worker left them on the main thread while the slot code started them from the picture thread, which was a no-op that also warned. They are moved to the worker thread explicitly and started lazily from there. --- .../card_picture_loader_worker.cpp | 40 ++++++++++++++++--- .../card_picture_loader_worker.h | 4 ++ 2 files changed, 39 insertions(+), 5 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 34092f361..4a2caaab4 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 @@ -17,8 +17,10 @@ #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 qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering +static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance +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()), @@ -60,11 +62,18 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); + // QTimer value members are not QObject children, so moveToThread on the worker doesn't move + // them. They must live in the worker's thread to be started from the slot code that runs there. + requestTimer.moveToThread(pictureLoaderThread); + dispatchTimer.moveToThread(pictureLoaderThread); + connect(this, &CardPictureLoaderWorker::imageLoadEnqueued, this, &CardPictureLoaderWorker::handleImageLoadEnqueued); connect(&requestTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::resetRequestQuota); - requestTimer.setInterval(1000); - requestTimer.start(); + requestTimer.setInterval(static_cast(QUOTA_RESET_INTERVAL_MS)); + + connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest); + dispatchTimer.setInterval(DISPATCH_INTERVAL_MS); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -147,8 +156,29 @@ void CardPictureLoaderWorker::resetRequestQuota() void CardPictureLoaderWorker::processQueuedRequests() { - while (requestQuota > 0 && processSingleRequest()) { + if (requestLoadQueue.isEmpty()) { + dispatchTimer.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(); +} + +void CardPictureLoaderWorker::dispatchQueuedRequest() +{ + if (requestLoadQueue.isEmpty() || requestQuota <= 0) { + dispatchTimer.stop(); + return; + } + + if (processSingleRequest()) { --requestQuota; + } else { + // 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 d1c519b7a..9f7fd9437 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 @@ -89,6 +89,9 @@ public slots: /** @brief Processes all queued requests respecting the request quota. */ void processQueuedRequests(); + /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ + void dispatchQueuedRequest(); + /** * @brief Processes a single queued request. * @return true if a request was processed, false if queue is empty. @@ -120,6 +123,7 @@ private: 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 QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited 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 09/11] [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 10/11] [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 11/11] [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; /**