Refactor/Cleanup NetworkRequest and Related Code (#4633)

Cleanup unused code (Twitch v5)
Add json methods that simplify sending JSON. This also sets the Accepts header, since here, when JSON is sent, only JSON is accepted as a response (this is only done in Helix).
Clarify helix request creations
Cleaned some clang-tidy suggestions in Network{Request,Private}

src/common/NetworkPrivate.cpp

@@ -72,12 +72,12 @@ void writeToCache(const std::shared_ptr<NetworkData> &data,
     }
 }
 
-void loadUncached(const std::shared_ptr<NetworkData> &data)
+void loadUncached(std::shared_ptr<NetworkData> &&data)
 {
     DebugCount::increase("http request started");
 
     NetworkRequester requester;
-    NetworkWorker *worker = new NetworkWorker;
+    auto *worker = new NetworkWorker;
 
     worker->moveToThread(&NetworkManager::workerThread);
 
@@ -89,7 +89,7 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
             data->timer_->start(data->timeoutMS_);
         }
 
-        auto reply = [&]() -> QNetworkReply * {
+        auto *reply = [&]() -> QNetworkReply * {
             switch (data->requestType_)
             {
                 case NetworkRequestType::Get:
@@ -245,12 +245,16 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
             if (data->onSuccess_)
             {
                 if (data->executeConcurrently_)
+                {
                     QtConcurrent::run([onSuccess = std::move(data->onSuccess_),
                                        result = std::move(result)] {
                         onSuccess(result);
                     });
+                }
                 else
+                {
                     data->onSuccess_(result);
+                }
             }
             // log("finished {}", data->request_.url().toString());
 
@@ -276,11 +280,15 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
             if (data->finally_)
             {
                 if (data->executeConcurrently_)
+                {
                     QtConcurrent::run([finally = std::move(data->finally_)] {
                         finally();
                     });
+                }
                 else
+                {
                     data->finally_();
+                }
             }
         };
 
@@ -316,87 +324,87 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
 }
 
 // First tried to load cached, then uncached.
-void loadCached(const std::shared_ptr<NetworkData> &data)
+void loadCached(std::shared_ptr<NetworkData> &&data)
 {
     QFile cachedFile(getPaths()->cacheDirectory() + "/" + data->getHash());
 
     if (!cachedFile.exists() || !cachedFile.open(QIODevice::ReadOnly))
     {
         // File didn't exist OR File could not be opened
-        loadUncached(data);
+        loadUncached(std::move(data));
         return;
     }
-    else
-    {
-        // XXX: check if bytes is empty?
-        QByteArray bytes = cachedFile.readAll();
-        NetworkResult result(bytes, 200);
 
-        qCDebug(chatterinoHTTP)
-            << QString("%1 [CACHED] 200 %2")
-                   .arg(networkRequestTypes.at(int(data->requestType_)),
-                        data->request_.url().toString());
-        if (data->onSuccess_)
+    // XXX: check if bytes is empty?
+    QByteArray bytes = cachedFile.readAll();
+    NetworkResult result(bytes, 200);
+
+    qCDebug(chatterinoHTTP)
+        << QString("%1 [CACHED] 200 %2")
+               .arg(networkRequestTypes.at(int(data->requestType_)),
+                    data->request_.url().toString());
+    if (data->onSuccess_)
+    {
+        if (data->executeConcurrently_ || isGuiThread())
         {
-            if (data->executeConcurrently_ || isGuiThread())
+            // XXX: If outcome is Failure, we should invalidate the cache file
+            // somehow/somewhere
+            /*auto outcome =*/
+            if (data->hasCaller_ && !data->caller_.get())
             {
-                // XXX: If outcome is Failure, we should invalidate the cache file
-                // somehow/somewhere
-                /*auto outcome =*/
+                return;
+            }
+            data->onSuccess_(result);
+        }
+        else
+        {
+            postToThread([data, result]() {
                 if (data->hasCaller_ && !data->caller_.get())
                 {
                     return;
                 }
+
                 data->onSuccess_(result);
-            }
-            else
-            {
-                postToThread([data, result]() {
-                    if (data->hasCaller_ && !data->caller_.get())
-                    {
-                        return;
-                    }
-
-                    data->onSuccess_(result);
-                });
-            }
+            });
         }
+    }
 
-        if (data->finally_)
+    if (data->finally_)
+    {
+        if (data->executeConcurrently_ || isGuiThread())
         {
-            if (data->executeConcurrently_ || isGuiThread())
+            if (data->hasCaller_ && !data->caller_.get())
             {
+                return;
+            }
+
+            data->finally_();
+        }
+        else
+        {
+            postToThread([data]() {
                 if (data->hasCaller_ && !data->caller_.get())
                 {
                     return;
                 }
 
                 data->finally_();
-            }
-            else
-            {
-                postToThread([data]() {
-                    if (data->hasCaller_ && !data->caller_.get())
-                    {
-                        return;
-                    }
-
-                    data->finally_();
-                });
-            }
+            });
         }
     }
 }
 
