From 5f142e8d525281426e5880b8f27c11e3dca4bf45 Mon Sep 17 00:00:00 2001
From: Rasmus Karlsson <rasmus.karlsson@pajlada.com>
Date: Mon, 16 Apr 2018 23:48:30 +0200
Subject: [PATCH] Add some error checks to Image::loadImage

Add default value to loadedPixmap
---
 src/messages/image.cpp      | 27 +++++++++++++++++++++++----
 src/messages/image.hpp      |  4 ++--
 src/util/networkrequest.hpp | 24 ++++++++++++++++++++----
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/src/messages/image.cpp b/src/messages/image.cpp
index c07ef71bd..0561c6b9c 100644
--- a/src/messages/image.cpp
+++ b/src/messages/image.cpp
@@ -21,14 +21,12 @@ namespace messages {
 
 Image::Image(const QString &url, qreal scale, const QString &name, const QString &tooltip,
              const QMargins &margin, bool isHat)
-    : currentPixmap(nullptr)
-    , url(url)
+    : url(url)
     , name(name)
     , tooltip(tooltip)
     , margin(margin)
     , ishat(isHat)
     , scale(scale)
-    , isLoading(false)
 {
     util::DebugCount::increase("images");
 }
@@ -65,7 +63,7 @@ void Image::loadImage()
     util::NetworkRequest req(this->getUrl());
     req.setCaller(this);
     req.setUseQuickLoadCache(true);
-    req.get([lli = this](QByteArray bytes) {
+    req.get([lli = this](QByteArray bytes) -> bool {
         QByteArray copy = QByteArray::fromRawData(bytes.constData(), bytes.length());
         QBuffer buffer(&copy);
         buffer.open(QIODevice::ReadOnly);
@@ -84,6 +82,19 @@ void Image::loadImage()
             util::DebugCount::decrease("loaded images");
         }
 
+        if (reader.imageCount() == -1) {
+            // An error occured in the reader
+            debug::Log("An error occured reading the image: '{}'", reader.errorString());
+            debug::Log("Image url: {}", lli->url);
+            return false;
+        }
+
+        if (reader.imageCount() == 0) {
+            debug::Log("Error: No images read in the buffer");
+            // No images read in the buffer. maybe a cache error?
+            return false;
+        }
+
         for (int index = 0; index < reader.imageCount(); ++index) {
             if (reader.read(&image)) {
                 auto pixmap = new QPixmap(QPixmap::fromImage(image));
@@ -101,6 +112,12 @@ void Image::loadImage()
             }
         }
 
+        if (lli->allFrames.size() != reader.imageCount()) {
+            // debug::Log("Error: Wrong amount of images read");
+            // One or more images failed to load from the buffer
+            // return false;
+        }
+
         if (lli->allFrames.size() > 1) {
             lli->animated = true;
 
@@ -116,6 +133,8 @@ void Image::loadImage()
 
         util::postToThread(
             [] { singletons::WindowManager::getInstance().layoutVisibleChatWidgets(); });
+
+        return true;
     });
 
     singletons::EmoteManager::getInstance().getGifUpdateSignal().connect([=]() {
diff --git a/src/messages/image.hpp b/src/messages/image.hpp
index f5b340e71..0203eca22 100644
--- a/src/messages/image.hpp
+++ b/src/messages/image.hpp
@@ -40,8 +40,8 @@ private:
         int duration;
     };
 
-    QPixmap *currentPixmap;
-    QPixmap *loadedPixmap;
+    QPixmap *currentPixmap = nullptr;
+    QPixmap *loadedPixmap = nullptr;
     std::vector<FrameData> allFrames;
     int currentFrame = 0;
     int currentFrameOffset = 0;
diff --git a/src/util/networkrequest.hpp b/src/util/networkrequest.hpp
index ce1b0251d..77c8fd8a1 100644
--- a/src/util/networkrequest.hpp
+++ b/src/util/networkrequest.hpp
@@ -135,9 +135,15 @@ public:
 
                     // qDebug() << "Loaded cached resource" << this->data.request.url();
 
-                    onFinished(bytes);
+                    bool success = onFinished(bytes);
 
                     cachedFile.close();
+
+                    if (!success) {
+                        // The images were not successfully loaded from the file
+                        // XXX: Invalidate the cache file so we don't attempt to load it again next
+                        // time
+                    }
                 }
             }
         }
@@ -160,7 +166,9 @@ public:
                                      return;
                                  }
 
-                                 QByteArray bytes = reply->readAll();
+                                 QByteArray readBytes = reply->readAll();
+                                 QByteArray bytes;
+                                 bytes.setRawData(readBytes.data(), readBytes.size());
                                  data.writeToCache(bytes);
                                  onFinished(bytes);
 
@@ -212,18 +220,26 @@ public:
     template <typename FinishedCallback>
     void getJSON(FinishedCallback onFinished)
     {
-        this->get([onFinished{std::move(onFinished)}](const QByteArray &bytes) {
+        this->get([onFinished{std::move(onFinished)}](const QByteArray &bytes) -> bool {
             auto object = parseJSONFromData(bytes);
             onFinished(object);
+
+            // XXX: Maybe return onFinished? For now I don't want to force onFinished to have a
+            // return value
+            return true;
         });
     }
 
     template <typename FinishedCallback>
     void getJSON2(FinishedCallback onFinished)
     {
-        this->get([onFinished{std::move(onFinished)}](const QByteArray &bytes) {
+        this->get([onFinished{std::move(onFinished)}](const QByteArray &bytes) -> bool {
             auto object = parseJSONFromData2(bytes);
             onFinished(object);
+
+            // XXX: Maybe return onFinished? For now I don't want to force onFinished to have a
+            // return value
+            return true;
         });
     }
 };