diff --git a/cockatrice/src/client/settings/cache_settings.h b/cockatrice/src/client/settings/cache_settings.h index 7a90d82f1..23cdb4dbf 100644 --- a/cockatrice/src/client/settings/cache_settings.h +++ b/cockatrice/src/client/settings/cache_settings.h @@ -58,33 +58,33 @@ signals: void themeChanged(); private: - QSettings *settings = nullptr; - ShortcutsSettings *shortcutsSettings = nullptr; - CardDatabaseSettings *cardDatabaseSettings = nullptr; - ServersSettings *serversSettings = nullptr; - MessageSettings *messageSettings = nullptr; - GameFiltersSettings *gameFiltersSettings = nullptr; - LayoutsSettings *layoutsSettings = nullptr; - DownloadSettings *downloadSettings = nullptr; - RecentsSettings *recentsSettings = nullptr; - CardOverrideSettings *cardOverrideSettings = nullptr; - DebugSettings *debugSettings = nullptr; - CardCounterSettings *cardCounterSettings = nullptr; - TabsSettings *tabsSettings = nullptr; - SoundSettings *soundSettings = nullptr; - GameSettings *gameSettings = nullptr; - ChatSettings *chatSettings = nullptr; - CacheStorageSettings *cacheStorageSettings = nullptr; - UpdatesSettings *updatesSettings = nullptr; - PersonalSettings *personalSettings = nullptr; - CardsDisplaySettings *cardsDisplaySettings = nullptr; - InterfaceSettings *interfaceSettings = nullptr; - DeckEditorSettings *deckEditorSettings = nullptr; - PathsSettings *pathsSettings = nullptr; - VisualDeckStorageSettings *visualDeckStorageSettings = nullptr; - AppearanceSettings *appearanceSettings = nullptr; - NetworkSettings *networkSettings = nullptr; - CommanderBracketSettings *commanderBracketSettings = nullptr; + QSettings *settings; + ShortcutsSettings *shortcutsSettings; + CardDatabaseSettings *cardDatabaseSettings; + ServersSettings *serversSettings; + MessageSettings *messageSettings; + GameFiltersSettings *gameFiltersSettings; + LayoutsSettings *layoutsSettings; + DownloadSettings *downloadSettings; + RecentsSettings *recentsSettings; + CardOverrideSettings *cardOverrideSettings; + DebugSettings *debugSettings; + CardCounterSettings *cardCounterSettings; + TabsSettings *tabsSettings; + SoundSettings *soundSettings; + GameSettings *gameSettings; + ChatSettings *chatSettings; + CacheStorageSettings *cacheStorageSettings; + UpdatesSettings *updatesSettings; + PersonalSettings *personalSettings; + CardsDisplaySettings *cardsDisplaySettings; + InterfaceSettings *interfaceSettings; + DeckEditorSettings *deckEditorSettings; + PathsSettings *pathsSettings; + VisualDeckStorageSettings *visualDeckStorageSettings; + AppearanceSettings *appearanceSettings; + NetworkSettings *networkSettings; + CommanderBracketSettings *commanderBracketSettings; QString themeName; diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index 8e34942e1..b8a54761a 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include #include @@ -44,7 +43,6 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) qRegisterMetaType("ExactCard"); connect(worker, &CardPictureLoaderWorker::imageLoaded, this, &CardPictureLoader::imageLoaded); - connect(worker, &CardPictureLoaderWorker::networkCacheCleared, this, &CardPictureLoader::networkCacheCleared); statusBar = new CardPictureLoaderStatusBar(nullptr); QMainWindow *mainWindow = qobject_cast(QApplication::activeWindow()); @@ -60,18 +58,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) CardPictureLoader::~CardPictureLoader() { - if (worker) { - // Capture the thread first: shutdownThread() blocks until the worker has been freed by the - // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. - QThread *pictureLoaderThread = worker->workerThread(); - const bool stopped = worker->shutdownThread(); - worker = nullptr; - // Deleting a QThread that is still running is undefined behaviour, so only free it once the - // bounded wait in shutdownThread() confirmed that it stopped. - if (stopped) { - delete pictureLoaderThread; - } - } + worker->deleteLater(); } void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size) @@ -470,17 +457,7 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - // During teardown the worker is released before this singleton, so a queued clear may still - // arrive with no worker left to run it. - CardPictureLoaderWorker *worker = getInstance().worker; - if (!worker) { - return; - } - // The disk cache and redirect cache are owned by the worker thread, so the clear has to run - // there. Invoke it asynchronously to keep the GUI responsive while the worker may be walking - // the user's picture directories or recursively deleting the cache directory; callers that - // need to know when it is done can listen for networkCacheCleared(). - QMetaObject::invokeMethod(worker, &CardPictureLoaderWorker::clearNetworkCache, Qt::QueuedConnection); + getInstance().worker->clearNetworkCache(); } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h index 6224befbf..0a4934e6d 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h @@ -117,9 +117,6 @@ public: public slots: /** * @brief Clears the network disk cache of the worker. - * - * The clear runs on the worker thread, so this returns before it has completed; connect to - * networkCacheCleared() to act once it is done. */ static void clearNetworkCache(); @@ -134,10 +131,6 @@ public slots: void installPrintingOverride(const ExactCard &originalCard, const ExactCard &overrideCard); void installPrintingOverrideOnLoad(const ExactCard &originalCard, const ExactCard &overrideCard); -signals: - /** @brief Emitted after the worker has finished clearing the network and redirect caches. */ - void networkCacheCleared(); - private slots: /** * @brief Triggered when the user changes the picture download settings. diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp index 12165e8f3..c82fca403 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp @@ -101,7 +101,7 @@ QImage CardPictureLoaderLocal::tryLoadCardImageFromDisk(const QString &setName, for (const QString &path : candidatePaths) { QFileInfo fileInfo(path); QDir dir = fileInfo.dir(); - QString baseName = fileInfo.completeBaseName(); + QString baseName = fileInfo.fileName(); if (!dir.exists()) { continue; 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 0fd6a42d5..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 @@ -16,16 +16,13 @@ #include #include -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 -static constexpr int THREAD_SHUTDOWN_WAIT_MS = 5000; ///< Bounded wait for the worker thread to stop at exit +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 CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), - hostRequestLimits(SettingsCache::instance().downloads().getHostRequestLimits()) + requestQuota(MAX_REQUESTS_PER_SEC) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -60,59 +57,20 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() localLoader = new CardPictureLoaderLocal(this); pictureLoaderThread = new QThread; - // The worker object frees itself once its thread finishes, so no event loop is left - // running and the QThread is never destroyed while still executing. - connect(pictureLoaderThread, &QThread::finished, this, &QObject::deleteLater); pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); - // 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(static_cast(QUOTA_RESET_INTERVAL_MS)); - - 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(); }); + requestTimer.setInterval(1000); + requestTimer.start(); } CardPictureLoaderWorker::~CardPictureLoaderWorker() { saveRedirectCache(); -} - -bool CardPictureLoaderWorker::shutdownThread() -{ - // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as - // its event loop exits, so nothing - not even a member read - may run once wait() returns. - // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. - QThread *thread = pictureLoaderThread; - if (!thread) { - return true; - } - thread->quit(); - // Only an unbounded wait() would guarantee the thread stops, but this runs from a function-local - // static destructor after main() has returned, with no UI left to interrupt a worker stuck in a - // slow slot or on a stalled filesystem. Bound the wait and leave such a thread to the OS rather - // than hanging the process forever. - if (!thread->wait(THREAD_SHUTDOWN_WAIT_MS)) { - qCWarning(CardPictureLoaderWorkerLog) << "Picture loader worker thread did not stop within" - << THREAD_SHUTDOWN_WAIT_MS << "ms; leaving it to be torn down by the OS"; - return false; - } - return true; -} - -QThread *CardPictureLoaderWorker::workerThread() const -{ - return pictureLoaderThread; + pictureLoaderThread->deleteLater(); } void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) @@ -141,17 +99,7 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture // Check for cached redirects QUrl cachedRedirect = getCachedRedirect(url); if (!cachedRedirect.isEmpty()) { - // The status bar still needs to reclaim this URL's widget even when we hand the request back - // for a deferred retry instead of dispatching it onto the network. emit imageRequestSucceeded(url); - // The redirect target is a different host, which may itself be in 429 backoff; hand the - // entry back to its worker so it waits the backoff out instead of dispatching straight - // onto the backed-off host. - if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(cachedRedirect.host(), - QDateTime::currentDateTime())) { - worker->scheduleDeferredRetry(cachedRedirect.host()); - return nullptr; - } return makeRequest(cachedRedirect, worker); } @@ -162,258 +110,67 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture // 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 = !requestTouchesNetwork(url); + 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); QNetworkReply *reply = networkManager->get(req); - // Track in-flight replies per host so the unlocked fast path can bound how many requests it - // issues at once, instead of creating replies that time out before Qt opens a connection. - const QString host = url.host(); - hostInFlight.insert(host, hostInFlight.value(host) + 1); - - // Release the in-flight slot when the reply is destroyed, not when it emits `finished`, and use - // the worker (not the work object) as the context object: a reply can go away without ever - // finishing (aborted, or a work object deleted while a reply is still pending), and a connection - // bound to that work object's lifetime would then never run, permanently shrinking the fast - // path's concurrency until it wedges. This way the slot is released exactly once. - connect(reply, &QObject::destroyed, this, - [this, host] { hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1)); }); - - // Connect reply handling; the work object is the context so its handler dies with it. - connect(reply, &QNetworkReply::finished, worker, [worker, reply] { worker->handleNetworkReply(reply); }); + // Connect reply handling + connect(reply, &QNetworkReply::finished, worker, [reply, worker] { worker->handleNetworkReply(reply); }); return reply; } void CardPictureLoaderWorker::resetRequestQuota() { + requestQuota = MAX_REQUESTS_PER_SEC; + QDateTime now = QDateTime::currentDateTime(); - for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end();) { + for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { - if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { - // A developer-unlocked host that fell back after a 429 recovers towards the default - // allowance; once it gets there it becomes unlocked (fast-path) again. - if (it.value() + 1 >= DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT) { - it = hostRequestQuota.erase(it); - continue; - } - it.value() += 1; - } else { - // Recover towards the host's effective allowance ceiling, which may be - // lowered by the user's per-host request limits. - it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); - } + it.value() = qMin(MAX_REQUESTS_PER_SEC, it.value() + 1); } - ++it; } - // Forget the per-second allowances; each host's allowance is re-seeded lazily from its - // reduced sustained quota the first time it is dispatched in the new second, so a host that - // enters the queue mid-second no longer falls through to a fresh full quota. - hostQuotaRemaining.clear(); + for (const auto &request : requestLoadQueue) { + const QString host = request.first.host(); + hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); + } - updateTimerState(); + processQueuedRequests(); } void CardPictureLoaderWorker::processQueuedRequests() { - // QTimer must be started from the thread it lives in; if this public slot is ever reached from - // another thread, replay it on the worker's event loop instead of letting start() fail silently. - if (thread() != QThread::currentThread()) { - QMetaObject::invokeMethod(this, &CardPictureLoaderWorker::processQueuedRequests, Qt::QueuedConnection); - return; - } - updateTimerState(); -} - -void CardPictureLoaderWorker::dispatchQueuedRequest() -{ - if (requestLoadQueue.isEmpty()) { - // All queued requests have been dispatched; stop the pacing timers. - updateTimerState(); - return; - } - - QDateTime now = QDateTime::currentDateTime(); - bool dispatched = false; - // Set while an unlocked host still has queued work blocked only by the in-flight cap; the - // timer must keep running so it gets another try as soon as a slot frees. A host blocked by - // its 429 backoff instead waits for the next quota-reset tick to restart the dispatcher. - bool unlockedCapped = false; - - // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the pacing and the per-host - // allowance: dispatch their queued requests back-to-back, bounded by their 429 backoff and the - // per-host in-flight cap so a large burst can't queue replies that time out before Qt opens a - // connection for them. - for (int i = 0; i < requestLoadQueue.size();) { - const auto &request = requestLoadQueue.at(i); - // Dispatch decisions must key on the host the request will actually go to, not the URL that - // merely redirects to it: a redirect learned after this URL was queued would otherwise - // bypass the in-flight cap and drain the whole queue onto the target host unchecked. - const QUrl resolvedUrl = resolveCachedRedirect(request.first); - const QString host = resolvedUrl.host(); - if (isUnlockedHost(host)) { - if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { - ++i; - continue; - } - if (hostInFlight.value(host) < MAX_IN_FLIGHT_PER_HOST) { - makeRequest(resolvedUrl, request.second); - requestLoadQueue.removeAt(i); - dispatched = true; - continue; - } - unlockedCapped = true; - } - ++i; - } - - if (requestLoadQueue.isEmpty()) { - dispatchTimer.stop(); - requestTimer.stop(); - return; - } - - if (processSingleRequest()) { - dispatched = true; - } - - // Keep the timer running while there is progress to make or unlocked work waiting on a free - // in-flight slot; otherwise no host has allowance left this second, so wait for the quota reset. - if (!dispatched && !unlockedCapped) { - dispatchTimer.stop(); - } -} - -void CardPictureLoaderWorker::updateTimerState() -{ - // Never restart an active timer: that would reset the pacing countdown and a burst of enqueues - // could keep starving the dispatcher, so only (re)start a timer that has actually stopped. - if (requestLoadQueue.isEmpty()) { - dispatchTimer.stop(); - // Forget per-second allowances once nothing is pending: a stale zero would otherwise delay - // the next single request by a full quota-reset interval. - hostQuotaRemaining.clear(); - } else if (!dispatchTimer.isActive()) { - dispatchTimer.start(); - } - - // The quota timer resets allowances every second and is also the only thing that heals a host - // after a 429 (see resetRequestQuota). It must keep ticking while work is queued or a host is - // still recovering below the ceiling, and only winds down once no host needs recovery anymore. - // Keeping it alive during such idle periods lets reduced quotas recover as intended. - bool hostRecovering = false; - for (auto it = hostRequestQuota.cbegin(); it != hostRequestQuota.cend(); ++it) { - if (it.value() < MAX_REQUESTS_PER_SEC) { - hostRecovering = true; - break; - } - } - - if (!requestLoadQueue.isEmpty() || hostRecovering) { - if (!requestTimer.isActive()) { - requestTimer.start(); - } - } else if (requestTimer.isActive()) { - requestTimer.stop(); + while (requestQuota > 0 && processSingleRequest()) { + --requestQuota; } } bool CardPictureLoaderWorker::processSingleRequest() { - QDateTime now = QDateTime::currentDateTime(); for (int i = 0; i < requestLoadQueue.size(); ++i) { - // Copy the entry: takeAt(i) below erases within the list this reference points into. - const auto request = requestLoadQueue.at(i); - // Resolve cached redirects so the rate-limit and allowance arithmetic keys on the host the - // request will actually hit (see resolveCachedRedirect). - const QUrl resolvedUrl = resolveCachedRedirect(request.first); - const QString host = resolvedUrl.host(); - // Don't dispatch requests to a host that is currently in its 429 backoff; hand the entry - // back to its worker so it can wait the backoff out or fall through to another source, - // instead of leaving it parked in the queue with no reply pending. Only applies to - // requests that will actually touch the network: one that will be served from the disk - // cache costs nothing and shouldn't wait out the 429. - if (requestTouchesNetwork(request.first) && - CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { - // The queued URL is usually a cached-redirect target whose host differs from - // cardToDownload.getCurrentUrl(), so scheduleDeferredRetry() (which waits out the - // blocked host's deadline) is used instead of startNextPicDownload() looping on the - // original host. - auto entry = requestLoadQueue.takeAt(i); - if (host != entry.first.host()) { - // A cached redirect target is what is blocked, which the work object would not - // discover from its own URL; wait out that specific host (with jitter) instead. - entry.second->scheduleDeferredRetry(host); - } else { - entry.second->startNextPicDownload(); - } - return true; - } - // Unlocked hosts are handled by dispatchQueuedRequest's fast path, bounded by the in-flight - // cap; they must not fall through to the per-host allowance arithmetic below. - if (isUnlockedHost(host)) { - continue; - } - int ceiling = hostAllowanceCeiling(host); - if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { - // A 429 dropped this unlocked host out of the fast path and installed a concrete - // allowance; pace it against that allowance until the recovery loop unlocks it again. - ceiling = hostRequestQuota.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - } - // Seed the allowance lazily so a host that enters the queue mid-second gets its reduced - // per-host allowance, clamped against the ceiling so a lowered user cap applies from this - // second onward. - if (!hostQuotaRemaining.contains(host)) { - hostQuotaRemaining.insert(host, qMin(ceiling, hostRequestQuota.value(host, ceiling))); - } - int allowance = hostQuotaRemaining.value(host); + const auto &request = requestLoadQueue.at(i); + QString host = request.first.host(); + int allowance = hostQuotaRemaining.value(host, MAX_REQUESTS_PER_SEC); if (allowance > 0) { hostQuotaRemaining.insert(host, allowance - 1); - auto entry = requestLoadQueue.takeAt(i); - makeRequest(resolvedUrl, entry.second); + makeRequest(request.first, request.second); + requestLoadQueue.removeAt(i); return true; } } return false; } -bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url) const -{ - bool useNetworkCache = static_cast( - SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == - CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && - (cache->metaData(url).isValid() || !picDownload); - return !useNetworkCache; -} - -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); -} - -bool CardPictureLoaderWorker::isUnlockedHost(const QString &host) const -{ - return hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestQuota.contains(host); -} - void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - const int ceiling = hostAllowanceCeiling(host); - // An unlocked host has no per-host allowance to halve. Install one instead so it drops out of - // the unlocked fast path and is paced like a throttled host; the recovery loop in - // resetRequestQuota() then walks it back up and unlocks it again. - const int base = - ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : ceiling; - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, base) / 2)); + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } @@ -471,22 +228,6 @@ QUrl CardPictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const return {}; } -QUrl CardPictureLoaderWorker::resolveCachedRedirect(const QUrl &url) const -{ - // Follow the whole cached-redirect chain so dispatch keys on the host that is really hit. The - // depth bound keeps a corrupt or self-referencing cache entry from spinning us forever. - QUrl resolved = url; - int depth = 0; - while (depth++ < MAX_REDIRECT_CHAIN_DEPTH) { - QUrl target = getCachedRedirect(resolved); - if (target.isEmpty() || target == resolved) { - break; - } - resolved = target; - } - return resolved; -} - void CardPictureLoaderWorker::loadRedirectCache() { QSettings settings(cacheFilePath, QSettings::IniFormat); @@ -539,5 +280,4 @@ void CardPictureLoaderWorker::clearNetworkCache() { networkManager->cache()->clear(); redirectCache.clear(); - emit networkCacheCleared(); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index 8b136f309..d1c519b7a 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -74,37 +74,10 @@ public: */ void onHostRateLimited(const QString &host); - /** - * @brief Stops the worker thread and reports whether it stopped. - * - * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an - * exit request to the worker's event loop and QThread::wait() blocks (bounded) until the loop - * has returned and the thread finished. Only QThread members are touched here, so this method - * is safe to call from the owning thread. The worker object itself is freed by the finished() - * -> deleteLater chain (see the constructor); the QThread object is deleted afterwards by the - * owner (CardPictureLoader::~CardPictureLoader), not by this method. - * - * @return true if the thread stopped within the timeout, false if it is still running (in - * which case the owner must not delete the QThread). - */ - [[nodiscard]] bool shutdownThread(); - - /** - * @brief Returns the worker's QThread. - * @return The worker thread - * - * Only meaningful while the worker object is alive; capture it before calling shutdownThread(). - */ - QThread *workerThread() const; + /** @brief Clears the network cache and redirect cache. */ + void clearNetworkCache(); public slots: - /** - * @brief Clears the network cache and redirect cache. - * - * Runs on the worker thread; invoke it via a queued call when coming from another thread, - * since both caches are owned by the worker thread. - */ - void clearNetworkCache(); /** * @brief Makes a network request for the given URL using the specified worker. * @param url URL to load @@ -113,12 +86,9 @@ public slots: */ QNetworkReply *makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *workThread); - /** @brief Ensures the pacing and quota-reset timers reflect the current queue and recovery state. */ + /** @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. @@ -148,56 +118,18 @@ private: bool picDownload; ///< Whether downloading images from network is enabled QQueue> requestLoadQueue; ///< Queue of pending network requests + int requestQuota; ///< Remaining requests allowed per second QTimer requestTimer; ///< Timer to reset the request quota - QTimer dispatchTimer; ///< Timer pacing individual network requests QHash hostRequestQuota; ///< Sustained per-host request allowance - QHash hostRequestLimits; ///< User-set per-host request allowances QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited - QHash hostInFlight; ///< Network replies currently in flight, per host - - /** @brief Maximum concurrent in-flight network replies per host. */ - static constexpr int MAX_IN_FLIGHT_PER_HOST = 6; - - /** @brief Bound on how many cached-redirect hops dispatch resolution will follow. */ - static constexpr int MAX_REDIRECT_CHAIN_DEPTH = 10; 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 Whether a host may skip dispatch pacing and per-host allowance entirely. - * - * A host is unlocked while it has no user limit and no reduced allowance installed by a 429. - * A 429 drops it out of the fast path until resetRequestQuota() walks the allowance back up. - */ - [[nodiscard]] bool isUnlockedHost(const QString &host) const; - /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; - /** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk - * cache. */ - [[nodiscard]] bool requestTouchesNetwork(const QUrl &url) const; - - /** - * @brief Follows the cached-redirect chain to the URL that will actually be requested. - * @param url The URL to resolve - * @return The final URL after chasing cached redirects, or @p url itself if none lead elsewhere - * - * Dispatch decisions (unlocked-host fast path, 429 backoff, in-flight cap) must key on the host - * a request really goes to, not the URL that merely redirects to it. - */ - [[nodiscard]] QUrl resolveCachedRedirect(const QUrl &url) const; - /** @brief Loads redirect cache from disk. */ void loadRedirectCache(); @@ -207,9 +139,6 @@ private: /** @brief Removes stale redirect entries older than TTL. */ void cleanStaleEntries(); - /** @brief Starts or stops the pacing and quota-reset timers to match the queue and recovery state. */ - void updateTimerState(); - private slots: /** @brief Resets the request quota for rate-limiting. */ void resetRequestQuota(); @@ -229,9 +158,6 @@ signals: /** @brief Emitted when a network request successfully completes. */ void imageRequestSucceeded(const QUrl &url); - - /** @brief Emitted after clearNetworkCache() has finished clearing both caches. */ - void networkCacheCleared(); }; #endif // PICTURE_LOADER_WORKER_H diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index 19c6be469..66c56337c 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,13 +22,8 @@ static const QStringList MD5_BLACKLIST = { "fbc7d763c08771c260b39e2115414eeb" // Current card back hash }; -const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() -{ - return s_rateLimiter; -} - -CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad) - : QObject(worker), cardToDownload(CardPictureToLoad(toLoad)), +CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) + : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) { // Hook up signals to the orchestrator @@ -173,7 +168,7 @@ void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply) << "PictureLoader: [card: " << cardToDownload.getCard().getName() << " set: " << cardToDownload.getSetName() << "]: Too many requests from " << host << ", backing off until " << backoffUntil.toString(Qt::ISODate) << ", retrying the same url"; - scheduleDeferredRetry(host); + scheduleDeferredRetry(); } else { qCWarning(CardPictureLoaderWorkerWorkLog).nospace() << "PictureLoader: [card: " << cardToDownload.getCard().getName() @@ -278,16 +273,14 @@ QImage CardPictureLoaderWorkerWork::tryLoadImageFromReply(QNetworkReply *reply) return imgReader.read(); } -void CardPictureLoaderWorkerWork::scheduleDeferredRetry(const QString &preferredHost) +void CardPictureLoaderWorkerWork::scheduleDeferredRetry() { QDateTime now = QDateTime::currentDateTime(); - // Prefer waiting on the server that is actually blocking the request: callers hand in the - // rate-limited host when it differs from the current URL (e.g. a cached redirect target still - // in backoff), otherwise fall back to the current URL's server so we retry the same source. - QString waitHost = preferredHost.isEmpty() ? QUrl(cardToDownload.getCurrentUrl()).host() : preferredHost; - QDateTime backoffUntil = s_rateLimiter.deadline(waitHost); - if (!s_rateLimiter.isRateLimited(waitHost, now)) { + // Prefer waiting on the current URL's server so we retry the same source. + QString currentHost = QUrl(cardToDownload.getCurrentUrl()).host(); + QDateTime backoffUntil = s_rateLimiter.deadline(currentHost); + if (!s_rateLimiter.isRateLimited(currentHost, now)) { backoffUntil = s_rateLimiter.earliestDeadline(now); } 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 84a7dafdb..1e56a4373 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 @@ -10,7 +10,6 @@ #include #include #include -#include #include #include #include @@ -37,36 +36,13 @@ class CardPictureLoaderWorkerWork : public QObject public: /** * @brief Constructs a worker for downloading a specific card image. - * @param worker The orchestrating CardPictureLoaderWorker; the work object becomes its child so - * it is destroyed with the worker even if it never reaches concludeImageLoad(). + * @param worker The orchestrating CardPictureLoaderWorker * @param toLoad The ExactCard to download */ - explicit CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad); + explicit CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad); CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading - /** @brief Shared per-server 429 backoff state. */ - static const ServerRateLimiter &rateLimiter(); - - /** - * @brief Starts downloading the next URL for this card. - * - * Skips URLs whose server is currently in 429 backoff, either waiting the - * backoff out or falling through to the other configured sources. - */ - void startNextPicDownload(); - - /** - * @brief Schedules a deferred retry after the relevant server backoff expires. - * @param preferredHost The server that is actually blocking the request, or an empty - * string to use the current URL's server - * - * Waits on the blocking server's backoff deadline, otherwise on the earliest active - * backoff. If no servers are in backoff, concludes with failure. Otherwise resets the - * CardPictureToLoad indices and retries after the backoff period. - */ - void scheduleDeferredRetry(const QString &preferredHost = {}); - public slots: /** * @brief Handles a finished network reply for the card image. @@ -79,6 +55,9 @@ private: static ServerRateLimiter s_rateLimiter; ///< Shared per-server 429 backoff state + /** @brief Starts downloading the next URL for this card. */ + void startNextPicDownload(); + /** @brief Called when all URLs have been exhausted or download failed. */ void picDownloadFailed(); @@ -103,6 +82,16 @@ private: */ void concludeImageLoad(const QImage &image); + /** + * @brief Schedules a deferred retry after the relevant server backoff expires. + * + * Waits on the current URL's server when it is the reason we are blocked, + * otherwise on the earliest active backoff. If no servers are in backoff, + * concludes with failure. Otherwise resets the CardPictureToLoad indices and + * retries after the backoff period. + */ + void scheduleDeferredRetry(); + private slots: /** @brief Updates the picDownload setting when it changes. */ void picDownloadChanged(); diff --git a/cockatrice/src/interface/theme_manager.cpp b/cockatrice/src/interface/theme_manager.cpp index d93dfcf62..d86ed77f9 100644 --- a/cockatrice/src/interface/theme_manager.cpp +++ b/cockatrice/src/interface/theme_manager.cpp @@ -95,11 +95,9 @@ struct PaletteColorInfo static QString usableDefaultStyle(const QString &style) { - // The Windows 11 native style is broken: dragging cards across zones can - // shrink the board to a tiny grey window that is unfixable without - // rejoining. It is never usable, so guard against it no matter how it was - // requested (OS default or an explicit "windows11" theme choice) and fall - // back to the Vista style. + // The Windows 11 native style is broken: when the OS default + // ("System" theme selection) would use it, fall back to the Vista style. + // Explicitly choosing "windows11" in a theme is still honored. return style.compare("windows11", Qt::CaseInsensitive) == 0 ? QStringLiteral("windowsvista") : style; } @@ -111,10 +109,7 @@ ThemeManager::ThemeManager(QObject *parent) : QObject(parent) ensureThemeDirectoryExists(); #if (QT_VERSION >= QT_VERSION_CHECK(6, 5, 0)) connect(QGuiApplication::styleHints(), &QStyleHints::colorSchemeChanged, this, [this] { - // Reload so scheme-qualified assets and palettes follow the OS, but do - // NOT recapture defaultPalette: qApp->palette() already carries the - // currently-applied theme palette at this point, so recapturing it - // would contaminate the base for every later theme switch. + defaultPalette = qApp->palette(); themeChangedSlot(); }); #endif @@ -412,10 +407,6 @@ void ThemeManager::applyStyleAndPalette(const QString &themeName, } } - // The Windows 11 style is broken even when selected explicitly in a theme, - // so sanitize the resolved name here rather than trusting the theme config. - styleName = usableDefaultStyle(styleName); - QStyle *style = QStyleFactory::create(styleName); if (!style) { style = QStyleFactory::create(usableDefaultStyle(defaultStyleName)); diff --git a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp index 415eef6b9..f161f0f19 100644 --- a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp @@ -62,12 +62,6 @@ AppearanceSettingsPage::AppearanceSettingsPage() // Qt widget style; "System" lets the application decide styleCombo.addItem(tr("System"), QStringLiteral("System")); for (const QString &key : QStyleFactory::keys()) { - // The Windows 11 native style is broken (board rendering glitches when - // moving cards), so never offer it; it is already sanitized at apply - // time in ThemeManager. - if (key.compare("windows11", Qt::CaseInsensitive) == 0) { - continue; - } styleCombo.addItem(key, key); } @@ -511,7 +505,7 @@ void AppearanceSettingsPage::retranslateUi() openThemeButton.setText(tr("Open themes folder")); schemeComboLabel.setText(tr("Active theme palette:")); styleComboLabel.setText(tr("Active theme style:")); - styleCombo.setToolTip(tr("Qt widget style saved to this theme (\"System\" lets the application decide)")); + styleCombo.setToolTip(tr("Qt widget style saved to this theme (\"Default\" lets the application decide)")); editPaletteButton.setText(tr("Edit theme palette")); homeTabGroupBox->setTitle(tr("Home tab settings")); 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 bdbb38c95..f3eac05b8 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,9 +10,7 @@ #include #include #include -#include #include -#include #include #include #include @@ -53,9 +51,7 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() urlList->setDragDropMode(QAbstractItemView::InternalMove); connect(urlList->model(), &QAbstractItemModel::rowsMoved, this, &DeckEditorSettingsPage::urlListChanged); - for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { - addUrlItem(url); - } + urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); aAdd = new QAction(this); aAdd->setIcon(themePixmap(QStringLiteral("icons/increment"))); @@ -69,16 +65,11 @@ 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; @@ -126,9 +117,7 @@ void DeckEditorSettingsPage::resetDownloadedURLsButtonClicked() { SettingsCache::instance().downloads().resetToDefaultURLs(); urlList->clear(); - for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { - addUrlItem(url); - } + urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); QMessageBox::information(this, tr("Success"), tr("Download URLs have been reset.")); } @@ -137,7 +126,7 @@ void DeckEditorSettingsPage::actAddURL() bool ok; QString msg = QInputDialog::getText(this, tr("Add URL"), tr("URL:"), QLineEdit::Normal, QString(), &ok); if (ok) { - addUrlItem(msg); + urlList->addItem(msg); storeSettings(); } } @@ -152,14 +141,12 @@ void DeckEditorSettingsPage::actRemoveURL() void DeckEditorSettingsPage::actEditURL() { - QListWidgetItem *item = urlList->currentItem(); - if (item) { - const QString oldText = urlForItem(item); + if (urlList->currentItem()) { + QString oldText = urlList->currentItem()->text(); bool ok; QString msg = QInputDialog::getText(this, tr("Edit URL"), tr("URL:"), QLineEdit::Normal, oldText, &ok); if (ok) { - item->setData(Qt::UserRole, msg); - item->setText(urlLabel(msg)); + urlList->currentItem()->setText(msg); storeSettings(); } } @@ -171,133 +158,10 @@ void DeckEditorSettingsPage::storeSettings() QStringList downloadUrls; for (int i = 0; i < urlList->count(); i++) { - const QString url = urlForItem(urlList->item(i)); - qInfo() << "Priority" << i << ":" << url; - downloadUrls << url; + qInfo() << "Priority" << i << ":" << urlList->item(i)->text(); + downloadUrls << urlList->item(i)->text(); } SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); - - // Drop per-host limits whose host is no longer referenced by any configured URL, so removing - // a URL doesn't leave a stale throttle behind that reactivates if the host is re-added. - QSet usedHosts; - for (const QString &url : downloadUrls) { - const QString host = QUrl(url).host(); - if (!host.isEmpty()) { - usedHosts.insert(host); - } - } - QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); - bool limitsChanged = false; - for (auto it = limits.begin(); it != limits.end();) { - // Prune only limits for hosts that are neither referenced by a configured URL nor carry a - // developer cap. Capped hosts are often redirect targets (e.g. api.scryfall.com redirects - // to cards.scryfall.io) that never appear in the URL list, yet they are exactly the hosts - // the throttle applies to, so dropping them when a URL is removed would silently re-enable - // free-running traffic to a rate-sensitive server. - if (!usedHosts.contains(it.key()) && !DownloadSettings::getDeveloperHostCaps().contains(it.key())) { - it = limits.erase(it); - limitsChanged = true; - } else { - ++it; - } - } - if (limitsChanged) { - SettingsCache::instance().downloads().setHostRequestLimits(limits); - } - - refreshUrlItems(); -} - -QListWidgetItem *DeckEditorSettingsPage::addUrlItem(const QString &url) -{ - auto *item = new QListWidgetItem(urlLabel(url)); - item->setData(Qt::UserRole, url); - urlList->addItem(item); - return item; -} - -QString DeckEditorSettingsPage::urlForItem(const QListWidgetItem *item) const -{ - return item->data(Qt::UserRole).toString(); -} - -QString DeckEditorSettingsPage::urlLabel(const QString &url) const -{ - const QString host = QUrl(url).host(); - if (host.isEmpty()) { - return url; - } - - const QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); - const int devCap = - DownloadSettings::getDeveloperHostCaps().value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); - if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !limits.contains(host)) { - return tr("%1 (unlimited)").arg(url); - } - - const int requested = limits.value( - host, devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : devCap); - const int effective = SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); - return tr("%1 (%2/s)").arg(url).arg(effective); -} - -void DeckEditorSettingsPage::refreshUrlItems() -{ - for (int i = 0; i < urlList->count(); ++i) { - QListWidgetItem *item = urlList->item(i); - item->setText(urlLabel(urlForItem(item))); - } -} - -void DeckEditorSettingsPage::actAdjustRateLimit() -{ - if (urlList->currentItem() == nullptr) { - QMessageBox::information(this, tr("Adjust Rate Limit"), tr("Select a URL in the list first.")); - return; - } - - const QString host = QUrl(urlForItem(urlList->currentItem())).host(); - if (host.isEmpty()) { - QMessageBox::information(this, tr("Adjust Rate Limit"), tr("The selected URL does not have a valid host.")); - return; - } - - 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; - QString prompt; - if (unlocked) { - minimum = 0; // 0 means "unlimited" - maximum = DownloadSettings::UNLOCKED_HOST_LIMIT_MAX; - defaultValue = currentLimits.value(host, 0); - prompt = tr("Requests per second (0 = unlimited, fastest; up to %1):").arg(maximum); - } else { - minimum = DownloadSettings::MIN_HOST_REQUEST_LIMIT; - maximum = devCap; - defaultValue = currentLimits.value(host, devCap); - prompt = tr("Requests per second (developer maximum is %1):").arg(maximum); - } - - const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), prompt, 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); - refreshUrlItems(); } void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) @@ -380,8 +244,4 @@ void DeckEditorSettingsPage::retranslateUi() aAdd->setText(tr("Add New URL")); aEdit->setText(tr("Edit URL")); aRemove->setText(tr("Remove URL")); - aRateLimit->setText(tr("Adjust Rate Limit")); - - // The per-URL rate limit suffixes are translated, so refresh them when the language changes. - refreshUrlItems(); -} +} \ No newline at end of file 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 23e33fa20..5db009c8a 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 @@ -27,7 +27,6 @@ private slots: void actAddURL(); void actRemoveURL(); void actEditURL(); - void actAdjustRateLimit(); void resetDownloadedURLsButtonClicked(); private: @@ -35,7 +34,7 @@ private: QLabel urlLinkLabel; QCheckBox picDownloadCheckBox; QListWidget *urlList; - QAction *aAdd, *aEdit, *aRemove, *aRateLimit; + QAction *aAdd, *aEdit, *aRemove; QCheckBox mcDownloadSpoilersCheckBox; QLabel msDownloadSpoilersLabel; QGroupBox *mpGeneralGroupBox; @@ -47,18 +46,6 @@ private: QLabel infoOnSpoilersLabel; QPushButton *mpSpoilerPathButton; QPushButton *updateNowButton; - - /** @brief Adds a list item for the given URL, storing the raw URL alongside its displayed label. */ - QListWidgetItem *addUrlItem(const QString &url); - - /** @brief Returns the raw URL stored on a list item. */ - [[nodiscard]] QString urlForItem(const QListWidgetItem *item) const; - - /** @brief Returns the display label for a URL, including its current effective rate limit. */ - [[nodiscard]] QString urlLabel(const QString &url) const; - - /** @brief Refreshes the displayed label of every URL item after limits or settings change. */ - void refreshUrlItems(); }; #endif // COCKATRICE_DECK_EDITOR_SETTINGS_PAGE_H diff --git a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp index 4aad91115..17838e501 100644 --- a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp @@ -181,14 +181,9 @@ StorageSettingsPage::StorageSettingsPage() void StorageSettingsPage::clearDownloadedPicsButtonClicked() { - // The network cache is cleared asynchronously on the worker thread, so wait for the completion - // signal before confirming; the in-memory pixmap cache is cleared synchronously right away. - connect( - &CardPictureLoader::getInstance(), &CardPictureLoader::networkCacheCleared, this, - [this] { QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); }, - Qt::SingleShotConnection); - CardPictureLoader::clearPixmapCache(); CardPictureLoader::clearNetworkCache(); + CardPictureLoader::clearPixmapCache(); + QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); } void StorageSettingsPage::clearImageBackupsButtonClicked() diff --git a/cockatrice/themes/System/palette-default-dark.toml b/cockatrice/themes/System/palette-default-dark.toml index 9dfe6e19f..e101935b4 100644 --- a/cockatrice/themes/System/palette-default-dark.toml +++ b/cockatrice/themes/System/palette-default-dark.toml @@ -1,7 +1,67 @@ -# The System theme derives all of its colors from the platform palette -# (e.g. qt6ct's custom color scheme on Linux), so it must NOT ship palette -# overrides. Only the identity app colors are provided here, so buttons keep -# their classic greens while the actual palette stays native to the OS. +[Palette] +WindowText = #ffffffff +Button = #ff383838 +Light = #ff737373 +Midlight = #ff525252 +Dark = #ff161616 +Mid = #ff252525 +Text = #ffffffff +BrightText = #ffb4dd8b +ButtonText = #ffffffff +Base = #ff2b2b2b +Window = #ff1c1c1c +Shadow = #ff000000 +HighlightedText = #ff000000 +Link = #ffcde4b6 +LinkVisited = #ff99d999 +AlternateBase = #ff242424 +ToolTipBase = #ffffffdc +ToolTipText = #ff000000 +PlaceholderText = #6effffff + +[Palette.Disabled] +WindowText = #ff9d9d9d +Button = #ff1c1c1c +Light = #ff737373 +Midlight = #ff525252 +Dark = #ff161616 +Mid = #ff252525 +Text = #ff9d9d9d +BrightText = #ffb4dd8b +ButtonText = #ff787878 +Base = #ff1c1c1c +Window = #ff1c1c1c +Shadow = #ff000000 +HighlightedText = #ff9d9d9d +Link = #ff308cc6 +LinkVisited = #ffb450ff +AlternateBase = #ff242424 +ToolTipBase = #ffffffdc +ToolTipText = #ff000000 +PlaceholderText = #46ffffff + +[Palette.Inactive] +WindowText = #ffffffff +Button = #ff383838 +Light = #ff737373 +Midlight = #ff525252 +Dark = #ff161616 +Mid = #ff252525 +Text = #ffffffff +BrightText = #ffb4dd8b +ButtonText = #ffffffff +Base = #ff2b2b2b +Window = #ff1c1c1c +Shadow = #ff000000 +HighlightedText = #ffffffff +Link = #ffcde4b6 +LinkVisited = #ff99d999 +AlternateBase = #ff242424 +ToolTipBase = #ffffffdc +ToolTipText = #ff000000 +PlaceholderText = #6effffff + + [AppColors] AccentStrong = #ff148c3c AccentSoft = #ff78c850 diff --git a/cockatrice/themes/System/palette-default-light.toml b/cockatrice/themes/System/palette-default-light.toml index 9dfe6e19f..14c215cdf 100644 --- a/cockatrice/themes/System/palette-default-light.toml +++ b/cockatrice/themes/System/palette-default-light.toml @@ -1,7 +1,67 @@ -# The System theme derives all of its colors from the platform palette -# (e.g. qt6ct's custom color scheme on Linux), so it must NOT ship palette -# overrides. Only the identity app colors are provided here, so buttons keep -# their classic greens while the actual palette stays native to the OS. +[Palette] +WindowText = #ff000000 +Button = #fff0f0f0 +Light = #ffffffff +Midlight = #ffe3e3e3 +Dark = #ffa0a0a0 +Mid = #ffa0a0a0 +Text = #ff000000 +BrightText = #ffffffff +ButtonText = #ff000000 +Base = #ffffffff +Window = #fff0f0f0 +Shadow = #ff696969 +HighlightedText = #ffffffff +Link = #ff0d5f28 +LinkVisited = #ff08401b +AlternateBase = #ffe9e7e3 +ToolTipBase = #ffffffdc +ToolTipText = #ff000000 +PlaceholderText = #80000000 + +[Palette.Disabled] +WindowText = #ff787878 +Button = #fff0f0f0 +Light = #ffffffff +Midlight = #fff7f7f7 +Dark = #ffa0a0a0 +Mid = #ffa0a0a0 +Text = #ff787878 +BrightText = #ffffffff +ButtonText = #ff787878 +Base = #fff0f0f0 +Window = #fff0f0f0 +Shadow = #ff000000 +HighlightedText = #ffffffff +Link = #ff0000ff +LinkVisited = #ffff00ff +AlternateBase = #fff7f7f7 +ToolTipBase = #ffffffdc +ToolTipText = #ff000000 +PlaceholderText = #80000000 + +[Palette.Inactive] +WindowText = #ff000000 +Button = #fff0f0f0 +Light = #ffffffff +Midlight = #ffe3e3e3 +Dark = #ffa0a0a0 +Mid = #ffa0a0a0 +Text = #ff000000 +BrightText = #ffffffff +ButtonText = #ff000000 +Base = #ffffffff +Window = #fff0f0f0 +Shadow = #ff696969 +HighlightedText = #ff000000 +Link = #ff0d5f28 +LinkVisited = #ff08401b +AlternateBase = #ffe9e7e3 +ToolTipBase = #ffffffdc +ToolTipText = #ff000000 +PlaceholderText = #80000000 + + [AppColors] AccentStrong = #ff148c3c -AccentSoft = #ff78c850 +AccentSoft = #ff78c850 \ No newline at end of file diff --git a/cockatrice/themes/System/theme.cfg b/cockatrice/themes/System/theme.cfg index 23fffccbb..4b756a5e8 100644 --- a/cockatrice/themes/System/theme.cfg +++ b/cockatrice/themes/System/theme.cfg @@ -1,5 +1,5 @@ [Appearance] -ColorScheme = System +ColorScheme = Light [Style] Name = System diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index 6e22a84f4..eb73e58ee 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -12,22 +12,6 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { const QString DownloadSettings::SCRYFALL_NAMED_LOCALIZED_URL = "https://api.scryfall.com/cards/named?fuzzy=!localizedName!&lang=!sflang!&format=image&face=!prop:side!"; -// 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 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}, - // 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) { @@ -81,57 +65,3 @@ void DownloadSettings::setDownloadSpoilerStatus(bool _spoilerStatus) setValue(_spoilerStatus, "downloadSpoilers"); emit downloadSpoilerStatusChanged(); } - -QHash DownloadSettings::getHostRequestLimits() const -{ - auto settings = getSettings(); - if (!defaultGroup.isEmpty()) { - settings.beginGroup(defaultGroup); - } - settings.beginGroup("hostRequestLimits"); - - QHash hostRequestLimits; - const QStringList hosts = settings.childKeys(); - for (const QString &host : hosts) { - hostRequestLimits.insert(host, settings.value(host).toInt()); - } - - settings.endGroup(); - if (!defaultGroup.isEmpty()) { - settings.endGroup(); - } - return hostRequestLimits; -} - -void DownloadSettings::setHostRequestLimits(const QHash &hostRequestLimits) -{ - auto settings = getSettings(); - if (!defaultGroup.isEmpty()) { - settings.beginGroup(defaultGroup); - } - - // Drop the legacy single-key form (an opaque @Variant blob) written by earlier builds so each - // host is stored as a plain, hand-editable key in its own subgroup. - settings.remove("hostRequestLimits"); - settings.beginGroup("hostRequestLimits"); - settings.remove(QString()); - for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { - settings.setValue(it.key(), it.value()); - } - settings.endGroup(); - - if (!defaultGroup.isEmpty()) { - settings.endGroup(); - } - settings.sync(); - emit hostRequestLimitsChanged(); -} - -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 7eca77df6..ae49884f0 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -9,8 +9,6 @@ #include "settings_manager.h" -#include - class DownloadSettings : public SettingsManager { Q_OBJECT @@ -18,35 +16,8 @@ class DownloadSettings : public SettingsManager static const QStringList DEFAULT_DOWNLOAD_URLS; static const QString SCRYFALL_NAMED_LOCALIZED_URL; - 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 Upper bound offered to the user when lowering an unlocked host's allowance. - * - * Unlocked hosts have no developer cap, so `clampHostRequestLimit` puts no upper bound on - * them; this only bounds what the settings dialog offers, and matches the widest paced - * allowance a user is documented to be able to hand-edit in `downloads.ini`. Choosing 0 - * below this restores the "unlimited" fast path. - */ - static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; - /** @brief Developer cap marking a host as never throttled per host or by the dispatch pacing. */ - 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; @@ -58,23 +29,9 @@ 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/CMakeLists.txt b/tests/CMakeLists.txt index d23d9ba04..7bb834d7e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -155,9 +155,3 @@ add_subdirectory(loading_from_clipboard) add_subdirectory(movecard_tests) add_subdirectory(oracle) add_subdirectory(settings) - -# picture_loader_benchmark links libcockatrice_settings, which only exists when -# a client/UI-capable target is being built. -if(WITH_ORACLE OR WITH_CLIENT) - add_subdirectory(picture_loader_benchmark) -endif() diff --git a/tests/loader_local_matching_test.cpp b/tests/loader_local_matching_test.cpp index 6ce684fdd..7ffa7157d 100644 --- a/tests/loader_local_matching_test.cpp +++ b/tests/loader_local_matching_test.cpp @@ -68,18 +68,6 @@ protected: loader = nullptr; } - /** - * @brief Destroys and rebuilds the loader so the CUSTOM index is re-scanned. - * - * The CUSTOM-folder index is snapshotted at construction; rebuild it after - * writing files so freshly placed images are discoverable. - */ - void rebuildLoader() - { - delete loader; - loader = new CardPictureLoaderLocal(nullptr); - } - /** * @brief Writes a valid 1x1 PNG under the sandboxed pics path. */ @@ -174,28 +162,6 @@ TEST_F(LocalMatcherTest, SetFolderCandidateTakesPrecedenceOverRootFallback) EXPECT_EQ(image.pixelColor(0, 0), QColor(Qt::red)) << "The set-folder candidate must be preferred"; } -TEST_F(LocalMatcherTest, CustomSubfolderResolvesExactFile) -{ - // Images in the CUSTOM folder are indexed by their full file path (extension included), - // unlike the extension-less candidate paths built from the naming schemes. - writePngUnderPics("CUSTOM/poker/TestCard.png"); - rebuildLoader(); - - const QImage image = loader->tryLoad(cardFor("TestCard", "", "")); - - EXPECT_FALSE(image.isNull()) << "A CUSTOM-folder image in a subdirectory must resolve"; -} - -TEST_F(LocalMatcherTest, CustomSubfolderIgnoresSuffixedFiles) -{ - writePngUnderPics("CUSTOM/poker/TestCard (1).png"); - rebuildLoader(); - - const QImage image = loader->tryLoad(cardFor("TestCard", "", "")); - - EXPECT_TRUE(image.isNull()) << "A suffixed CUSTOM-folder file must not satisfy an exact name lookup"; -} - } // namespace int main(int argc, char **argv) diff --git a/tests/picture_loader_benchmark/CMakeLists.txt b/tests/picture_loader_benchmark/CMakeLists.txt deleted file mode 100644 index a3b75fe88..000000000 --- a/tests/picture_loader_benchmark/CMakeLists.txt +++ /dev/null @@ -1,26 +0,0 @@ -# Manual benchmark hitting the real card image hosts. Not registered with -# add_test(): it requires a cards.xml, touches the network for minutes at a -# time, and needs network access. Build it explicitly and run by hand. -add_executable( - picture_loader_benchmark_test - ${VERSION_STRING_CPP} - ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp - ../../cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp - ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp - ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp - ../../cockatrice/src/client/settings/cache_settings.h - picture_loader_benchmark.cpp - settings_cache_mock.cpp -) - -target_include_directories(picture_loader_benchmark_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) - -target_link_libraries( - picture_loader_benchmark_test - libcockatrice_card - libcockatrice_settings - libcockatrice_interfaces - libcockatrice_utility - Threads::Threads - ${TEST_QT_MODULES} -) diff --git a/tests/picture_loader_benchmark/picture_loader_benchmark.cpp b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp deleted file mode 100644 index f991f2ec0..000000000 --- a/tests/picture_loader_benchmark/picture_loader_benchmark.cpp +++ /dev/null @@ -1,547 +0,0 @@ -/* - * Picture loader benchmark / regression suite against the real card image hosts. - * - * Deliberately not registered with ctest: it hits live Scryfall / Gatherer - * endpoints at ~10 requests per second and takes minutes. Run it by hand. - * - * picture_loader_benchmark_test --carddb /path/to/cards.xml [options] - * - * Modes - * ----- - * default : for every URL template in the configured download list (or for each - * --url given), load #count pictures twice: once cold (network) and - * once cached (served from the QNetworkDiskCache). Both passes must - * load every card with zero failures. The cached pass must complete - * well under the cold time, which is the regression gate for serving - * cached pictures instead of re-fetching them. The cold pass must stay - * above a pacing lower bound, the regression gate for burst-free - * request throttling. - * --stress: two CardPictureLoaderWorker instances loading the same cards - * concurrently against one host (~20 req/s aggregate), which forces - * real 429 responses. Both workers must still complete 100% of their - * cards via the shared backoff logic. - */ - -#include "client/settings/cache_settings.h" -#include "interface/card_picture_loader/card_picture_loader_worker.h" -#include "interface/card_picture_loader/card_picture_to_load.h" - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -class BenchmarkCardDatabasePathProvider : public ICardDatabasePathProvider -{ -public: - BenchmarkCardDatabasePathProvider(QString _cardsXml, QString _customSetsDir) - : cardsXml(std::move(_cardsXml)), customSetsDir(std::move(_customSetsDir)) - { - } - - QString getCardDatabasePath() const override - { - return cardsXml; - } - - QString getCustomCardDatabasePath() const override - { - return customSetsDir; - } - - QString getTokenDatabasePath() const override - { - return QString(); - } - - QString getSpoilerCardDatabasePath() const override - { - return QString(); - } - -private: - QString cardsXml; - QString customSetsDir; -}; - -struct PassResult -{ - int enqueued = 0; - int finished = 0; - int failed = 0; - qint64 elapsedMs = 0; - QStringList failedCards; -}; - -static QList selectCardsForTemplate(CardDatabase &db, const QString &urlTemplate, int maxCards) -{ - QList selected; - const QList cards = db.getCardList().values(); - for (const CardInfoPtr &card : cards) { - if (selected.size() >= maxCards) { - break; - } - const SetToPrintingsMap &sets = card->getSets(); - if (sets.isEmpty()) { - continue; - } - const QList printings = sets.first(); - if (printings.isEmpty()) { - continue; - } - const ExactCard cardToLoad(card, printings.first()); - if (CardPictureToLoad(cardToLoad).transformUrl(urlTemplate).isEmpty()) { - continue; - } - selected.append(cardToLoad); - } - return selected; -} - -static PassResult runPass(CardPictureLoaderWorker *worker, const QList &cards, int timeoutMs) -{ - PassResult result; - result.enqueued = cards.size(); - - QEventLoop loop; - QTimer watchdog; - watchdog.setSingleShot(true); - watchdog.setInterval(timeoutMs); - QObject::connect(&watchdog, &QTimer::timeout, &loop, &QEventLoop::quit); - - QElapsedTimer clock; - QObject::connect(worker, &CardPictureLoaderWorker::imageLoaded, &loop, - [&](const ExactCard &card, const QImage &image) { - ++result.finished; - if (image.isNull()) { - ++result.failed; - if (result.failedCards.size() < 10) { - result.failedCards.append(card.getName()); - } - } - if (result.finished >= result.enqueued) { - loop.quit(); - } - }); - - clock.start(); - for (const ExactCard &card : cards) { - worker->enqueueImageLoad(card); - } - watchdog.start(); - loop.exec(); - result.elapsedMs = clock.elapsed(); - return result; -} - -struct StressResult -{ - PassResult a; - PassResult b; - int http429Count = 0; -}; - -struct LogCounters -{ - int http429 = 0; - QMutex mutex; -}; - -static std::atomic s_activeCounters{nullptr}; -static QtMessageHandler s_previousMessageHandler = nullptr; - -static void stressLogHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg) -{ - if (LogCounters *counters = s_activeCounters.load(std::memory_order_acquire); - counters && msg.contains(QStringLiteral("Too many requests from"))) { - QMutexLocker locker(&counters->mutex); - ++counters->http429; - } - if (s_previousMessageHandler) { - s_previousMessageHandler(type, context, msg); - } else { - // qInstallMessageHandler() reports the built-in handler as nullptr, so a plain `if` would - // swallow every message - including the 429 warnings this run is meant to surface. Fall - // back to Qt's message pattern written to stderr instead. - std::fprintf(stderr, "%s\n", qPrintable(qFormatLogMessage(type, context, msg))); - } -} - -// Stops a worker's thread and frees it. shutdownThread()'s bounded wait guarantees that the worker -// object was freed by its finished() -> deleteLater chain, so the thread itself can then be deleted -// safely. A worker whose thread refused to stop is left alone (and leaked) rather than freed while -// still running. -static void destroyWorker(CardPictureLoaderWorker *worker) -{ - if (!worker) { - return; - } - QThread *thread = worker->workerThread(); - if (worker->shutdownThread()) { - delete thread; - } -} - -static StressResult runStress(CardPictureLoaderWorker *workerA, - CardPictureLoaderWorker *workerB, - const QList &cards, - int timeoutMs) -{ - StressResult result; - result.a.enqueued = cards.size(); - result.b.enqueued = cards.size(); - - int completed = 0; - QMutex completedMutex; - - QEventLoop loop; - QTimer watchdog; - watchdog.setSingleShot(true); - watchdog.setInterval(timeoutMs); - QObject::connect(&watchdog, &QTimer::timeout, &loop, &QEventLoop::quit); - - const auto finishOne = [&](PassResult &pass, const ExactCard &card, const QImage &image) { - ++pass.finished; - if (image.isNull()) { - ++pass.failed; - if (pass.failedCards.size() < 10) { - pass.failedCards.append(card.getName()); - } - } - QMutexLocker locker(&completedMutex); - ++completed; - if (completed >= result.a.enqueued + result.b.enqueued) { - loop.quit(); - } - }; - - QElapsedTimer clock; - QObject::connect(workerA, &CardPictureLoaderWorker::imageLoaded, &loop, - [&](const ExactCard &card, const QImage &image) { finishOne(result.a, card, image); }); - QObject::connect(workerB, &CardPictureLoaderWorker::imageLoaded, &loop, - [&](const ExactCard &card, const QImage &image) { finishOne(result.b, card, image); }); - - // Count 429 responses as seen by the shared rate limiter. - LogCounters counters; - s_activeCounters = &counters; - s_previousMessageHandler = qInstallMessageHandler(stressLogHandler); - - clock.start(); - for (const ExactCard &card : cards) { - workerA->enqueueImageLoad(card); - workerB->enqueueImageLoad(card); - } - watchdog.start(); - loop.exec(); - const qint64 elapsedMs = clock.elapsed(); - result.a.elapsedMs = elapsedMs; - result.b.elapsedMs = elapsedMs; - - // Stop both workers before touching the counters or restoring the message handler: their - // threads log from stressLogHandler, and must not outlive the stack-local counters (which is - // guaranteed on the watchdog path, where requests and deferred retries are still pending). - destroyWorker(workerA); - destroyWorker(workerB); - - result.http429Count = counters.http429; - s_activeCounters.store(nullptr, std::memory_order_release); - qInstallMessageHandler(s_previousMessageHandler); - return result; -} - -static QString formatDuration(qint64 ms) -{ - return QStringLiteral("%1.%2 s").arg(ms / 1000).arg((ms % 1000) / 100); -} - -static bool likelyRedirects(const QString &urlTemplate) -{ - return urlTemplate.contains(QStringLiteral("api.scryfall.com")); -} - -static QString hostOf(const QString &urlTemplate) -{ - return QUrl(urlTemplate).host(); -} - -static void printUsage() -{ - std::printf("usage: picture_loader_benchmark_test --carddb [options]\n" - "\n" - "Loads card pictures from the real configured hosts (not a mock server) and\n" - "verifies the picture loader's pacing / cache 429 behavior.\n" - "\n" - "options:\n" - " --carddb cards.xml to load card data from (required)\n" - " --count cards to load per template (default 300)\n" - " --url