refactor: change the way instance names are handled

While working on pack updating, instance naming always gets in the way,
since we need both way of respecting the user's name choice, and a
standarized way of getting the original pack name / version.

This tries to circunvent such problems by abstracting away the naming
schema into it's own struct, holding both the original name / version,
and the user-defined name, so that everyone can be happy and world peace
can be achieved! (at least that's what i'd hope :c).

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

launcher/InstanceCopyTask.cpp

@@ -44,7 +44,7 @@ void InstanceCopyTask::copyFinished()
     auto instanceSettings = std::make_shared<INISettingsObject>(FS::PathCombine(m_stagingPath, "instance.cfg"));
 
     InstancePtr inst(new NullInstance(m_globalSettings, instanceSettings, m_stagingPath));
-    inst->setName(m_instName);
+    inst->setName(name());
     inst->setIconKey(m_instIcon);
     if(!m_keepPlaytime) {
         inst->resetTimePlayed();

launcher/InstanceImportTask.cpp

@@ -255,7 +255,7 @@ void InstanceImportTask::processFlame()
 {
     auto* inst_creation_task = new FlameCreationTask(m_stagingPath, m_globalSettings, m_parent);
 
-    inst_creation_task->setName(m_instName);
+    inst_creation_task->setName(*this);
     inst_creation_task->setIcon(m_instIcon);
     inst_creation_task->setGroup(m_instGroup);
     
@@ -278,7 +278,7 @@ void InstanceImportTask::processTechnic()
     shared_qobject_ptr<Technic::TechnicPackProcessor> packProcessor = new Technic::TechnicPackProcessor();
     connect(packProcessor.get(), &Technic::TechnicPackProcessor::succeeded, this, &InstanceImportTask::emitSucceeded);
     connect(packProcessor.get(), &Technic::TechnicPackProcessor::failed, this, &InstanceImportTask::emitFailed);
-    packProcessor->run(m_globalSettings, m_instName, m_instIcon, m_stagingPath);
+    packProcessor->run(m_globalSettings, name(), m_instIcon, m_stagingPath);
 }
 
 void InstanceImportTask::processMultiMC()
@@ -292,7 +292,7 @@ void InstanceImportTask::processMultiMC()
     instance.resetTimePlayed();
 
     // set a new nice name
-    instance.setName(m_instName);
+    instance.setName(name());
 
     // if the icon was specified by user, use that. otherwise pull icon from the pack
     if (m_instIcon != "default") {
@@ -317,7 +317,7 @@ void InstanceImportTask::processModrinth()
 {
     auto* inst_creation_task = new ModrinthCreationTask(m_stagingPath, m_globalSettings, m_parent, m_sourceUrl.toString());
 
-    inst_creation_task->setName(m_instName);
+    inst_creation_task->setName(*this);
     inst_creation_task->setIcon(m_instIcon);
     inst_creation_task->setGroup(m_instGroup);
     

launcher/InstanceList.cpp

@@ -778,19 +778,14 @@ class InstanceStaging : public Task {
     const unsigned minBackoff = 1;
     const unsigned maxBackoff = 16;
    public:
-    InstanceStaging(InstanceList* parent, InstanceTask* child, const QString& stagingPath, const QString& instanceName, const QString& groupName)
+    InstanceStaging(InstanceList* parent, InstanceTask* child, QString stagingPath, InstanceName const& instanceName, QString groupName)
-        : backoff(minBackoff, maxBackoff)
+        : m_parent(parent), backoff(minBackoff, maxBackoff), m_stagingPath(std::move(stagingPath)), m_instance_name(std::move(instanceName)), m_groupName(std::move(groupName))
     {
-        m_parent = parent;
         m_child.reset(child);
         connect(child, &Task::succeeded, this, &InstanceStaging::childSucceded);
         connect(child, &Task::failed, this, &InstanceStaging::childFailed);
         connect(child, &Task::status, this, &InstanceStaging::setStatus);
         connect(child, &Task::progress, this, &InstanceStaging::setProgress);
-        m_instanceName = instanceName;
-        m_groupName = groupName;
-        m_stagingPath = stagingPath;
-        m_backoffTimer.setSingleShot(true);
         connect(&m_backoffTimer, &QTimer::timeout, this, &InstanceStaging::childSucceded);
     }
 
@@ -820,7 +815,7 @@ class InstanceStaging : public Task {
     void childSucceded()
     {
         unsigned sleepTime = backoff();
-        if (m_parent->commitStagedInstance(m_stagingPath, m_instanceName, m_groupName, m_child->shouldOverride()))
+        if (m_parent->commitStagedInstance(m_stagingPath, m_instance_name, m_groupName, m_child->shouldOverride()))
         {
             emitSucceeded();
             return;
@@ -830,7 +825,7 @@ class InstanceStaging : public Task {
             emitFailed(tr("Failed to commit instance, even after multiple retries. It is being blocked by something."));
             return;
         }
-        qDebug() << "Failed to commit instance" << m_instanceName << "Initiating backoff:" << sleepTime;
+        qDebug() << "Failed to commit instance" << m_instance_name.name() << "Initiating backoff:" << sleepTime;
         m_backoffTimer.start(sleepTime * 500);
     }
     void childFailed(const QString& reason)
@@ -840,6 +835,7 @@ class InstanceStaging : public Task {
     }
 
    private:
+    InstanceList * m_parent;
     /*
      * WHY: the whole reason why this uses an exponential backoff retry scheme is antivirus on Windows.
      * Basically, it starts messing things up while the launcher is extracting/creating instances
@@ -847,9 +843,8 @@ class InstanceStaging : public Task {
      */
     ExponentialSeries backoff;
     QString m_stagingPath;
-    InstanceList * m_parent;
     unique_qobject_ptr<InstanceTask> m_child;
-    QString m_instanceName;
+    InstanceName m_instance_name;
     QString m_groupName;
     QTimer m_backoffTimer;
 };
@@ -859,7 +854,7 @@ Task* InstanceList::wrapInstanceTask(InstanceTask* task)
     auto stagingPath = getStagedInstancePath();
     task->setStagingPath(stagingPath);
     task->setParentSettings(m_globalSettings);
-    return new InstanceStaging(this, task, stagingPath, task->name(), task->group());
+    return new InstanceStaging(this, task, stagingPath, *task, task->group());
 }
 
 QString InstanceList::getStagedInstancePath()
@@ -879,22 +874,23 @@ QString InstanceList::getStagedInstancePath()
     return path;
 }
 
-bool InstanceList::commitStagedInstance(const QString& path, const QString& instanceName, const QString& groupName, bool should_override)
+bool InstanceList::commitStagedInstance(QString path, InstanceName const& instanceName, QString groupName, bool should_override)
 {
     QDir dir;
     QString instID;
     InstancePtr inst;
 
-    QString raw_inst_name = instanceName.section(' ', 0, -2);
     if (should_override) {
         // This is to avoid problems when the instance folder gets manually renamed
-        if ((inst = getInstanceByManagedName(raw_inst_name))) {
+        if ((inst = getInstanceByManagedName(instanceName.originalName()))) {
+            instID = QFileInfo(inst->instanceRoot()).fileName();
+        } else if ((inst = getInstanceByManagedName(instanceName.modifiedName()))) {
             instID = QFileInfo(inst->instanceRoot()).fileName();
         } else {
-            instID = FS::RemoveInvalidFilenameChars(raw_inst_name, '-');
+            instID = FS::RemoveInvalidFilenameChars(instanceName.modifiedName(), '-');
         }
     } else {
-        instID = FS::DirNameFromString(raw_inst_name, m_instDir);
+        instID = FS::DirNameFromString(instanceName.modifiedName(), m_instDir);
     }
 
     {
@@ -910,7 +906,7 @@ bool InstanceList::commitStagedInstance(const QString& path, const QString& inst
             if (!inst)
                 inst = getInstanceById(instID);
             if (inst)
-                inst->setName(instanceName);
+                inst->setName(instanceName.name());
         } else {
             if (!dir.rename(path, destination)) {
                 qWarning() << "Failed to move" << path << "to" << destination;

launcher/InstanceList.h

@@ -24,10 +24,10 @@
 
 #include "BaseInstance.h"
 
-#include "QObjectPtr.h"
-
 class QFileSystemWatcher;
 class InstanceTask;
+struct InstanceName;
+
 using InstanceId = QString;
 using GroupId = QString;
 using InstanceLocator = std::pair<InstancePtr, int>;
@@ -133,7 +133,7 @@ public:
      * should_override is used when another similar instance already exists, and we want to override it
      * - for instance, when updating it.
      */
-    bool commitStagedInstance(const QString & keyPath, const QString& instanceName, const QString & groupName, bool should_override);
+    bool commitStagedInstance(QString keyPath, const InstanceName& instanceName, QString groupName, bool should_override);
 
     /**
      * Destroy a previously created staging area given by @keyPath - used when creation fails.

launcher/InstanceTask.cpp

@@ -1,9 +1,34 @@
 #include "InstanceTask.h"
 
-InstanceTask::InstanceTask()
+QString InstanceName::name() const
 {
+    if (!m_modified_name.isEmpty())
+        return modifiedName();
+    return QString("%1 %2").arg(m_original_name, m_original_version);
 }
 
-InstanceTask::~InstanceTask()
+QString InstanceName::originalName() const
 {
+    return m_original_name;
 }
+
+QString InstanceName::modifiedName() const
+{
+    if (!m_modified_name.isEmpty())
+        return m_modified_name;
+    return m_original_name;
+}
+
+QString InstanceName::version() const
+{
+    return m_original_version;
+}
+
+void InstanceName::setName(InstanceName& other)
+{
+   m_original_name = other.m_original_name; 
+   m_original_version = other.m_original_version; 
+   m_modified_name = other.m_modified_name;
+}
+
+InstanceTask::InstanceTask() : Task(), InstanceName() {}

launcher/InstanceTask.h

@@ -1,56 +1,50 @@
 #pragma once
 
-#include "tasks/Task.h"
 #include "settings/SettingsObject.h"
+#include "tasks/Task.h"
 
-class InstanceTask : public Task
+struct InstanceName {
-{
+   public:
+    InstanceName() = default;
+    InstanceName(QString name, QString version) : m_original_name(std::move(name)), m_original_version(std::move(version)) {}
+
+    [[nodiscard]] QString modifiedName() const;
+    [[nodiscard]] QString originalName() const;
+    [[nodiscard]] QString name() const;
+    [[nodiscard]] QString version() const;
+
+    void setName(QString name) { m_modified_name = name; }
+    void setName(InstanceName& other);
+
+   protected:
+    QString m_original_name;
+    QString m_original_version;
+
+    QString m_modified_name;
+};
+
+class InstanceTask : public Task, public InstanceName {
     Q_OBJECT
-public:
+   public:
-    explicit InstanceTask();
+    InstanceTask();
-    virtual ~InstanceTask();
+    ~InstanceTask() override = default;
 
-    void setParentSettings(SettingsObjectPtr settings)
+    void setParentSettings(SettingsObjectPtr settings) { m_globalSettings = settings; }
-    {
-        m_globalSettings = settings;
-    }
 
-    void setStagingPath(const QString &stagingPath)
+    void setStagingPath(const QString& stagingPath) { m_stagingPath = stagingPath; }
-    {
-        m_stagingPath = stagingPath;
-    }
 
-    void setName(const QString &name)
+    void setIcon(const QString& icon) { m_instIcon = icon; }
-    {
-        m_instName = name;
-    }
-    QString name() const
-    {
-        return m_instName;
-    }
 
-    void setIcon(const QString &icon)
+    void setGroup(const QString& group) { m_instGroup = group; }
-    {
+    QString group() const { return m_instGroup; }
-        m_instIcon = icon;
-    }
-
-    void setGroup(const QString &group)
-    {
-        m_instGroup = group;
-    }
-    QString group() const
-    {
-        return m_instGroup;
-    }
 
     bool shouldOverride() const { return m_override_existing; }
 
-protected:
+   protected:
     void setOverride(bool override) { m_override_existing = override; }
 
-protected: /* data */
+   protected: /* data */
     SettingsObjectPtr m_globalSettings;
-    QString m_instName;
     QString m_instIcon;
     QString m_instGroup;
     QString m_stagingPath;

launcher/minecraft/VanillaInstanceCreationTask.cpp

@@ -1,9 +1,9 @@
 #include "VanillaInstanceCreationTask.h"
 
 #include "FileSystem.h"
-#include "settings/INISettingsObject.h"
 #include "minecraft/MinecraftInstance.h"
 #include "minecraft/PackProfile.h"
+#include "settings/INISettingsObject.h"
 
 VanillaCreationTask::VanillaCreationTask(BaseVersionPtr version, QString loader, BaseVersionPtr loader_version)
     : InstanceCreationTask(), m_version(version), m_using_loader(true), m_loader(loader), m_loader_version(loader_version)
@@ -23,7 +23,7 @@ bool VanillaCreationTask::createInstance()
         if(m_using_loader)
             components->setComponentVersion(m_loader, m_loader_version->descriptor());
 
-        inst.setName(m_instName);
+        inst.setName(name());
         inst.setIconKey(m_instIcon);
     }
     instance_settings->resumeSave();

launcher/modplatform/atlauncher/ATLPackInstallTask.cpp

@@ -1005,7 +1005,7 @@ void PackInstallTask::install()
 
     components->saveNow();
 
-    instance.setName(m_instName);
+    instance.setName(name());
     instance.setIconKey(m_instIcon);
     instance.setManagedPack("atlauncher", m_pack_safe_name, m_pack_name, m_version_name, m_version_name);
     instanceSettings->resumeSave();

launcher/modplatform/flame/FlameInstanceCreationTask.cpp

@@ -5,6 +5,7 @@
 
 #include "Application.h"
 #include "FileSystem.h"
+#include "InstanceList.h"
 #include "Json.h"
 
 #include "minecraft/MinecraftInstance.h"
@@ -288,7 +289,8 @@ bool FlameCreationTask::createInstance()
         FS::deletePath(jarmodsPath);
     }
 
-    instance.setName(m_instName);
+    instance.setManagedPack("flame", {}, m_pack.name, {}, m_pack.version);
+    instance.setName(name());
 
     m_mod_id_resolver = new Flame::FileResolvingTask(APPLICATION->network(), m_pack);
     connect(m_mod_id_resolver.get(), &Flame::FileResolvingTask::succeeded, this, [this, &loop] { idResolverSucceeded(loop); });

launcher/modplatform/legacy_ftb/PackInstallTask.cpp

@@ -228,7 +228,7 @@ void PackInstallTask::install()
 
     progress(4, 4);
 
-    instance.setName(m_instName);
+    instance.setName(name());
     if(m_instIcon == "default")
     {
         m_instIcon = "ftb_logo";

launcher/modplatform/modpacksch/FTBPackInstallTask.cpp

@@ -335,7 +335,7 @@ void PackInstallTask::install()
 
     components->saveNow();
 
-    instance.setName(m_instName);
+    instance.setName(name());
     instance.setIconKey(m_instIcon);
     instance.setManagedPack("modpacksch", QString::number(m_pack.id), m_pack.name, QString::number(m_version.id), m_version.name);
     instanceSettings->resumeSave();

launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp

@@ -8,8 +8,6 @@
 #include "minecraft/MinecraftInstance.h"
 #include "minecraft/PackProfile.h"
 
-#include "modplatform/ModIndex.h"
-
 #include "net/ChecksumValidator.h"
 
 #include "settings/INISettingsObject.h"
@@ -30,11 +28,10 @@ bool ModrinthCreationTask::updateInstance()
     auto instance_list = APPLICATION->instances();
 
     // FIXME: How to handle situations when there's more than one install already for a given modpack?
-    // Based on the way we create the instance name (name + " " + version). Is there a better way?
+    auto inst = instance_list->getInstanceByManagedName(originalName());
-    auto inst = instance_list->getInstanceByManagedName(m_instName.section(' ', 0, -2));
 
     if (!inst) {
-        inst = instance_list->getInstanceById(m_instName);
+        inst = instance_list->getInstanceById(originalName());
 
         if (!inst)
             return false;
@@ -163,8 +160,9 @@ bool ModrinthCreationTask::createInstance()
     } else {
         instance.setIconKey("modrinth");
     }
-    instance.setName(m_instName);
+
     instance.setManagedPack("modrinth", getManagedPackID(), m_managed_name, m_managed_id, {});
+    instance.setName(name());
     instance.saveNow();
 
     m_files_job = new NetJob(tr("Mod download"), APPLICATION->network());

launcher/modplatform/technic/SingleZipPackInstallTask.cpp

@@ -133,7 +133,7 @@ void Technic::SingleZipPackInstallTask::extractFinished()
     shared_qobject_ptr<Technic::TechnicPackProcessor> packProcessor = new Technic::TechnicPackProcessor();
     connect(packProcessor.get(), &Technic::TechnicPackProcessor::succeeded, this, &Technic::SingleZipPackInstallTask::emitSucceeded);
     connect(packProcessor.get(), &Technic::TechnicPackProcessor::failed, this, &Technic::SingleZipPackInstallTask::emitFailed);
-    packProcessor->run(m_globalSettings, m_instName, m_instIcon, m_stagingPath, m_minecraftVersion);
+    packProcessor->run(m_globalSettings, name(), m_instIcon, m_stagingPath, m_minecraftVersion);
 }
 
 void Technic::SingleZipPackInstallTask::extractAborted()

launcher/modplatform/technic/SolderPackInstallTask.cpp

@@ -214,7 +214,7 @@ void Technic::SolderPackInstallTask::extractFinished()
     shared_qobject_ptr<Technic::TechnicPackProcessor> packProcessor = new Technic::TechnicPackProcessor();
     connect(packProcessor.get(), &Technic::TechnicPackProcessor::succeeded, this, &Technic::SolderPackInstallTask::emitSucceeded);
     connect(packProcessor.get(), &Technic::TechnicPackProcessor::failed, this, &Technic::SolderPackInstallTask::emitFailed);
-    packProcessor->run(m_globalSettings, m_instName, m_instIcon, m_stagingPath, m_minecraftVersion, true);
+    packProcessor->run(m_globalSettings, name(), m_instIcon, m_stagingPath, m_minecraftVersion, true);
 }
 
 void Technic::SolderPackInstallTask::extractAborted()

launcher/ui/dialogs/NewInstanceDialog.cpp

@@ -177,13 +177,30 @@ NewInstanceDialog::~NewInstanceDialog()
     delete ui;
 }
 
-void NewInstanceDialog::setSuggestedPack(const QString& name, InstanceTask* task)
+void NewInstanceDialog::setSuggestedPack(QString name, InstanceTask* task)
 {
     creationTask.reset(task);
-    ui->instNameTextBox->setPlaceholderText(name);
 
-    if(!task)
+    ui->instNameTextBox->setPlaceholderText(name);
-    {
+    importVersion.clear();
+
+    if (!task) {
+        ui->iconButton->setIcon(APPLICATION->icons()->getIcon("default"));
+        importIcon = false;
+    }
+
+    auto allowOK = task && !instName().isEmpty();
+    m_buttons->button(QDialogButtonBox::Ok)->setEnabled(allowOK);
+}
+
+void NewInstanceDialog::setSuggestedPack(QString name, QString version, InstanceTask* task)
+{
+    creationTask.reset(task);
+
+    ui->instNameTextBox->setPlaceholderText(name);
+    importVersion = version;
+
+    if (!task) {
         ui->iconButton->setIcon(APPLICATION->icons()->getIcon("default"));
         importIcon = false;
     }
@@ -214,7 +231,11 @@ InstanceTask * NewInstanceDialog::extractTask()
 {
     InstanceTask * extracted = creationTask.get();
     creationTask.release();
-    extracted->setName(instName());
+
+    InstanceName inst_name(ui->instNameTextBox->placeholderText().trimmed(), importVersion);
+    inst_name.setName(ui->instNameTextBox->text().trimmed());
+    extracted->setName(inst_name);
+
     extracted->setGroup(instGroup());
     extracted->setIcon(iconKey());
     return extracted;

launcher/ui/dialogs/NewInstanceDialog.h

@@ -37,7 +37,6 @@
 
 #include <QDialog>
 
-#include "BaseVersion.h"
 #include "ui/pages/BasePageProvider.h"
 #include "InstanceTask.h"
 
@@ -61,7 +60,8 @@ public:
 
     void updateDialogState();
 
-    void setSuggestedPack(const QString & name = QString(), InstanceTask * task = nullptr);
+    void setSuggestedPack(QString name = QString(), InstanceTask * task = nullptr);
+    void setSuggestedPack(QString name, QString version, InstanceTask * task = nullptr);
     void setSuggestedIconFromFile(const QString &path, const QString &name);
     void setSuggestedIcon(const QString &key);
 
@@ -95,5 +95,7 @@ private:
     QString importIconPath;
     QString importIconName;
 
+    QString importVersion;
+
     void importIconNow();
 };

launcher/ui/pages/modplatform/atlauncher/AtlPage.cpp

@@ -117,7 +117,7 @@ void AtlPage::suggestCurrent()
     }
 
     auto uiSupport = new AtlUserInteractionSupportImpl(this);
-    dialog->setSuggestedPack(selected.name + " " + selectedVersion, new ATLauncher::PackInstallTask(uiSupport, selected.name, selectedVersion));
+    dialog->setSuggestedPack(selected.name, selectedVersion, new ATLauncher::PackInstallTask(uiSupport, selected.name, selectedVersion));
 
     auto editedLogoName = selected.safeName;
     auto url = QString(BuildConfig.ATL_DOWNLOAD_SERVER_URL + "launcher/images/%1.png").arg(selected.safeName.toLower());

launcher/ui/pages/modplatform/ftb/FtbPage.cpp

@@ -127,7 +127,7 @@ void FtbPage::suggestCurrent()
         return;
     }
 
-    dialog->setSuggestedPack(selected.name + " " + selectedVersion, new ModpacksCH::PackInstallTask(selected, selectedVersion, this));
+    dialog->setSuggestedPack(selected.name, selectedVersion, new ModpacksCH::PackInstallTask(selected, selectedVersion, this));
     for(auto art : selected.art) {
         if(art.type == "square") {
             QString editedLogoName;

launcher/ui/pages/modplatform/legacy_ftb/Page.cpp

@@ -176,7 +176,7 @@ void Page::suggestCurrent()
         return;
     }
 
-    dialog->setSuggestedPack(selected.name + " " + selectedVersion, new PackInstallTask(APPLICATION->network(), selected, selectedVersion));
+    dialog->setSuggestedPack(selected.name, selectedVersion, new PackInstallTask(APPLICATION->network(), selected, selectedVersion));
     QString editedLogoName;
     if(selected.logo.toLower().startsWith("ftb"))
     {

launcher/ui/pages/modplatform/modrinth/ModrinthPage.cpp

@@ -294,7 +294,7 @@ void ModrinthPage::suggestCurrent()
 
     for (auto& ver : current.versions) {
         if (ver.id == selectedVersion) {
-            dialog->setSuggestedPack(current.name + " " + ver.version, new InstanceImportTask(ver.download_url, this));
+            dialog->setSuggestedPack(current.name, ver.version, new InstanceImportTask(ver.download_url, this));
             auto iconName = current.iconName;
             m_model->getLogo(iconName, current.iconUrl.toString(),
                              [this, iconName](QString logo) { dialog->setSuggestedIconFromFile(logo, iconName); });

launcher/ui/pages/modplatform/technic/TechnicPage.cpp

@@ -271,11 +271,11 @@ void TechnicPage::selectVersion() {
 
     if (!current.isSolder)
     {
-        dialog->setSuggestedPack(current.name + " " + selectedVersion, new Technic::SingleZipPackInstallTask(current.url, current.minecraftVersion));
+        dialog->setSuggestedPack(current.name, selectedVersion, new Technic::SingleZipPackInstallTask(current.url, current.minecraftVersion));
     }
     else
     {
-        dialog->setSuggestedPack(current.name + " " + selectedVersion, new Technic::SolderPackInstallTask(APPLICATION->network(), current.url, current.slug, selectedVersion, current.minecraftVersion));
+        dialog->setSuggestedPack(current.name, selectedVersion, new Technic::SolderPackInstallTask(APPLICATION->network(), current.url, current.slug, selectedVersion, current.minecraftVersion));
     }
 }