refactor+fix: add new tests for Resource models and fix issues

Signed-off-by: flow <flowlnlnln@gmail.com>

launcher/minecraft/mod/ModFolderModel.cpp

@@ -165,7 +165,7 @@ int ModFolderModel::columnCount(const QModelIndex &parent) const
 Task* ModFolderModel::createUpdateTask()
 {
     auto index_dir = indexDir();
-    auto task = new ModFolderLoadTask(dir(), index_dir, m_is_indexed, m_first_folder_load);
+    auto task = new ModFolderLoadTask(dir(), index_dir, m_is_indexed, m_first_folder_load, this);
     m_first_folder_load = false;
     return task;
 }
@@ -181,6 +181,9 @@ bool ModFolderModel::uninstallMod(const QString& filename, bool preserve_metadat
         if(mod->fileinfo().fileName() == filename){
             auto index_dir = indexDir();
             mod->destroy(index_dir, preserve_metadata);
+
+            update();
+
             return true;
         }
     }
@@ -206,6 +209,9 @@ bool ModFolderModel::deleteMods(const QModelIndexList& indexes)
         auto index_dir = indexDir();
         m->destroy(index_dir);
     }
+
+    update();
+
     return true;
 }
 
@@ -268,14 +274,13 @@ void ModFolderModel::onUpdateSucceeded()
 
     applyUpdates(current_set, new_set, new_mods);
     
-    update_results.reset();
     m_current_update_task.reset();
 
-    emit updateFinished();
+    if (m_scheduled_update) {
-
-    if(m_scheduled_update) {
         m_scheduled_update = false;
         update();
+    } else {
+        emit updateFinished();
     }
 }
 
@@ -299,9 +304,6 @@ void ModFolderModel::onParseSucceeded(int ticket, QString mod_id)
         resource->finishResolvingWithDetails(std::move(result->details));
 
     emit dataChanged(index(row), index(row, columnCount(QModelIndex()) - 1));
-
-    parse_task->deleteLater();
-    m_active_parse_tasks.remove(ticket);
 }
 
 

launcher/minecraft/mod/ModFolderModel_test.cpp

