From ba393570a330be1ae48849d2bee6f45a116008ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= <44814898+BruebachL@users.noreply.github.com> Date: Sun, 8 Nov 2026 17:00:28 +0100 Subject: [PATCH] Address review: name decoded image fields, test cancelled requests Use designated initializers for DecodedImage so a field reorder cannot silently swap the image and its error string. Extract the cancelled-request sweep shared by the dispatcher and the single-request path into takeCancelledRequests(), and cover it: one status retirement per vanished work item, in queue order, with live entries kept. --- .../card_picture_loader_image_formats.h | 4 +- .../card_picture_loader_request_queue.h | 40 +++++++++ .../card_picture_loader_worker.cpp | 16 ++-- .../card_picture_loader_worker.h | 4 +- tests/CMakeLists.txt | 1 + tests/image_request_queue_test.cpp | 85 +++++++++++++++++++ 6 files changed, 137 insertions(+), 13 deletions(-) create mode 100644 cockatrice/src/interface/card_picture_loader/card_picture_loader_request_queue.h create mode 100644 tests/image_request_queue_test.cpp diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h index 139cdabbf..8c91a9592 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_image_formats.h @@ -55,14 +55,14 @@ inline DecodedImage readImage(QIODevice *device) QMovie movie(&buffer); movie.start(); movie.stop(); - return {movie.currentImage(), movie.lastErrorString()}; + return DecodedImage{.image = movie.currentImage(), .error = movie.lastErrorString()}; } QImageReader reader; reader.setDecideFormatFromContent(true); reader.setDevice(device); QImage image = reader.read(); - return {image, image.isNull() ? reader.errorString() : QString()}; + return DecodedImage{.image = image, .error = image.isNull() ? reader.errorString() : QString()}; } } // namespace CardPictureLoaderImageFormats diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_request_queue.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_request_queue.h new file mode 100644 index 000000000..e00f43002 --- /dev/null +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_request_queue.h @@ -0,0 +1,40 @@ +#ifndef COCKATRICE_CARD_PICTURE_LOADER_REQUEST_QUEUE_H +#define COCKATRICE_CARD_PICTURE_LOADER_REQUEST_QUEUE_H + +#include +#include +#include +#include +#include + +class CardPictureLoaderWorkerWork; + +/** + * @brief One queued picture request. + * + * The work pointer is null once the owning work item was destroyed; the URL still + * owes its queued status entry a completion, so it must be retired from the queue. + */ +using CardPictureLoaderRequest = QPair>; + +/** + * @brief Removes entry.first for every entry whose second (a QPointer) is null. + * + * The removed URLs are returned in queue order and the surviving entries keep + * theirs. The caller emits one completion per returned URL: every queued request + * retires exactly one status entry, including the ones whose work item vanished. + */ +template QList takeCancelledRequests(QQueue &queue) +{ + QList cancelled; + for (int i = 0; i < queue.size();) { + if (queue.at(i).second) { + ++i; + } else { + cancelled.append(queue.takeAt(i).first); + } + } + return cancelled; +} + +#endif // COCKATRICE_CARD_PICTURE_LOADER_REQUEST_QUEUE_H 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 96e859906..d376d8204 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 @@ -296,13 +296,11 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() // 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 (const QUrl &cancelled : takeCancelledRequests(requestLoadQueue)) { + emit imageRequestSucceeded(cancelled); + } for (int i = 0; i < requestLoadQueue.size();) { const auto &request = requestLoadQueue.at(i); - if (!request.second) { - const auto cancelled = requestLoadQueue.takeAt(i); - emit imageRequestSucceeded(cancelled.first); - continue; - } // 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. @@ -379,14 +377,12 @@ void CardPictureLoaderWorker::updateTimerState() bool CardPictureLoaderWorker::processSingleRequest() { QDateTime now = QDateTime::currentDateTime(); + for (const QUrl &cancelled : takeCancelledRequests(requestLoadQueue)) { + emit imageRequestSucceeded(cancelled); + } 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); - if (!request.second) { - requestLoadQueue.removeAt(i--); - emit imageRequestSucceeded(request.first); - continue; - } // 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); 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 1b3f63e59..f7bbbff02 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 @@ -1,6 +1,8 @@ #ifndef PICTURE_LOADER_WORKER_H #define PICTURE_LOADER_WORKER_H +#include "card_picture_loader_request_queue.h" + #include #include #include @@ -154,7 +156,7 @@ private: QString cacheFilePath; ///< Path to persistent redirect cache file static constexpr int CacheTTLInDays = 30; ///< Time-to-live for redirect cache entries (days) bool picDownload; ///< Whether downloading images from network is enabled - QQueue>> requestLoadQueue; ///< Queue of pending network requests + QQueue requestLoadQueue; ///< Queue of pending network requests QTimer requestTimer; ///< Timer to reset the request quota QTimer dispatchTimer; ///< Timer pacing individual network requests diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 97a9beb4b..ae90ee896 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -171,6 +171,7 @@ endfunction() # --- Unit tests ------------------------------------------------------------------ +cockatrice_add_test(NAME image_request_queue_test SOURCES image_request_queue_test.cpp) cockatrice_add_test(NAME image_redirect_chain_test SOURCES image_redirect_chain_test.cpp) cockatrice_add_test(NAME expression_test SOURCES expression_test.cpp LIBS libcockatrice_utility) cockatrice_add_test(NAME clamped_arithmetic_test SOURCES clamped_arithmetic_test.cpp LIBS libcockatrice_utility) diff --git a/tests/image_request_queue_test.cpp b/tests/image_request_queue_test.cpp new file mode 100644 index 000000000..b911483c1 --- /dev/null +++ b/tests/image_request_queue_test.cpp @@ -0,0 +1,85 @@ +#include "../cockatrice/src/interface/card_picture_loader/card_picture_loader_request_queue.h" + +#include +#include +#include +#include +#include + +namespace +{ +using TestRequest = QPair>; + +QUrl url(int index) +{ + return QUrl(QStringLiteral("https://cards.example/%1").arg(index)); +} + +TestRequest live(int index, QObject &owner) +{ + return qMakePair(url(index), QPointer(&owner)); +} + +TestRequest cancelled(int index) +{ + return qMakePair(url(index), QPointer()); +} + +QList queuedUrls(const QQueue &queue) +{ + QList urls; + for (const auto &entry : queue) { + urls.append(entry.first); + } + return urls; +} +} // namespace + +TEST(PictureLoaderRequestQueue, EmptyQueueHasNothingToRetire) +{ + QQueue queue; + EXPECT_TRUE(takeCancelledRequests(queue).isEmpty()); + EXPECT_TRUE(queue.isEmpty()); +} + +TEST(PictureLoaderRequestQueue, LiveRequestsStayQueuedInOrder) +{ + QObject first; + QObject second; + QQueue queue; + queue.enqueue(live(1, first)); + queue.enqueue(live(2, second)); + + EXPECT_TRUE(takeCancelledRequests(queue).isEmpty()); + EXPECT_EQ(queuedUrls(queue), (QList{url(1), url(2)})); +} + +TEST(PictureLoaderRequestQueue, CancelledRequestsAreRetiredInQueueOrder) +{ + QObject owner; + QQueue queue; + queue.enqueue(live(1, owner)); + queue.enqueue(cancelled(2)); + queue.enqueue(live(3, owner)); + queue.enqueue(cancelled(4)); + queue.enqueue(cancelled(5)); + + // Exactly one status entry is retired per vanished work item, in queue order. + EXPECT_EQ(takeCancelledRequests(queue), (QList{url(2), url(4), url(5)})); + // The surviving requests keep their relative order. + EXPECT_EQ(queuedUrls(queue), (QList{url(1), url(3)})); + + // Nothing is retired twice on a later sweep. + EXPECT_TRUE(takeCancelledRequests(queue).isEmpty()); + EXPECT_EQ(queuedUrls(queue), (QList{url(1), url(3)})); +} + +TEST(PictureLoaderRequestQueue, AllCancelledEmptiesTheQueue) +{ + QQueue queue; + queue.enqueue(cancelled(1)); + queue.enqueue(cancelled(2)); + + EXPECT_EQ(takeCancelledRequests(queue), (QList{url(1), url(2)})); + EXPECT_TRUE(queue.isEmpty()); +}