From a76753d2999a353cd6da7edfdb77b71d83fee1a5 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 8 May 2022 11:11:54 +0200 Subject: [PATCH 1/5] Improve error handling when a project fails to load --- core_lib/src/qminiz.cpp | 26 ++++++++++++++---- core_lib/src/qminiz.h | 2 +- core_lib/src/structure/filemanager.cpp | 38 +++++++++++++++++++------- core_lib/src/structure/filemanager.h | 4 +-- core_lib/src/util/pencilerror.h | 1 + 5 files changed, 52 insertions(+), 19 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index b7f7ece7e5..cc7792671a 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -23,19 +23,33 @@ GNU General Public License for more details. #include "util.h" -bool MiniZ::isZip(const QString& sZipFilePath) +Status MiniZ::sanityCheck(const QString& sZipFilePath) { mz_zip_archive* mz = new mz_zip_archive; OnScopeExit(delete mz); mz_zip_zero_struct(mz); QByteArray utf8Bytes = sZipFilePath.toUtf8(); - mz_bool ok = mz_zip_reader_init_file(mz, utf8Bytes.constData(), 0); - if (!ok) return false; + mz_bool readOk = mz_zip_reader_init_file(mz, utf8Bytes.constData(), 0); - int num = mz_zip_reader_get_num_files(mz); + mz_zip_error read_err = mz_zip_get_last_error(mz); + + mz_bool closeOk = mz_zip_reader_end(mz); + + mz_zip_error close_err = mz_zip_get_last_error(mz); - mz_zip_reader_end(mz); - return (num > 0); + if (!readOk || !closeOk) { + DebugDetails dd; + if (read_err != MZ_ZIP_NO_ERROR) { + dd << QString("Miniz found an error while reading the file. - %1, %2").arg(static_cast(read_err)).arg(mz_zip_get_error_string(read_err)); + } + if (close_err != MZ_ZIP_NO_ERROR) { + dd << QString("Miniz found an error while closing file file. - %1, %2").arg(static_cast(close_err)).arg(mz_zip_get_error_string(close_err)); + } + return Status(Status::ERROR_MINIZ_FAIL, dd); + } + + + return Status::OK; } size_t MiniZ::istreamReadCallback(void *pOpaque, mz_uint64 file_ofs, void * pBuf, size_t n) diff --git a/core_lib/src/qminiz.h b/core_lib/src/qminiz.h index 9528b2f7cd..cc05045d1e 100644 --- a/core_lib/src/qminiz.h +++ b/core_lib/src/qminiz.h @@ -22,7 +22,7 @@ GNU General Public License for more details. namespace MiniZ { - bool isZip(const QString& sZipFilePath); + Status sanityCheck(const QString& sZipFilePath); size_t istreamReadCallback(void *pOpaque, mz_uint64 file_ofs, void * pBuf, size_t n); Status compressFolder(QString zipFilePath, QString srcFolderPath, const QStringList& fileList, QString mimetype); Status uncompressFolder(QString zipFilePath, QString destPath); diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index de1a7f9111..c9bc69da00 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -50,11 +50,16 @@ Object* FileManager::load(const QString& sFileName) QString strDataFolder; // Test file format: new zipped .pclx or old .pcl? - bool oldFormat = isOldForamt(sFileName); - dd << QString("Is old format: ").append(oldFormat ? "true" : "false"); + Status archiveSt = isArchiveFormat(sFileName); - if (oldFormat) + QString isArchive = "Is archive: "; + if (archiveSt.ok()) { + dd << isArchive.append("true"); + } + + if (archiveSt == Status::NOT_ARCHIVE_FORMAT) { + dd << isArchive.append("false"); dd << "Recognized Old Pencil2D File Format (*.pcl) !"; strMainXMLFile = sFileName; @@ -64,7 +69,15 @@ Object* FileManager::load(const QString& sFileName) { dd << "Recognized New zipped Pencil2D File Format (*.pclx) !"; - unzip(sFileName, obj->workingDir()); + Status sanityCheck = MiniZ::sanityCheck(sFileName); + + // Let's check if we can read the file before we try to unzip. + if (!sanityCheck.ok()) { + dd.collect(sanityCheck.details()); + } else { + Status unzipStatus = unzip(sFileName, obj->workingDir()); + dd.collect(unzipStatus.details()); + } strMainXMLFile = QDir(obj->workingDir()).filePath(PFF_XML_FILE_NAME); strDataFolder = QDir(obj->workingDir()).filePath(PFF_DATA_DIR); @@ -198,9 +211,12 @@ bool FileManager::loadObjectOldWay(Object* object, const QDomElement& root) return object->loadXML(root, [this] { progressForward(); }); } -bool FileManager::isOldForamt(const QString& fileName) const +Status FileManager::isArchiveFormat(const QString& fileName) const { - return !(MiniZ::isZip(fileName)); + if (QFileInfo(fileName).suffix().compare(PFF_BIG_LETTER_EXTENSION, Qt::CaseInsensitive) != 0) { + return Status::NOT_ARCHIVE_FORMAT; + } + return Status::OK; } Status FileManager::save(const Object* object, const QString& sFileName) @@ -255,8 +271,8 @@ Status FileManager::save(const Object* object, const QString& sFileName) QString sMainXMLFile; QString sDataFolder; - const bool isOldType = sFileName.endsWith(PFF_OLD_EXTENSION); - if (isOldType) + Status isArchiveSt = isArchiveFormat(sFileName); + if (isArchiveSt == Status::NOT_ARCHIVE_FORMAT) { dd << "Old Pencil2D File Format (*.pcl) !"; @@ -266,6 +282,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) else { dd << "New zipped Pencil2D File Format (*.pclx) !"; + dd.collect(MiniZ::sanityCheck(sFileName).details()); sTempWorkingFolder = object->workingDir(); Q_ASSERT(QDir(sTempWorkingFolder).exists()); @@ -311,7 +328,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) progressForward(); - if (!isOldType) + if (isArchiveSt.ok()) { dd << "Miniz"; @@ -681,7 +698,7 @@ Status FileManager::writePalette(const Object* object, const QString& dataFolder return Status::OK; } -void FileManager::unzip(const QString& strZipFile, const QString& strUnzipTarget) +Status FileManager::unzip(const QString& strZipFile, const QString& strUnzipTarget) { // removes the previous directory first - better approach removePFFTmpDirectory(strUnzipTarget); @@ -690,6 +707,7 @@ void FileManager::unzip(const QString& strZipFile, const QString& strUnzipTarget Q_ASSERT(s.ok()); mstrLastTempFolder = strUnzipTarget; + return s; } QList FileManager::loadPaletteFile(QString strFilename) diff --git a/core_lib/src/structure/filemanager.h b/core_lib/src/structure/filemanager.h index a8571d87d6..71ce94b4db 100644 --- a/core_lib/src/structure/filemanager.h +++ b/core_lib/src/structure/filemanager.h @@ -55,11 +55,11 @@ class FileManager : public QObject void progressRangeChanged(int maxValue); private: - void unzip(const QString& strZipFile, const QString& strUnzipTarget); + Status unzip(const QString& strZipFile, const QString& strUnzipTarget); bool loadObject(Object*, const QDomElement& root); bool loadObjectOldWay(Object*, const QDomElement& root); - bool isOldForamt(const QString& fileName) const; + Status isArchiveFormat(const QString& fileName) const; bool loadPalette(Object*); Status writeKeyFrameFiles(const Object* obj, const QString& dataFolder, QStringList& filesWritten); Status writeMainXml(const Object* obj, const QString& mainXmlPath, QStringList& filesWritten); diff --git a/core_lib/src/util/pencilerror.h b/core_lib/src/util/pencilerror.h index e35870f6cd..9a7ef9ef87 100644 --- a/core_lib/src/util/pencilerror.h +++ b/core_lib/src/util/pencilerror.h @@ -56,6 +56,7 @@ class Status ERROR_INVALID_XML_FILE, ERROR_INVALID_PENCIL_FILE, ERROR_MINIZ_FAIL, + NOT_ARCHIVE_FORMAT, // General ERROR_INVALID_LAYER_TYPE, From ac88f037ae1b3235c67076f8fecb6b3633a45b3b Mon Sep 17 00:00:00 2001 From: MrStevns Date: Wed, 4 May 2022 17:47:04 +0200 Subject: [PATCH 2/5] Avoid corrupting the existing project if the save process didn't succeed --- core_lib/src/structure/filemanager.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index c9bc69da00..c915703326 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -330,10 +330,15 @@ Status FileManager::save(const Object* object, const QString& sFileName) if (isArchiveSt.ok()) { - dd << "Miniz"; - QString sBackupFile = backupPreviousFile(sFileName); + if (!saveOk) { + return Status(Status::FAIL, dd, + tr("Internal Error"), + tr("An internal error occurred. Your file may not be saved successfully.")); + } + + dd << "Miniz"; Status stMiniz = MiniZ::compressFolder(sFileName, sTempWorkingFolder, filesToZip, "application/x-pencil2d-pclx"); if (!stMiniz.ok()) { From 0de72ae3e5a5736a8612f5f2f93ad532aae21d75 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Wed, 4 May 2022 20:58:48 +0200 Subject: [PATCH 3/5] Fix a case where backup would be overwritten If you opened the backup, saved and the project corrupted, a backup wouldn't be made. The file would instead be renamed. --- core_lib/src/structure/filemanager.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index c915703326..ed5224fabb 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -569,10 +569,19 @@ QString FileManager::backupPreviousFile(const QString& fileName) return ""; QFileInfo info(fileName); - QString sBackupFile = info.completeBaseName() + ".backup." + info.suffix(); + int backupCount = 1; + QDir directory(info.absoluteDir()); + QString backupString = "backup"; + for (QFileInfo info : directory.entryInfoList(QDir::Filter::Files)) { + if (info.completeBaseName().contains(backupString)) { + backupCount++; + } + } + + QString sBackupFile = info.baseName() + "." + backupString + QString::number(backupCount) + "." + info.suffix(); QString sBackupFileFullPath = QDir(info.absolutePath()).filePath(sBackupFile); - bool ok = QFile::rename(info.absoluteFilePath(), sBackupFileFullPath); + bool ok = QFile::copy(info.absoluteFilePath(), sBackupFileFullPath); if (!ok) { FILEMANAGER_LOG("Cannot backup the previous file"); From 737d89c7983677238673ff5858e99efb68d678ce Mon Sep 17 00:00:00 2001 From: MrStevns Date: Thu, 12 May 2022 07:28:04 +0200 Subject: [PATCH 4/5] Remove status from isArchive method --- core_lib/src/structure/filemanager.cpp | 23 +++++++++-------------- core_lib/src/structure/filemanager.h | 2 +- core_lib/src/util/pencilerror.h | 1 - 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index ed5224fabb..75337d4730 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -50,16 +50,11 @@ Object* FileManager::load(const QString& sFileName) QString strDataFolder; // Test file format: new zipped .pclx or old .pcl? - Status archiveSt = isArchiveFormat(sFileName); + bool isArchive = isArchiveFormat(sFileName); + QString isArchiveStr = "Is archive: " + QString(isArchive); - QString isArchive = "Is archive: "; - if (archiveSt.ok()) { - dd << isArchive.append("true"); - } - - if (archiveSt == Status::NOT_ARCHIVE_FORMAT) + if (!isArchive) { - dd << isArchive.append("false"); dd << "Recognized Old Pencil2D File Format (*.pcl) !"; strMainXMLFile = sFileName; @@ -211,12 +206,12 @@ bool FileManager::loadObjectOldWay(Object* object, const QDomElement& root) return object->loadXML(root, [this] { progressForward(); }); } -Status FileManager::isArchiveFormat(const QString& fileName) const +bool FileManager::isArchiveFormat(const QString& fileName) const { if (QFileInfo(fileName).suffix().compare(PFF_BIG_LETTER_EXTENSION, Qt::CaseInsensitive) != 0) { - return Status::NOT_ARCHIVE_FORMAT; + return false; } - return Status::OK; + return true; } Status FileManager::save(const Object* object, const QString& sFileName) @@ -271,8 +266,8 @@ Status FileManager::save(const Object* object, const QString& sFileName) QString sMainXMLFile; QString sDataFolder; - Status isArchiveSt = isArchiveFormat(sFileName); - if (isArchiveSt == Status::NOT_ARCHIVE_FORMAT) + bool isArchive = isArchiveFormat(sFileName); + if (!isArchive) { dd << "Old Pencil2D File Format (*.pcl) !"; @@ -328,7 +323,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) progressForward(); - if (isArchiveSt.ok()) + if (isArchive) { QString sBackupFile = backupPreviousFile(sFileName); diff --git a/core_lib/src/structure/filemanager.h b/core_lib/src/structure/filemanager.h index 71ce94b4db..fa946d3800 100644 --- a/core_lib/src/structure/filemanager.h +++ b/core_lib/src/structure/filemanager.h @@ -59,7 +59,7 @@ class FileManager : public QObject bool loadObject(Object*, const QDomElement& root); bool loadObjectOldWay(Object*, const QDomElement& root); - Status isArchiveFormat(const QString& fileName) const; + bool isArchiveFormat(const QString& fileName) const; bool loadPalette(Object*); Status writeKeyFrameFiles(const Object* obj, const QString& dataFolder, QStringList& filesWritten); Status writeMainXml(const Object* obj, const QString& mainXmlPath, QStringList& filesWritten); diff --git a/core_lib/src/util/pencilerror.h b/core_lib/src/util/pencilerror.h index 9a7ef9ef87..e35870f6cd 100644 --- a/core_lib/src/util/pencilerror.h +++ b/core_lib/src/util/pencilerror.h @@ -56,7 +56,6 @@ class Status ERROR_INVALID_XML_FILE, ERROR_INVALID_PENCIL_FILE, ERROR_MINIZ_FAIL, - NOT_ARCHIVE_FORMAT, // General ERROR_INVALID_LAYER_TYPE, From 2526ce523882a53fc50790a69a8c2aef576835f0 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Thu, 12 May 2022 19:26:03 +0200 Subject: [PATCH 5/5] Make sure backup count checks for the specific file --- core_lib/src/structure/filemanager.cpp | 37 +++++++++++++++++--------- core_lib/src/structure/filemanager.h | 1 + core_lib/src/util/fileformat.h | 1 + 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 75337d4730..2a8156a900 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -558,25 +558,38 @@ void FileManager::handleOpenProjectError(Status::ErrorCode error, const DebugDet removePFFTmpDirectory(mstrLastTempFolder); } +int FileManager::countExistingBackups(const QString& fileName) const +{ + QFileInfo fileInfo(fileName); + QDir directory(fileInfo.absoluteDir()); + const QString& baseName = fileInfo.completeBaseName(); + + int backupCount = 0; + for (QFileInfo dirFileInfo : directory.entryInfoList(QDir::Filter::Files)) { + QString searchFileBaseName = dirFileInfo.completeBaseName(); + if (baseName.compare(searchFileBaseName) == 0 && searchFileBaseName.contains(PFF_BACKUP_IDENTIFIER)) { + backupCount++; + } + } + + return backupCount; +} + QString FileManager::backupPreviousFile(const QString& fileName) { if (!QFile::exists(fileName)) return ""; - QFileInfo info(fileName); - int backupCount = 1; - QDir directory(info.absoluteDir()); - QString backupString = "backup"; - for (QFileInfo info : directory.entryInfoList(QDir::Filter::Files)) { - if (info.completeBaseName().contains(backupString)) { - backupCount++; - } - } + QFileInfo fileInfo(fileName); + QString baseName = fileInfo.completeBaseName(); + + int backupCount = countExistingBackups(fileName) + 1; // start index 1 + QString countStr = QString::number(backupCount); - QString sBackupFile = info.baseName() + "." + backupString + QString::number(backupCount) + "." + info.suffix(); - QString sBackupFileFullPath = QDir(info.absolutePath()).filePath(sBackupFile); + QString sBackupFile = baseName + "." + PFF_BACKUP_IDENTIFIER + countStr + "." + fileInfo.suffix(); + QString sBackupFileFullPath = QDir(fileInfo.absolutePath()).filePath(sBackupFile); - bool ok = QFile::copy(info.absoluteFilePath(), sBackupFileFullPath); + bool ok = QFile::copy(fileInfo.absoluteFilePath(), sBackupFileFullPath); if (!ok) { FILEMANAGER_LOG("Cannot backup the previous file"); diff --git a/core_lib/src/structure/filemanager.h b/core_lib/src/structure/filemanager.h index fa946d3800..75d5ec75ed 100644 --- a/core_lib/src/structure/filemanager.h +++ b/core_lib/src/structure/filemanager.h @@ -73,6 +73,7 @@ class FileManager : public QObject QString backupPreviousFile(const QString& fileName); void deleteBackupFile(const QString& fileName); + int countExistingBackups(const QString& fileName) const; void progressForward(); diff --git a/core_lib/src/util/fileformat.h b/core_lib/src/util/fileformat.h index c246b9b7c5..138b205552 100644 --- a/core_lib/src/util/fileformat.h +++ b/core_lib/src/util/fileformat.h @@ -26,6 +26,7 @@ GNU General Public License for more details. #define PFF_OLD_BIG_LETTER_EXTENSION "PCL" #define PFF_EXTENSION ".pclx" #define PFF_BIG_LETTER_EXTENSION "PCLX" +#define PFF_BACKUP_IDENTIFIER "backup" #define PFF_OPEN_PROJECT_EXT_FILTER \ QCoreApplication::translate("FileFormat", "Pencil2D formats") + " (*.pclx *.pcl);;" + QCoreApplication::translate("FileFormat", "Pencil2D Project") + " (*.pclx);;" + QCoreApplication::translate("FileFormat", "Legacy Pencil2D Project") + " (*.pcl)"