[PictureLoader] Reduce downtime between load attempts (#6020)

* [PictureLoader] Reduce downtime between load attempts

* rename some stuff

* better comments

* Fix segfault from status bar

Pass just the relevant data through the signals to the status bar, instead of passing the entire Work object.
That way the data is detached from the Work object and we won't segfault when Work self-deletes before status bar tries to use that data.

* Rename method

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

@@ -2,23 +2,22 @@
 
 PictureLoaderRequestStatusDisplayWidget::PictureLoaderRequestStatusDisplayWidget(QWidget *parent,
                                                                                  const QUrl &_url,
-                                                                                 PictureLoaderWorkerWork *worker)
+                                                                                 const CardInfoPtr &card,
+                                                                                 const QString &setName)
     : QWidget(parent)
 {
     layout = new QHBoxLayout(this);
 
-    if (worker->cardToDownload.getCard()) {
+    name = new QLabel(this);
-        name = new QLabel(this);
+    name->setText(card->getName());
-        name->setText(worker->cardToDownload.getCard()->getName());
+    setShortname = new QLabel(this);
-        setShortname = new QLabel(this);
+    setShortname->setText(setName);
-        setShortname->setText(worker->cardToDownload.getSetName());
+    providerId = new QLabel(this);
-        providerId = new QLabel(this);
+    providerId->setText(card->getProperty("uuid"));
-        providerId->setText(worker->cardToDownload.getCard()->getProperty("uuid"));
 
-        layout->addWidget(name);
+    layout->addWidget(name);
-        layout->addWidget(setShortname);
+    layout->addWidget(setShortname);
-        layout->addWidget(providerId);
+    layout->addWidget(providerId);
-    }
 
     startTime = new QLabel(QDateTime::currentDateTime().toString(), this);
     elapsedTime = new QLabel("0", this);

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

@@ -10,8 +10,10 @@ class PictureLoaderRequestStatusDisplayWidget : public QWidget
 {
     Q_OBJECT
 public:
-    PictureLoaderRequestStatusDisplayWidget(QWidget *parent, const QUrl &url, PictureLoaderWorkerWork *worker);
+    PictureLoaderRequestStatusDisplayWidget(QWidget *parent,
-    PictureLoaderWorkerWork *worker;
+                                            const QUrl &url,
+                                            const CardInfoPtr &card,
+                                            const QString &setName);
 
     void setFinished()
     {

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

@@ -38,15 +38,14 @@ void PictureLoaderStatusBar::cleanOldEntries()
     }
 }
 
-void PictureLoaderStatusBar::addQueuedImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker)
+void PictureLoaderStatusBar::addQueuedImageLoad(const QUrl &url, const CardInfoPtr &card, const QString &setName)
 {
-    loadLog->addSettingsWidget(new PictureLoaderRequestStatusDisplayWidget(loadLog, url, worker));
+    loadLog->addSettingsWidget(new PictureLoaderRequestStatusDisplayWidget(loadLog, url, card, setName));
     progressBar->setMaximum(progressBar->maximum() + 1);
 }
 
-void PictureLoaderStatusBar::addSuccessfulImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker)
+void PictureLoaderStatusBar::addSuccessfulImageLoad(const QUrl &url)
 {
-    Q_UNUSED(worker)
     progressBar->setValue(progressBar->value() + 1);
     for (PictureLoaderRequestStatusDisplayWidget *statusDisplayWidget :
          loadLog->popup->findChildren<PictureLoaderRequestStatusDisplayWidget *>()) {

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

@@ -15,8 +15,8 @@ public:
     explicit PictureLoaderStatusBar(QWidget *parent);
 
 public slots:
-    void addQueuedImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker);
+    void addQueuedImageLoad(const QUrl &url, const CardInfoPtr &card, const QString &setName);
-    void addSuccessfulImageLoad(const QUrl &url, PictureLoaderWorkerWork *worker);
+    void addSuccessfulImageLoad(const QUrl &url);
     void cleanOldEntries();
 
 private:

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

@@ -12,7 +12,10 @@
 #include <QThread>
 #include <utility>
 
-PictureLoaderWorker::PictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().getPicDownload())
+static constexpr int MAX_REQUESTS_PER_SEC = 10;
+
+PictureLoaderWorker::PictureLoaderWorker()
+    : QObject(nullptr), picDownload(SettingsCache::instance().getPicDownload()), requestQuota(MAX_REQUESTS_PER_SEC)
 {
     networkManager = new QNetworkAccessManager(this);
     // We need a timeout to ensure requests don't hang indefinitely in case of
@@ -48,7 +51,7 @@ PictureLoaderWorker::PictureLoaderWorker() : QObject(nullptr), picDownload(Setti
 
     connect(this, &PictureLoaderWorker::imageLoadEnqueued, this, &PictureLoaderWorker::handleImageLoadEnqueued);
 
-    connect(&requestTimer, &QTimer::timeout, this, &PictureLoaderWorker::processQueuedRequests);
+    connect(&requestTimer, &QTimer::timeout, this, &PictureLoaderWorker::resetRequestQuota);
     requestTimer.setInterval(1000);
     requestTimer.start();
 }
@@ -69,7 +72,8 @@ void PictureLoaderWorker::queueRequest(const QUrl &url, PictureLoaderWorkerWork
         makeRequest(url, worker);
     } else {
         requestLoadQueue.append(qMakePair(url, worker));
-        emit imageLoadQueued(url, worker);
+        emit imageLoadQueued(url, worker->cardToDownload.getCard(), worker->cardToDownload.getSetName());
+        processQueuedRequests();
     }
 }
 
