diff --git a/launcher/InstanceCreationTask.cpp b/launcher/InstanceCreationTask.cpp index a82bdfe8..1b1a8c15 100644 --- a/launcher/InstanceCreationTask.cpp +++ b/launcher/InstanceCreationTask.cpp @@ -1,6 +1,7 @@ #include "InstanceCreationTask.h" #include +#include InstanceCreationTask::InstanceCreationTask() = default; @@ -19,19 +20,37 @@ void InstanceCreationTask::executeTask() return; } - // If this is set, it means we're updating an instance. Since the previous step likely - // removed some old files, we'd better not let the user abort the next task, since it'd - // put the instance in an invalid state. - // TODO: Figure out an unexpensive way of making such file removal a recoverable transaction. - setAbortStatus(!shouldOverride()); + if (!createInstance()) { + if (m_abort) + return; - if (createInstance()) { - emitSucceeded(); + qWarning() << "Instance creation failed!"; + if (!m_error_message.isEmpty()) + qWarning() << "Reason: " << m_error_message; + emitFailed(tr("Error while creating new instance.")); return; } - qWarning() << "Instance creation failed!"; - if (!m_error_message.isEmpty()) - qWarning() << "Reason: " << m_error_message; - emitFailed(tr("Error while creating new instance.")); + // If this is set, it means we're updating an instance. So, we now need to remove the + // files scheduled to, and we'd better not let the user abort in the middle of it, since it'd + // put the instance in an invalid state. + if (shouldOverride()) { + setAbortStatus(false); + setStatus(tr("Removing old conflicting files...")); + qDebug() << "Removing old files"; + + for (auto path : m_files_to_remove) { + if (!QFile::exists(path)) + continue; + qDebug() << "Removing" << path; + if (!QFile::remove(path)) { + qCritical() << "Couldn't remove the old conflicting files."; + emitFailed(tr("Failed to remove old conflicting files.")); + return; + } + } + } + + emitSucceeded(); + return; } diff --git a/launcher/InstanceCreationTask.h b/launcher/InstanceCreationTask.h index 9b51c448..03ee1a7a 100644 --- a/launcher/InstanceCreationTask.h +++ b/launcher/InstanceCreationTask.h @@ -39,6 +39,8 @@ class InstanceCreationTask : public InstanceTask { protected: bool m_abort = false; + QStringList m_files_to_remove; + private: QString m_error_message; }; diff --git a/launcher/modplatform/flame/FlameInstanceCreationTask.cpp b/launcher/modplatform/flame/FlameInstanceCreationTask.cpp index 5acb6300..e3521a38 100644 --- a/launcher/modplatform/flame/FlameInstanceCreationTask.cpp +++ b/launcher/modplatform/flame/FlameInstanceCreationTask.cpp @@ -120,15 +120,17 @@ bool FlameCreationTask::updateInstance() files_iterator++; } - QString old_minecraft_dir(inst->gameRoot()); + QDir old_minecraft_dir(inst->gameRoot()); // We will remove all the previous overrides, to prevent duplicate files! // TODO: Currently 'overrides' will always override the stuff on update. How do we preserve unchanged overrides? // FIXME: We may want to do something about disabled mods. auto old_overrides = Override::readOverrides("overrides", old_index_folder); for (auto entry : old_overrides) { - qDebug() << "Removing" << entry; - old_minecraft_dir.remove(entry); + if (entry.isEmpty()) + continue; + qDebug() << "Scheduling" << entry << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(entry)); } // Remove remaining old files (we need to do an API request to know which ids are which files...) @@ -143,7 +145,7 @@ bool FlameCreationTask::updateInstance() QEventLoop loop; - connect(job, &NetJob::succeeded, this, [raw_response, fileIds, old_inst_dir, &old_files, old_minecraft_dir] { + connect(job, &NetJob::succeeded, this, [this, raw_response, fileIds, old_inst_dir, &old_files, old_minecraft_dir] { // Parse the API response QJsonParseError parse_error{}; auto doc = QJsonDocument::fromJson(*raw_response, &parse_error); @@ -180,10 +182,9 @@ bool FlameCreationTask::updateInstance() if (file.fileName.isEmpty() || file.targetFolder.isEmpty()) continue; - qDebug() << "Removing" << file.fileName << "at" << file.targetFolder; - QString path(FS::PathCombine(old_minecraft_dir, file.targetFolder, file.fileName)); - if (!QFile::remove(path)) - qDebug() << "Failed to remove file at" << path; + QString relative_path(FS::PathCombine(file.targetFolder, file.fileName)); + qDebug() << "Scheduling" << relative_path << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(relative_path)); } }); connect(job, &NetJob::finished, &loop, &QEventLoop::quit); @@ -334,14 +335,17 @@ bool FlameCreationTask::createInstance() loop.exec(); - if (m_instance) { + bool did_succeed = getError().isEmpty(); + + if (m_instance && did_succeed) { + setAbortStatus(false); auto inst = m_instance.value(); inst->copyManagedPack(instance); inst->setName(instance.name()); } - return getError().isEmpty(); + return did_succeed; } void FlameCreationTask::idResolverSucceeded(QEventLoop& loop) @@ -421,7 +425,6 @@ void FlameCreationTask::setupDownloadJob(QEventLoop& loop) m_mod_id_resolver.reset(); connect(m_files_job.get(), &NetJob::succeeded, this, [&]() { m_files_job.reset(); - emitSucceeded(); }); connect(m_files_job.get(), &NetJob::failed, [&](QString reason) { m_files_job.reset(); diff --git a/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp b/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp index a0c67876..7d19639c 100644 --- a/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp +++ b/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp @@ -105,12 +105,15 @@ bool ModrinthCreationTask::updateInstance() } QDir old_minecraft_dir(inst->gameRoot()); + // Some files were removed from the old version, and some will be downloaded in an updated version, // so we're fine removing them! if (!old_files.empty()) { for (auto const& file : old_files) { - qDebug() << "Removing" << file.path; - old_minecraft_dir.remove(file.path); + if (file.path.isEmpty()) + continue; + qDebug() << "Scheduling" << file.path << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(file.path)); } } @@ -119,14 +122,18 @@ bool ModrinthCreationTask::updateInstance() // FIXME: We may want to do something about disabled mods. auto old_overrides = Override::readOverrides("overrides", old_index_folder); for (auto entry : old_overrides) { - qDebug() << "Removing" << entry; - old_minecraft_dir.remove(entry); + if (entry.isEmpty()) + continue; + qDebug() << "Scheduling" << entry << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(entry)); } auto old_client_overrides = Override::readOverrides("client-overrides", old_index_folder); for (auto entry : old_overrides) { - qDebug() << "Removing" << entry; - old_minecraft_dir.remove(entry); + if (entry.isEmpty()) + continue; + qDebug() << "Scheduling" << entry << "for removal"; + m_files_to_remove.append(old_minecraft_dir.absoluteFilePath(entry)); } } @@ -244,7 +251,8 @@ bool ModrinthCreationTask::createInstance() loop.exec(); - if (m_instance) { + if (m_instance && ended_well) { + setAbortStatus(false); auto inst = m_instance.value(); inst->copyManagedPack(instance);