@@ -1,119 +0,0 @@
-// SPDX-License-Identifier: GPL-3.0-only
-/*
-*  PolyMC - Minecraft Launcher
-*  Copyright (C) 2022 Sefa Eyeoglu <contact@scrumplex.net>
-*
-*  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.
-*
-*  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.
-*
-*  You should have received a copy of the GNU General Public License
-*  along with this program.  If not, see <https://www.gnu.org/licenses/>.
-*
-* 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 <QTest>
-#include <QTemporaryDir>
-#include <QTimer>
-
-#include "FileSystem.h"
-#include "minecraft/mod/ModFolderModel.h"
-
-class ModFolderModelTest : public QObject
-{
-    Q_OBJECT
-
-private
-slots:
-    // test for GH-1178 - install a folder with files to a mod list
-    void test_1178()
-    {
-        // source
-        QString source = QFINDTESTDATA("testdata/test_folder");
-
-        // sanity check
-        QVERIFY(!source.endsWith('/'));
-
-        auto verify = [](QString path)
-        {
-            QDir target_dir(FS::PathCombine(path, "test_folder"));
-            QVERIFY(target_dir.entryList().contains("pack.mcmeta"));
-            QVERIFY(target_dir.entryList().contains("assets"));
-        };
-
-        // 1. test with no trailing /
-        {
-            QString folder = source;
-            QTemporaryDir tempDir;
-
-            QEventLoop loop;
-
-            ModFolderModel m(tempDir.path(), true);
-
-            connect(&m, &ModFolderModel::updateFinished, &loop, &QEventLoop::quit);
-
-            QTimer expire_timer;
-            expire_timer.callOnTimeout(&loop, &QEventLoop::quit);
-            expire_timer.setSingleShot(true);
-            expire_timer.start(4000);
-
-            m.installMod(folder);
-
-            loop.exec();
-
-            QVERIFY2(expire_timer.isActive(), "Timer has expired. The update never finished.");
-            expire_timer.stop();
-
-            verify(tempDir.path());
-        }
-
-        // 2. test with trailing /
-        {
-            QString folder = source + '/';
-            QTemporaryDir tempDir;
-            QEventLoop loop;
-            ModFolderModel m(tempDir.path(), true);
-
-            connect(&m, &ModFolderModel::updateFinished, &loop, &QEventLoop::quit);
-
-            QTimer expire_timer;
-            expire_timer.callOnTimeout(&loop, &QEventLoop::quit);
-            expire_timer.setSingleShot(true);
-            expire_timer.start(4000);
-
-            m.installMod(folder);
-
-            loop.exec();
-
-            QVERIFY2(expire_timer.isActive(), "Timer has expired. The update never finished.");
-            expire_timer.stop();
-
-            verify(tempDir.path());
-        }
-    }
-};
-
-QTEST_GUILESS_MAIN(ModFolderModelTest)
-
-#include "ModFolderModel_test.moc"

launcher/minecraft/mod/ResourceFolderModel.cpp

@@ -24,8 +24,6 @@ bool ResourceFolderModel::startWatching(const QStringList paths)
     if (m_is_watching)
         return false;
 
-    update();
-
     auto couldnt_be_watched = m_watcher.addPaths(paths);
     for (auto path : paths) {
         if (couldnt_be_watched.contains(path))
@@ -34,6 +32,8 @@ bool ResourceFolderModel::startWatching(const QStringList paths)
             qDebug() << "Started watching " << path;
     }
 
+    update();
+
     m_is_watching = !m_is_watching;
     return m_is_watching;
 }
@@ -105,7 +105,8 @@ bool ResourceFolderModel::installResource(QString original_path)
             QFileInfo new_path_file_info(new_path);
             resource.setFile(new_path_file_info);
 
-            update();
+            if (!m_is_watching)
+                return update();
 
             return true;
         }
@@ -123,7 +124,8 @@ bool ResourceFolderModel::installResource(QString original_path)
             QFileInfo newpathInfo(new_path);
             resource.setFile(newpathInfo);
 
-            update();
+            if (!m_is_watching)
+                return update();
 
             return true;
         }
@@ -136,8 +138,13 @@ bool ResourceFolderModel::installResource(QString original_path)
 bool ResourceFolderModel::uninstallResource(QString file_name)
 {
     for (auto& resource : m_resources) {
-        if (resource->fileinfo().fileName() == file_name)
+        if (resource->fileinfo().fileName() == file_name) {
-            return resource->destroy();
+            auto res = resource->destroy();
+
+            update();
+
+            return res;
+        }
     }
     return false;
 }
@@ -156,13 +163,21 @@ bool ResourceFolderModel::deleteResources(const QModelIndexList& indexes)
         }
 
         auto& resource = m_resources.at(i.row());
+
         resource->destroy();
     }
+
+    update();
+
     return true;
 }
 
+static QMutex s_update_task_mutex;
 bool ResourceFolderModel::update()
 {
+    // We hold a lock here to prevent race conditions on the m_current_update_task reset.
+    QMutexLocker lock(&s_update_task_mutex);
+
     // Already updating, so we schedule a future update and return.
     if (m_current_update_task) {
         m_scheduled_update = true;
@@ -183,7 +198,7 @@ bool ResourceFolderModel::update()
     return true;
 }
 
-void ResourceFolderModel::resolveResource(Resource::WeakPtr res)
+void ResourceFolderModel::resolveResource(Resource::Ptr res)
 {
     if (!res->shouldResolve()) {
         return;
@@ -205,6 +220,8 @@ void ResourceFolderModel::resolveResource(Resource::WeakPtr res)
         task, &Task::succeeded, this, [=] { onParseSucceeded(ticket, res->internal_id()); }, Qt::ConnectionType::QueuedConnection);
     connect(
         task, &Task::failed, this, [=] { onParseFailed(ticket, res->internal_id()); }, Qt::ConnectionType::QueuedConnection);
+    connect(
+        task, &Task::finished, this, [=] { m_active_parse_tasks.remove(ticket); }, Qt::ConnectionType::QueuedConnection);
 
     auto* thread_pool = QThreadPool::globalInstance();
     thread_pool->start(task);
@@ -229,15 +246,13 @@ void ResourceFolderModel::onUpdateSucceeded()
 
     applyUpdates(current_set, new_set, new_resources);
 
-    update_results.reset();
-    m_current_update_task->deleteLater();
     m_current_update_task.reset();
 
-    emit updateFinished();
-
     if (m_scheduled_update) {
         m_scheduled_update = false;
         update();
+    } else {
+        emit updateFinished();
     }
 }
 
@@ -247,9 +262,6 @@ void ResourceFolderModel::onParseSucceeded(int ticket, QString resource_id)
     if (iter == m_active_parse_tasks.constEnd())
         return;
 
-    (*iter)->deleteLater();
-    m_active_parse_tasks.remove(ticket);
-
     int row = m_resources_index[resource_id];
     emit dataChanged(index(row), index(row, columnCount(QModelIndex()) - 1));
 }
@@ -259,6 +271,12 @@ Task* ResourceFolderModel::createUpdateTask()
     return new BasicFolderLoadTask(m_dir);
 }
 
+
+bool ResourceFolderModel::hasPendingParseTasks() const
+{
+    return !m_active_parse_tasks.isEmpty();
+}
+
 void ResourceFolderModel::directoryChanged(QString path)
 {
     update();

launcher/minecraft/mod/ResourceFolderModel.h

@@ -62,7 +62,7 @@ class ResourceFolderModel : public QAbstractListModel {
     virtual bool update();
 
     /** Creates a new parse task, if needed, for 'res' and start it.*/