-void load(const std::shared_ptr<NetworkData> &data)
+void load(std::shared_ptr<NetworkData> &&data)
 {
     if (data->cache_)
     {
-        QtConcurrent::run(loadCached, data);
+        QtConcurrent::run([data = std::move(data)]() mutable {
+            loadCached(std::move(data));
+        });
     }
     else
     {
-        loadUncached(data);
+        loadUncached(std::move(data));
     }
 }
 

src/common/NetworkPrivate.hpp

@@ -68,6 +68,6 @@ private:
     QString hash_;
 };
 
-void load(const std::shared_ptr<NetworkData> &data);
+void load(std::shared_ptr<NetworkData> &&data);
 
 }  // namespace chatterino

src/common/NetworkRequest.cpp

@@ -1,14 +1,8 @@
 #include "common/NetworkRequest.hpp"
 
 #include "common/NetworkPrivate.hpp"
-#include "common/Outcome.hpp"
 #include "common/QLogging.hpp"
 #include "common/Version.hpp"
-#include "debug/AssertInGuiThread.hpp"
-#include "providers/twitch/TwitchCommon.hpp"
-#include "singletons/Paths.hpp"
-#include "util/DebugCount.hpp"
-#include "util/PostToThread.hpp"
 
 #include <QDebug>
 #include <QFile>
@@ -28,7 +22,7 @@ NetworkRequest::NetworkRequest(const std::string &url,
     this->initializeDefaultValues();
 }
 
-NetworkRequest::NetworkRequest(QUrl url, NetworkRequestType requestType)
+NetworkRequest::NetworkRequest(const QUrl &url, NetworkRequestType requestType)
     : data(new NetworkData)
 {
     this->data->request_.setUrl(url);
@@ -37,10 +31,7 @@ NetworkRequest::NetworkRequest(QUrl url, NetworkRequestType requestType)
     this->initializeDefaultValues();
 }
 
