From c3ceefbafbbeb3d31630ef329405ebaacdf9fce5 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 12 Aug 2022 17:09:56 -0300 Subject: [PATCH] refactor+fix: add new tests for Resource models and fix issues Signed-off-by: flow --- launcher/minecraft/mod/ModFolderModel.cpp | 18 +- .../minecraft/mod/ModFolderModel_test.cpp | 119 ---------- .../minecraft/mod/ResourceFolderModel.cpp | 46 ++-- launcher/minecraft/mod/ResourceFolderModel.h | 40 ++-- .../mod/ResourceFolderModel_test.cpp | 219 ++++++++++++++++++ .../minecraft/mod/tasks/BasicFolderLoadTask.h | 13 +- .../minecraft/mod/tasks/LocalModParseTask.cpp | 12 +- .../minecraft/mod/tasks/LocalModParseTask.h | 5 + .../minecraft/mod/testdata/supercoolmod.jar | Bin 0 -> 14 bytes 9 files changed, 313 insertions(+), 159 deletions(-) delete mode 100644 launcher/minecraft/mod/ModFolderModel_test.cpp create mode 100644 launcher/minecraft/mod/ResourceFolderModel_test.cpp create mode 100644 launcher/minecraft/mod/testdata/supercoolmod.jar diff --git a/launcher/minecraft/mod/ModFolderModel.cpp b/launcher/minecraft/mod/ModFolderModel.cpp index 9e07dc89..15c713b8 100644 --- a/launcher/minecraft/mod/ModFolderModel.cpp +++ b/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); } diff --git a/launcher/minecraft/mod/ModFolderModel_test.cpp b/launcher/minecraft/mod/ModFolderModel_test.cpp deleted file mode 100644 index 1b50ebd6..00000000 --- a/launcher/minecraft/mod/ModFolderModel_test.cpp +++ /dev/null @@ -1,119 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0-only -/* -* PolyMC - Minecraft Launcher -* Copyright (C) 2022 Sefa Eyeoglu -* -* 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 . -* -* 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 -#include -#include - -#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" diff --git a/launcher/minecraft/mod/ResourceFolderModel.cpp b/launcher/minecraft/mod/ResourceFolderModel.cpp index c27a5e2d..b7213c47 100644 --- a/launcher/minecraft/mod/ResourceFolderModel.cpp +++ b/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) - return resource->destroy(); + if (resource->fileinfo().fileName() == file_name) { + 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(); diff --git a/launcher/minecraft/mod/ResourceFolderModel.h b/launcher/minecraft/mod/ResourceFolderModel.h index 59d2388a..b3a474ba 100644 --- a/launcher/minecraft/mod/ResourceFolderModel.h +++ b/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& current_set, QSet QSet kept_set = current_set; kept_set.intersect(new_set); - for (auto& kept : kept_set) { - auto row = m_resources_index[kept]; + for (auto const& kept : kept_set) { + 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& current_set, QSet // 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; - resolveResource(new_resource); + m_resources[row].reset(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& current_set, QSet for (auto& removed : removed_set) removed_rows.append(m_resources_index[removed]); - std::sort(removed_rows.begin(), removed_rows.end()); - - for (int i = 0; i < removed_rows.size(); i++) - removed_rows[i] -= i; + std::sort(removed_rows.begin(), removed_rows.end(), std::greater()); 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(); } } diff --git a/launcher/minecraft/mod/ResourceFolderModel_test.cpp b/launcher/minecraft/mod/ResourceFolderModel_test.cpp new file mode 100644 index 00000000..5e29e6aa --- /dev/null +++ b/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 +* +* 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 . +* +* 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 +#include +#include + +#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" diff --git a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h index 2944c747..cc02a9b9 100644 --- a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h +++ b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h @@ -17,7 +17,7 @@ class BasicFolderLoadTask : public Task Q_OBJECT public: struct Result { - QMap resources; + QMap resources; }; using ResultPtr = std::shared_ptr; @@ -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; }; diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp b/launcher/minecraft/mod/tasks/LocalModParseTask.cpp index 8a0273c9..c486bd46 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp +++ b/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(); } diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.h b/launcher/minecraft/mod/tasks/LocalModParseTask.h index e0a10218..4bbf3c85 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.h +++ b/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; }; diff --git a/launcher/minecraft/mod/testdata/supercoolmod.jar b/launcher/minecraft/mod/testdata/supercoolmod.jar new file mode 100644 index 0000000000000000000000000000000000000000..d8cf98605c15cb63a70aeb34bc121e1427cd5055 GIT binary patch literal 14 VcmXTPNL5HmEiO^W%}>$e0stl+1gHQ2 literal 0 HcmV?d00001