From 8615c4c3b0444c17311c68813065051ab2cd5ad6 Mon Sep 17 00:00:00 2001
From: RickyRister <42636155+RickyRister@users.noreply.github.com>
Date: Wed, 25 Jun 2025 06:22:51 -0700
Subject: [PATCH] [PictureLoader] Refactor PictureLoaderWorkerWork (#6009)

* Extract tryLoadImageFromReply

* Make imageIsBlackListed static

* Make picDownloadFailed private
---
 .../picture_loader_worker_work.cpp            | 84 +++++++++++--------
 .../picture_loader_worker_work.h              |  6 +-
 2 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp b/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp
index bcc5772f4..a92116721 100644
--- a/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp
+++ b/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp
@@ -13,7 +13,7 @@
 #include <QThread>
 
 // Card back returned by gatherer when card is not found
-QStringList PictureLoaderWorkerWork::md5Blacklist = QStringList() << "db0c48db407a907c16ade38de048a441";
+static const QStringList MD5_BLACKLIST = {"db0c48db407a907c16ade38de048a441"};
 
 PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *worker, const CardInfoPtr &toLoad)
     : QObject(nullptr), cardToDownload(toLoad), picDownload(SettingsCache::instance().getPicDownload())
@@ -32,8 +32,7 @@ PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *work
 
     connect(pictureLoaderThread, &QThread::started, this, &PictureLoaderWorkerWork::startNextPicDownload);
 
-    // clean up worker once loading finishes
-    connect(this, &PictureLoaderWorkerWork::imageLoaded, this, &QObject::deleteLater);
+    // clean up threads once loading finishes
     connect(this, &QObject::destroyed, pictureLoaderThread, &QThread::quit);
     connect(pictureLoaderThread, &QThread::finished, pictureLoaderThread, &QObject::deleteLater);
 
@@ -75,10 +74,16 @@ void PictureLoaderWorkerWork::picDownloadFailed()
             << " set: " << cardToDownload.getSetName() << "]: Picture NOT found, "
             << (picDownload ? "download failed" : "downloads disabled")
             << ", no more url combinations to try: BAILING OUT";
-        emit imageLoaded(cardToDownload.getCard(), QImage());
+        concludeImageLoad(QImage());
     }
 }
 
+static bool imageIsBlackListed(const QByteArray &picData)
+{
+    QString md5sum = QCryptographicHash::hash(picData, QCryptographicHash::Md5).toHex();
+    return MD5_BLACKLIST.contains(md5sum);
+}
+
 void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
 {
     bool isFromCache = reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool();
@@ -134,15 +139,34 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
         return;
     }
 
-    QImage testImage;
+    QImage image = tryLoadImageFromReply(reply);
 
-    QImageReader imgReader;
-    imgReader.setDecideFormatFromContent(true);
-    imgReader.setDevice(reply);
+    if (image.isNull()) {
+        qCDebug(PictureLoaderWorkerWorkLog).nospace()
+            << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
+            << " set: " << cardToDownload.getSetName() << "]: Possible " << (isFromCache ? "cached" : "downloaded")
+            << " picture at " << reply->url().toDisplayString() << " could not be loaded: " << reply->errorString();
 
-    bool logSuccessMessage = false;
+        reply->deleteLater();
+        picDownloadFailed();
+    } else {
+        qCDebug(PictureLoaderWorkerWorkLog).nospace()
+            << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
+            << " set: " << cardToDownload.getSetName() << "]: Image successfully "
+            << (isFromCache ? "loaded from cached" : "downloaded from") << " url " << reply->url().toDisplayString();
 
-    static const int riffHeaderSize = 12; // RIFF_HEADER_SIZE from webp/format_constants.h
+        reply->deleteLater();
+        concludeImageLoad(image);
+    }
+}
+
+/**
+ * @param reply The reply to load the image from
+ * @return The loaded image, or an empty QImage if loading failed
+ */
+QImage PictureLoaderWorkerWork::tryLoadImageFromReply(QNetworkReply *reply)
+{
+    static constexpr int riffHeaderSize = 12; // RIFF_HEADER_SIZE from webp/format_constants.h
     auto replyHeader = reply->peek(riffHeaderSize);
 
     if (replyHeader.startsWith("RIFF") && replyHeader.endsWith("WEBP")) {
@@ -153,37 +177,27 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
         movie.start();
         movie.stop();
 
-        emit imageLoaded(cardToDownload.getCard(), movie.currentImage());
-        logSuccessMessage = true;
-    } else if (imgReader.read(&testImage)) {
-        emit imageLoaded(cardToDownload.getCard(), testImage);
-        logSuccessMessage = true;
-    } else {
-        qCDebug(PictureLoaderWorkerWorkLog).nospace()
-            << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
-            << " set: " << cardToDownload.getSetName() << "]: Possible " << (isFromCache ? "cached" : "downloaded")
-            << " picture at " << reply->url().toDisplayString() << " could not be loaded: " << reply->errorString();
-
-        picDownloadFailed();
+        return movie.currentImage();
     }
 
-    if (logSuccessMessage) {
-        qCDebug(PictureLoaderWorkerWorkLog).nospace()
-            << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
-            << " set: " << cardToDownload.getSetName() << "]: Image successfully "
-            << (isFromCache ? "loaded from cached" : "downloaded from") << " url " << reply->url().toDisplayString();
-    }
+    QImageReader imgReader;
+    imgReader.setDecideFormatFromContent(true);
+    imgReader.setDevice(reply);
 
-    reply->deleteLater();
+    return imgReader.read();
+}
+
+/**
+ * Call this method when the image has finished being loaded.
+ * @param image The image that was loaded. Empty QImage indicates failure.
+ */
+void PictureLoaderWorkerWork::concludeImageLoad(const QImage &image)
+{
+    emit imageLoaded(cardToDownload.getCard(), image);
+    this->deleteLater();
 }
 
 void PictureLoaderWorkerWork::picDownloadChanged()
 {
     picDownload = SettingsCache::instance().getPicDownload();
 }
-
-bool PictureLoaderWorkerWork::imageIsBlackListed(const QByteArray &picData)
-{
-    QString md5sum = QCryptographicHash::hash(picData, QCryptographicHash::Md5).toHex();
-    return md5Blacklist.contains(md5sum);
-}
\ No newline at end of file
diff --git a/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.h b/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.h
index 5db97938e..8bccecd6d 100644
--- a/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.h
+++ b/cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.h
@@ -31,16 +31,16 @@ public:
 
 public slots:
     void picDownloadFinished(QNetworkReply *reply);
-    void picDownloadFailed();
 
 private:
-    static QStringList md5Blacklist;
     QThread *pictureLoaderThread;
     QNetworkAccessManager *networkManager;
     bool picDownload, downloadRunning, loadQueueRunning;
 
     void startNextPicDownload();
-    bool imageIsBlackListed(const QByteArray &);
+    void picDownloadFailed();
+    QImage tryLoadImageFromReply(QNetworkReply *reply);
+    void concludeImageLoad(const QImage &image);
 
 private slots:
     void picDownloadChanged();