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.

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
 

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 <QList>
+#include <QPair>
+#include <QPointer>
+#include <QQueue>
+#include <QUrl>
+
+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<QUrl, QPointer<CardPictureLoaderWorkerWork>>;
+
+/**
+ * @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 <typename Entry> QList<QUrl> takeCancelledRequests(QQueue<Entry> &queue)
+{
+    QList<QUrl> 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

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);

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 <QHash>
 #include <QLoggingCategory>
 #include <QObject>
@@ -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<QPair<QUrl, QPointer<CardPictureLoaderWorkerWork>>> requestLoadQueue; ///< Queue of pending network requests
+    QQueue<CardPictureLoaderRequest> requestLoadQueue; ///< Queue of pending network requests
 
     QTimer requestTimer;                    ///< Timer to reset the request quota
     QTimer dispatchTimer;                   ///< Timer pacing individual network requests

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)

tests/image_request_queue_test.cpp

@@ -0,0 +1,85 @@
+#include "../cockatrice/src/interface/card_picture_loader/card_picture_loader_request_queue.h"
+
+#include <QObject>
+#include <QPointer>
+#include <QQueue>
+#include <QUrl>
+#include <gtest/gtest.h>
+
+namespace
+{
+using TestRequest = QPair<QUrl, QPointer<QObject>>;
+
+QUrl url(int index)
+{
+    return QUrl(QStringLiteral("https://cards.example/%1").arg(index));
+}
+
+TestRequest live(int index, QObject &owner)
+{
+    return qMakePair(url(index), QPointer<QObject>(&owner));
+}
+
+TestRequest cancelled(int index)
+{
+    return qMakePair(url(index), QPointer<QObject>());
+}
+
+QList<QUrl> queuedUrls(const QQueue<TestRequest> &queue)
+{
+    QList<QUrl> urls;
+    for (const auto &entry : queue) {
+        urls.append(entry.first);
+    }
+    return urls;
+}
+} // namespace
+
+TEST(PictureLoaderRequestQueue, EmptyQueueHasNothingToRetire)
+{
+    QQueue<TestRequest> queue;
+    EXPECT_TRUE(takeCancelledRequests(queue).isEmpty());
+    EXPECT_TRUE(queue.isEmpty());
+}
+
+TEST(PictureLoaderRequestQueue, LiveRequestsStayQueuedInOrder)
+{
+    QObject first;
+    QObject second;
+    QQueue<TestRequest> queue;
+    queue.enqueue(live(1, first));
+    queue.enqueue(live(2, second));
+
+    EXPECT_TRUE(takeCancelledRequests(queue).isEmpty());
+    EXPECT_EQ(queuedUrls(queue), (QList<QUrl>{url(1), url(2)}));
+}
+
+TEST(PictureLoaderRequestQueue, CancelledRequestsAreRetiredInQueueOrder)
+{
+    QObject owner;
+    QQueue<TestRequest> 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<QUrl>{url(2), url(4), url(5)}));
+    // The surviving requests keep their relative order.
+    EXPECT_EQ(queuedUrls(queue), (QList<QUrl>{url(1), url(3)}));
+
+    // Nothing is retired twice on a later sweep.
+    EXPECT_TRUE(takeCancelledRequests(queue).isEmpty());
+    EXPECT_EQ(queuedUrls(queue), (QList<QUrl>{url(1), url(3)}));
+}
+
+TEST(PictureLoaderRequestQueue, AllCancelledEmptiesTheQueue)
+{
+    QQueue<TestRequest> queue;
+    queue.enqueue(cancelled(1));
+    queue.enqueue(cancelled(2));
+
+    EXPECT_EQ(takeCancelledRequests(queue), (QList<QUrl>{url(1), url(2)}));
+    EXPECT_TRUE(queue.isEmpty());
+}