[PictureLoader] Properly run reply processing on Work's thread

cockatrice/src/client/ui/picture_loader/picture_loader_worker.cpp

@@ -92,34 +92,7 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo
     QNetworkReply *reply = networkManager->get(req);
 
     // Connect reply handling
-    connect(reply, &QNetworkReply::finished, this, [this, reply, url, worker]() {
-        QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
-        if (redirectTarget.isValid()) {
-            QUrl redirectUrl = redirectTarget.toUrl();
-            if (redirectUrl.isRelative()) {
-                redirectUrl = url.resolved(redirectUrl);
-            }
-            cacheRedirect(url, redirectUrl);
-        }
-
-        if (reply->error() == QNetworkReply::NoError) {
-            worker->picDownloadFinished(reply);
-            emit imageLoadSuccessful(url, worker);
-
-            // If we hit a cached image, we get to make another request for free.
-            if (reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool()) {
-                if (!requestLoadQueue.isEmpty()) {
-                    auto request = requestLoadQueue.takeFirst();
-                    makeRequest(request.first, request.second);
-                }
-            }
-        } else if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 429) {
-            qInfo() << "Too many requests.";
-        } else {
-            worker->picDownloadFinished(reply);
-        }
-        reply->deleteLater();
-    });
+    connect(reply, &QNetworkReply::finished, worker, [reply, worker] { worker->handleNetworkReply(reply); });
 
     return reply;
 }
@@ -127,11 +100,19 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo
 void PictureLoaderWorker::processQueuedRequests()
 {
     for (int i = 0; i < 10; i++) {
+        processSingleRequest();
+    }
+}
+
+/**
+ * Immediately processes a single queued request
+ */
+void PictureLoaderWorker::processSingleRequest()
+{
     if (!requestLoadQueue.isEmpty()) {
         auto request = requestLoadQueue.takeFirst();
         makeRequest(request.first, request.second);
     }
-    }
 }
 
 void PictureLoaderWorker::enqueueImageLoad(const CardInfoPtr &card)
@@ -176,6 +157,11 @@ void PictureLoaderWorker::cacheRedirect(const QUrl &originalUrl, const QUrl &red
     // saveRedirectCache();
 }
 
+void PictureLoaderWorker::removedCachedUrl(const QUrl &url)
+{
+    networkManager->cache()->remove(url);
+}
+
 QUrl PictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const
 {
     if (redirectCache.contains(originalUrl)) {

cockatrice/src/client/ui/picture_loader/picture_loader_worker.h

@@ -38,7 +38,10 @@ public:
 public slots:
     QNetworkReply *makeRequest(const QUrl &url, PictureLoaderWorkerWork *workThread);
     void processQueuedRequests();
+    void processSingleRequest();
     void imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image);
+    void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
+    void removedCachedUrl(const QUrl &url);
 
 private:
     static QStringList md5Blacklist;
@@ -56,7 +59,6 @@ private:
     PictureLoaderLocal *localLoader;
     QSet<CardInfoPtr> currentlyLoading; // for deduplication purposes
 
-    void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
     QUrl getCachedRedirect(const QUrl &originalUrl) const;
     void loadRedirectCache();
     void saveRedirectCache() const;

cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp

@@ -19,10 +19,11 @@ PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *work
     : QObject(nullptr), cardToDownload(toLoad), picDownload(SettingsCache::instance().getPicDownload())
 {
     // Hook up signals to the orchestrator
-    connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest,
-            Qt::QueuedConnection);
-    connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::imageLoadedSuccessfully,
-            Qt::QueuedConnection);
+    connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest);
+    connect(this, &PictureLoaderWorkerWork::cachedImageHit, worker, &PictureLoaderWorker::processSingleRequest);
+    connect(this, &PictureLoaderWorkerWork::urlRedirected, worker, &PictureLoaderWorker::cacheRedirect);
+    connect(this, &PictureLoaderWorkerWork::cachedUrlInvalidated, worker, &PictureLoaderWorker::removedCachedUrl);
+    connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::imageLoadedSuccessfully);
 
     // Hook up signals to settings
     connect(&SettingsCache::instance(), SIGNAL(picDownloadChanged()), this, SLOT(picDownloadChanged()));
@@ -78,24 +79,56 @@ void PictureLoaderWorkerWork::picDownloadFailed()
     }
 }
 