-    virtual void resolveResource(Resource::WeakPtr res);
+    virtual void resolveResource(Resource::Ptr res);
 
     [[nodiscard]] size_t size() const { return m_resources.size(); };
     [[nodiscard]] bool empty() const { return size() == 0; }
@@ -71,6 +71,13 @@ class ResourceFolderModel : public QAbstractListModel {
 
     [[nodiscard]] QDir const& dir() const { return m_dir; }
 
+    /** Checks whether there's any parse tasks being done.
+     *
+     *  Since they can be quite expensive, and are usually done in a separate thread, if we were to destroy the model while having
+     *  such tasks would introduce an undefined behavior, most likely resulting in a crash.
+     */
+    [[nodiscard]] bool hasPendingParseTasks() const;
+
     /* Qt behavior */
 
     /* Basic columns */
@@ -228,10 +235,12 @@ void ResourceFolderModel::applyUpdates(QSet<QString>& current_set, QSet<QString>
         QSet<QString> kept_set = current_set;
         kept_set.intersect(new_set);
 
-        for (auto& kept : kept_set) {
+        for (auto const& kept : kept_set) {
-            auto row = m_resources_index[kept];
+            auto row_it = m_resources_index.constFind(kept);
+            Q_ASSERT(row_it != m_resources_index.constEnd());
+            auto row = row_it.value();
 
-            auto new_resource = new_resources[kept];
+            auto& new_resource = new_resources[kept];
             auto const& current_resource = m_resources[row];
 
             if (new_resource->dateTimeChanged() == current_resource->dateTimeChanged()) {
@@ -242,11 +251,12 @@ void ResourceFolderModel::applyUpdates(QSet<QString>& current_set, QSet<QString>
             // If the resource is resolving, but something about it changed, we don't want to
             // continue the resolving.
             if (current_resource->isResolving()) {
-                m_active_parse_tasks.remove(current_resource->resolutionTicket());
+                auto task = (*m_active_parse_tasks.find(current_resource->resolutionTicket())).get();
+                task->abort();
             }
 
-            m_resources[row] = new_resource;
+            m_resources[row].reset(new_resource);
-            resolveResource(new_resource);
+            resolveResource(m_resources.at(row));
             emit dataChanged(index(row, 0), index(row, columnCount(QModelIndex()) - 1));
         }
     }
@@ -260,21 +270,21 @@ void ResourceFolderModel::applyUpdates(QSet<QString>& current_set, QSet<QString>
         for (auto& removed : removed_set)
             removed_rows.append(m_resources_index[removed]);
 
-        std::sort(removed_rows.begin(), removed_rows.end());
+        std::sort(removed_rows.begin(), removed_rows.end(), std::greater<int>());
-
-        for (int i = 0; i < removed_rows.size(); i++)
-            removed_rows[i] -= i;
 
         for (auto& removed_index : removed_rows) {
-            beginRemoveRows(QModelIndex(), removed_index, removed_index);
-
             auto removed_it = m_resources.begin() + removed_index;
+
+            Q_ASSERT(removed_it != m_resources.end());
+            Q_ASSERT(removed_set.contains(removed_it->get()->internal_id()));
+
             if ((*removed_it)->isResolving()) {
-                m_active_parse_tasks.remove((*removed_it)->resolutionTicket());
+                auto task = (*m_active_parse_tasks.find((*removed_it)->resolutionTicket())).get();
+                task->abort();
             }
 
+            beginRemoveRows(QModelIndex(), removed_index, removed_index);
             m_resources.erase(removed_it);
-
             endRemoveRows();
         }
     }

launcher/minecraft/mod/ResourceFolderModel_test.cpp

@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-3.0-only
+/*
+*  PolyMC - Minecraft Launcher
+*  Copyright (C) 2022 Sefa Eyeoglu <contact@scrumplex.net>
+*
+*  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.
+*
+*  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.
+*
+*  You should have received a copy of the GNU General Public License
+*  along with this program.  If not, see <https://www.gnu.org/licenses/>.
+*
+* 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 <QTest>
+#include <QTemporaryDir>
+#include <QTimer>
+
+#include "FileSystem.h"
+
+#include "minecraft/mod/ModFolderModel.h"
+#include "minecraft/mod/ResourceFolderModel.h"
+
+#define EXEC_UPDATE_TASK(EXEC, VERIFY)                                                  \
+    QEventLoop loop;                                                                    \
+                                                                                        \
+    connect(&model, &ResourceFolderModel::updateFinished, &loop, &QEventLoop::quit);    \
+                                                                                        \
+    QTimer expire_timer;                                                                \
+    expire_timer.callOnTimeout(&loop, &QEventLoop::quit);                               \
+    expire_timer.setSingleShot(true);                                                   \
+    expire_timer.start(4000);                                                           \
+                                                                                        \
+    VERIFY(EXEC);                                                                       \
+    loop.exec();                                                                        \
+                                                                                        \
+    QVERIFY2(expire_timer.isActive(), "Timer has expired. The update never finished."); \
+    expire_timer.stop();                                                                \
+                                                                                        \
+    disconnect(&model, nullptr, nullptr, nullptr);
+
+class ResourceFolderModelTest : public QObject
+{
+    Q_OBJECT
+
+private
+slots:
+    // test for GH-1178 - install a folder with files to a mod list
+    void test_1178()
+    {
+        // source
+        QString source = QFINDTESTDATA("testdata/test_folder");
+
+        // sanity check
+        QVERIFY(!source.endsWith('/'));
+
+        auto verify = [](QString path)
+        {
+            QDir target_dir(FS::PathCombine(path, "test_folder"));
+            QVERIFY(target_dir.entryList().contains("pack.mcmeta"));
+            QVERIFY(target_dir.entryList().contains("assets"));
+        };
+
+        // 1. test with no trailing /
+        {
+            QString folder = source;
+            QTemporaryDir tempDir;
+
+            QEventLoop loop;
+
+            ModFolderModel m(tempDir.path(), true);
+
+            connect(&m, &ModFolderModel::updateFinished, &loop, &QEventLoop::quit);
+
+            QTimer expire_timer;
+            expire_timer.callOnTimeout(&loop, &QEventLoop::quit);
+            expire_timer.setSingleShot(true);
+            expire_timer.start(4000);
+
+            m.installMod(folder);
+
+            loop.exec();
+
+            QVERIFY2(expire_timer.isActive(), "Timer has expired. The update never finished.");
+            expire_timer.stop();
+
+            verify(tempDir.path());
+        }
+
+        // 2. test with trailing /
+        {
+            QString folder = source + '/';
+            QTemporaryDir tempDir;
+            QEventLoop loop;
+            ModFolderModel m(tempDir.path(), true);
+
+            connect(&m, &ModFolderModel::updateFinished, &loop, &QEventLoop::quit);
+
+            QTimer expire_timer;
+            expire_timer.callOnTimeout(&loop, &QEventLoop::quit);
+            expire_timer.setSingleShot(true);
+            expire_timer.start(4000);
+
+            m.installMod(folder);
+
+            loop.exec();
+
+            QVERIFY2(expire_timer.isActive(), "Timer has expired. The update never finished.");
+            expire_timer.stop();
+
+            verify(tempDir.path());
+        }
+    }
+
+    void test_addFromWatch()
+    {
+        QString source = QFINDTESTDATA("testdata");
+
+        ModFolderModel model(source);
+
+        QCOMPARE(model.size(), 0);
+
+        EXEC_UPDATE_TASK(model.startWatching(), )
+
+        for (auto mod : model.allMods())
+            qDebug() << mod->name();
+
+        QCOMPARE(model.size(), 2);
+
+        model.stopWatching();
+
+        while (model.hasPendingParseTasks()) {
+            QTest::qSleep(20);
+            QCoreApplication::processEvents();
+        }
+    }
+
+    void test_removeResource()
+    {
+        QString folder_resource = QFINDTESTDATA("testdata/test_folder");
+        QString file_mod = QFINDTESTDATA("testdata/supercoolmod.jar");
+
+        QTemporaryDir tmp;
+
+        ResourceFolderModel model(QDir(tmp.path()));
+
+        QCOMPARE(model.size(), 0);
+
+        {
+            EXEC_UPDATE_TASK(model.installResource(file_mod), QVERIFY)
+        }
+
+        QCOMPARE(model.size(), 1);
+        qDebug() << "Added first mod.";
+
+        {
+            EXEC_UPDATE_TASK(model.startWatching(), )
+        }
+
+        QCOMPARE(model.size(), 1);
+        qDebug() << "Started watching the temp folder.";
+        
+        {
+            EXEC_UPDATE_TASK(model.installResource(folder_resource), QVERIFY)
+        }
+
+        QCOMPARE(model.size(), 2);
+        qDebug() << "Added second mod.";
+
+        {
+            EXEC_UPDATE_TASK(model.uninstallResource("supercoolmod.jar"), QVERIFY);
+        }
+
+        QCOMPARE(model.size(), 1);
+        qDebug() << "Removed first mod.";
+
+        QString mod_file_name {model.at(0).fileinfo().fileName()};
+        QVERIFY(!mod_file_name.isEmpty());
+
+        {
+            EXEC_UPDATE_TASK(model.uninstallResource(mod_file_name), QVERIFY);
+        }
+
+        QCOMPARE(model.size(), 0);
+        qDebug() << "Removed second mod.";
+
+        model.stopWatching();
+
+        while (model.hasPendingParseTasks()) {
+            QTest::qSleep(20);
+            QCoreApplication::processEvents();
+        }
+    }
+};
+
+QTEST_GUILESS_MAIN(ResourceFolderModelTest)
+
+#include "ResourceFolderModel_test.moc"

launcher/minecraft/mod/tasks/BasicFolderLoadTask.h

@@ -17,7 +17,7 @@ class BasicFolderLoadTask : public Task
     Q_OBJECT
 public:
     struct Result {
-        QMap<QString, Resource*> resources;
+        QMap<QString, Resource::Ptr> resources;
     };
     using ResultPtr = std::shared_ptr<Result>;
 
@@ -27,6 +27,10 @@ public:
 
 public:
     BasicFolderLoadTask(QDir dir) : Task(nullptr, false), m_dir(dir), m_result(new Result) {}
+
+    [[nodiscard]] bool canAbort() const override { return true; }
+    bool abort() override { m_aborted = true; return true; }
+
     void executeTask() override
     {
         m_dir.refresh();
@@ -35,10 +39,15 @@ public:
             m_result->resources.insert(resource->internal_id(), resource);
         }
 
-        emitSucceeded();
+        if (m_aborted)
+            emitAborted();
+        else
+            emitSucceeded();
     }
 
 private:
     QDir m_dir;
     ResultPtr m_result;
+
+    bool m_aborted = false;
 };

launcher/minecraft/mod/tasks/LocalModParseTask.cpp

@@ -497,6 +497,12 @@ void LocalModParseTask::processAsLitemod()
     zip.close();
 }
 
+bool LocalModParseTask::abort()
+{
+    m_aborted = true;
+    return true;
+}
+
 void LocalModParseTask::executeTask()
 {
     switch(m_type)
@@ -513,5 +519,9 @@ void LocalModParseTask::executeTask()
         default:
             break;
     }
-    emitSucceeded();
+
+    if (m_aborted)
+        emitAborted();
+    else
+        emitSucceeded();
 }

launcher/minecraft/mod/tasks/LocalModParseTask.h

@@ -20,6 +20,9 @@ public:
         return m_result;
     }
 
+    [[nodiscard]] bool canAbort() const override { return true; }
+    bool abort() override;
+
     LocalModParseTask(int token, ResourceType type, const QFileInfo & modFile);
     void executeTask() override;
 
@@ -35,4 +38,6 @@ private:
     ResourceType m_type;
     QFileInfo m_modFile;
     ResultPtr m_result;
+
+    bool m_aborted = false;
 };

launcher/minecraft/mod/testdata/supercoolmod.jar

@@ -0,0 +1 @@
+the best mod.