[PictureLoader] Improve image negotiation, cache recovery and redirects (#7703)

* Fix picture-loader cache formats, redirects and offline queue handling

* Format picture-loader changes using project style

* 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.

---------

Co-authored-by: Lukas Brübach <44814898+BruebachL@users.noreply.github.com>

tests/CMakeLists.txt

@@ -171,6 +171,8 @@ 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)
 cockatrice_add_test(NAME test_age_formatting SOURCES test_age_formatting.cpp LIBS libcockatrice_utility)

tests/image_redirect_chain_test.cpp

@@ -0,0 +1,82 @@
+#include "../cockatrice/src/interface/card_picture_loader/redirect_chain.h"
+
+#include <gtest/gtest.h>
+
+namespace
+{
+using RedirectCache = QHash<QUrl, QPair<QUrl, QDateTime>>;
+
+QUrl url(int index)
+{
+    return QUrl(QStringLiteral("https://cards.example/%1").arg(index));
+}
+
+RedirectCache chain(int hops)
+{
+    RedirectCache cache;
+    for (int index = 0; index < hops; ++index) {
+        cache.insert(url(index), {url(index + 1), QDateTime()});
+    }
+    return cache;
+}
+} // namespace
+
+TEST(ImageRedirectChain, MissingHopReturnsLastKnownUrlWithoutRemovingMappings)
+{
+    auto cache = chain(2);
+    const auto original = cache;
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(2));
+    EXPECT_EQ(cache, original);
+    EXPECT_EQ(resolveImageRedirects(url(9), cache, 10), url(9));
+}
+
+TEST(ImageRedirectChain, ExactlyMaxHopsResolvesAndKeepsCache)
+{
+    auto cache = chain(10);
+    const auto original = cache;
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(10));
+    EXPECT_EQ(cache, original);
+}
+
+TEST(ImageRedirectChain, CycleReturnsOriginalAndPurgesVisitedMappings)
+{
+    auto cache = chain(2);
+    cache.insert(url(2), {url(1), QDateTime()});
+    cache.insert(url(9), {url(10), QDateTime()});
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(0));
+    EXPECT_FALSE(cache.contains(url(0)));
+    EXPECT_FALSE(cache.contains(url(1)));
+    EXPECT_FALSE(cache.contains(url(2)));
+    EXPECT_TRUE(cache.contains(url(9)));
+}
+
+TEST(ImageRedirectChain, OverlongChainReturnsOriginalAndPurgesVisitedMappings)
+{
+    auto cache = chain(12);
+    cache.insert(url(20), {url(21), QDateTime()});
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(0));
+    for (int index = 0; index < 10; ++index) {
+        EXPECT_FALSE(cache.contains(url(index)));
+    }
+    EXPECT_TRUE(cache.contains(url(10)));
+    EXPECT_TRUE(cache.contains(url(11)));
+    EXPECT_TRUE(cache.contains(url(20)));
+}
+
+TEST(ImageRedirectChain, EmptyTargetStopsAtItsSource)
+{
+    auto cache = chain(1);
+    cache.insert(url(1), {QUrl(), QDateTime()});
+    const auto original = cache;
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 10), url(1));
+    EXPECT_EQ(cache, original);
+}
+
+TEST(ImageRedirectChain, ZeroHopsDoesNotFollowOrRemoveMappings)
+{
+    auto cache = chain(1);
+    EXPECT_EQ(resolveImageRedirects(url(5), cache, 0), url(5));
+    EXPECT_TRUE(cache.contains(url(0)));
+    EXPECT_EQ(resolveImageRedirects(url(0), cache, 0), url(0));
+    EXPECT_TRUE(cache.contains(url(0)));
+}

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());
+}