@@ -78,7 +82,7 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo
     // Check for cached redirects
     QUrl cachedRedirect = getCachedRedirect(url);
     if (!cachedRedirect.isEmpty()) {
-        emit imageLoadSuccessful(url, worker);
+        emit imageLoadSuccessful(url);
         return makeRequest(cachedRedirect, worker);
     }
 
@@ -95,22 +99,34 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo
     return reply;
 }
 
+void PictureLoaderWorker::resetRequestQuota()
+{
+    requestQuota = MAX_REQUESTS_PER_SEC;
+    processQueuedRequests();
+}
+
+/**
+ * Keeps processing requests from the queue until it is empty or until the quota runs out.
+ */
 void PictureLoaderWorker::processQueuedRequests()
 {
-    for (int i = 0; i < 10; i++) {
+    while (requestQuota > 0 && processSingleRequest()) {
-        processSingleRequest();
+        --requestQuota;
     }
 }
 
 /**
- * Immediately processes a single queued request
+ * Immediately processes a single queued request. No-ops if the load queue is empty
+ * @return If a request was processed
  */
-void PictureLoaderWorker::processSingleRequest()
+bool PictureLoaderWorker::processSingleRequest()
 {
     if (!requestLoadQueue.isEmpty()) {
         auto request = requestLoadQueue.takeFirst();
         makeRequest(request.first, request.second);
+        return true;
     }
+    return false;
 }
 
 void PictureLoaderWorker::enqueueImageLoad(const CardInfoPtr &card)
@@ -132,7 +148,7 @@ void PictureLoaderWorker::handleImageLoadEnqueued(const CardInfoPtr &card)
     // try to load image from local first
     QImage image = localLoader->tryLoad(card);
     if (!image.isNull()) {
-        imageLoadedSuccessfully(card, image);
+        handleImageLoaded(card, image);
     } else {
         // queue up to load image from remote only after local loading failed
         new PictureLoaderWorkerWork(this, card);
@@ -140,10 +156,9 @@ void PictureLoaderWorker::handleImageLoadEnqueued(const CardInfoPtr &card)
 }
 
 /**
- * Called when image loading is done
+ * Called when image loading is done. Failures are indicated by an empty QImage.
- * Contrary to the name, this is called on both success and failure. Failures are indicated by an empty QImage.
  */
-void PictureLoaderWorker::imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image)
+void PictureLoaderWorker::handleImageLoaded(const CardInfoPtr &card, const QImage &image)
 {
     currentlyLoading.remove(card);
     emit imageLoaded(card, image);

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

@@ -38,8 +38,8 @@ public:
 public slots:
     QNetworkReply *makeRequest(const QUrl &url, PictureLoaderWorkerWork *workThread);
     void processQueuedRequests();
-    void processSingleRequest();
+    bool processSingleRequest();
-    void imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image);
+    void handleImageLoaded(const CardInfoPtr &card, const QImage &image);
     void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
     void removedCachedUrl(const QUrl &url);
 
@@ -52,7 +52,9 @@ private:
     static constexpr int CacheTTLInDays = 30;          // TODO: Make user configurable
     bool picDownload;
     QQueue<QPair<QUrl, PictureLoaderWorkerWork *>> requestLoadQueue;
-    QTimer requestTimer; // Timer for processing delayed requests
+
+    int requestQuota;
+    QTimer requestTimer; // Timer for refreshing request quota
 
     PictureLoaderLocal *localLoader;
     QSet<CardInfoPtr> currentlyLoading; // for deduplication purposes
@@ -63,13 +65,14 @@ private:
     void cleanStaleEntries();
 
 private slots:
+    void resetRequestQuota();
     void handleImageLoadEnqueued(const CardInfoPtr &card);
 
 signals:
     void imageLoadEnqueued(const CardInfoPtr &card);
     void imageLoaded(CardInfoPtr card, const QImage &image);
-    void imageLoadQueued(const QUrl &url, PictureLoaderWorkerWork *worker);
+    void imageLoadQueued(const QUrl &url, const CardInfoPtr &card, const QString &setName);
-    void imageLoadSuccessful(const QUrl &url, PictureLoaderWorkerWork *worker);
+    void imageLoadSuccessful(const QUrl &url);
 };
 
 #endif // PICTURE_LOADER_WORKER_H

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

@@ -22,7 +22,7 @@ PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *work
     connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest);
     connect(this, &PictureLoaderWorkerWork::urlRedirected, worker, &PictureLoaderWorker::cacheRedirect);
     connect(this, &PictureLoaderWorkerWork::cachedUrlInvalidated, worker, &PictureLoaderWorker::removedCachedUrl);
-    connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::imageLoadedSuccessfully);
+    connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::handleImageLoaded);
 
     // Hook up signals to settings
     connect(&SettingsCache::instance(), SIGNAL(picDownloadChanged()), this, SLOT(picDownloadChanged()));