-NetworkRequest::~NetworkRequest()
-{
-    //assert(!this->data || this->executed_);
-}
+NetworkRequest::~NetworkRequest() = default;
 
 NetworkRequest NetworkRequest::type(NetworkRequestType newRequestType) &&
 {
@@ -63,25 +54,25 @@ NetworkRequest NetworkRequest::caller(const QObject *caller) &&
 
 NetworkRequest NetworkRequest::onReplyCreated(NetworkReplyCreatedCallback cb) &&
 {
-    this->data->onReplyCreated_ = cb;
+    this->data->onReplyCreated_ = std::move(cb);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::onError(NetworkErrorCallback cb) &&
 {
-    this->data->onError_ = cb;
+    this->data->onError_ = std::move(cb);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::onSuccess(NetworkSuccessCallback cb) &&
 {
-    this->data->onSuccess_ = cb;
+    this->data->onSuccess_ = std::move(cb);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::finally(NetworkFinallyCallback cb) &&
 {
-    this->data->finally_ = cb;
+    this->data->finally_ = std::move(cb);
     return std::move(*this);
 }
 
@@ -106,6 +97,13 @@ NetworkRequest NetworkRequest::header(const char *headerName,
     return std::move(*this);
 }
 
+NetworkRequest NetworkRequest::header(QNetworkRequest::KnownHeaders header,
+                                      const QVariant &value) &&
+{
+    this->data->request_.setHeader(header, value);
+    return std::move(*this);
+}
+
 NetworkRequest NetworkRequest::headerList(
     const std::vector<std::pair<QByteArray, QByteArray>> &headers) &&
 {
@@ -129,20 +127,6 @@ NetworkRequest NetworkRequest::concurrent() &&
     return std::move(*this);
 }
 
-NetworkRequest NetworkRequest::authorizeTwitchV5(const QString &clientID,
-                                                 const QString &oauthToken) &&
-{
-    // TODO: make two overloads, with and without oauth token
-    auto tmp = std::move(*this)
-                   .header("Client-ID", clientID)
-                   .header("Accept", "application/vnd.twitchtv.v5+json");
-
-    if (!oauthToken.isEmpty())
-        return std::move(tmp).header("Authorization", "OAuth " + oauthToken);
-    else
-        return tmp;
-}
-
 NetworkRequest NetworkRequest::multiPart(QHttpMultiPart *payload) &&
 {
     payload->setParent(this->data->lifetimeManager_);
@@ -207,10 +191,28 @@ void NetworkRequest::initializeDefaultValues()
     this->data->request_.setRawHeader("User-Agent", userAgent);
 }
 
-// Helper creator functions
-NetworkRequest NetworkRequest::twitchRequest(QUrl url)
+NetworkRequest NetworkRequest::json(const QJsonArray &root) &&
 {
-    return NetworkRequest(url).authorizeTwitchV5(getDefaultClientID());
+    return std::move(*this).json(QJsonDocument(root));
+}
+
+NetworkRequest NetworkRequest::json(const QJsonObject &root) &&
+{
+    return std::move(*this).json(QJsonDocument(root));
+}
+
+NetworkRequest NetworkRequest::json(const QJsonDocument &document) &&
+{
+    return std::move(*this).json(document.toJson(QJsonDocument::Compact));
+}
+
+NetworkRequest NetworkRequest::json(const QByteArray &payload) &&
+{
+    return std::move(*this)
+        .payload(payload)
+        .header(QNetworkRequest::ContentTypeHeader, "application/json")
+        .header(QNetworkRequest::ContentLengthHeader, payload.length())
+        .header("Accept", "application/json");
 }
 
 }  // namespace chatterino

src/common/NetworkRequest.hpp

@@ -6,6 +6,10 @@
 
 #include <memory>
 
+class QJsonArray;
+class QJsonObject;
+class QJsonDocument;
+
 namespace chatterino {
 
 struct NetworkData;
@@ -24,8 +28,8 @@ public:
     explicit NetworkRequest(
         const std::string &url,
         NetworkRequestType requestType = NetworkRequestType::Get);
-    explicit NetworkRequest(
-        QUrl url, NetworkRequestType requestType = NetworkRequestType::Get);
+    explicit NetworkRequest(const QUrl &url, NetworkRequestType requestType =
+                                                 NetworkRequestType::Get);
 
     // Enable move
     NetworkRequest(NetworkRequest &&other) = default;
@@ -54,23 +58,25 @@ public:
     NetworkRequest header(const char *headerName, const char *value) &&;
     NetworkRequest header(const char *headerName, const QByteArray &value) &&;
     NetworkRequest header(const char *headerName, const QString &value) &&;
+    NetworkRequest header(QNetworkRequest::KnownHeaders header,
+                          const QVariant &value) &&;
     NetworkRequest headerList(
         const std::vector<std::pair<QByteArray, QByteArray>> &headers) &&;
     NetworkRequest timeout(int ms) &&;
     NetworkRequest concurrent() &&;
-    NetworkRequest authorizeTwitchV5(const QString &clientID,
-                                     const QString &oauthToken = QString()) &&;
     NetworkRequest multiPart(QHttpMultiPart *payload) &&;
     /**
      * This will change `RedirectPolicyAttribute`.
      * `QNetworkRequest`'s defaults are used by default (Qt 5: no-follow, Qt 6: follow).
      */
     NetworkRequest followRedirects(bool on) &&;
+    NetworkRequest json(const QJsonObject &root) &&;
+    NetworkRequest json(const QJsonArray &root) &&;
+    NetworkRequest json(const QJsonDocument &document) &&;
+    NetworkRequest json(const QByteArray &payload) &&;
 
     void execute();
 
-    static NetworkRequest twitchRequest(QUrl url);
-
 private:
     void initializeDefaultValues();
 };