Merge pull request #966 from flowln/refactor_tasks

Reduce code duplication in tasks, fix some bugs and add some tests

7 files changed, 183 insertions, 193 deletions

diff --git a/launcher/tasks/ConcurrentTask.cpp b/launcher/tasks/ConcurrentTask.cpp
index ab7cbd03..484ac58e 100644
--- a/launcher/tasks/ConcurrentTask.cpp
+++ b/launcher/tasks/ConcurrentTask.cpp
@@ -37,32 +37,39 @@ void ConcurrentTask::executeTask()
 {
     m_total_size = m_queue.size();
 
-    for (int i = 0; i < m_total_max_size; i++) {
+    int num_starts = std::min(m_total_max_size, m_total_size);
+    for (int i = 0; i < num_starts; i++) {
         QMetaObject::invokeMethod(this, &ConcurrentTask::startNext, Qt::QueuedConnection);
     }
 }
 
 bool ConcurrentTask::abort()
 {
+    m_queue.clear();
+    m_aborted = true;
+
     if (m_doing.isEmpty()) {
         // Don't call emitAborted() here, we want to bypass the 'is the task running' check
         emit aborted();
         emit finished();
 
-        m_aborted = true;
         return true;
     }
 
-    m_queue.clear();
+    bool suceedeed = true;
 
-    m_aborted = true;
-    for (auto task : m_doing)
-        m_aborted &= task->abort();
+    QMutableHashIterator<Task*, Task::Ptr> doing_iter(m_doing);
+    while (doing_iter.hasNext()) {
+        auto task = doing_iter.next();
+        suceedeed &= (task.value())->abort();
+    }
 
-    if (m_aborted)
+    if (suceedeed)
         emitAborted();
+    else
+        emitFailed(tr("Failed to abort all running tasks."));
 
-    return m_aborted;
+    return suceedeed;
 }
 
 void ConcurrentTask::startNext()
@@ -70,7 +77,7 @@ void ConcurrentTask::startNext()
     if (m_aborted || m_doing.count() > m_total_max_size)
         return;
 
-    if (m_queue.isEmpty() && m_doing.isEmpty()) {
+    if (m_queue.isEmpty() && m_doing.isEmpty() && !wasSuccessful()) {
         emitSucceeded();
         return;
     }
@@ -131,11 +138,6 @@ void ConcurrentTask::subTaskStatus(const QString& msg)
 
 void ConcurrentTask::subTaskProgress(qint64 current, qint64 total)
 {
-    if (total == 0) {
-        setProgress(0, 100);
-        return;
-    }
-
     m_stepProgress = current;
     m_stepTotalProgress = total;
 }
diff --git a/launcher/tasks/ConcurrentTask.h b/launcher/tasks/ConcurrentTask.h
index 5898899d..f1279d32 100644
--- a/launcher/tasks/ConcurrentTask.h
+++ b/launcher/tasks/ConcurrentTask.h
@@ -9,7 +9,9 @@ class ConcurrentTask : public Task {
     Q_OBJECT
 public:
     explicit ConcurrentTask(QObject* parent = nullptr, QString task_name = "", int max_concurrent = 6);
-    virtual ~ConcurrentTask();
+    ~ConcurrentTask() override;
+
+    bool canAbort() const override { return true; }
 
     inline auto isMultiStep() const -> bool override { return m_queue.size() > 1; };
     auto getStepProgress() const -> qint64 override;
diff --git a/launcher/tasks/MultipleOptionsTask.cpp b/launcher/tasks/MultipleOptionsTask.cpp
index 6e853568..5ad6181f 100644
--- a/launcher/tasks/MultipleOptionsTask.cpp
+++ b/launcher/tasks/MultipleOptionsTask.cpp
@@ -6,43 +6,22 @@ MultipleOptionsTask::MultipleOptionsTask(QObject* parent, const QString& task_na
 
 void MultipleOptionsTask::startNext()
 {
-    Task* previous = nullptr;
-    if (m_currentIndex != -1) {
-        previous = m_queue[m_currentIndex].get();
-        disconnect(previous, 0, this, 0);
-    }
-
-    m_currentIndex++;
-    if ((previous && previous->wasSuccessful())) {
+    if (m_done.size() != m_failed.size()) {
         emitSucceeded();
         return;
     }
 
-    Task::Ptr next = m_queue[m_currentIndex];
-
-    connect(next.get(), &Task::failed, this, &MultipleOptionsTask::subTaskFailed);
-    connect(next.get(), &Task::succeeded, this, &MultipleOptionsTask::startNext);
-
-    connect(next.get(), &Task::status, this, &MultipleOptionsTask::subTaskStatus);
-    connect(next.get(), &Task::stepStatus, this, &MultipleOptionsTask::subTaskStatus);
-    
-    connect(next.get(), &Task::progress, this, &MultipleOptionsTask::subTaskProgress);
-
-    qDebug() << QString("Making attemp %1 out of %2").arg(m_currentIndex + 1).arg(m_queue.size());
-    setStatus(tr("Making attempt #%1 out of %2").arg(m_currentIndex + 1).arg(m_queue.size()));
-    setStepStatus(next->isMultiStep() ? next->getStepStatus() : next->getStatus());
+    if (m_queue.isEmpty()) {
+        emitFailed(tr("All attempts have failed!"));
+        qWarning() << "All attempts have failed!";
+        return;
+    }
 
-    next->start();
+    ConcurrentTask::startNext();
 }
 
-void MultipleOptionsTask::subTaskFailed(QString const& reason)
+void MultipleOptionsTask::updateState()
 {
-    qDebug() << QString("Failed attempt #%1 of %2. Reason: %3").arg(m_currentIndex + 1).arg(m_queue.size()).arg(reason);
-    if(m_currentIndex < m_queue.size() - 1) {
-        startNext();
-        return;
-    }
-
-    qWarning() << QString("All attempts have failed!");
-    emitFailed();
+    setProgress(m_done.count(), m_total_size);
+    setStatus(tr("Attempting task %1 out of %2").arg(QString::number(m_doing.count() + m_done.count()), QString::number(m_total_size)));
 }
diff --git a/launcher/tasks/MultipleOptionsTask.h b/launcher/tasks/MultipleOptionsTask.h
index 7c508b00..db7d4d9a 100644
--- a/launcher/tasks/MultipleOptionsTask.h
+++ b/launcher/tasks/MultipleOptionsTask.h
@@ -5,15 +5,13 @@
 /* This task type will attempt to do run each of it's subtasks in sequence,
  * until one of them succeeds. When that happens, the remaining tasks will not run.
  * */
-class MultipleOptionsTask : public SequentialTask
-{
+class MultipleOptionsTask : public SequentialTask {
     Q_OBJECT
-public:
-    explicit MultipleOptionsTask(QObject *parent = nullptr, const QString& task_name = "");
-    virtual ~MultipleOptionsTask() = default;
+   public:
+    explicit MultipleOptionsTask(QObject* parent = nullptr, const QString& task_name = "");
+    ~MultipleOptionsTask() override = default;
 
-private
-slots:
+   private slots:
     void startNext() override;
-    void subTaskFailed(const QString &msg) override;
+    void updateState() override;
 };
diff --git a/launcher/tasks/SequentialTask.cpp b/launcher/tasks/SequentialTask.cpp
index f1e1a889..a34137cb 100644
--- a/launcher/tasks/SequentialTask.cpp
+++ b/launcher/tasks/SequentialTask.cpp
@@ -2,107 +2,21 @@
 
 #include <QDebug>
 
-SequentialTask::SequentialTask(QObject* parent, const QString& task_name) : Task(parent), m_name(task_name), m_currentIndex(-1) {}
-
-SequentialTask::~SequentialTask()
-{
-    for(auto task : m_queue){
-        if(task)
-            task->deleteLater();
-    }
-}
-
-auto SequentialTask::getStepProgress() const -> qint64
-{
-    return m_stepProgress;
-}
-
-auto SequentialTask::getStepTotalProgress() const -> qint64
-{
-    return m_stepTotalProgress;
-}
-
-void SequentialTask::addTask(Task::Ptr task)
-{
-    m_queue.append(task);
-}
-
-void SequentialTask::executeTask()
-{
-    m_currentIndex = -1;
-    startNext();
-}
-
-bool SequentialTask::abort()
-{
-    if(m_currentIndex == -1 || m_currentIndex >= m_queue.size()) {
-        if(m_currentIndex == -1) {
-            // Don't call emitAborted() here, we want to bypass the 'is the task running' check
-            emit aborted();
-            emit finished();
-        }
-
-        m_aborted = true;
-        return true;
-    }
-
-    bool succeeded = m_queue[m_currentIndex]->abort();
-    m_aborted = succeeded;
-
-    if (succeeded)
-        emitAborted();
-
-    return succeeded;
-}
+SequentialTask::SequentialTask(QObject* parent, QString task_name) : ConcurrentTask(parent, task_name, 1) {}
 
 void SequentialTask::startNext()
 {
-    if (m_aborted)
-        return;
-
-    if (m_currentIndex != -1 && m_currentIndex < m_queue.size()) {
-        Task::Ptr previous = m_queue.at(m_currentIndex);
-        disconnect(previous.get(), 0, this, 0);
-    }
-
-    m_currentIndex++;
-    if (m_queue.isEmpty() || m_currentIndex >= m_queue.size()) {
-        emitSucceeded();
+    if (m_failed.size() > 0) {
+        emitFailed(tr("One of the tasks failed!"));
+        qWarning() << m_failed.constBegin()->get()->failReason();
         return;
     }
-    Task::Ptr next = m_queue[m_currentIndex];
-
-    connect(next.get(), SIGNAL(failed(QString)), this, SLOT(subTaskFailed(QString)));
-    connect(next.get(), SIGNAL(succeeded()), this, SLOT(startNext()));
-
-    connect(next.get(), SIGNAL(status(QString)), this, SLOT(subTaskStatus(QString)));
-    connect(next.get(), SIGNAL(stepStatus(QString)), this, SLOT(subTaskStatus(QString)));
-    
-    connect(next.get(), SIGNAL(progress(qint64, qint64)), this, SLOT(subTaskProgress(qint64, qint64)));
 
-    setStatus(tr("Executing task %1 out of %2").arg(m_currentIndex + 1).arg(m_queue.size()));
-    setStepStatus(next->isMultiStep() ? next->getStepStatus() : next->getStatus());
-
-    setProgress(m_currentIndex + 1, m_queue.count());
-
-    next->start();
+    ConcurrentTask::startNext();
 }
 
-void SequentialTask::subTaskFailed(const QString& msg)
+void SequentialTask::updateState()
 {
-    emitFailed(msg);
-}
-void SequentialTask::subTaskStatus(const QString& msg)
-{
-    setStepStatus(msg);
-}
-void SequentialTask::subTaskProgress(qint64 current, qint64 total)
-{
-    if (total == 0) {
-        setProgress(0, 100);
-        return;
-    }
-
-    m_stepProgress = current;
-    m_stepTotalProgress = total;
+    setProgress(m_done.count(), m_total_size);
+    setStatus(tr("Executing task %1 out of %2").arg(QString::number(m_doing.count() + m_done.count()), QString::number(m_total_size)));
 }
diff --git a/launcher/tasks/SequentialTask.h b/launcher/tasks/SequentialTask.h
index f5a58b1b..5eace96e 100644
--- a/launcher/tasks/SequentialTask.h
+++ b/launcher/tasks/SequentialTask.h
@@ -1,49 +1,21 @@
 #pragma once
 
-#include "Task.h"
-#include "QObjectPtr.h"
-
-#include <QQueue>
-
-class SequentialTask : public Task
-{
+#include "ConcurrentTask.h"
+
+/** A concurrent task that only allows one concurrent task :)
+ *
+ *  This should be used when there's a need to maintain a strict ordering of task executions, and
+ *  the starting of a task is contingent on the success of the previous one.
+ *
+ *  See MultipleOptionsTask if that's not the case.
+ */
+class SequentialTask : public ConcurrentTask {
     Q_OBJECT
-public:
-    explicit SequentialTask(QObject *parent = nullptr, const QString& task_name = "");
-    virtual ~SequentialTask();
-
-    inline auto isMultiStep() const -> bool override { return m_queue.size() > 1; };
-    auto getStepProgress() const -> qint64 override;
-    auto getStepTotalProgress() const -> qint64 override;
-
-    inline auto getStepStatus() const -> QString override { return m_step_status; }
-
-    void addTask(Task::Ptr task);
-
-public slots:
-    bool abort() override;
-
-protected
-slots:
-    void executeTask() override;
-
-    virtual void startNext();
-    virtual void subTaskFailed(const QString &msg);
-    virtual void subTaskStatus(const QString &msg);
-    virtual void subTaskProgress(qint64 current, qint64 total);
-
-protected:
-    void setStepStatus(QString status) { m_step_status = status; emit stepStatus(status); };
-
-protected:
-    QString m_name;
-    QString m_step_status;
-
-    QQueue<Task::Ptr > m_queue;
-    int m_currentIndex;
-
-    qint64 m_stepProgress = 0;
-    qint64 m_stepTotalProgress = 100;
+   public:
+    explicit SequentialTask(QObject* parent = nullptr, QString task_name = "");
+    ~SequentialTask() override = default;
 
-    bool m_aborted = false;
+   protected:
+    void startNext() override;
+    void updateState() override;
 };
diff --git a/launcher/tasks/Task_test.cpp b/launcher/tasks/Task_test.cpp
index ef153a6a..b56ee8a6 100644
--- a/launcher/tasks/Task_test.cpp
+++ b/launcher/tasks/Task_test.cpp
@@ -1,5 +1,8 @@
 #include <QTest>
 
+#include "ConcurrentTask.h"
+#include "MultipleOptionsTask.h"
+#include "SequentialTask.h"
 #include "Task.h"
 
 /* Does nothing. Only used for testing. */
@@ -9,7 +12,10 @@ class BasicTask : public Task {
     friend class TaskTest;
 
    private:
-    void executeTask() override {};   
+    void executeTask() override
+    {
+        emitSucceeded();
+    };
 };
 
 /* Does nothing. Only used for testing. */
@@ -60,6 +66,123 @@ class TaskTest : public QObject {
         QCOMPARE(t.getProgress(), current);
         QCOMPARE(t.getTotalProgress(), total);
     }
+
+    void test_basicRun(){
+        BasicTask t;
+        QObject::connect(&t, &Task::finished, [&]{ QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been."); });
+        t.start();
+
+        QVERIFY2(QTest::qWaitFor([&]() {
+            return t.isFinished();
+        }, 1000), "Task didn't finish as it should.");
+    }
+
+    void test_basicConcurrentRun(){
+        BasicTask t1;
+        BasicTask t2;
+        BasicTask t3;
+
+        ConcurrentTask t;
+
+        t.addTask(&t1);
+        t.addTask(&t2);
+        t.addTask(&t3);
+
+        QObject::connect(&t, &Task::finished, [&]{
+                QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been.");
+                QVERIFY(t1.wasSuccessful());
+                QVERIFY(t2.wasSuccessful());
+                QVERIFY(t3.wasSuccessful());
+        });
+
+        t.start();
+        QVERIFY2(QTest::qWaitFor([&]() {
+            return t.isFinished();
+        }, 1000), "Task didn't finish as it should.");
+    }
+
+    // Tests if starting new tasks after the 6 initial ones is working
+    void test_moreConcurrentRun(){
+        BasicTask t1, t2, t3, t4, t5, t6, t7, t8, t9;
+
+        ConcurrentTask t;
+
+        t.addTask(&t1);
+        t.addTask(&t2);
+        t.addTask(&t3);
+        t.addTask(&t4);
+        t.addTask(&t5);
+        t.addTask(&t6);
+        t.addTask(&t7);
+        t.addTask(&t8);
+        t.addTask(&t9);
+
+        QObject::connect(&t, &Task::finished, [&]{
+                QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been.");
+                QVERIFY(t1.wasSuccessful());
+                QVERIFY(t2.wasSuccessful());
+                QVERIFY(t3.wasSuccessful());
+                QVERIFY(t4.wasSuccessful());
+                QVERIFY(t5.wasSuccessful());
+                QVERIFY(t6.wasSuccessful());
+                QVERIFY(t7.wasSuccessful());
+                QVERIFY(t8.wasSuccessful());
+                QVERIFY(t9.wasSuccessful());
+        });
+
+        t.start();
+        QVERIFY2(QTest::qWaitFor([&]() {
+            return t.isFinished();
+        }, 1000), "Task didn't finish as it should.");
+    }
+
+    void test_basicSequentialRun(){
+        BasicTask t1;
+        BasicTask t2;
+        BasicTask t3;
+
+        SequentialTask t;
+
+        t.addTask(&t1);
+        t.addTask(&t2);
+        t.addTask(&t3);
+
+        QObject::connect(&t, &Task::finished, [&]{
+                QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been.");
+                QVERIFY(t1.wasSuccessful());
+                QVERIFY(t2.wasSuccessful());
+                QVERIFY(t3.wasSuccessful());
+        });
+
+        t.start();
+        QVERIFY2(QTest::qWaitFor([&]() {
+            return t.isFinished();
+        }, 1000), "Task didn't finish as it should.");
+    }
+
+    void test_basicMultipleOptionsRun(){
+        BasicTask t1;
+        BasicTask t2;
+        BasicTask t3;
+
+        MultipleOptionsTask t;
+
+        t.addTask(&t1);
+        t.addTask(&t2);
+        t.addTask(&t3);
+
+        QObject::connect(&t, &Task::finished, [&]{
+                QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been.");
+                QVERIFY(t1.wasSuccessful());
+                QVERIFY(!t2.wasSuccessful());
+                QVERIFY(!t3.wasSuccessful());
+        });
+
+        t.start();
+        QVERIFY2(QTest::qWaitFor([&]() {
+            return t.isFinished();
+        }, 1000), "Task didn't finish as it should.");
+    }
 };
 
 QTEST_GUILESS_MAIN(TaskTest)