From 34f5552c7d66310c94238b2280aa71420af6dadf Mon Sep 17 00:00:00 2001
From: RickyRister <42636155+RickyRister@users.noreply.github.com>
Date: Sun, 22 Jun 2025 13:19:20 -0700
Subject: [PATCH] [PictureLoader] Remove redundant startNextPicDownload calls;
 clean up (#5998)

* Add docs

* Remove unused signal

* Remove redundant startNextPicDownload

* explicitly emit signals

* delete reply first if able

* Log download fail at warning
---
 .../picture_loader_worker_work.cpp            | 25 +++++++++----------
 .../picture_loader_worker_work.h              |  1 -
 .../ui/picture_loader/picture_to_load.cpp     | 10 ++++++++
 3 files changed, 22 insertions(+), 14 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 12fbf4d80..3f508aee0 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
@@ -16,7 +16,7 @@
 QStringList PictureLoaderWorkerWork::md5Blacklist = QStringList() << "db0c48db407a907c16ade38de048a441";
 
 PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *worker, const CardInfoPtr &toLoad)
-    : QThread(nullptr), cardToDownload(toLoad)
+    : QThread(nullptr), cardToDownload(toLoad), picDownload(SettingsCache::instance().getPicDownload())
 {
     // Hook up signals to the orchestrator
     connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest,
@@ -57,6 +57,10 @@ void PictureLoaderWorkerWork::startNextPicDownload()
     }
 }
 
+/**
+ * Starts another pic download using the next possible url combination for the card.
+ * If all possibilities are exhausted, then concludes the image loading with an empty QImage.
+ */
 void PictureLoaderWorkerWork::picDownloadFailed()
 {
     /* Take advantage of short-circuiting here to call the nextUrl until one
@@ -66,14 +70,13 @@ void PictureLoaderWorkerWork::picDownloadFailed()
     if (cardToDownload.nextUrl() || cardToDownload.nextSet()) {
         startNextPicDownload();
     } else {
-        qCDebug(PictureLoaderWorkerWorkLog).nospace()
+        qCWarning(PictureLoaderWorkerWorkLog).nospace()
             << "PictureLoader: [card: " << cardToDownload.getCard()->getCorrectedName()
             << " set: " << cardToDownload.getSetName() << "]: Picture NOT found, "
             << (picDownload ? "download failed" : "downloads disabled")
             << ", no more url combinations to try: BAILING OUT";
-        imageLoaded(cardToDownload.getCard(), QImage());
+        emit imageLoaded(cardToDownload.getCard(), QImage());
     }
-    emit startLoadQueue();
 }
 
 void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
@@ -89,7 +92,7 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
 
             networkManager->cache()->remove(reply->url());
 
-            requestImageDownload(reply->url(), this);
+            emit requestImageDownload(reply->url(), this);
         } else {
             qCDebug(PictureLoaderWorkerWorkLog).nospace()
                 << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
@@ -97,7 +100,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
                 << " failed for url " << reply->url().toDisplayString() << " (" << reply->errorString() << ")";
 
             picDownloadFailed();
-            startNextPicDownload();
         }
 
         reply->deleteLater();
@@ -113,8 +115,8 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
             << " set: " << cardToDownload.getSetName() << "]: following "
             << (isFromCache ? "cached redirect" : "redirect") << " to " << redirectUrl.toDisplayString();
-        requestImageDownload(redirectUrl, this);
         reply->deleteLater();
+        emit requestImageDownload(redirectUrl, this);
         return;
     }
 
@@ -127,9 +129,8 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << " set: " << cardToDownload.getSetName()
             << "]: Picture found, but blacklisted, will consider it as not found";
 
-        picDownloadFailed();
         reply->deleteLater();
-        startNextPicDownload();
+        picDownloadFailed();
         return;
     }
 
@@ -152,10 +153,10 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
         movie.start();
         movie.stop();
 
-        imageLoaded(cardToDownload.getCard(), movie.currentImage());
+        emit imageLoaded(cardToDownload.getCard(), movie.currentImage());
         logSuccessMessage = true;
     } else if (imgReader.read(&testImage)) {
-        imageLoaded(cardToDownload.getCard(), testImage);
+        emit imageLoaded(cardToDownload.getCard(), testImage);
         logSuccessMessage = true;
     } else {
         qCDebug(PictureLoaderWorkerWorkLog).nospace()
@@ -171,8 +172,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
             << " set: " << cardToDownload.getSetName() << "]: Image successfully "
             << (isFromCache ? "loaded from cached" : "downloaded from") << " url " << reply->url().toDisplayString();
-    } else {
-        startNextPicDownload();
     }
 
     reply->deleteLater();
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 ac0796460..887479591 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
@@ -46,7 +46,6 @@ private slots:
     void picDownloadChanged();
 
 signals:
-    void startLoadQueue();
     void imageLoaded(CardInfoPtr card, const QImage &image);
     void requestImageDownload(const QUrl &url, PictureLoaderWorkerWork *instance);
 };
diff --git a/cockatrice/src/client/ui/picture_loader/picture_to_load.cpp b/cockatrice/src/client/ui/picture_loader/picture_to_load.cpp
index 208a3ed3a..9bbe50e82 100644
--- a/cockatrice/src/client/ui/picture_loader/picture_to_load.cpp
+++ b/cockatrice/src/client/ui/picture_loader/picture_to_load.cpp
@@ -84,6 +84,11 @@ void PictureToLoad::populateSetUrls()
     (void)nextUrl();
 }
 
+/**
+ * Advances the currentSet to the next set in the list. Then repopulates the url list with the urls from that set.
+ * If we are already at the end of the list, then currentSet is set to empty.
+ * @return If we are already at the end of the list
+ */
 bool PictureToLoad::nextSet()
 {
     if (!sortedSets.isEmpty()) {
@@ -95,6 +100,11 @@ bool PictureToLoad::nextSet()
     return false;
 }
 
+/**
+ * Advances the currentUrl to the next url in the list.
+ * If we are already at the end of the list, then currentUrl is set to empty.
+ * @return If we are already at the end of the list
+ */
 bool PictureToLoad::nextUrl()
 {
     if (!currentSetUrls.isEmpty()) {