+/**
+ *
+ * @param reply The reply. Takes ownership of the object
+ */
+void PictureLoaderWorkerWork::handleNetworkReply(QNetworkReply *reply)
+{
+    QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
+    if (redirectTarget.isValid()) {
+        QUrl url = reply->request().url();
+        QUrl redirectUrl = redirectTarget.toUrl();
+        if (redirectUrl.isRelative()) {
+            redirectUrl = url.resolved(redirectUrl);
+        }
+        emit urlRedirected(url, redirectUrl);
+    }
+
+    if (reply->error()) {
+        handleFailedReply(reply);
+    } else {
+        handleSuccessfulReply(reply);
+
+        // If we hit a cached image, we get to make another request for free.
+        if (reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool()) {
+            emit cachedImageHit();
+        }
+    }
+
+    reply->deleteLater();
+}
+
 static bool imageIsBlackListed(const QByteArray &picData)
 {
     QString md5sum = QCryptographicHash::hash(picData, QCryptographicHash::Md5).toHex();
     return MD5_BLACKLIST.contains(md5sum);
 }
 
-void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
+void PictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply)
 {
+    if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 429) {
+        qCWarning(PictureLoaderWorkerWorkLog) << "Too many requests.";
+    } else {
         bool isFromCache = reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool();
 
-    if (reply->error()) {
         if (isFromCache) {
             qCDebug(PictureLoaderWorkerWorkLog).nospace()
                 << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
                 << " set: " << cardToDownload.getSetName() << "]: Removing corrupted cache file for url "
                 << reply->url().toDisplayString() << " and retrying (" << reply->errorString() << ")";
 
-            networkManager->cache()->remove(reply->url());
+            emit cachedUrlInvalidated(reply->url());
 
             emit requestImageDownload(reply->url(), this);
         } else {
@@ -106,10 +139,12 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
 
             picDownloadFailed();
         }
-
-        reply->deleteLater();
-        return;
     }
+}
+
+void PictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply)
+{
+    bool isFromCache = reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool();
 
     // List of status codes from https://doc.qt.io/qt-6/qnetworkreply.html#redirected
     int statusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
@@ -120,7 +155,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
             << " set: " << cardToDownload.getSetName() << "]: following "
             << (isFromCache ? "cached redirect" : "redirect") << " to " << redirectUrl.toDisplayString();
-        reply->deleteLater();
         emit requestImageDownload(redirectUrl, this);
         return;
     }
@@ -134,7 +168,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << " set: " << cardToDownload.getSetName()
             << "]: Picture found, but blacklisted, will consider it as not found";
 
-        reply->deleteLater();
         picDownloadFailed();
         return;
     }
@@ -147,7 +180,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << " set: " << cardToDownload.getSetName() << "]: Possible " << (isFromCache ? "cached" : "downloaded")
             << " picture at " << reply->url().toDisplayString() << " could not be loaded: " << reply->errorString();
 
-        reply->deleteLater();
         picDownloadFailed();
     } else {
         qCDebug(PictureLoaderWorkerWorkLog).nospace()
@@ -155,7 +187,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << " set: " << cardToDownload.getSetName() << "]: Image successfully "
             << (isFromCache ? "loaded from cached" : "downloaded from") << " url " << reply->url().toDisplayString();
 
-        reply->deleteLater();
         concludeImageLoad(image);
     }
 }

cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.h

@@ -30,7 +30,7 @@ public:
     PictureToLoad cardToDownload;
 
 public slots:
-    void picDownloadFinished(QNetworkReply *reply);
+    void handleNetworkReply(QNetworkReply *reply);
 
 private:
     QThread *pictureLoaderThread;
@@ -39,6 +39,8 @@ private:
 
     void startNextPicDownload();
     void picDownloadFailed();
+    void handleFailedReply(const QNetworkReply *reply);
+    void handleSuccessfulReply(QNetworkReply *reply);
     QImage tryLoadImageFromReply(QNetworkReply *reply);
     void concludeImageLoad(const QImage &image);
 
@@ -53,6 +55,13 @@ signals:
      */
     void imageLoaded(CardInfoPtr card, const QImage &image);
     void requestImageDownload(const QUrl &url, PictureLoaderWorkerWork *instance);
+
+    /**
+     * We hit a cached image from the network cache; we get to make another request for free.
+     */
+    void cachedImageHit();
+    void urlRedirected(const QUrl &originalUrl, const QUrl &redirectUrl);
+    void cachedUrlInvalidated(const QUrl &url);
 };
 
 #endif // PICTURE_LOADER_WORKER_WORK_H