From e7cf9932a9695417d40d895ac6174186f074f053 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 12 Aug 2022 17:06:20 -0300 Subject: [PATCH] refactor: simplify Mod structure No need to keep track of pointers left and right. A single one already gives enough headaches! Signed-off-by: flow --- launcher/CMakeLists.txt | 4 +- launcher/minecraft/mod/Mod.cpp | 50 +++----- launcher/minecraft/mod/Mod.h | 14 +-- launcher/minecraft/mod/ModDetails.h | 33 +++-- launcher/minecraft/mod/ModFolderModel.cpp | 2 +- .../minecraft/mod/tasks/LocalModParseTask.cpp | 114 +++++++++--------- .../minecraft/mod/tasks/LocalModParseTask.h | 2 +- .../minecraft/mod/tasks/ModFolderLoadTask.cpp | 4 +- .../minecraft/mod/tasks/ModFolderLoadTask.h | 2 +- 9 files changed, 107 insertions(+), 118 deletions(-) diff --git a/launcher/CMakeLists.txt b/launcher/CMakeLists.txt index badd0eaa..4f5fa2fc 100644 --- a/launcher/CMakeLists.txt +++ b/launcher/CMakeLists.txt @@ -381,8 +381,8 @@ ecm_add_test(minecraft/Library_test.cpp LINK_LIBRARIES Launcher_logic Qt${QT_VER # FIXME: shares data with FileSystem test # TODO: needs testdata -ecm_add_test(minecraft/mod/ModFolderModel_test.cpp LINK_LIBRARIES Launcher_logic Qt${QT_VERSION_MAJOR}::Test - TEST_NAME ModFolderModel) +ecm_add_test(minecraft/mod/ResourceFolderModel_test.cpp LINK_LIBRARIES Launcher_logic Qt${QT_VERSION_MAJOR}::Test + TEST_NAME ResourceFolderModel) ecm_add_test(minecraft/ParseUtils_test.cpp LINK_LIBRARIES Launcher_logic Qt${QT_VERSION_MAJOR}::Test TEST_NAME ParseUtils) diff --git a/launcher/minecraft/mod/Mod.cpp b/launcher/minecraft/mod/Mod.cpp index ed91d999..5e186471 100644 --- a/launcher/minecraft/mod/Mod.cpp +++ b/launcher/minecraft/mod/Mod.cpp @@ -44,13 +44,7 @@ #include "MetadataHandler.h" #include "Version.h" -namespace { - -ModDetails invalidDetails; - -} - -Mod::Mod(const QFileInfo& file) : Resource(file) +Mod::Mod(const QFileInfo& file) : Resource(file), m_local_details() { m_enabled = (file.suffix() != "disabled"); } @@ -59,7 +53,7 @@ Mod::Mod(const QDir& mods_dir, const Metadata::ModStruct& metadata) : Mod(mods_dir.absoluteFilePath(metadata.filename)) { m_name = metadata.name; - m_temp_metadata = std::make_shared(std::move(metadata)); + m_local_details.metadata = std::make_shared(std::move(metadata)); } auto Mod::enable(bool value) -> bool @@ -95,22 +89,14 @@ auto Mod::enable(bool value) -> bool void Mod::setStatus(ModStatus status) { - if (m_localDetails) { - m_localDetails->status = status; - } else { - m_temp_status = status; - } + m_local_details.status = status; } -void Mod::setMetadata(const Metadata::ModStruct& metadata) +void Mod::setMetadata(std::shared_ptr&& metadata) { if (status() == ModStatus::NoMetadata) setStatus(ModStatus::Installed); - if (m_localDetails) { - m_localDetails->metadata = std::make_shared(std::move(metadata)); - } else { - m_temp_metadata = std::make_shared(std::move(metadata)); - } + m_local_details.metadata = metadata; } std::pair Mod::compare(const Resource& other, SortType type) const @@ -176,7 +162,7 @@ auto Mod::destroy(QDir& index_dir, bool preserve_metadata) -> bool auto Mod::details() const -> const ModDetails& { - return m_localDetails ? *m_localDetails : invalidDetails; + return m_local_details; } auto Mod::name() const -> QString @@ -213,35 +199,29 @@ auto Mod::authors() const -> QStringList auto Mod::status() const -> ModStatus { - if (!m_localDetails) - return m_temp_status; return details().status; } auto Mod::metadata() -> std::shared_ptr { - if (m_localDetails) - return m_localDetails->metadata; - return m_temp_metadata; + return m_local_details.metadata; } auto Mod::metadata() const -> const std::shared_ptr { - if (m_localDetails) - return m_localDetails->metadata; - return m_temp_metadata; + return m_local_details.metadata; } -void Mod::finishResolvingWithDetails(std::shared_ptr details) +void Mod::finishResolvingWithDetails(ModDetails&& details) { m_is_resolving = false; m_is_resolved = true; - m_localDetails = details; - setStatus(m_temp_status); + std::shared_ptr metadata = details.metadata; + if (details.status == ModStatus::Unknown) + details.status = m_local_details.status; - if (m_localDetails && m_temp_metadata && m_temp_metadata->isValid()) { - setMetadata(*m_temp_metadata); - m_temp_metadata.reset(); - } + m_local_details = std::move(details); + if (metadata) + setMetadata(std::move(metadata)); } diff --git a/launcher/minecraft/mod/Mod.h b/launcher/minecraft/mod/Mod.h index 49326c83..b9b57058 100644 --- a/launcher/minecraft/mod/Mod.h +++ b/launcher/minecraft/mod/Mod.h @@ -68,7 +68,8 @@ public: auto metadata() const -> const std::shared_ptr; void setStatus(ModStatus status); - void setMetadata(const Metadata::ModStruct& metadata); + void setMetadata(std::shared_ptr&& metadata); + void setMetadata(const Metadata::ModStruct& metadata) { setMetadata(std::make_shared(metadata)); } auto enable(bool value) -> bool; @@ -78,17 +79,10 @@ public: // Delete all the files of this mod auto destroy(QDir& index_dir, bool preserve_metadata = false) -> bool; - void finishResolvingWithDetails(std::shared_ptr details); + void finishResolvingWithDetails(ModDetails&& details); protected: - /* If the mod has metadata, this will be filled in the constructor, and passed to - * the ModDetails when calling finishResolvingWithDetails */ - std::shared_ptr m_temp_metadata; - - /* Set the mod status while it doesn't have local details just yet */ - ModStatus m_temp_status = ModStatus::NoMetadata; - - std::shared_ptr m_localDetails; + ModDetails m_local_details; bool m_enabled = true; }; diff --git a/launcher/minecraft/mod/ModDetails.h b/launcher/minecraft/mod/ModDetails.h index 3e0a7ab0..118b156f 100644 --- a/launcher/minecraft/mod/ModDetails.h +++ b/launcher/minecraft/mod/ModDetails.h @@ -46,34 +46,49 @@ enum class ModStatus { Installed, // Both JAR and Metadata are present NotInstalled, // Only the Metadata is present NoMetadata, // Only the JAR is present + Unknown, // Default status }; struct ModDetails { /* Mod ID as defined in the ModLoader-specific metadata */ - QString mod_id; + QString mod_id = {}; /* Human-readable name */ - QString name; + QString name = {}; /* Human-readable mod version */ - QString version; + QString version = {}; /* Human-readable minecraft version */ - QString mcversion; + QString mcversion = {}; /* URL for mod's home page */ - QString homeurl; + QString homeurl = {}; /* Human-readable description */ - QString description; + QString description = {}; /* List of the author's names */ - QStringList authors; + QStringList authors = {}; /* Installation status of the mod */ - ModStatus status; + ModStatus status = ModStatus::Unknown; /* Metadata information, if any */ - std::shared_ptr metadata; + std::shared_ptr metadata = nullptr; + + ModDetails() = default; + + /** Metadata should be handled manually to properly set the mod status. */ + ModDetails(ModDetails& other) + : mod_id(other.mod_id) + , name(other.name) + , version(other.version) + , mcversion(other.mcversion) + , homeurl(other.homeurl) + , description(other.description) + , authors(other.authors) + , status(other.status) + {} }; diff --git a/launcher/minecraft/mod/ModFolderModel.cpp b/launcher/minecraft/mod/ModFolderModel.cpp index 8bdab16e..9e07dc89 100644 --- a/launcher/minecraft/mod/ModFolderModel.cpp +++ b/launcher/minecraft/mod/ModFolderModel.cpp @@ -296,7 +296,7 @@ void ModFolderModel::onParseSucceeded(int ticket, QString mod_id) auto result = cast_task->result(); if (result && resource) - resource->finishResolvingWithDetails(result->details); + resource->finishResolvingWithDetails(std::move(result->details)); emit dataChanged(index(row), index(row, columnCount(QModelIndex()) - 1)); diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp b/launcher/minecraft/mod/tasks/LocalModParseTask.cpp index fe3716ce..8a0273c9 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp +++ b/launcher/minecraft/mod/tasks/LocalModParseTask.cpp @@ -20,22 +20,22 @@ namespace { // OLD format: // https://github.com/MinecraftForge/FML/wiki/FML-mod-information-file/5bf6a2d05145ec79387acc0d45c958642fb049fc -std::shared_ptr ReadMCModInfo(QByteArray contents) +ModDetails ReadMCModInfo(QByteArray contents) { - auto getInfoFromArray = [&](QJsonArray arr)->std::shared_ptr + auto getInfoFromArray = [&](QJsonArray arr) -> ModDetails { if (!arr.at(0).isObject()) { - return nullptr; + return {}; } - std::shared_ptr details = std::make_shared(); + ModDetails details; auto firstObj = arr.at(0).toObject(); - details->mod_id = firstObj.value("modid").toString(); + details.mod_id = firstObj.value("modid").toString(); auto name = firstObj.value("name").toString(); // NOTE: ignore stupid example mods copies where the author didn't even bother to change the name if(name != "Example Mod") { - details->name = name; + details.name = name; } - details->version = firstObj.value("version").toString(); + details.version = firstObj.value("version").toString(); auto homeurl = firstObj.value("url").toString().trimmed(); if(!homeurl.isEmpty()) { @@ -45,8 +45,8 @@ std::shared_ptr ReadMCModInfo(QByteArray contents) homeurl.prepend("http://"); } } - details->homeurl = homeurl; - details->description = firstObj.value("description").toString(); + details.homeurl = homeurl; + details.description = firstObj.value("description").toString(); QJsonArray authors = firstObj.value("authorList").toArray(); if (authors.size() == 0) { // FIXME: what is the format of this? is there any? @@ -55,7 +55,7 @@ std::shared_ptr ReadMCModInfo(QByteArray contents) for (auto author: authors) { - details->authors.append(author.toString()); + details.authors.append(author.toString()); } return details; }; @@ -83,7 +83,7 @@ std::shared_ptr ReadMCModInfo(QByteArray contents) { qCritical() << "BAD stuff happened to mod json:"; qCritical() << contents; - return nullptr; + return {}; } auto arrVal = jsonDoc.object().value("modlist"); if(arrVal.isUndefined()) { @@ -94,13 +94,13 @@ std::shared_ptr ReadMCModInfo(QByteArray contents) return getInfoFromArray(arrVal.toArray()); } } - return nullptr; + return {}; } // https://github.com/MinecraftForge/Documentation/blob/5ab4ba6cf9abc0ac4c0abd96ad187461aefd72af/docs/gettingstarted/structuring.md -std::shared_ptr ReadMCModTOML(QByteArray contents) +ModDetails ReadMCModTOML(QByteArray contents) { - std::shared_ptr details = std::make_shared(); + ModDetails details; char errbuf[200]; // top-level table @@ -108,7 +108,7 @@ std::shared_ptr ReadMCModTOML(QByteArray contents) if(!tomlData) { - return nullptr; + return {}; } // array defined by [[mods]] @@ -116,7 +116,7 @@ std::shared_ptr ReadMCModTOML(QByteArray contents) if(!tomlModsArr) { qWarning() << "Corrupted mods.toml? Couldn't find [[mods]] array!"; - return nullptr; + return {}; } // we only really care about the first element, since multiple mods in one file is not supported by us at the moment @@ -124,33 +124,33 @@ std::shared_ptr ReadMCModTOML(QByteArray contents) if(!tomlModsTable0) { qWarning() << "Corrupted mods.toml? [[mods]] didn't have an element at index 0!"; - return nullptr; + return {}; } // mandatory properties - always in [[mods]] toml_datum_t modIdDatum = toml_string_in(tomlModsTable0, "modId"); if(modIdDatum.ok) { - details->mod_id = modIdDatum.u.s; + details.mod_id = modIdDatum.u.s; // library says this is required for strings free(modIdDatum.u.s); } toml_datum_t versionDatum = toml_string_in(tomlModsTable0, "version"); if(versionDatum.ok) { - details->version = versionDatum.u.s; + details.version = versionDatum.u.s; free(versionDatum.u.s); } toml_datum_t displayNameDatum = toml_string_in(tomlModsTable0, "displayName"); if(displayNameDatum.ok) { - details->name = displayNameDatum.u.s; + details.name = displayNameDatum.u.s; free(displayNameDatum.u.s); } toml_datum_t descriptionDatum = toml_string_in(tomlModsTable0, "description"); if(descriptionDatum.ok) { - details->description = descriptionDatum.u.s; + details.description = descriptionDatum.u.s; free(descriptionDatum.u.s); } @@ -173,7 +173,7 @@ std::shared_ptr ReadMCModTOML(QByteArray contents) } if(!authors.isEmpty()) { - details->authors.append(authors); + details.authors.append(authors); } toml_datum_t homeurlDatum = toml_string_in(tomlData, "displayURL"); @@ -200,7 +200,7 @@ std::shared_ptr ReadMCModTOML(QByteArray contents) homeurl.prepend("http://"); } } - details->homeurl = homeurl; + details.homeurl = homeurl; // this seems to be recursive, so it should free everything toml_free(tomlData); @@ -209,20 +209,20 @@ std::shared_ptr ReadMCModTOML(QByteArray contents) } // https://fabricmc.net/wiki/documentation:fabric_mod_json -std::shared_ptr ReadFabricModInfo(QByteArray contents) +ModDetails ReadFabricModInfo(QByteArray contents) { QJsonParseError jsonError; QJsonDocument jsonDoc = QJsonDocument::fromJson(contents, &jsonError); auto object = jsonDoc.object(); auto schemaVersion = object.contains("schemaVersion") ? object.value("schemaVersion").toInt(0) : 0; - std::shared_ptr details = std::make_shared(); + ModDetails details; - details->mod_id = object.value("id").toString(); - details->version = object.value("version").toString(); + details.mod_id = object.value("id").toString(); + details.version = object.value("version").toString(); - details->name = object.contains("name") ? object.value("name").toString() : details->mod_id; - details->description = object.value("description").toString(); + details.name = object.contains("name") ? object.value("name").toString() : details.mod_id; + details.description = object.value("description").toString(); if (schemaVersion >= 1) { @@ -230,10 +230,10 @@ std::shared_ptr ReadFabricModInfo(QByteArray contents) for (auto author: authors) { if(author.isObject()) { - details->authors.append(author.toObject().value("name").toString()); + details.authors.append(author.toObject().value("name").toString()); } else { - details->authors.append(author.toString()); + details.authors.append(author.toString()); } } @@ -243,7 +243,7 @@ std::shared_ptr ReadFabricModInfo(QByteArray contents) if (contact.contains("homepage")) { - details->homeurl = contact.value("homepage").toString(); + details.homeurl = contact.value("homepage").toString(); } } } @@ -251,50 +251,50 @@ std::shared_ptr ReadFabricModInfo(QByteArray contents) } // https://github.com/QuiltMC/rfcs/blob/master/specification/0002-quilt.mod.json.md -std::shared_ptr ReadQuiltModInfo(QByteArray contents) +ModDetails ReadQuiltModInfo(QByteArray contents) { QJsonParseError jsonError; QJsonDocument jsonDoc = QJsonDocument::fromJson(contents, &jsonError); auto object = Json::requireObject(jsonDoc, "quilt.mod.json"); auto schemaVersion = Json::ensureInteger(object.value("schema_version"), 0, "Quilt schema_version"); - std::shared_ptr details = std::make_shared(); + ModDetails details; // https://github.com/QuiltMC/rfcs/blob/be6ba280d785395fefa90a43db48e5bfc1d15eb4/specification/0002-quilt.mod.json.md if (schemaVersion == 1) { auto modInfo = Json::requireObject(object.value("quilt_loader"), "Quilt mod info"); - details->mod_id = Json::requireString(modInfo.value("id"), "Mod ID"); - details->version = Json::requireString(modInfo.value("version"), "Mod version"); + details.mod_id = Json::requireString(modInfo.value("id"), "Mod ID"); + details.version = Json::requireString(modInfo.value("version"), "Mod version"); auto modMetadata = Json::ensureObject(modInfo.value("metadata")); - details->name = Json::ensureString(modMetadata.value("name"), details->mod_id); - details->description = Json::ensureString(modMetadata.value("description")); + details.name = Json::ensureString(modMetadata.value("name"), details.mod_id); + details.description = Json::ensureString(modMetadata.value("description")); auto modContributors = Json::ensureObject(modMetadata.value("contributors")); // We don't really care about the role of a contributor here - details->authors += modContributors.keys(); + details.authors += modContributors.keys(); auto modContact = Json::ensureObject(modMetadata.value("contact")); if (modContact.contains("homepage")) { - details->homeurl = Json::requireString(modContact.value("homepage")); + details.homeurl = Json::requireString(modContact.value("homepage")); } } return details; } -std::shared_ptr ReadForgeInfo(QByteArray contents) +ModDetails ReadForgeInfo(QByteArray contents) { - std::shared_ptr details = std::make_shared(); + ModDetails details; // Read the data - details->name = "Minecraft Forge"; - details->mod_id = "Forge"; - details->homeurl = "http://www.minecraftforge.net/forum/"; + details.name = "Minecraft Forge"; + details.mod_id = "Forge"; + details.homeurl = "http://www.minecraftforge.net/forum/"; INIFile ini; if (!ini.loadFile(contents)) return details; @@ -304,35 +304,35 @@ std::shared_ptr ReadForgeInfo(QByteArray contents) QString revision = ini.get("forge.revision.number", "0").toString(); QString build = ini.get("forge.build.number", "0").toString(); - details->version = major + "." + minor + "." + revision + "." + build; + details.version = major + "." + minor + "." + revision + "." + build; return details; } -std::shared_ptr ReadLiteModInfo(QByteArray contents) +ModDetails ReadLiteModInfo(QByteArray contents) { - std::shared_ptr details = std::make_shared(); + ModDetails details; QJsonParseError jsonError; QJsonDocument jsonDoc = QJsonDocument::fromJson(contents, &jsonError); auto object = jsonDoc.object(); if (object.contains("name")) { - details->mod_id = details->name = object.value("name").toString(); + details.mod_id = details.name = object.value("name").toString(); } if (object.contains("version")) { - details->version = object.value("version").toString(""); + details.version = object.value("version").toString(""); } else { - details->version = object.value("revision").toString(""); + details.version = object.value("revision").toString(""); } - details->mcversion = object.value("mcversion").toString(); + details.mcversion = object.value("mcversion").toString(); auto author = object.value("author").toString(); if(!author.isEmpty()) { - details->authors.append(author); + details.authors.append(author); } - details->description = object.value("description").toString(); - details->homeurl = object.value("url").toString(); + details.description = object.value("description").toString(); + details.homeurl = object.value("url").toString(); return details; } @@ -366,7 +366,7 @@ void LocalModParseTask::processAsZip() file.close(); // to replace ${file.jarVersion} with the actual version, as needed - if (m_result->details && m_result->details->version == "${file.jarVersion}") + if (m_result->details.version == "${file.jarVersion}") { if (zip.setCurrentFile("META-INF/MANIFEST.MF")) { @@ -395,7 +395,7 @@ void LocalModParseTask::processAsZip() manifestVersion = "NONE"; } - m_result->details->version = manifestVersion; + m_result->details.version = manifestVersion; file.close(); } diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.h b/launcher/minecraft/mod/tasks/LocalModParseTask.h index dbecb449..e0a10218 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.h +++ b/launcher/minecraft/mod/tasks/LocalModParseTask.h @@ -13,7 +13,7 @@ class LocalModParseTask : public Task Q_OBJECT public: struct Result { - std::shared_ptr details; + ModDetails details; }; using ResultPtr = std::shared_ptr; ResultPtr result() const { diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp index 44cb1d5f..a56ba8ab 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp +++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp @@ -38,8 +38,8 @@ #include "minecraft/mod/MetadataHandler.h" -ModFolderLoadTask::ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan) - : Task(nullptr, false), m_mods_dir(mods_dir), m_index_dir(index_dir), m_is_indexed(is_indexed), m_clean_orphan(clean_orphan), m_result(new Result()) +ModFolderLoadTask::ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan, QObject* parent) + : Task(parent, false), m_mods_dir(mods_dir), m_index_dir(index_dir), m_is_indexed(is_indexed), m_clean_orphan(clean_orphan), m_result(new Result()) {} void ModFolderLoadTask::executeTask() diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.h b/launcher/minecraft/mod/tasks/ModFolderLoadTask.h index 86f3f67f..840e95e1 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.h +++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.h @@ -57,7 +57,7 @@ public: } public: - ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan = false); + ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan = false, QObject* parent = nullptr); void executeTask() override;