From 8c8eabf7ac1920b47792b26790f3646cb6693ec0 Mon Sep 17 00:00:00 2001 From: flow Date: Thu, 21 Apr 2022 22:12:14 -0300 Subject: refactor: organize a little more the code in launcher/net/ This also reduces some code duplication by using some Task logic in NetAction. --- launcher/InstanceImportTask.cpp | 14 +- launcher/minecraft/AssetsUtils.cpp | 2 +- launcher/net/ByteArraySink.h | 67 ++++--- launcher/net/Download.cpp | 60 +++---- launcher/net/Download.h | 14 +- launcher/net/FileSink.cpp | 50 +++--- launcher/net/FileSink.h | 36 ++-- launcher/net/MetaCacheSink.cpp | 22 +-- launcher/net/MetaCacheSink.h | 27 +-- launcher/net/NetAction.h | 127 +++++-------- launcher/net/NetJob.cpp | 270 ++++++++++++++-------------- launcher/net/NetJob.h | 109 +++++------ launcher/net/Sink.h | 54 +++--- launcher/screenshots/ImgurAlbumCreation.cpp | 15 +- launcher/screenshots/ImgurAlbumCreation.h | 12 +- launcher/screenshots/ImgurUpload.cpp | 13 +- launcher/screenshots/ImgurUpload.h | 2 +- launcher/tasks/Task.h | 4 +- launcher/translations/TranslationsModel.cpp | 2 +- 19 files changed, 433 insertions(+), 467 deletions(-) diff --git a/launcher/InstanceImportTask.cpp b/launcher/InstanceImportTask.cpp index 1a13c997..fc3432c1 100644 --- a/launcher/InstanceImportTask.cpp +++ b/launcher/InstanceImportTask.cpp @@ -14,27 +14,25 @@ */ #include "InstanceImportTask.h" +#include +#include "Application.h" #include "BaseInstance.h" #include "FileSystem.h" -#include "Application.h" #include "MMCZip.h" #include "NullInstance.h" -#include "settings/INISettingsObject.h" +#include "icons/IconList.h" #include "icons/IconUtils.h" -#include +#include "settings/INISettingsObject.h" // FIXME: this does not belong here, it's Minecraft/Flame specific +#include +#include "Json.h" #include "minecraft/MinecraftInstance.h" #include "minecraft/PackProfile.h" #include "modplatform/flame/FileResolvingTask.h" #include "modplatform/flame/PackManifest.h" -#include "Json.h" -#include #include "modplatform/technic/TechnicPackProcessor.h" -#include "icons/IconList.h" -#include "Application.h" - InstanceImportTask::InstanceImportTask(const QUrl sourceUrl) { m_sourceUrl = sourceUrl; diff --git a/launcher/minecraft/AssetsUtils.cpp b/launcher/minecraft/AssetsUtils.cpp index 7290aeb4..281f730f 100644 --- a/launcher/minecraft/AssetsUtils.cpp +++ b/launcher/minecraft/AssetsUtils.cpp @@ -297,7 +297,7 @@ NetAction::Ptr AssetObject::getDownloadAction() auto rawHash = QByteArray::fromHex(hash.toLatin1()); objectDL->addValidator(new Net::ChecksumValidator(QCryptographicHash::Sha1, rawHash)); } - objectDL->m_total_progress = size; + objectDL->setProgress(objectDL->getProgress(), size); return objectDL; } return nullptr; diff --git a/launcher/net/ByteArraySink.h b/launcher/net/ByteArraySink.h index 20e6764c..75a66574 100644 --- a/launcher/net/ByteArraySink.h +++ b/launcher/net/ByteArraySink.h @@ -3,60 +3,59 @@ #include "Sink.h" namespace Net { + /* * Sink object for downloads that uses an external QByteArray it doesn't own as a target. */ -class ByteArraySink : public Sink -{ -public: - ByteArraySink(QByteArray *output) - :m_output(output) - { - // nil - }; +class ByteArraySink : public Sink { + public: + ByteArraySink(QByteArray* output) : m_output(output){}; - virtual ~ByteArraySink() - { - // nil - } + virtual ~ByteArraySink() = default; -public: - JobStatus init(QNetworkRequest & request) override + public: + auto init(QNetworkRequest& request) -> Task::State override { + if(!m_output) + return Task::State::Failed; + m_output->clear(); - if(initAllValidators(request)) - return Job_InProgress; - return Job_Failed; + if (initAllValidators(request)) + return Task::State::Running; + return Task::State::Failed; }; - JobStatus write(QByteArray & data) override + auto write(QByteArray& data) -> Task::State override { + if(!m_output) + return Task::State::Failed; + m_output->append(data); - if(writeAllValidators(data)) - return Job_InProgress; - return Job_Failed; + if (writeAllValidators(data)) + return Task::State::Running; + return Task::State::Failed; } - JobStatus abort() override + auto abort() -> Task::State override { + if(!m_output) + return Task::State::Failed; + m_output->clear(); failAllValidators(); - return Job_Failed; + return Task::State::Failed; } - JobStatus finalize(QNetworkReply &reply) override + auto finalize(QNetworkReply& reply) -> Task::State override { - if(finalizeAllValidators(reply)) - return Job_Finished; - return Job_Failed; + if (finalizeAllValidators(reply)) + return Task::State::Succeeded; + return Task::State::Failed; } - bool hasLocalData() override - { - return false; - } + auto hasLocalData() -> bool override { return false; } -private: - QByteArray * m_output; + private: + QByteArray* m_output; }; -} +} // namespace Net diff --git a/launcher/net/Download.cpp b/launcher/net/Download.cpp index 65cc8f67..5b5a04db 100644 --- a/launcher/net/Download.cpp +++ b/launcher/net/Download.cpp @@ -30,7 +30,7 @@ namespace Net { Download::Download() : NetAction() { - m_status = Job_NotStarted; + m_state = State::Inactive; } Download::Ptr Download::makeCached(QUrl url, MetaEntryPtr entry, Options options) @@ -68,29 +68,29 @@ void Download::addValidator(Validator* v) m_sink->addValidator(v); } -void Download::startImpl() +void Download::executeTask() { - if (m_status == Job_Aborted) { + if (getState() == Task::State::AbortedByUser) { qWarning() << "Attempt to start an aborted Download:" << m_url.toString(); emit aborted(m_index_within_job); return; } + QNetworkRequest request(m_url); - m_status = m_sink->init(request); - switch (m_status) { - case Job_Finished: + m_state = m_sink->init(request); + switch (m_state) { + case State::Succeeded: emit succeeded(m_index_within_job); qDebug() << "Download cache hit " << m_url.toString(); return; - case Job_InProgress: + case State::Running: qDebug() << "Downloading " << m_url.toString(); break; - case Job_Failed_Proceed: // this is meaningless in this context. We do need a sink. - case Job_NotStarted: - case Job_Failed: + case State::Inactive: + case State::Failed: emit failed(m_index_within_job); return; - case Job_Aborted: + case State::AbortedByUser: return; } @@ -111,8 +111,7 @@ void Download::startImpl() void Download::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) { - m_total_progress = bytesTotal; - m_progress = bytesReceived; + setProgress(bytesReceived, bytesTotal); emit netActionProgress(m_index_within_job, bytesReceived, bytesTotal); } @@ -120,17 +119,17 @@ void Download::downloadError(QNetworkReply::NetworkError error) { if (error == QNetworkReply::OperationCanceledError) { qCritical() << "Aborted " << m_url.toString(); - m_status = Job_Aborted; + m_state = State::AbortedByUser; } else { if (m_options & Option::AcceptLocalFiles) { if (m_sink->hasLocalData()) { - m_status = Job_Failed_Proceed; + m_state = State::Succeeded; return; } } // error happened during download. qCritical() << "Failed " << m_url.toString() << " with reason " << error; - m_status = Job_Failed; + m_state = State::Failed; } } @@ -194,7 +193,8 @@ bool Download::handleRedirect() m_url = QUrl(redirect.toString()); qDebug() << "Following redirect to " << m_url.toString(); - start(m_network); + startAction(m_network); + return true; } @@ -207,19 +207,20 @@ void Download::downloadFinished() } // if the download failed before this point ... - if (m_status == Job_Failed_Proceed) { + if (m_state == State::Succeeded) // pretend to succeed so we continue processing :) + { qDebug() << "Download failed but we are allowed to proceed:" << m_url.toString(); m_sink->abort(); m_reply.reset(); emit succeeded(m_index_within_job); return; - } else if (m_status == Job_Failed) { + } else if (m_state == State::Failed) { qDebug() << "Download failed in previous step:" << m_url.toString(); m_sink->abort(); m_reply.reset(); emit failed(m_index_within_job); return; - } else if (m_status == Job_Aborted) { + } else if (m_state == State::AbortedByUser) { qDebug() << "Download aborted in previous step:" << m_url.toString(); m_sink->abort(); m_reply.reset(); @@ -231,12 +232,12 @@ void Download::downloadFinished() auto data = m_reply->readAll(); if (data.size()) { qDebug() << "Writing extra" << data.size() << "bytes to" << m_target_path; - m_status = m_sink->write(data); + m_state = m_sink->write(data); } // otherwise, finalize the whole graph - m_status = m_sink->finalize(*m_reply.get()); - if (m_status != Job_Finished) { + m_state = m_sink->finalize(*m_reply.get()); + if (m_state != State::Succeeded) { qDebug() << "Download failed to finalize:" << m_url.toString(); m_sink->abort(); m_reply.reset(); @@ -250,10 +251,10 @@ void Download::downloadFinished() void Download::downloadReadyRead() { - if (m_status == Job_InProgress) { + if (m_state == State::Running) { auto data = m_reply->readAll(); - m_status = m_sink->write(data); - if (m_status == Job_Failed) { + m_state = m_sink->write(data); + if (m_state == State::Failed) { qCritical() << "Failed to process response chunk for " << m_target_path; } // qDebug() << "Download" << m_url.toString() << "gained" << data.size() << "bytes"; @@ -269,12 +270,7 @@ bool Net::Download::abort() if (m_reply) { m_reply->abort(); } else { - m_status = Job_Aborted; + m_state = State::AbortedByUser; } return true; } - -bool Net::Download::canAbort() -{ - return true; -} diff --git a/launcher/net/Download.h b/launcher/net/Download.h index 0f9bfe7f..231ad6a7 100644 --- a/launcher/net/Download.h +++ b/launcher/net/Download.h @@ -27,7 +27,7 @@ class Download : public NetAction { Q_OBJECT -public: /* types */ +public: typedef shared_qobject_ptr Ptr; enum class Option { @@ -36,7 +36,7 @@ public: /* types */ }; Q_DECLARE_FLAGS(Options, Option) -protected: /* con/des */ +protected: explicit Download(); public: virtual ~Download(){}; @@ -44,16 +44,16 @@ public: static Download::Ptr makeByteArray(QUrl url, QByteArray *output, Options options = Option::NoOptions); static Download::Ptr makeFile(QUrl url, QString path, Options options = Option::NoOptions); -public: /* methods */ +public: QString getTargetFilepath() { return m_target_path; } void addValidator(Validator * v); bool abort() override; - bool canAbort() override; + bool canAbort() const override { return true; }; -private: /* methods */ +private: bool handleRedirect(); protected slots: @@ -64,9 +64,9 @@ protected slots: void downloadReadyRead() override; public slots: - void startImpl() override; + void executeTask() override; -private: /* data */ +private: // FIXME: remove this, it has no business being here. QString m_target_path; std::unique_ptr m_sink; diff --git a/launcher/net/FileSink.cpp b/launcher/net/FileSink.cpp index 7e9b8929..0d8b09bb 100644 --- a/launcher/net/FileSink.cpp +++ b/launcher/net/FileSink.cpp @@ -1,25 +1,15 @@ #include "FileSink.h" + #include -#include + #include "FileSystem.h" namespace Net { -FileSink::FileSink(QString filename) - :m_filename(filename) -{ - // nil -} - -FileSink::~FileSink() -{ - // nil -} - -JobStatus FileSink::init(QNetworkRequest& request) +Task::State FileSink::init(QNetworkRequest& request) { auto result = initCache(request); - if(result != Job_InProgress) + if(result != Task::State::Running) { return result; } @@ -27,27 +17,27 @@ JobStatus FileSink::init(QNetworkRequest& request) if (!FS::ensureFilePathExists(m_filename)) { qCritical() << "Could not create folder for " + m_filename; - return Job_Failed; + return Task::State::Failed; } wroteAnyData = false; m_output_file.reset(new QSaveFile(m_filename)); if (!m_output_file->open(QIODevice::WriteOnly)) { qCritical() << "Could not open " + m_filename + " for writing"; - return Job_Failed; + return Task::State::Failed; } if(initAllValidators(request)) - return Job_InProgress; - return Job_Failed; + return Task::State::Running; + return Task::State::Failed; } -JobStatus FileSink::initCache(QNetworkRequest &) +Task::State FileSink::initCache(QNetworkRequest &) { - return Job_InProgress; + return Task::State::Running; } -JobStatus FileSink::write(QByteArray& data) +Task::State FileSink::write(QByteArray& data) { if (!writeAllValidators(data) || m_output_file->write(data) != data.size()) { @@ -55,20 +45,20 @@ JobStatus FileSink::write(QByteArray& data) m_output_file->cancelWriting(); m_output_file.reset(); wroteAnyData = false; - return Job_Failed; + return Task::State::Failed; } wroteAnyData = true; - return Job_InProgress; + return Task::State::Running; } -JobStatus FileSink::abort() +Task::State FileSink::abort() { m_output_file->cancelWriting(); failAllValidators(); - return Job_Failed; + return Task::State::Failed; } -JobStatus FileSink::finalize(QNetworkReply& reply) +Task::State FileSink::finalize(QNetworkReply& reply) { bool gotFile = false; QVariant statusCodeV = reply.attribute(QNetworkRequest::HttpStatusCodeAttribute); @@ -86,13 +76,13 @@ JobStatus FileSink::finalize(QNetworkReply& reply) // ask validators for data consistency // we only do this for actual downloads, not 'your data is still the same' cache hits if(!finalizeAllValidators(reply)) - return Job_Failed; + return Task::State::Failed; // nothing went wrong... if (!m_output_file->commit()) { qCritical() << "Failed to commit changes to " << m_filename; m_output_file->cancelWriting(); - return Job_Failed; + return Task::State::Failed; } } // then get rid of the save file @@ -101,9 +91,9 @@ JobStatus FileSink::finalize(QNetworkReply& reply) return finalizeCache(reply); } -JobStatus FileSink::finalizeCache(QNetworkReply &) +Task::State FileSink::finalizeCache(QNetworkReply &) { - return Job_Finished; + return Task::State::Succeeded; } bool FileSink::hasLocalData() diff --git a/launcher/net/FileSink.h b/launcher/net/FileSink.h index 875fe511..9d77b3d0 100644 --- a/launcher/net/FileSink.h +++ b/launcher/net/FileSink.h @@ -1,28 +1,30 @@ #pragma once -#include "Sink.h" + #include +#include "Sink.h" + namespace Net { -class FileSink : public Sink -{ -public: /* con/des */ - FileSink(QString filename); - virtual ~FileSink(); +class FileSink : public Sink { + public: + FileSink(QString filename) : m_filename(filename){}; + virtual ~FileSink() = default; + + public: + auto init(QNetworkRequest& request) -> Task::State override; + auto write(QByteArray& data) -> Task::State override; + auto abort() -> Task::State override; + auto finalize(QNetworkReply& reply) -> Task::State override; -public: /* methods */ - JobStatus init(QNetworkRequest & request) override; - JobStatus write(QByteArray & data) override; - JobStatus abort() override; - JobStatus finalize(QNetworkReply & reply) override; - bool hasLocalData() override; + auto hasLocalData() -> bool override; -protected: /* methods */ - virtual JobStatus initCache(QNetworkRequest &); - virtual JobStatus finalizeCache(QNetworkReply &reply); + protected: + virtual auto initCache(QNetworkRequest&) -> Task::State; + virtual auto finalizeCache(QNetworkReply& reply) -> Task::State; -protected: /* data */ + protected: QString m_filename; bool wroteAnyData = false; std::unique_ptr m_output_file; }; -} +} // namespace Net diff --git a/launcher/net/MetaCacheSink.cpp b/launcher/net/MetaCacheSink.cpp index 5cdf0460..34ba9f56 100644 --- a/launcher/net/MetaCacheSink.cpp +++ b/launcher/net/MetaCacheSink.cpp @@ -12,17 +12,13 @@ MetaCacheSink::MetaCacheSink(MetaEntryPtr entry, ChecksumValidator * md5sum) addValidator(md5sum); } -MetaCacheSink::~MetaCacheSink() -{ - // nil -} - -JobStatus MetaCacheSink::initCache(QNetworkRequest& request) +Task::State MetaCacheSink::initCache(QNetworkRequest& request) { if (!m_entry->isStale()) { - return Job_Finished; + return Task::State::Succeeded; } + // check if file exists, if it does, use its information for the request QFile current(m_filename); if(current.exists() && current.size() != 0) @@ -36,25 +32,31 @@ JobStatus MetaCacheSink::initCache(QNetworkRequest& request) request.setRawHeader(QString("If-None-Match").toLatin1(), m_entry->getETag().toLatin1()); } } - return Job_InProgress; + + return Task::State::Running; } -JobStatus MetaCacheSink::finalizeCache(QNetworkReply & reply) +Task::State MetaCacheSink::finalizeCache(QNetworkReply & reply) { QFileInfo output_file_info(m_filename); + if(wroteAnyData) { m_entry->setMD5Sum(m_md5Node->hash().toHex().constData()); } + m_entry->setETag(reply.rawHeader("ETag").constData()); + if (reply.hasRawHeader("Last-Modified")) { m_entry->setRemoteChangedTimestamp(reply.rawHeader("Last-Modified").constData()); } + m_entry->setLocalChangedTimestamp(output_file_info.lastModified().toUTC().toMSecsSinceEpoch()); m_entry->setStale(false); APPLICATION->metacache()->updateEntry(m_entry); - return Job_Finished; + + return Task::State::Succeeded; } bool MetaCacheSink::hasLocalData() diff --git a/launcher/net/MetaCacheSink.h b/launcher/net/MetaCacheSink.h index edcf7ad1..431e10a8 100644 --- a/launcher/net/MetaCacheSink.h +++ b/launcher/net/MetaCacheSink.h @@ -1,22 +1,23 @@ #pragma once -#include "FileSink.h" + #include "ChecksumValidator.h" +#include "FileSink.h" #include "net/HttpMetaCache.h" namespace Net { -class MetaCacheSink : public FileSink -{ -public: /* con/des */ - MetaCacheSink(MetaEntryPtr entry, ChecksumValidator * md5sum); - virtual ~MetaCacheSink(); - bool hasLocalData() override; +class MetaCacheSink : public FileSink { + public: + MetaCacheSink(MetaEntryPtr entry, ChecksumValidator* md5sum); + virtual ~MetaCacheSink() = default; + + auto hasLocalData() -> bool override; -protected: /* methods */ - JobStatus initCache(QNetworkRequest & request) override; - JobStatus finalizeCache(QNetworkReply & reply) override; + protected: + auto initCache(QNetworkRequest& request) -> Task::State override; + auto finalizeCache(QNetworkReply& reply) -> Task::State override; -private: /* data */ + private: MetaEntryPtr m_entry; - ChecksumValidator * m_md5Node; + ChecksumValidator* m_md5Node; }; -} +} // namespace Net diff --git a/launcher/net/NetAction.h b/launcher/net/NetAction.h index efb20953..e15716f6 100644 --- a/launcher/net/NetAction.h +++ b/launcher/net/NetAction.h @@ -1,108 +1,81 @@ -/* Copyright 2013-2021 MultiMC Contributors +// SPDX-License-Identifier: GPL-3.0-only +/* + * PolyMC - Minecraft Launcher * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, version 3. * - * http://www.apache.org/licenses/LICENSE-2.0 + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * This file incorporates work covered by the following copyright and + * permission notice: + * + * Copyright 2013-2021 MultiMC Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ #pragma once -#include -#include -#include #include -#include +#include -enum JobStatus -{ - Job_NotStarted, - Job_InProgress, - Job_Finished, - Job_Failed, - Job_Aborted, - /* - * FIXME: @NUKE this confuses the task failing with us having a fallback in the form of local data. Clear up the confusion. - * Same could be true for aborted task - the presence of pre-existing result is a separate concern - */ - Job_Failed_Proceed -}; +#include "QObjectPtr.h" +#include "tasks/Task.h" -class NetAction : public QObject -{ +class NetAction : public Task { Q_OBJECT -protected: - explicit NetAction() : QObject(nullptr) {}; + protected: + explicit NetAction() : Task(nullptr) {}; -public: + public: using Ptr = shared_qobject_ptr; - virtual ~NetAction() {}; - - bool isRunning() const - { - return m_status == Job_InProgress; - } - bool isFinished() const - { - return m_status >= Job_Finished; - } - bool wasSuccessful() const - { - return m_status == Job_Finished || m_status == Job_Failed_Proceed; - } + virtual ~NetAction() = default; - qint64 totalProgress() const - { - return m_total_progress; - } - qint64 currentProgress() const - { - return m_progress; - } - virtual bool abort() - { - return false; - } - virtual bool canAbort() - { - return false; - } - QUrl url() - { - return m_url; - } + QUrl url() { return m_url; } -signals: + signals: void started(int index); void netActionProgress(int index, qint64 current, qint64 total); void succeeded(int index); void failed(int index); void aborted(int index); -protected slots: + protected slots: virtual void downloadProgress(qint64 bytesReceived, qint64 bytesTotal) = 0; virtual void downloadError(QNetworkReply::NetworkError error) = 0; virtual void downloadFinished() = 0; virtual void downloadReadyRead() = 0; -public slots: - void start(shared_qobject_ptr network) { + public slots: + void startAction(shared_qobject_ptr network) + { m_network = network; - startImpl(); + executeTask(); } -protected: - virtual void startImpl() = 0; + protected: + void executeTask() override {}; -public: + public: shared_qobject_ptr m_network; /// index within the parent job, FIXME: nuke @@ -113,10 +86,4 @@ public: /// source URL QUrl m_url; - - qint64 m_progress = 0; - qint64 m_total_progress = 1; - -protected: - JobStatus m_status = Job_NotStarted; }; diff --git a/launcher/net/NetJob.cpp b/launcher/net/NetJob.cpp index 9bad89ed..d08d6c4d 100644 --- a/launcher/net/NetJob.cpp +++ b/launcher/net/NetJob.cpp @@ -1,79 +1,170 @@ -/* Copyright 2013-2021 MultiMC Contributors +// SPDX-License-Identifier: GPL-3.0-only +/* + * PolyMC - Minecraft Launcher * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, version 3. * - * http://www.apache.org/licenses/LICENSE-2.0 + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * This file incorporates work covered by the following copyright and + * permission notice: + * + * Copyright 2013-2021 MultiMC Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ #include "NetJob.h" #include "Download.h" -#include +auto NetJob::addNetAction(NetAction::Ptr action) -> bool +{ + action->m_index_within_job = m_downloads.size(); + m_downloads.append(action); + part_info pi; + m_parts_progress.append(pi); + + partProgress(m_parts_progress.count() - 1, action->getProgress(), action->getTotalProgress()); + + if (action->isRunning()) { + connect(action.get(), &NetAction::succeeded, this, &NetJob::partSucceeded); + connect(action.get(), &NetAction::failed, this, &NetJob::partFailed); + connect(action.get(), &NetAction::netActionProgress, this, &NetJob::partProgress); + } else { + m_todo.append(m_parts_progress.size() - 1); + } + + return true; +} + +auto NetJob::canAbort() const -> bool +{ + bool canFullyAbort = true; + + // can abort the downloads on the queue? + for (auto index : m_todo) { + auto part = m_downloads[index]; + canFullyAbort &= part->canAbort(); + } + // can abort the active downloads? + for (auto index : m_doing) { + auto part = m_downloads[index]; + canFullyAbort &= part->canAbort(); + } + + return canFullyAbort; +} + +void NetJob::executeTask() +{ + // hack that delays early failures so they can be caught easier + QMetaObject::invokeMethod(this, "startMoreParts", Qt::QueuedConnection); +} + +auto NetJob::getFailedFiles() -> QStringList +{ + QStringList failed; + for (auto index : m_failed) { + failed.push_back(m_downloads[index]->url().toString()); + } + failed.sort(); + return failed; +} + +auto NetJob::abort() -> bool +{ + bool fullyAborted = true; + + // fail all downloads on the queue + m_failed.unite(m_todo.toSet()); + m_todo.clear(); + + // abort active downloads + auto toKill = m_doing.toList(); + for (auto index : toKill) { + auto part = m_downloads[index]; + fullyAborted &= part->abort(); + } + + return fullyAborted; +} void NetJob::partSucceeded(int index) { // do progress. all slots are 1 in size at least - auto &slot = parts_progress[index]; + auto& slot = m_parts_progress[index]; partProgress(index, slot.total_progress, slot.total_progress); m_doing.remove(index); m_done.insert(index); - downloads[index].get()->disconnect(this); + m_downloads[index].get()->disconnect(this); + startMoreParts(); } void NetJob::partFailed(int index) { m_doing.remove(index); - auto &slot = parts_progress[index]; - if (slot.failures == 3) - { + + auto& slot = m_parts_progress[index]; + // Can try 3 times before failing by definitive + if (slot.failures == 3) { m_failed.insert(index); - } - else - { + } else { slot.failures++; m_todo.enqueue(index); } - downloads[index].get()->disconnect(this); + + m_downloads[index].get()->disconnect(this); + startMoreParts(); } void NetJob::partAborted(int index) { m_aborted = true; + m_doing.remove(index); m_failed.insert(index); - downloads[index].get()->disconnect(this); + m_downloads[index].get()->disconnect(this); + startMoreParts(); } void NetJob::partProgress(int index, qint64 bytesReceived, qint64 bytesTotal) { - auto &slot = parts_progress[index]; + auto& slot = m_parts_progress[index]; slot.current_progress = bytesReceived; slot.total_progress = bytesTotal; int done = m_done.size(); int doing = m_doing.size(); - int all = parts_progress.size(); + int all = m_parts_progress.size(); qint64 bytesAll = 0; qint64 bytesTotalAll = 0; - for(auto & partIdx: m_doing) - { - auto part = parts_progress[partIdx]; + for (auto& partIdx : m_doing) { + auto part = m_parts_progress[partIdx]; // do not count parts with unknown/nonsensical total size - if(part.total_progress <= 0) - { + if (part.total_progress <= 0) { continue; } bytesAll += part.current_progress; @@ -85,134 +176,53 @@ void NetJob::partProgress(int index, qint64 bytesReceived, qint64 bytesTotal) auto current_total = all * 1000; // HACK: make sure it never jumps backwards. // FAIL: This breaks if the size is not known (or is it something else?) and jumps to 1000, so if it is 1000 reset it to inprogress - if(m_current_progress == 1000) { + if (m_current_progress == 1000) { m_current_progress = inprogress; } - if(m_current_progress > current) - { + if (m_current_progress > current) { current = m_current_progress; } m_current_progress = current; setProgress(current, current_total); } -void NetJob::executeTask() -{ - // hack that delays early failures so they can be caught easier - QMetaObject::invokeMethod(this, "startMoreParts", Qt::QueuedConnection); -} - void NetJob::startMoreParts() { - if(!isRunning()) - { - // this actually makes sense. You can put running downloads into a NetJob and then not start it until much later. + if (!isRunning()) { + // this actually makes sense. You can put running m_downloads into a NetJob and then not start it until much later. return; } + // OK. We are actively processing tasks, proceed. // Check for final conditions if there's nothing in the queue. - if(!m_todo.size()) - { - if(!m_doing.size()) - { - if(!m_failed.size()) - { + if (!m_todo.size()) { + if (!m_doing.size()) { + if (!m_failed.size()) { emitSucceeded(); - } - else if(m_aborted) - { + } else if (m_aborted) { emitAborted(); - } - else - { + } else { emitFailed(tr("Job '%1' failed to process:\n%2").arg(objectName()).arg(getFailedFiles().join("\n"))); } } return; } - // There's work to do, try to start more parts. - while (m_doing.size() < 6) - { - if(!m_todo.size()) + + // There's work to do, try to start more parts, to a maximum of 6 concurrent ones. + while (m_doing.size() < 6) { + if (m_todo.size() == 0) return; int doThis = m_todo.dequeue(); m_doing.insert(doThis); - auto part = downloads[doThis]; - // connect signals :D - connect(part.get(), SIGNAL(succeeded(int)), SLOT(partSucceeded(int))); - connect(part.get(), SIGNAL(failed(int)), SLOT(partFailed(int))); - connect(part.get(), SIGNAL(aborted(int)), SLOT(partAborted(int))); - connect(part.get(), SIGNAL(netActionProgress(int, qint64, qint64)), - SLOT(partProgress(int, qint64, qint64))); - part->start(m_network); - } -} - - -QStringList NetJob::getFailedFiles() -{ - QStringList failed; - for (auto index: m_failed) - { - failed.push_back(downloads[index]->url().toString()); - } - failed.sort(); - return failed; -} -bool NetJob::canAbort() const -{ - bool canFullyAbort = true; - // can abort the waiting? - for(auto index: m_todo) - { - auto part = downloads[index]; - canFullyAbort &= part->canAbort(); - } - // can abort the active? - for(auto index: m_doing) - { - auto part = downloads[index]; - canFullyAbort &= part->canAbort(); - } - return canFullyAbort; -} + auto part = m_downloads[doThis]; -bool NetJob::abort() -{ - bool fullyAborted = true; - // fail all waiting - m_failed.unite(m_todo.toSet()); - m_todo.clear(); - // abort active - auto toKill = m_doing.toList(); - for(auto index: toKill) - { - auto part = downloads[index]; - fullyAborted &= part->abort(); - } - return fullyAborted; -} + // connect signals :D + connect(part.get(), &NetAction::succeeded, this, &NetJob::partSucceeded); + connect(part.get(), &NetAction::failed, this, &NetJob::partFailed); + connect(part.get(), &NetAction::aborted, this, &NetJob::partAborted); + connect(part.get(), &NetAction::netActionProgress, this, &NetJob::partProgress); -bool NetJob::addNetAction(NetAction::Ptr action) -{ - action->m_index_within_job = downloads.size(); - downloads.append(action); - part_info pi; - parts_progress.append(pi); - partProgress(parts_progress.count() - 1, action->currentProgress(), action->totalProgress()); - - if(action->isRunning()) - { - connect(action.get(), SIGNAL(succeeded(int)), SLOT(partSucceeded(int))); - connect(action.get(), SIGNAL(failed(int)), SLOT(partFailed(int))); - connect(action.get(), SIGNAL(netActionProgress(int, qint64, qint64)), SLOT(partProgress(int, qint64, qint64))); + part->startAction(m_network); } - else - { - m_todo.append(parts_progress.size() - 1); - } - return true; } - -NetJob::~NetJob() = default; diff --git a/launcher/net/NetJob.h b/launcher/net/NetJob.h index fdea710f..c397e2a1 100644 --- a/launcher/net/NetJob.h +++ b/launcher/net/NetJob.h @@ -1,88 +1,97 @@ -/* Copyright 2013-2021 MultiMC Contributors +// SPDX-License-Identifier: GPL-3.0-only +/* + * PolyMC - Minecraft Launcher * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, version 3. * - * http://www.apache.org/licenses/LICENSE-2.0 + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * This file incorporates work covered by the following copyright and + * permission notice: + * + * Copyright 2013-2021 MultiMC Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ #pragma once + #include + +#include #include "NetAction.h" -#include "Download.h" -#include "HttpMetaCache.h" #include "tasks/Task.h" -#include "QObjectPtr.h" -class NetJob; +// Those are included so that they are also included by anyone using NetJob +#include "net/Download.h" +#include "net/HttpMetaCache.h" -class NetJob : public Task -{ +class NetJob : public Task { Q_OBJECT -public: + + public: using Ptr = shared_qobject_ptr; explicit NetJob(QString job_name, shared_qobject_ptr network) : Task(), m_network(network) { setObjectName(job_name); } - virtual ~NetJob(); + virtual ~NetJob() = default; - bool addNetAction(NetAction::Ptr action); + void executeTask() override; - NetAction::Ptr operator[](int index) - { - return downloads[index]; - } - const NetAction::Ptr at(const int index) - { - return downloads.at(index); - } - NetAction::Ptr first() - { - if (downloads.size()) - return downloads[0]; - return NetAction::Ptr(); - } - int size() const - { - return downloads.size(); - } - QStringList getFailedFiles(); + auto canAbort() const -> bool override; - bool canAbort() const override; + auto addNetAction(NetAction::Ptr action) -> bool; -private slots: - void startMoreParts(); + auto operator[](int index) -> NetAction::Ptr { return m_downloads[index]; } + auto at(int index) -> const NetAction::Ptr { return m_downloads.at(index); } + auto size() const -> int { return m_downloads.size(); } + auto first() -> NetAction::Ptr { return m_downloads.size() != 0 ? m_downloads[0] : NetAction::Ptr{}; } -public slots: - virtual void executeTask() override; - virtual bool abort() override; + auto getFailedFiles() -> QStringList; + + public slots: + // Qt can't handle auto at the start for some reason? + bool abort() override; + + private slots: + void startMoreParts(); -private slots: void partProgress(int index, qint64 bytesReceived, qint64 bytesTotal); void partSucceeded(int index); void partFailed(int index); void partAborted(int index); -private: + private: shared_qobject_ptr m_network; - struct part_info - { + struct part_info { qint64 current_progress = 0; qint64 total_progress = 1; int failures = 0; }; - QList downloads; - QList parts_progress; + + QList m_downloads; + QList m_parts_progress; QQueue m_todo; QSet m_doing; QSet m_done; diff --git a/launcher/net/Sink.h b/launcher/net/Sink.h index d367fb15..3b2a7f8d 100644 --- a/launcher/net/Sink.h +++ b/launcher/net/Sink.h @@ -5,33 +5,30 @@ #include "Validator.h" namespace Net { -class Sink -{ -public: /* con/des */ - Sink() {}; - virtual ~Sink() {}; +class Sink { + public: + Sink() = default; + virtual ~Sink(){}; -public: /* methods */ - virtual JobStatus init(QNetworkRequest & request) = 0; - virtual JobStatus write(QByteArray & data) = 0; - virtual JobStatus abort() = 0; - virtual JobStatus finalize(QNetworkReply & reply) = 0; + public: + virtual Task::State init(QNetworkRequest& request) = 0; + virtual Task::State write(QByteArray& data) = 0; + virtual Task::State abort() = 0; + virtual Task::State finalize(QNetworkReply& reply) = 0; virtual bool hasLocalData() = 0; - void addValidator(Validator * validator) + void addValidator(Validator* validator) { - if(validator) - { + if (validator) { validators.push_back(std::shared_ptr(validator)); } } -protected: /* methods */ - bool finalizeAllValidators(QNetworkReply & reply) + protected: /* methods */ + bool finalizeAllValidators(QNetworkReply& reply) { - for(auto & validator: validators) - { - if(!validator->validate(reply)) + for (auto& validator : validators) { + if (!validator->validate(reply)) return false; } return true; @@ -39,32 +36,29 @@ protected: /* methods */ bool failAllValidators() { bool success = true; - for(auto & validator: validators) - { + for (auto& validator : validators) { success &= validator->abort(); } return success; } - bool initAllValidators(QNetworkRequest & request) + bool initAllValidators(QNetworkRequest& request) { - for(auto & validator: validators) - { - if(!validator->init(request)) + for (auto& validator : validators) { + if (!validator->init(request)) return false; } return true; } - bool writeAllValidators(QByteArray & data) + bool writeAllValidators(QByteArray& data) { - for(auto & validator: validators) - { - if(!validator->write(data)) + for (auto& validator : validators) { + if (!validator->write(data)) return false; } return true; } -protected: /* data */ + protected: /* data */ std::vector> validators; }; -} +} // namespace Net diff --git a/launcher/screenshots/ImgurAlbumCreation.cpp b/launcher/screenshots/ImgurAlbumCreation.cpp index d5de302a..81fac929 100644 --- a/launcher/screenshots/ImgurAlbumCreation.cpp +++ b/launcher/screenshots/ImgurAlbumCreation.cpp @@ -13,12 +13,12 @@ ImgurAlbumCreation::ImgurAlbumCreation(QList screenshots) : NetAction(), m_screenshots(screenshots) { m_url = BuildConfig.IMGUR_BASE_URL + "album.json"; - m_status = Job_NotStarted; + m_state = State::Inactive; } -void ImgurAlbumCreation::startImpl() +void ImgurAlbumCreation::executeTask() { - m_status = Job_InProgress; + m_state = State::Running; QNetworkRequest request(m_url); request.setHeader(QNetworkRequest::UserAgentHeader, BuildConfig.USER_AGENT_UNCACHED); request.setHeader(QNetworkRequest::ContentTypeHeader, "application/x-www-form-urlencoded"); @@ -43,11 +43,11 @@ void ImgurAlbumCreation::startImpl() void ImgurAlbumCreation::downloadError(QNetworkReply::NetworkError error) { qDebug() << m_reply->errorString(); - m_status = Job_Failed; + m_state = State::Failed; } void ImgurAlbumCreation::downloadFinished() { - if (m_status != Job_Failed) + if (m_state != State::Failed) { QByteArray data = m_reply->readAll(); m_reply.reset(); @@ -68,7 +68,7 @@ void ImgurAlbumCreation::downloadFinished() } m_deleteHash = object.value("data").toObject().value("deletehash").toString(); m_id = object.value("data").toObject().value("id").toString(); - m_status = Job_Finished; + m_state = State::Succeeded; emit succeeded(m_index_within_job); return; } @@ -82,7 +82,6 @@ void ImgurAlbumCreation::downloadFinished() } void ImgurAlbumCreation::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) { - m_total_progress = bytesTotal; - m_progress = bytesReceived; + setProgress(bytesReceived, bytesTotal); emit netActionProgress(m_index_within_job, bytesReceived, bytesTotal); } diff --git a/launcher/screenshots/ImgurAlbumCreation.h b/launcher/screenshots/ImgurAlbumCreation.h index cb048a23..4cb0ed5d 100644 --- a/launcher/screenshots/ImgurAlbumCreation.h +++ b/launcher/screenshots/ImgurAlbumCreation.h @@ -24,16 +24,14 @@ public: protected slots: - virtual void downloadProgress(qint64 bytesReceived, qint64 bytesTotal); - virtual void downloadError(QNetworkReply::NetworkError error); - virtual void downloadFinished(); - virtual void downloadReadyRead() - { - } + void downloadProgress(qint64 bytesReceived, qint64 bytesTotal) override; + void downloadError(QNetworkReply::NetworkError error) override; + void downloadFinished() override; + void downloadReadyRead() override {} public slots: - virtual void startImpl(); + void executeTask() override; private: QList m_screenshots; diff --git a/launcher/screenshots/ImgurUpload.cpp b/launcher/screenshots/ImgurUpload.cpp index 76a84947..0f0fd79c 100644 --- a/launcher/screenshots/ImgurUpload.cpp +++ b/launcher/screenshots/ImgurUpload.cpp @@ -13,13 +13,13 @@ ImgurUpload::ImgurUpload(ScreenShot::Ptr shot) : NetAction(), m_shot(shot) { m_url = BuildConfig.IMGUR_BASE_URL + "upload.json"; - m_status = Job_NotStarted; + m_state = State::Inactive; } -void ImgurUpload::startImpl() +void ImgurUpload::executeTask() { finished = false; - m_status = Job_InProgress; + m_state = Task::State::Running; QNetworkRequest request(m_url); request.setHeader(QNetworkRequest::UserAgentHeader, BuildConfig.USER_AGENT_UNCACHED); request.setRawHeader("Authorization", QString("Client-ID %1").arg(BuildConfig.IMGUR_CLIENT_ID).toStdString().c_str()); @@ -63,7 +63,7 @@ void ImgurUpload::downloadError(QNetworkReply::NetworkError error) qCritical() << "Double finished ImgurUpload!"; return; } - m_status = Job_Failed; + m_state = Task::State::Failed; finished = true; m_reply.reset(); emit failed(m_index_within_job); @@ -99,14 +99,13 @@ void ImgurUpload::downloadFinished() m_shot->m_imgurId = object.value("data").toObject().value("id").toString(); m_shot->m_url = object.value("data").toObject().value("link").toString(); m_shot->m_imgurDeleteHash = object.value("data").toObject().value("deletehash").toString(); - m_status = Job_Finished; + m_state = Task::State::Succeeded; finished = true; emit succeeded(m_index_within_job); return; } void ImgurUpload::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) { - m_total_progress = bytesTotal; - m_progress = bytesReceived; + setProgress(bytesReceived, bytesTotal); emit netActionProgress(m_index_within_job, bytesReceived, bytesTotal); } diff --git a/launcher/screenshots/ImgurUpload.h b/launcher/screenshots/ImgurUpload.h index cf54f58d..a1040551 100644 --- a/launcher/screenshots/ImgurUpload.h +++ b/launcher/screenshots/ImgurUpload.h @@ -21,7 +21,7 @@ slots: public slots: - void startImpl() override; + void executeTask() override; private: ScreenShot::Ptr m_shot; diff --git a/launcher/tasks/Task.h b/launcher/tasks/Task.h index 344a024e..61855160 100644 --- a/launcher/tasks/Task.h +++ b/launcher/tasks/Task.h @@ -52,6 +52,8 @@ class Task : public QObject { virtual bool canAbort() const { return false; } + auto getState() const -> State { return m_state; } + QString getStatus() { return m_status; } virtual auto getStepStatus() const -> QString { return m_status; } @@ -90,7 +92,7 @@ class Task : public QObject { void setStatus(const QString& status); void setProgress(qint64 current, qint64 total); - private: + protected: State m_state = State::Inactive; QStringList m_Warnings; QString m_failReason = ""; diff --git a/launcher/translations/TranslationsModel.cpp b/launcher/translations/TranslationsModel.cpp index 250854d3..fbd17060 100644 --- a/launcher/translations/TranslationsModel.cpp +++ b/launcher/translations/TranslationsModel.cpp @@ -667,7 +667,7 @@ void TranslationsModel::downloadTranslation(QString key) auto dl = Net::Download::makeCached(QUrl(BuildConfig.TRANSLATIONS_BASE_URL + lang->file_name), entry); auto rawHash = QByteArray::fromHex(lang->file_sha1.toLatin1()); dl->addValidator(new Net::ChecksumValidator(QCryptographicHash::Sha1, rawHash)); - dl->m_total_progress = lang->file_size; + dl->setProgress(dl->getProgress(), lang->file_size); d->m_dl_job = new NetJob("Translation for " + key, APPLICATION->network()); d->m_dl_job->addNetAction(dl); -- cgit From efa3fbff39bf0dabebdf1c6330090ee320895a4d Mon Sep 17 00:00:00 2001 From: flow Date: Tue, 26 Apr 2022 21:25:42 -0300 Subject: refactor: remove some superfluous signals Since now we're inheriting from Task, some signals can be reused. --- launcher/net/Download.cpp | 21 ++++++++++----------- launcher/net/NetAction.h | 10 ++-------- launcher/net/NetJob.cpp | 14 +++++++------- launcher/screenshots/ImgurAlbumCreation.cpp | 10 +++++----- launcher/screenshots/ImgurUpload.cpp | 12 ++++++------ launcher/tasks/Task.cpp | 3 +-- launcher/tasks/Task.h | 3 ++- 7 files changed, 33 insertions(+), 40 deletions(-) diff --git a/launcher/net/Download.cpp b/launcher/net/Download.cpp index 5b5a04db..5e5d64fa 100644 --- a/launcher/net/Download.cpp +++ b/launcher/net/Download.cpp @@ -72,7 +72,7 @@ void Download::executeTask() { if (getState() == Task::State::AbortedByUser) { qWarning() << "Attempt to start an aborted Download:" << m_url.toString(); - emit aborted(m_index_within_job); + emitAborted(); return; } @@ -80,7 +80,7 @@ void Download::executeTask() m_state = m_sink->init(request); switch (m_state) { case State::Succeeded: - emit succeeded(m_index_within_job); + emit succeeded(); qDebug() << "Download cache hit " << m_url.toString(); return; case State::Running: @@ -88,7 +88,7 @@ void Download::executeTask() break; case State::Inactive: case State::Failed: - emit failed(m_index_within_job); + emitFailed(); return; case State::AbortedByUser: return; @@ -102,8 +102,8 @@ void Download::executeTask() QNetworkReply* rep = m_network->get(request); m_reply.reset(rep); - connect(rep, SIGNAL(downloadProgress(qint64, qint64)), SLOT(downloadProgress(qint64, qint64))); - connect(rep, SIGNAL(finished()), SLOT(downloadFinished())); + connect(rep, &QNetworkReply::downloadProgress, this, &Download::downloadProgress); + connect(rep, &QNetworkReply::finished, this, &Download::downloadFinished); connect(rep, SIGNAL(error(QNetworkReply::NetworkError)), SLOT(downloadError(QNetworkReply::NetworkError))); connect(rep, &QNetworkReply::sslErrors, this, &Download::sslErrors); connect(rep, &QNetworkReply::readyRead, this, &Download::downloadReadyRead); @@ -112,7 +112,6 @@ void Download::executeTask() void Download::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) { setProgress(bytesReceived, bytesTotal); - emit netActionProgress(m_index_within_job, bytesReceived, bytesTotal); } void Download::downloadError(QNetworkReply::NetworkError error) @@ -212,19 +211,19 @@ void Download::downloadFinished() qDebug() << "Download failed but we are allowed to proceed:" << m_url.toString(); m_sink->abort(); m_reply.reset(); - emit succeeded(m_index_within_job); + emit succeeded(); return; } else if (m_state == State::Failed) { qDebug() << "Download failed in previous step:" << m_url.toString(); m_sink->abort(); m_reply.reset(); - emit failed(m_index_within_job); + emitFailed(); return; } else if (m_state == State::AbortedByUser) { qDebug() << "Download aborted in previous step:" << m_url.toString(); m_sink->abort(); m_reply.reset(); - emit aborted(m_index_within_job); + emitAborted(); return; } @@ -241,12 +240,12 @@ void Download::downloadFinished() qDebug() << "Download failed to finalize:" << m_url.toString(); m_sink->abort(); m_reply.reset(); - emit failed(m_index_within_job); + emitFailed(); return; } m_reply.reset(); qDebug() << "Download succeeded:" << m_url.toString(); - emit succeeded(m_index_within_job); + emit succeeded(); } void Download::downloadReadyRead() diff --git a/launcher/net/NetAction.h b/launcher/net/NetAction.h index e15716f6..86a37ee6 100644 --- a/launcher/net/NetAction.h +++ b/launcher/net/NetAction.h @@ -43,7 +43,7 @@ class NetAction : public Task { Q_OBJECT protected: - explicit NetAction() : Task(nullptr) {}; + explicit NetAction() : Task() {}; public: using Ptr = shared_qobject_ptr; @@ -51,13 +51,7 @@ class NetAction : public Task { virtual ~NetAction() = default; QUrl url() { return m_url; } - - signals: - void started(int index); - void netActionProgress(int index, qint64 current, qint64 total); - void succeeded(int index); - void failed(int index); - void aborted(int index); + auto index() -> int { return m_index_within_job; } protected slots: virtual void downloadProgress(qint64 bytesReceived, qint64 bytesTotal) = 0; diff --git a/launcher/net/NetJob.cpp b/launcher/net/NetJob.cpp index d08d6c4d..a9f89da4 100644 --- a/launcher/net/NetJob.cpp +++ b/launcher/net/NetJob.cpp @@ -45,9 +45,9 @@ auto NetJob::addNetAction(NetAction::Ptr action) -> bool partProgress(m_parts_progress.count() - 1, action->getProgress(), action->getTotalProgress()); if (action->isRunning()) { - connect(action.get(), &NetAction::succeeded, this, &NetJob::partSucceeded); - connect(action.get(), &NetAction::failed, this, &NetJob::partFailed); - connect(action.get(), &NetAction::netActionProgress, this, &NetJob::partProgress); + connect(action.get(), &NetAction::succeeded, [this, action]{ partSucceeded(action->index()); }); + connect(action.get(), &NetAction::failed, [this, action](QString){ partFailed(action->index()); }); + connect(action.get(), &NetAction::progress, [this, action](qint64 done, qint64 total) { partProgress(action->index(), done, total); }); } else { m_todo.append(m_parts_progress.size() - 1); } @@ -218,10 +218,10 @@ void NetJob::startMoreParts() auto part = m_downloads[doThis]; // connect signals :D - connect(part.get(), &NetAction::succeeded, this, &NetJob::partSucceeded); - connect(part.get(), &NetAction::failed, this, &NetJob::partFailed); - connect(part.get(), &NetAction::aborted, this, &NetJob::partAborted); - connect(part.get(), &NetAction::netActionProgress, this, &NetJob::partProgress); + connect(part.get(), &NetAction::succeeded, this, [this, part]{ partSucceeded(part->index()); }); + connect(part.get(), &NetAction::failed, this, [this, part](QString){ partFailed(part->index()); }); + connect(part.get(), &NetAction::aborted, this, [this, part]{ partAborted(part->index()); }); + connect(part.get(), &NetAction::progress, this, [this, part](qint64 done, qint64 total) { partProgress(part->index(), done, total); }); part->startAction(m_network); } diff --git a/launcher/screenshots/ImgurAlbumCreation.cpp b/launcher/screenshots/ImgurAlbumCreation.cpp index 81fac929..f94527c8 100644 --- a/launcher/screenshots/ImgurAlbumCreation.cpp +++ b/launcher/screenshots/ImgurAlbumCreation.cpp @@ -56,32 +56,32 @@ void ImgurAlbumCreation::downloadFinished() if (jsonError.error != QJsonParseError::NoError) { qDebug() << jsonError.errorString(); - emit failed(m_index_within_job); + emitFailed(); return; } auto object = doc.object(); if (!object.value("success").toBool()) { qDebug() << doc.toJson(); - emit failed(m_index_within_job); + emitFailed(); return; } m_deleteHash = object.value("data").toObject().value("deletehash").toString(); m_id = object.value("data").toObject().value("id").toString(); m_state = State::Succeeded; - emit succeeded(m_index_within_job); + emit succeeded(); return; } else { qDebug() << m_reply->readAll(); m_reply.reset(); - emit failed(m_index_within_job); + emitFailed(); return; } } void ImgurAlbumCreation::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) { setProgress(bytesReceived, bytesTotal); - emit netActionProgress(m_index_within_job, bytesReceived, bytesTotal); + emit progress(bytesReceived, bytesTotal); } diff --git a/launcher/screenshots/ImgurUpload.cpp b/launcher/screenshots/ImgurUpload.cpp index 0f0fd79c..05314de7 100644 --- a/launcher/screenshots/ImgurUpload.cpp +++ b/launcher/screenshots/ImgurUpload.cpp @@ -28,7 +28,7 @@ void ImgurUpload::executeTask() QFile f(m_shot->m_file.absoluteFilePath()); if (!f.open(QFile::ReadOnly)) { - emit failed(m_index_within_job); + emitFailed(); return; } @@ -66,7 +66,7 @@ void ImgurUpload::downloadError(QNetworkReply::NetworkError error) m_state = Task::State::Failed; finished = true; m_reply.reset(); - emit failed(m_index_within_job); + emitFailed(); } void ImgurUpload::downloadFinished() { @@ -84,7 +84,7 @@ void ImgurUpload::downloadFinished() qDebug() << "imgur server did not reply with JSON" << jsonError.errorString(); finished = true; m_reply.reset(); - emit failed(m_index_within_job); + emitFailed(); return; } auto object = doc.object(); @@ -93,7 +93,7 @@ void ImgurUpload::downloadFinished() qDebug() << "Screenshot upload not successful:" << doc.toJson(); finished = true; m_reply.reset(); - emit failed(m_index_within_job); + emitFailed(); return; } m_shot->m_imgurId = object.value("data").toObject().value("id").toString(); @@ -101,11 +101,11 @@ void ImgurUpload::downloadFinished() m_shot->m_imgurDeleteHash = object.value("data").toObject().value("deletehash").toString(); m_state = Task::State::Succeeded; finished = true; - emit succeeded(m_index_within_job); + emit succeeded(); return; } void ImgurUpload::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) { setProgress(bytesReceived, bytesTotal); - emit netActionProgress(m_index_within_job, bytesReceived, bytesTotal); + emit progress(bytesReceived, bytesTotal); } diff --git a/launcher/tasks/Task.cpp b/launcher/tasks/Task.cpp index 57307b43..68e0e8a7 100644 --- a/launcher/tasks/Task.cpp +++ b/launcher/tasks/Task.cpp @@ -99,8 +99,7 @@ void Task::emitAborted() m_state = State::AbortedByUser; m_failReason = "Aborted."; qDebug() << "Task" << describe() << "aborted."; - emit failed(m_failReason); - emit finished(); + emit aborted(); } void Task::emitSucceeded() diff --git a/launcher/tasks/Task.h b/launcher/tasks/Task.h index 61855160..e09c57ae 100644 --- a/launcher/tasks/Task.h +++ b/launcher/tasks/Task.h @@ -73,6 +73,7 @@ class Task : public QObject { virtual void progress(qint64 current, qint64 total); void finished(); void succeeded(); + void aborted(); void failed(QString reason); void status(QString status); @@ -86,7 +87,7 @@ class Task : public QObject { protected slots: virtual void emitSucceeded(); virtual void emitAborted(); - virtual void emitFailed(QString reason); + virtual void emitFailed(QString reason = ""); public slots: void setStatus(const QString& status); -- cgit From 040ee919e5ea71364daa08c30e09c843976f5734 Mon Sep 17 00:00:00 2001 From: flow Date: Wed, 27 Apr 2022 18:36:11 -0300 Subject: refactor: more net cleanup This runs clang-tidy on some other files in launcher/net/. This also makes use of some JSON wrappers in HttpMetaCache, instead of using the Qt stuff directly. Lastly, this removes useless null checks (crashes don't occur because of this, but because of concurrent usage / free of the QByteArray pointer), and fix a fixme in Download.h --- launcher/net/ByteArraySink.h | 11 +-- launcher/net/ChecksumValidator.h | 48 +++++----- launcher/net/Download.cpp | 24 ++--- launcher/net/Download.h | 59 ++++++------- launcher/net/FileSink.cpp | 47 +++++----- launcher/net/HttpMetaCache.cpp | 184 +++++++++++++++++---------------------- launcher/net/HttpMetaCache.h | 107 +++++++++-------------- launcher/net/Mode.h | 9 +- launcher/net/Sink.h | 33 +++---- 9 files changed, 225 insertions(+), 297 deletions(-) diff --git a/launcher/net/ByteArraySink.h b/launcher/net/ByteArraySink.h index 75a66574..8ae30bb3 100644 --- a/launcher/net/ByteArraySink.h +++ b/launcher/net/ByteArraySink.h @@ -6,6 +6,8 @@ namespace Net { /* * Sink object for downloads that uses an external QByteArray it doesn't own as a target. + * FIXME: It is possible that the QByteArray is freed while we're doing some operation on it, + * causing a segmentation fault. */ class ByteArraySink : public Sink { public: @@ -16,9 +18,6 @@ class ByteArraySink : public Sink { public: auto init(QNetworkRequest& request) -> Task::State override { - if(!m_output) - return Task::State::Failed; - m_output->clear(); if (initAllValidators(request)) return Task::State::Running; @@ -27,9 +26,6 @@ class ByteArraySink : public Sink { auto write(QByteArray& data) -> Task::State override { - if(!m_output) - return Task::State::Failed; - m_output->append(data); if (writeAllValidators(data)) return Task::State::Running; @@ -38,9 +34,6 @@ class ByteArraySink : public Sink { auto abort() -> Task::State override { - if(!m_output) - return Task::State::Failed; - m_output->clear(); failAllValidators(); return Task::State::Failed; diff --git a/launcher/net/ChecksumValidato