From 6c1bb091b13b94a1efd745059c25b5698a6fe44d Mon Sep 17 00:00:00 2001 From: Christian Heimlich Date: Tue, 11 Feb 2025 19:51:44 -0500 Subject: [PATCH] Move backup facilities out of IInstall to dedicated class --- CMakeLists.txt | 2 +- app/CMakeLists.txt | 2 + app/src/import/backup.cpp | 181 ++++++++++++++++++ app/src/import/backup.h | 111 +++++++++++ app/src/import/worker.cpp | 44 +---- app/src/kernel/controller.cpp | 8 +- .../implementation/attractmode/am-data.cpp | 5 +- .../interface/lr-install-interface.cpp | 100 ++-------- .../launcher/interface/lr-install-interface.h | 64 ------- 9 files changed, 323 insertions(+), 194 deletions(-) create mode 100644 app/src/import/backup.cpp create mode 100644 app/src/import/backup.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 5a12bac..fe4f892 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -74,7 +74,7 @@ endif() include(OB/FetchQx) ob_fetch_qx( - REF "d12b1a3dd8445ba3bae1271e4a6fc6fcb0420dfd" + REF "66ccfeff2eddd912fff7e0116539bbfe84e9503c" COMPONENTS ${FIL_QX_COMPONENTS} ) diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index 9c241a0..c5dfd75 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -23,6 +23,8 @@ add_custom_target(fil_copy_clifp # ------------------ Setup FIL -------------------------- set(FIL_SOURCE + import/backup.h + import/backup.cpp import/details.h import/details.cpp import/properties.h diff --git a/app/src/import/backup.cpp b/app/src/import/backup.cpp new file mode 100644 index 0000000..5dc6871 --- /dev/null +++ b/app/src/import/backup.cpp @@ -0,0 +1,181 @@ +// Unit Includes +#include "backup.h" + +// Qt Includes +#include +#include + +namespace Import +{ + +//=============================================================================================================== +// BackupError +//=============================================================================================================== + +//-Constructor------------------------------------------------------------- +//Private: +BackupError::BackupError(Type t, const QString& s) : + mType(t), + mSpecific(s) +{} + +//Public: +BackupError::BackupError() : + mType(NoError) +{} + +//-Instance Functions------------------------------------------------------------- +//Public: +bool BackupError::isValid() const { return mType != NoError; } +QString BackupError::specific() const { return mSpecific; } +BackupError::Type BackupError::type() const { return mType; } + +//Private: +Qx::Severity BackupError::deriveSeverity() const { return Qx::Err; } +quint32 BackupError::deriveValue() const { return mType; } +QString BackupError::derivePrimary() const { return ERR_STRINGS.value(mType); } +QString BackupError::deriveSecondary() const { return mSpecific; } +QString BackupError::deriveCaption() const { return CAPTION_REVERT_ERR; } + +//=============================================================================================================== +// BackupManager +//=============================================================================================================== + +//-Constructor------------------------------------------------------------- +//Private: +BackupManager::BackupManager() {} + +//-Class Functions------------------------------------------------------------- +//Private: +QString BackupManager::filePathToBackupPath(const QString& filePath) +{ + return filePath + '.' + BACKUP_FILE_EXT; +} + +//Public: +BackupManager* BackupManager::instance() { static BackupManager inst; return &inst; } + +//-Instance Functions------------------------------------------------------------- +//Private: +BackupError BackupManager::backup(const QString& path, bool (*fn)(const QString& a, const QString& b)) +{ + // Prevent double+ backups + if(mRevertablePaths.contains(path)) + return BackupError(); + + // Note revertable + mRevertablePaths.insert(path); + + // Backup if exists + if(QFile::exists(path)) + { + QString backupPath = filePathToBackupPath(path); + + if(QFile::exists(backupPath) && QFileInfo(backupPath).isFile()) + { + if(!QFile::remove(backupPath)) + return BackupError(BackupError::FileWontDelete, backupPath); + } + + if(!fn(path, backupPath)) + return BackupError(BackupError::FileWontBackup, path); + } + + return BackupError(); +} + +BackupError BackupManager::restore(QSet::const_iterator pathItr) +{ + Q_ASSERT(pathItr != mRevertablePaths.cend()); + + const QString path = *pathItr; + mRevertablePaths.erase(pathItr); + QString backupPath = filePathToBackupPath(path); + + if(QFile::exists(path) && !QFile::remove(path)) + return BackupError(BackupError::FileWontDelete, path); + + if(!QFile::exists(path) && QFile::exists(backupPath) && !QFile::rename(backupPath, path)) + return BackupError(BackupError::FileWontRestore, backupPath); + + return BackupError(); +} + +//Public: +BackupError BackupManager::backupCopy(const QString& path) +{ + return backup(path, [](const QString& a, const QString& b){ return QFile::copy(a, b); }); +} + +BackupError BackupManager::backupRename(const QString& path) +{ + return backup(path, [](const QString& a, const QString& b){ return QFile::rename(a, b); }); +} + +BackupError BackupManager::restore(const QString& path) +{ + auto store = mRevertablePaths.constFind(path); + if(store == mRevertablePaths.cend()) + return BackupError(); + + return restore(store); +} + +BackupError BackupManager::safeReplace(const QString& src, const QString& dst, bool symlink) +{ + // Maybe make sure destination folder exists here? + + // Backup + QString backupPath = filePathToBackupPath(dst); + bool dstOccupied = QFile::exists(dst); + if(dstOccupied) + if(!QFile::rename(dst, backupPath)) // Temp backup + return BackupError(BackupError::FileWontBackup, dst); + + // Replace + std::error_code replaceError; + if(symlink) + std::filesystem::create_symlink(src.toStdString(), dst.toStdString(), replaceError); + else + replaceError = QFile::copy(src, dst) ? std::error_code() : std::make_error_code(std::io_errc::stream); + + // Restore on fail + if(replaceError) + { + if(dstOccupied) + QFile::rename(backupPath, dst); + return BackupError(BackupError::FileWontReplace, src); + } + + // Remove backup immediately + if(dstOccupied) + QFile::remove(backupPath); + else // Mark new files (only) as revertible so that existing ones will remain in the event of a revert + mRevertablePaths.insert(dst); + + return BackupError(); +} + +int BackupManager::revertQueueCount() const { return mRevertablePaths.size(); } + +int BackupManager::revertNextChange(BackupError& error, bool skipOnFail) +{ + // Ensure error message is null + error = BackupError(); + + // Delete new files and restore backups if present + if(!mRevertablePaths.isEmpty()) + { + BackupError rErr = restore(mRevertablePaths.cbegin()); + if(rErr && !skipOnFail) + error = rErr; + + return mRevertablePaths.size(); + } + + // Return 0 if all empty (shouldn't be reached if function is used correctly) + qWarning("Reversion function called with no reverts left!"); + return 0; +} + +} diff --git a/app/src/import/backup.h b/app/src/import/backup.h new file mode 100644 index 0000000..5d274b2 --- /dev/null +++ b/app/src/import/backup.h @@ -0,0 +1,111 @@ +#ifndef BACKUP_H +#define BACKUP_H + +// Qt Includes +#include +#include + +// Qx Includes +#include + +using namespace Qt::StringLiterals; + +/* TODO: The approach, or at least the language around doing a full revert (i.e. emptying the revert + * queue) could use some touch-up. + */ + +namespace Import +{ + +class QX_ERROR_TYPE(BackupError, "Lr::BackupError", 1301) +{ + friend class BackupManager; + //-Class Enums------------------------------------------------------------- +public: + enum Type + { + NoError, + FileWontDelete, + FileWontRestore, + FileWontBackup, + FileWontReplace + }; + +//-Class Variables------------------------------------------------------------- +private: + static inline const QHash ERR_STRINGS{ + {NoError, u""_s}, + {FileWontDelete, u"Cannot remove a file. It may need to be deleted manually."_s}, + {FileWontRestore, u"Cannot restore a file backup. It may need to be renamed manually.."_s}, + {FileWontBackup, u"Cannot backup file."_s}, + {FileWontReplace, u"A file that was part of a safe replace operation could not be transfered."_s} + }; + + static inline const QString CAPTION_REVERT_ERR = u"Error managing backups"_s; + +//-Instance Variables------------------------------------------------------------- +private: + Type mType; + QString mSpecific; + +//-Constructor------------------------------------------------------------- +private: + BackupError(Type t, const QString& s); + +public: + BackupError(); + +//-Instance Functions------------------------------------------------------------- +public: + bool isValid() const; + Type type() const; + QString specific() const; + +private: + Qx::Severity deriveSeverity() const override; + quint32 deriveValue() const override; + QString derivePrimary() const override; + QString deriveSecondary() const override; + QString deriveCaption() const override; +}; + +class BackupManager +{ +//-Class Variables----------------------------------------------------------------------------------------------- +private: + // Files + static inline const QString BACKUP_FILE_EXT = u"fbk"_s; + +//-Instance Variables------------------------------------------------------------- +private: + QSet mRevertablePaths; + +//-Constructor------------------------------------------------------------- +private: + BackupManager(); + +//-Class Functions------------------------------------------------------------- +private: + static QString filePathToBackupPath(const QString& filePath); + +public: + static BackupManager* instance(); + +//-Instance Functions------------------------------------------------------------- +private: + BackupError backup(const QString& path, bool (*fn)(const QString& a, const QString& b)); + BackupError restore(QSet::const_iterator pathItr); + +public: + BackupError backupCopy(const QString& path); + BackupError backupRename(const QString& path); + BackupError restore(const QString& path); + BackupError safeReplace(const QString& src, const QString& dst, bool symlink); + + int revertQueueCount() const; + int revertNextChange(BackupError& error, bool skipOnFail); +}; + +} + +#endif // BACKUP_H diff --git a/app/src/import/worker.cpp b/app/src/import/worker.cpp index d45eede..fedf4a1 100644 --- a/app/src/import/worker.cpp +++ b/app/src/import/worker.cpp @@ -14,6 +14,7 @@ // Project Includes #include "kernel/clifp.h" #include "import/details.h" +#include "import/backup.h" namespace Import { @@ -148,6 +149,7 @@ ImageTransferError Worker::transferImage(bool symlink, QString sourcePath, QStri QString sourceChecksum; QString destinationChecksum; + // TODO: Probably better to just byte-wise compare if(!Qx::calculateFileChecksum(sourceChecksum, source, QCryptographicHash::Md5).isFailure() && !Qx::calculateFileChecksum(destinationChecksum, destination, QCryptographicHash::Md5).isFailure() && sourceChecksum.compare(destinationChecksum, Qt::CaseInsensitive) == 0) @@ -160,42 +162,16 @@ ImageTransferError Worker::transferImage(bool symlink, QString sourcePath, QStri if(!destinationDir.mkpath(u"."_s)) return ImageTransferError(ImageTransferError::CantCreateDirectory, QString(), destinationDir.absolutePath()); - // Determine backup path - QString backupPath = Lr::IInstall::filePathToBackupPath(destinationInfo.absoluteFilePath()); - - // Temporarily backup image if it already exists (also acts as deletion marking in case images for the title were removed in an update) - if(destinationOccupied) - if(!QFile::rename(destinationPath, backupPath)) // Temp backup - return ImageTransferError(ImageTransferError::ImageWontBackup, QString(), destinationPath); - - // Linking error tracker - std::error_code linkError; - - // Handle transfer - if(symlink) - { - std::filesystem::create_symlink(sourcePath.toStdString(), destinationPath.toStdString(), linkError); - if(linkError) - { - QFile::rename(backupPath, destinationPath); // Restore Backup - return ImageTransferError(ImageTransferError::ImageWontLink, sourcePath, destinationPath); - } - else if(QFile::exists(backupPath)) - QFile::remove(backupPath); - else - mLauncherInstall->addRevertableFile(destinationPath); // Only queue image to be removed on failure if its new, so existing images aren't deleted on revert - } - else + // Transfer image + BackupError bErr = BackupManager::instance()->safeReplace(sourcePath, destinationPath, symlink); + if(bErr) { - if(!QFile::copy(sourcePath, destinationPath)) - { - QFile::rename(backupPath, destinationPath); // Restore Backup - return ImageTransferError(ImageTransferError::ImageWontCopy, sourcePath, destinationPath); - } - else if(QFile::exists(backupPath)) - QFile::remove(backupPath); + if(bErr.type() == BackupError::FileWontBackup) + return ImageTransferError(ImageTransferError::ImageWontBackup, QString(), destinationPath); + else if(bErr.type() == BackupError::FileWontReplace) + return ImageTransferError(symlink ? ImageTransferError::ImageWontLink : ImageTransferError::ImageWontCopy, QString(), destinationPath); else - mLauncherInstall->addRevertableFile(destinationPath); // Only queue image to be removed on failure if its new, so existing images aren't deleted on revert + qFatal("Unhandled image transfer error type."); } // Return null error on success diff --git a/app/src/kernel/controller.cpp b/app/src/kernel/controller.cpp index 47cdc06..c75dd19 100644 --- a/app/src/kernel/controller.cpp +++ b/app/src/kernel/controller.cpp @@ -12,6 +12,7 @@ // Project Includes #include "launcher/abstract/lr-registration.h" +#include "import/backup.h" /* TODO: Consider having this tool deploy a .ini file (or the like) into the target launcher install * (with the exact location probably being guided by the specific Install child) that saves the settings @@ -111,14 +112,15 @@ void Controller::revertAllLauncherChanges() // Trackers bool tempSkip = false; bool alwaysSkip = false; - Lr::RevertError currentError; + Import::BackupError currentError; int retryChoice; // Progress + auto bm = Import::BackupManager::instance(); mProgressPresenter.setMinimum(0); - mProgressPresenter.setMaximum(launcher->revertQueueCount()); + mProgressPresenter.setMaximum(bm->revertQueueCount()); mProgressPresenter.setCaption(CAPTION_REVERT); - while(launcher->revertNextChange(currentError, alwaysSkip || tempSkip) != 0) + while(bm->revertNextChange(currentError, alwaysSkip || tempSkip) != 0) { // Check for error if(!currentError.isValid()) diff --git a/app/src/launcher/implementation/attractmode/am-data.cpp b/app/src/launcher/implementation/attractmode/am-data.cpp index 505e487..6f222aa 100644 --- a/app/src/launcher/implementation/attractmode/am-data.cpp +++ b/app/src/launcher/implementation/attractmode/am-data.cpp @@ -494,9 +494,8 @@ Lr::DocHandlingError PlatformInterfaceWriter::writeOutOf() // Write overviews for(const auto& o : std::as_const(source()->mOverviews)) { - if(mOverviewWriter.writeOverview(o)) - source()->install()->addRevertableFile(mOverviewWriter.currentFilePath()); - else + // This uses QSaveFile as a form of "safe replace" write, so we don't need to manually back-up + if(!mOverviewWriter.writeOverview(o)) return Lr::DocHandlingError(*source(), Lr::DocHandlingError::DocWriteFailed, mOverviewWriter.fileErrorString()); } diff --git a/app/src/launcher/interface/lr-install-interface.cpp b/app/src/launcher/interface/lr-install-interface.cpp index 2a1ea03..7b508b0 100644 --- a/app/src/launcher/interface/lr-install-interface.cpp +++ b/app/src/launcher/interface/lr-install-interface.cpp @@ -1,38 +1,12 @@ // Unit Include #include "lr-install-interface.h" +// Project Includes +#include "import/backup.h" + namespace Lr { -//=============================================================================================================== -// RevertError -//=============================================================================================================== - -//-Constructor------------------------------------------------------------- -//Private: -RevertError::RevertError(Type t, const QString& s) : - mType(t), - mSpecific(s) -{} - -//Public: -RevertError::RevertError() : - mType(NoError) -{} - -//-Instance Functions------------------------------------------------------------- -//Public: -bool RevertError::isValid() const { return mType != NoError; } -QString RevertError::specific() const { return mSpecific; } -RevertError::Type RevertError::type() const { return mType; } - -//Private: -Qx::Severity RevertError::deriveSeverity() const { return Qx::Err; } -quint32 RevertError::deriveValue() const { return mType; } -QString RevertError::derivePrimary() const { return ERR_STRINGS.value(mType); } -QString RevertError::deriveSecondary() const { return mSpecific; } -QString RevertError::deriveCaption() const { return CAPTION_REVERT_ERR; } - //=============================================================================================================== // IInstall //=============================================================================================================== @@ -47,12 +21,6 @@ IInstall::IInstall(const QString& installPath) : //Public: IInstall::~IInstall() {} -//Public: -QString IInstall::filePathToBackupPath(const QString& filePath) -{ - return filePath + '.' + BACKUP_FILE_EXT; -} - //-Instance Functions-------------------------------------------------------------------------------------------- //Private: bool IInstall::containsAnyDataDoc(IDataDoc::Type type, const QList& names) const @@ -126,6 +94,7 @@ DocHandlingError IInstall::commitDataDocument(std::shared_ptr IDataDoc::Identifier id = docToSave->identifier(); // Check if the doc was saved previously to prevent double-backups + // TODO: SEE IF THIS LEVEL OF DISTINCTION IS NEEDED WITH NEW BACKUP STRAT bool wasDeleted = mDeletedDocuments.contains(id); bool wasModified = mModifiedDocuments.contains(id); bool wasUntouched = !wasDeleted && !wasModified; @@ -133,23 +102,14 @@ DocHandlingError IInstall::commitDataDocument(std::shared_ptr // Handle backup/revert prep if(wasUntouched) { - QString docPath = docToSave->path(); - mRevertableFilePaths.append(docPath); // Correctly handles if doc ends up deleted - // Backup - if(QFile::exists(docPath)) - { - QString backupPath = filePathToBackupPath(docPath); - - if(QFile::exists(backupPath) && QFileInfo(backupPath).isFile()) - { - if(!QFile::remove(backupPath)) - return DocHandlingError(*docToSave, DocHandlingError::CantRemoveBackup); - } - - if(!QFile::copy(docPath, backupPath)) - return DocHandlingError(*docToSave, DocHandlingError::CantCreateBackup); - } + QString docPath = docToSave->path(); + Import::BackupError bErr = Import::BackupManager::instance()->backupCopy(docPath); + if(bErr.type() == Import::BackupError::FileWontDelete) + return DocHandlingError(*docToSave, DocHandlingError::CantRemoveBackup); + else if(bErr.type() == Import::BackupError::FileWontBackup) + return DocHandlingError(*docToSave, DocHandlingError::CantCreateBackup); + Q_ASSERT(!bErr.isValid()); // All relevant types should be handled here } // Error State @@ -189,7 +149,6 @@ QString IInstall::path() const { return mRootDirectory.absolutePath(); } void IInstall::softReset() { - mRevertableFilePaths.clear(); mModifiedDocuments.clear(); mDeletedDocuments.clear(); mLeasedDocuments.clear(); @@ -231,43 +190,6 @@ bool IInstall::containsAnyPlaylist(const QList& names) const return containsAnyDataDoc(IDataDoc::Type::Playlist, names); } -void IInstall::addRevertableFile(const QString& filePath) { mRevertableFilePaths.append(filePath); } -int IInstall::revertQueueCount() const { return mRevertableFilePaths.size(); } - -int IInstall::revertNextChange(RevertError& error, bool skipOnFail) -{ - // Ensure error message is null - error = RevertError(); - - // Get operation count for return - int operationsLeft = mRevertableFilePaths.size(); - - // Delete new files and restore backups if present - if(!mRevertableFilePaths.isEmpty()) - { - QString filePath = mRevertableFilePaths.takeFirst(); - QString backupPath = filePathToBackupPath(filePath); - - if(QFile::exists(filePath) && !QFile::remove(filePath) && !skipOnFail) - { - error = RevertError(RevertError::FileWontDelete, filePath); - return operationsLeft; - } - - if(!QFile::exists(filePath) && QFile::exists(backupPath) && !QFile::rename(backupPath, filePath) && !skipOnFail) - { - error = RevertError(RevertError::FileWontRestore, backupPath); - return operationsLeft; - } - - // Decrement op count - return operationsLeft - 1; - } - - // Return 0 if all empty (shouldn't be reached if function is used correctly) - return 0; -} - /* These functions can be overridden by children as needed. * Work within them should be kept as minimal as possible since they are not accounted * for by the import progress indicator. diff --git a/app/src/launcher/interface/lr-install-interface.h b/app/src/launcher/interface/lr-install-interface.h index 6c01bd9..daf6b5e 100644 --- a/app/src/launcher/interface/lr-install-interface.h +++ b/app/src/launcher/interface/lr-install-interface.h @@ -11,61 +11,9 @@ namespace Lr { -class QX_ERROR_TYPE(RevertError, "Lr::RevertError", 1301) -{ - friend class IInstall; -//-Class Enums------------------------------------------------------------- -public: - enum Type - { - NoError = 0, - FileWontDelete = 1, - FileWontRestore = 2 - }; - -//-Class Variables------------------------------------------------------------- -private: - static inline const QHash ERR_STRINGS{ - {NoError, u""_s}, - {FileWontDelete, u"Cannot remove a file. It may need to be deleted manually."_s}, - {FileWontRestore, u"Cannot restore a file backup. It may need to be renamed manually.."_s} - }; - - static inline const QString CAPTION_REVERT_ERR = u"Error reverting changes"_s; - -//-Instance Variables------------------------------------------------------------- -private: - Type mType; - QString mSpecific; - -//-Constructor------------------------------------------------------------- -private: - RevertError(Type t, const QString& s); - -public: - RevertError(); - -//-Instance Functions------------------------------------------------------------- -public: - bool isValid() const; - Type type() const; - QString specific() const; - -private: - Qx::Severity deriveSeverity() const override; - quint32 deriveValue() const override; - QString derivePrimary() const override; - QString deriveSecondary() const override; - QString deriveCaption() const override; -}; - class IInstall { //-Class Variables----------------------------------------------------------------------------------------------- -private: - // Files - static inline const QString BACKUP_FILE_EXT = u"fbk"_s; - protected: // Files static inline const QString IMAGE_EXT = u"png"_s; @@ -88,9 +36,6 @@ class IInstall QSet mDeletedDocuments; QSet mLeasedDocuments; - // Backup/Deletion tracking - QStringList mRevertableFilePaths; - //-Constructor--------------------------------------------------------------------------------------------------- public: IInstall(const QString& installPath); @@ -103,10 +48,6 @@ class IInstall private: static void ensureModifiable(const QString& filePath); -public: - // TODO: Improve the backup system so that its more encapsulated and this doesn't need to be public - static QString filePathToBackupPath(const QString& filePath); - //-Instance Functions--------------------------------------------------------------------------------------------------------- private: // Support @@ -151,11 +92,6 @@ class IInstall virtual DocHandlingError commitPlatformDoc(std::unique_ptr platformDoc) = 0; virtual DocHandlingError commitPlaylistDoc(std::unique_ptr playlistDoc) = 0; - // Reversion - void addRevertableFile(const QString& filePath); - int revertQueueCount() const; - int revertNextChange(RevertError& error, bool skipOnFail); - // Import stage notifier hooks virtual Qx::Error preImport(); virtual Qx::Error postImport();