From 741a56dfdf53a47ce02d90f7232c994914c34815 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Fri, 22 Nov 2024 19:11:17 +0100 Subject: [PATCH 01/28] Fix case where saving with no undo changes would fail --- core_lib/src/managers/undoredomanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core_lib/src/managers/undoredomanager.cpp b/core_lib/src/managers/undoredomanager.cpp index 7dea4748fe..66e2a9ef2c 100644 --- a/core_lib/src/managers/undoredomanager.cpp +++ b/core_lib/src/managers/undoredomanager.cpp @@ -86,7 +86,7 @@ Status UndoRedoManager::save(Object* /*o*/) { if (mNewBackupSystemEnabled) { mUndoStack.setClean(); - } else { + } else if (!mLegacyBackupList.isEmpty() && mLegacyBackupIndex < mLegacyBackupList.count()) { mLegacyBackupAtSave = mLegacyBackupList[mLegacyBackupIndex]; } return Status::OK; From 0d9bf8a14119d178707afa5fdd3b3968f2b5b795 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Fri, 22 Nov 2024 19:16:15 +0100 Subject: [PATCH 02/28] QMiniz: Simplify logic --- core_lib/src/qminiz.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index 376075fb87..e57622f0a8 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -74,15 +74,14 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q } mz_zip_archive* mz = new mz_zip_archive; - mz_zip_zero_struct(mz); - - mz_bool ok = mz_zip_writer_init_file(mz, zipFilePath.toUtf8().data(), 0); - ScopeGuard mzScopeGuard([&] { - mz_zip_writer_end(mz); delete mz; }); + mz_zip_zero_struct(mz); + + mz_bool ok = mz_zip_writer_init_file(mz, zipFilePath.toUtf8().data(), 0); + if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); @@ -132,9 +131,6 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q ok &= mz_zip_writer_end(mz); - mzScopeGuard.dismiss(); - ScopeGuard mzScopeGuard2([&] { delete mz; }); - if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); From 871ccc9b5169ed28537a814c263619e3f6a554d3 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Fri, 22 Nov 2024 19:18:06 +0100 Subject: [PATCH 03/28] QMiniz: Make sure to abort when failing to compress file The problem with not aborting here is that while one file can fail, it will be noted down but if the next file succeeds, then it will seem like the project got compressed successfully. --- core_lib/src/qminiz.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index e57622f0a8..7351cafa7e 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -118,7 +118,8 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Cannot add %3: error %1, %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err), sRelativePath); + dd << QString("Cannot add %3: error %1, %2 - Aborting!").arg(static_cast(err)).arg(mz_zip_get_error_string(err), sRelativePath); + return Status(Status::FAIL, dd); } } ok &= mz_zip_writer_finalize_archive(mz); From 33115d410d20f1d2e77d351c5a126a3d9a9be6ea Mon Sep 17 00:00:00 2001 From: MrStevns Date: Fri, 22 Nov 2024 20:30:50 +0100 Subject: [PATCH 04/28] Improve the error detail log for readability And get rid of useless or confusing information. --- core_lib/src/structure/filemanager.cpp | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 5a696af457..87f93aab23 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -34,6 +34,7 @@ FileManager::FileManager(QObject* parent) : QObject(parent) Object* FileManager::load(const QString& sFileName) { DebugDetails dd; + dd << "\n[Project diagnostics]\n"; dd << QString("File name: ").append(sFileName); if (!QFile::exists(sFileName)) { @@ -50,19 +51,23 @@ Object* FileManager::load(const QString& sFileName) QString strMainXMLFile; QString strDataFolder; + QString workingDirPath = obj->workingDir(); + // Test file format: new zipped .pclx or old .pcl? bool isArchive = isArchiveFormat(sFileName); + QString fileFormat = "File format: %1"; if (!isArchive) { - dd << "Recognized Old Pencil2D File Format (*.pcl) !"; + dd << fileFormat.arg(".pcl"); strMainXMLFile = sFileName; strDataFolder = strMainXMLFile + "." + PFF_OLD_DATA_DIR; } else { - dd << "Recognized New zipped Pencil2D File Format (*.pclx) !"; + dd << fileFormat.arg(".pclx"); + dd << QString("Project extracted to: %1 ").arg(workingDirPath); Status sanityCheck = MiniZ::sanityCheck(sFileName); @@ -70,18 +75,14 @@ Object* FileManager::load(const QString& sFileName) if (!sanityCheck.ok()) { dd.collect(sanityCheck.details()); } else { - Status unzipStatus = unzip(sFileName, obj->workingDir()); + Status unzipStatus = unzip(sFileName, workingDirPath); dd.collect(unzipStatus.details()); } - strMainXMLFile = QDir(obj->workingDir()).filePath(PFF_XML_FILE_NAME); - strDataFolder = QDir(obj->workingDir()).filePath(PFF_DATA_DIR); + strMainXMLFile = QDir(workingDirPath).filePath(PFF_XML_FILE_NAME); + strDataFolder = QDir(workingDirPath).filePath(PFF_DATA_DIR); } - dd << QString("XML file: ").append(strMainXMLFile) - << QString("Data folder: ").append(strDataFolder) - << QString("Working folder: ").append(obj->workingDir()); - obj->setDataDir(strDataFolder); obj->setMainXMLFile(strMainXMLFile); @@ -92,15 +93,17 @@ Object* FileManager::load(const QString& sFileName) QFile file(strMainXMLFile); if (!file.exists()) { - dd << "Main XML file does not exist"; + dd << "Error: Main XML exists: No"; handleOpenProjectError(Status::ERROR_INVALID_XML_FILE, dd); return nullptr; } if (!file.open(QFile::ReadOnly)) { + dd << "Error: Main XML file is read only!"; handleOpenProjectError(Status::ERROR_FILE_CANNOT_OPEN, dd); return nullptr; } + dd << "Main XML exists: Yes"; QDomDocument xmlDoc; if (!xmlDoc.setContent(&file)) From 466f6bea0f3931f756278e683289b5db75c83b17 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Fri, 22 Nov 2024 20:57:52 +0100 Subject: [PATCH 05/28] ErrorDialog: Add copy to clipboard --- app/src/errordialog.cpp | 12 ++++++++++++ app/src/errordialog.h | 3 +++ 2 files changed, 15 insertions(+) diff --git a/app/src/errordialog.cpp b/app/src/errordialog.cpp index a69d243be3..e1d5ee9967 100644 --- a/app/src/errordialog.cpp +++ b/app/src/errordialog.cpp @@ -17,6 +17,9 @@ GNU General Public License for more details. #include "errordialog.h" #include "ui_errordialog.h" +#include +#include + ErrorDialog::ErrorDialog( QString title, QString description, QString details, QWidget *parent ) : QDialog( parent ), ui(new Ui::ErrorDialog) @@ -34,9 +37,18 @@ ErrorDialog::ErrorDialog( QString title, QString description, QString details, Q { ui->details->setText( QString( "
%1
" ).arg( details ) ); } + + QPushButton* copyToClipboard = new QPushButton(tr("Copy to Clipboard")); + ui->buttonBox->addButton(copyToClipboard, QDialogButtonBox::ActionRole); + + connect(copyToClipboard, &QPushButton::clicked, this, &ErrorDialog::onCopyToClipboard); } ErrorDialog::~ErrorDialog() { delete ui; } + +void ErrorDialog::onCopyToClipboard() { + QGuiApplication::clipboard()->setText(ui->details->toPlainText()); +} diff --git a/app/src/errordialog.h b/app/src/errordialog.h index 690c19ee7f..ae9b7169a1 100644 --- a/app/src/errordialog.h +++ b/app/src/errordialog.h @@ -32,6 +32,9 @@ class ErrorDialog : public QDialog explicit ErrorDialog(QString title, QString description, QString details = QString(), QWidget *parent = nullptr); ~ErrorDialog(); +public slots: + void onCopyToClipboard(); + private: Ui::ErrorDialog *ui; }; From 335d88df0f5cdbce12fc807e6b2b8ccbf85f9aef Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 09:32:51 +0100 Subject: [PATCH 06/28] Improve error details for readability - take 2 --- core_lib/src/qminiz.cpp | 6 ++++-- core_lib/src/structure/filemanager.cpp | 13 ++++++++----- core_lib/src/util/pencilerror.cpp | 19 ++++++++++--------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index 7351cafa7e..10f24b7afc 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -39,11 +39,13 @@ Status MiniZ::sanityCheck(const QString& sZipFilePath) if (!readOk || !closeOk) { DebugDetails dd; + + dd << "\n [Miniz sanity check]\n"; 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)); + dd << QString("Found an error while reading the file. Error code: %2, reason: %3").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)); + dd << QString("Found an error while closing the file. Error code: %2, reason: %3").arg(static_cast(close_err)).arg(mz_zip_get_error_string(close_err)); } return Status(Status::ERROR_MINIZ_FAIL, dd); } diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 87f93aab23..dc82fe65e2 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -74,6 +74,9 @@ Object* FileManager::load(const QString& sFileName) // Let's check if we can read the file before we try to unzip. if (!sanityCheck.ok()) { dd.collect(sanityCheck.details()); + dd << "\n Error: Miniz sanity check failed!"; + handleOpenProjectError(Status::ERROR_INVALID_XML_FILE, dd); + return nullptr; } else { Status unzipStatus = unzip(sFileName, workingDirPath); dd.collect(unzipStatus.details()); @@ -93,7 +96,7 @@ Object* FileManager::load(const QString& sFileName) QFile file(strMainXMLFile); if (!file.exists()) { - dd << "Error: Main XML exists: No"; + dd << "Error: No main XML exists!"; handleOpenProjectError(Status::ERROR_INVALID_XML_FILE, dd); return nullptr; } @@ -109,7 +112,7 @@ Object* FileManager::load(const QString& sFileName) if (!xmlDoc.setContent(&file)) { FILEMANAGER_LOG("Couldn't open the main XML file"); - dd << "Error parsing or opening the main XML file"; + dd << "Error: Unable to parse or open the main XML file"; handleOpenProjectError(Status::ERROR_INVALID_XML_FILE, dd); return nullptr; } @@ -118,7 +121,7 @@ Object* FileManager::load(const QString& sFileName) if (!(type.name() == "PencilDocument" || type.name() == "MyObject")) { FILEMANAGER_LOG("Invalid main XML doctype"); - dd << QString("Invalid main XML doctype: ").append(type.name()); + dd << QString("Error: Invalid main XML doctype: ").append(type.name()); handleOpenProjectError(Status::ERROR_INVALID_PENCIL_FILE, dd); return nullptr; } @@ -126,7 +129,7 @@ Object* FileManager::load(const QString& sFileName) QDomElement root = xmlDoc.documentElement(); if (root.isNull()) { - dd << "Main XML root node is null"; + dd << "Error: Main XML root node is null"; handleOpenProjectError(Status::ERROR_INVALID_PENCIL_FILE, dd); return nullptr; } @@ -147,7 +150,7 @@ Object* FileManager::load(const QString& sFileName) if (!ok) { obj.reset(); - dd << "Issue occurred during object loading"; + dd << "Error: Issue occurred during object loading"; handleOpenProjectError(Status::ERROR_INVALID_PENCIL_FILE, dd); return nullptr; } diff --git a/core_lib/src/util/pencilerror.cpp b/core_lib/src/util/pencilerror.cpp index 37198c48c3..cc6be5e934 100644 --- a/core_lib/src/util/pencilerror.cpp +++ b/core_lib/src/util/pencilerror.cpp @@ -18,6 +18,7 @@ GNU General Public License for more details. #include "pencilerror.h" #include #include +#include DebugDetails::DebugDetails() { @@ -55,22 +56,22 @@ void DebugDetails::appendSystemInfo() return; #if QT_VERSION >= QT_VERSION_CHECK(5, 4, 0) - mDetails << "System Info"; + mDetails << "\n[System Info]\n"; #if defined(PENCIL2D_RELEASE_BUILD) - mDetails << "Pencil2D version: " APP_VERSION " (stable)"; + mDetails << "  Pencil2D version: " APP_VERSION " (stable)"; #elif defined(PENCIL2D_NIGHTLY_BUILD) - mDetails << "Pencil2D version: " APP_VERSION " (nightly)"; + mDetails << "  Pencil2D version: " APP_VERSION " (nightly)"; #else - mDetails << "Pencil2D version: " APP_VERSION " (dev)"; + mDetails << "  Pencil2D version: " APP_VERSION " (dev)"; #endif #if defined(GIT_EXISTS) - mDetails << "Commit: " S__GIT_COMMIT_HASH; + mDetails << "  Commit: " S__GIT_COMMIT_HASH; #endif - mDetails << "Build ABI: " + QSysInfo::buildAbi(); - mDetails << "Kernel: " + QSysInfo::kernelType() + ", " + QSysInfo::kernelVersion(); - mDetails << "Operating System: " + QSysInfo::prettyProductName(); - mDetails << "end"; + mDetails << "  Build ABI: " + QSysInfo::buildAbi(); + mDetails << "  Kernel: " + QSysInfo::kernelType() + ", " + QSysInfo::kernelVersion(); + mDetails << "  Operating System: " + QSysInfo::prettyProductName(); + mDetails << "  Language: " + QLocale::system().name(); #endif } From 422cce57a85b4b2f62180a23b0abc0646021b978 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 10:43:18 +0100 Subject: [PATCH 07/28] Improve error details for readability - take 3 --- core_lib/src/structure/filemanager.cpp | 42 ++++++++++++++------------ 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index dc82fe65e2..6da38e74d4 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -34,7 +34,7 @@ FileManager::FileManager(QObject* parent) : QObject(parent) Object* FileManager::load(const QString& sFileName) { DebugDetails dd; - dd << "\n[Project diagnostics]\n"; + dd << "\n[Project LOAD diagnostics]\n"; dd << QString("File name: ").append(sFileName); if (!QFile::exists(sFileName)) { @@ -51,12 +51,10 @@ Object* FileManager::load(const QString& sFileName) QString strMainXMLFile; QString strDataFolder; - QString workingDirPath = obj->workingDir(); - // Test file format: new zipped .pclx or old .pcl? bool isArchive = isArchiveFormat(sFileName); - QString fileFormat = "File format: %1"; + QString fileFormat = "Project format: %1"; if (!isArchive) { dd << fileFormat.arg(".pcl"); @@ -66,15 +64,16 @@ Object* FileManager::load(const QString& sFileName) } else { + QString workingDirPath = obj->workingDir(); + dd << fileFormat.arg(".pclx"); - dd << QString("Project extracted to: %1 ").arg(workingDirPath); 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()); - dd << "\n Error: Miniz sanity check failed!"; + dd << "\n Error: Unable to extract project, miniz sanity check failed."; handleOpenProjectError(Status::ERROR_INVALID_XML_FILE, dd); return nullptr; } else { @@ -82,6 +81,8 @@ Object* FileManager::load(const QString& sFileName) dd.collect(unzipStatus.details()); } + dd << QString("Project extracted to: %1 ").arg(workingDirPath); + strMainXMLFile = QDir(workingDirPath).filePath(PFF_XML_FILE_NAME); strDataFolder = QDir(workingDirPath).filePath(PFF_DATA_DIR); } @@ -222,16 +223,16 @@ bool FileManager::isArchiveFormat(const QString& fileName) const Status FileManager::save(const Object* object, const QString& sFileName) { DebugDetails dd; - dd << __FUNCTION__; - dd << ("sFileName = " + sFileName); + dd << "\n[Project SAVE diagnostics]\n"; + dd << ("file name:" + sFileName); if (object == nullptr) { - dd << "Object parameter is null"; + dd << "Error: Object parameter is null"; return Status(Status::INVALID_ARGUMENT, dd); } if (sFileName.isEmpty()) { - dd << "File name is empty"; + dd << "Error: File name is empty, unable to save."; return Status(Status::INVALID_ARGUMENT, dd, tr("Invalid Save Path"), tr("The path is empty.")); @@ -246,7 +247,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) QFileInfo fileInfo(sFileName); if (fileInfo.isDir()) { - dd << "FileName points to a directory"; + dd << "Error: File name must point to a file, not a directory."; return Status(Status::INVALID_ARGUMENT, dd, tr("Invalid Save Path"), tr("The path (\"%1\") points to a directory.").arg(fileInfo.absoluteFilePath())); @@ -254,14 +255,14 @@ Status FileManager::save(const Object* object, const QString& sFileName) QFileInfo parentDirInfo(fileInfo.dir().absolutePath()); if (!parentDirInfo.exists()) { - dd << "The parent directory of sFileName does not exist"; + dd << QString("Error: The parent directory of %1 does not exist").arg(sFileName); return Status(Status::INVALID_ARGUMENT, dd, tr("Invalid Save Path"), tr("The directory (\"%1\") does not exist.").arg(parentDirInfo.absoluteFilePath())); } if ((fileInfo.exists() && !fileInfo.isWritable()) || !parentDirInfo.isWritable()) { - dd << "Filename points to a location that is not writable"; + dd << "Error: File name points to a location that is not writable"; return Status(Status::INVALID_ARGUMENT, dd, tr("Invalid Save Path"), tr("The path (\"%1\") is not writable.").arg(fileInfo.absoluteFilePath())); @@ -271,22 +272,23 @@ Status FileManager::save(const Object* object, const QString& sFileName) QString sMainXMLFile; QString sDataFolder; + QString fileFormat = QString("Project format: %1"); bool isArchive = isArchiveFormat(sFileName); if (!isArchive) { - dd << "Old Pencil2D File Format (*.pcl) !"; + dd << fileFormat.arg(".pcl"); sMainXMLFile = sFileName; sDataFolder = sMainXMLFile + "." + PFF_OLD_DATA_DIR; } else { - dd << "New zipped Pencil2D File Format (*.pclx) !"; - dd.collect(MiniZ::sanityCheck(sFileName).details()); - sTempWorkingFolder = object->workingDir(); + + dd << fileFormat.arg(".pclx"); + dd << QString("Project TEMP location: %1").arg(sTempWorkingFolder); + Q_ASSERT(QDir(sTempWorkingFolder).exists()); - dd << QString("TempWorkingFolder = ").append(sTempWorkingFolder); sMainXMLFile = QDir(sTempWorkingFolder).filePath(PFF_XML_FILE_NAME); sDataFolder = QDir(sTempWorkingFolder).filePath(PFF_OLD_DATA_DIR); @@ -299,7 +301,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) if (!dir.mkpath(sDataFolder)) { - dd << QString("dir.absolutePath() = %1").arg(dir.absolutePath()); + dd << QString("Error: Unable to create data directory, tried to save to: %1").arg(dir.absolutePath()); return Status(Status::FAIL, dd, tr("Cannot Create Data Directory"), tr("Failed to create directory \"%1\". Please make sure you have sufficient permissions.").arg(sDataFolder)); @@ -307,7 +309,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) } if (!dataInfo.isDir()) { - dd << QString("dataInfo.absoluteFilePath() = ").append(dataInfo.absoluteFilePath()); + dd << QString("Error: Expected data to be a directory but found %1 instead").arg(dataInfo.absoluteFilePath()); return Status(Status::FAIL, dd, tr("Cannot Create Data Directory"), From 8b705f8f0d4d1c1e587c03673588209d4d1d12c8 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 13:55:07 +0100 Subject: [PATCH 08/28] Write a note when a backup has been made --- core_lib/src/structure/filemanager.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 6da38e74d4..7af8cb357e 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -334,6 +334,10 @@ Status FileManager::save(const Object* object, const QString& sFileName) { QString sBackupFile = backupPreviousFile(sFileName); + if (!sBackupFile.isEmpty()) { + dd << QString("\nNote: A backup has been made here: %1").arg(sBackupFile); + } + if (!saveOk) { return Status(Status::FAIL, dd, tr("Internal Error"), From 30397b2f2ef1e946359ee03b94d0871064f570b1 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 14:09:40 +0100 Subject: [PATCH 09/28] FileManager: Fix logic does not account for existing backup files The logic didn't account for existing files with "backup" suffix, eg. A folder may contain: MyProject.pclx MyProject.backup1.pclx If the user opened MyProject.pclx, and failed to save it later, the logic would not count existing backup files like: "MyProject.backup1.pclx" and as such the result would be no backup being made. --- core_lib/src/structure/filemanager.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 7af8cb357e..bc00cc8f87 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -571,12 +571,15 @@ int FileManager::countExistingBackups(const QString& fileName) const { QFileInfo fileInfo(fileName); QDir directory(fileInfo.absoluteDir()); - const QString& baseName = fileInfo.completeBaseName(); + const QString& baseFileName = fileInfo.completeBaseName(); int backupCount = 0; for (const QFileInfo &dirFileInfo : directory.entryInfoList(QDir::Filter::Files)) { - QString searchFileBaseName = dirFileInfo.completeBaseName(); - if (baseName.compare(searchFileBaseName) == 0 && searchFileBaseName.contains(PFF_BACKUP_IDENTIFIER)) { + QString searchFileAbsPath = dirFileInfo.absoluteFilePath(); + QString searchFileName = dirFileInfo.baseName(); + + bool sameBaseName = baseFileName.compare(searchFileName) == 0; + if (sameBaseName && searchFileAbsPath.contains(PFF_BACKUP_IDENTIFIER)) { backupCount++; } } From e106ac5ce3fe56fb26eb9cd524d6c4c205f0dd4c Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 15:01:59 +0100 Subject: [PATCH 10/28] Improve layer saving diagnostics --- core_lib/src/structure/filemanager.cpp | 16 ++++++++++++---- core_lib/src/structure/layer.cpp | 2 +- core_lib/src/structure/layerbitmap.cpp | 2 +- core_lib/src/structure/layersound.cpp | 4 ++-- core_lib/src/structure/layervector.cpp | 8 ++++---- 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index bc00cc8f87..f5faada162 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -639,9 +639,10 @@ bool FileManager::loadPalette(Object* obj) Status FileManager::writeKeyFrameFiles(const Object* object, const QString& dataFolder, QStringList& filesFlushed) { DebugDetails dd; + dd << "\n[Keyframes WRITE diagnostics]\n"; const int numLayers = object->getLayerCount(); - dd << QString("Total %1 layers").arg(numLayers); + dd << QString("Total layer count: %1").arg(numLayers); for (int i = 0; i < numLayers; ++i) { @@ -661,25 +662,32 @@ Status FileManager::writeKeyFrameFiles(const Object* object, const QString& data { saveLayersOK = false; dd.collect(st.details()); - dd << QString(" !! Failed to save Layer[%1] %2").arg(i).arg(layer->name()); + dd << QString("\n Error: Failed to save Layer[%1] %2").arg(i).arg(layer->name()); } } - dd << "All Layers saved"; progressForward(); auto errorCode = (saveLayersOK) ? Status::OK : Status::FAIL; + + if (saveLayersOK) { + dd << "\n All Layers saved"; + } else { + dd << "\n Error: Unable to save all layers"; + } + return Status(errorCode, dd); } Status FileManager::writeMainXml(const Object* object, const QString& mainXmlPath, QStringList& filesWritten) { DebugDetails dd; + dd << "\n[XML WRITE diagnostics]\n"; QFile file(mainXmlPath); if (!file.open(QFile::WriteOnly | QFile::Text)) { - dd << "Failed to open Main XML" << mainXmlPath; + dd << QString("Error: Failed to open Main XML at: %1, \n Reason: %2").arg(mainXmlPath).arg(file.errorString()); return Status(Status::ERROR_FILE_CANNOT_OPEN, dd); } diff --git a/core_lib/src/structure/layer.cpp b/core_lib/src/structure/layer.cpp index c05ca919f6..2ac84e690a 100644 --- a/core_lib/src/structure/layer.cpp +++ b/core_lib/src/structure/layer.cpp @@ -340,7 +340,7 @@ bool Layer::loadKey(KeyFrame* pKey) Status Layer::save(const QString& sDataFolder, QStringList& attachedFiles, ProgressCallback progressStep) { DebugDetails dd; - dd << __FUNCTION__; + dd << "\n[Layer SAVE diagnostics]\n"; bool ok = true; diff --git a/core_lib/src/structure/layerbitmap.cpp b/core_lib/src/structure/layerbitmap.cpp index 974ce2b028..9728f52e36 100644 --- a/core_lib/src/structure/layerbitmap.cpp +++ b/core_lib/src/structure/layerbitmap.cpp @@ -95,7 +95,7 @@ Status LayerBitmap::saveKeyFrameFile(KeyFrame* keyframe, QString path) dd << "LayerBitmap::saveKeyFrame"; dd << QString(" KeyFrame.pos() = %1").arg(keyframe->pos()); dd << QString(" strFilePath = %1").arg(strFilePath); - dd << QString("BitmapImage could not be saved"); + dd << QString("Error: Failed to save BitmapImage"); dd.collect(st.details()); return Status(Status::FAIL, dd); } diff --git a/core_lib/src/structure/layersound.cpp b/core_lib/src/structure/layersound.cpp index 2cb42f72d0..512e9a5abe 100644 --- a/core_lib/src/structure/layersound.cpp +++ b/core_lib/src/structure/layersound.cpp @@ -145,11 +145,11 @@ Status LayerSound::saveKeyFrameFile(KeyFrame* key, QString path) key->setFileName(""); DebugDetails dd; - dd << __FUNCTION__; + dd << "LayerSound::saveKeyFrameFile"; dd << QString(" KeyFrame.pos() = %1").arg(key->pos()); dd << QString(" Key->fileName() = %1").arg(key->fileName()); dd << QString(" FilePath = %1").arg(sDestFileLocation); - dd << QString("Couldn't save the sound clip"); + dd << QString("Error: Failed to save SoundClip"); return Status(Status::FAIL, dd); } key->setFileName(sDestFileLocation); diff --git a/core_lib/src/structure/layervector.cpp b/core_lib/src/structure/layervector.cpp index deea0a3cdb..b789ada28d 100644 --- a/core_lib/src/structure/layervector.cpp +++ b/core_lib/src/structure/layervector.cpp @@ -92,10 +92,10 @@ Status LayerVector::saveKeyFrameFile(KeyFrame* keyFrame, QString path) vecImage->setFileName(""); DebugDetails dd; - dd << __FUNCTION__; - dd << QString("KeyFrame.pos() = %1").arg(keyFrame->pos()); - dd << QString("FilePath = ").append(strFilePath); - dd << "- VectorImage failed to write"; + dd << "LayerVector::saveKeyFrameFile"; + dd << QString(" KeyFrame.pos() = %1").arg(keyFrame->pos()); + dd << QString(" strFilePath = ").append(strFilePath); + dd << "Error: Failed to save VectorImage"; dd.collect(st.details()); return Status(Status::FAIL, dd); } From 93fd589a303ab1b968ea0fd033c3eb128611f82c Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 16:20:11 +0100 Subject: [PATCH 11/28] Fix where where project couldn't be saved because miniz tried to zip non existent files This is caused by the new logic actually catching when a file has gone missing while saving. While it may not fix any bugs inheritly, it should make it easier to diagnose problems in the future. --- core_lib/src/graphics/bitmap/bitmapimage.cpp | 9 +++++++-- core_lib/src/structure/layerbitmap.cpp | 4 +++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/core_lib/src/graphics/bitmap/bitmapimage.cpp b/core_lib/src/graphics/bitmap/bitmapimage.cpp index 6ec5b019f9..7a952fec06 100644 --- a/core_lib/src/graphics/bitmap/bitmapimage.cpp +++ b/core_lib/src/graphics/bitmap/bitmapimage.cpp @@ -742,9 +742,14 @@ Status BitmapImage::writeFile(const QString& filename) if(f.exists()) { bool b = f.remove(); - return (b) ? Status::OK : Status::FAIL; + if (!b) { + return Status::FAIL; + } } - return Status::SAFE; + + // The frame is likely empty, act like there's no file name + // so we don't end up writing to it later. + setFileName(""); } return Status::SAFE; } diff --git a/core_lib/src/structure/layerbitmap.cpp b/core_lib/src/structure/layerbitmap.cpp index 9728f52e36..fc88797d4e 100644 --- a/core_lib/src/structure/layerbitmap.cpp +++ b/core_lib/src/structure/layerbitmap.cpp @@ -193,7 +193,9 @@ QDomElement LayerBitmap::createDomElement(QDomDocument& doc) const imageTag.setAttribute("opacity", pImg->getOpacity()); layerElem.appendChild(imageTag); - Q_ASSERT(QFileInfo(pKeyFrame->fileName()).fileName() == fileName(pKeyFrame)); + if (!pKeyFrame->fileName().isEmpty()) { + Q_ASSERT(QFileInfo(pKeyFrame->fileName()).fileName() == fileName(pKeyFrame)); + } }); return layerElem; From fb0b24e686bf5e7adb8c99a9bf559d6b71ab913e Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 16:48:05 +0100 Subject: [PATCH 12/28] Improve diagnostics around qminiz and archiving --- core_lib/src/qminiz.cpp | 35 +++++++++++++------------- core_lib/src/structure/filemanager.cpp | 16 +++++++----- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index 10f24b7afc..74914ffcc2 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -87,7 +87,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Miniz writer init failed: error %1, %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("Miniz writer init failed. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); } // Add special uncompressed mimetype file to help with the identification of projects @@ -100,7 +100,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Cannot add mimetype: error %1").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("Cannot add mimetype. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); } } @@ -120,7 +120,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Cannot add %3: error %1, %2 - Aborting!").arg(static_cast(err)).arg(mz_zip_get_error_string(err), sRelativePath); + dd << QString("Cannot add %3. Error code: %1, reason: %2 - Aborting!").arg(static_cast(err)).arg(mz_zip_get_error_string(err), sRelativePath); return Status(Status::FAIL, dd); } } @@ -128,7 +128,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Miniz finalize archive failed: error %1, %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("Miniz finalize archive failed. Error code %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); return Status(Status::FAIL, dd); } @@ -151,6 +151,7 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) if (!QFile::exists(zipFilePath)) { + dd << QString("Error: Zip file does not exist."); return Status::FILE_NOT_FOUND; } @@ -165,17 +166,19 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) baseDir.makeAbsolute(); mz_zip_archive* mz = new mz_zip_archive; - mz_zip_zero_struct(mz); - - mz_bool ok = mz_zip_reader_init_file(mz, zipFilePath.toUtf8().data(), 0); - ScopeGuard mzScopeGuard([&] { - mz_zip_reader_end(mz); delete mz; }); - if (!ok) + mz_zip_zero_struct(mz); + + mz_bool ok = mz_zip_reader_init_file(mz, zipFilePath.toUtf8().data(), 0); + + if (!ok) { + mz_zip_error err = mz_zip_get_last_error(mz); + dd << QString("Error: Failed to init the zip reader. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); return Status(Status::FAIL, dd); + } int num = mz_zip_reader_get_num_files(mz); @@ -214,21 +217,19 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) if (!extractOK) { ok = false; - dd << "File extraction failed."; + mz_zip_error err = mz_zip_get_last_error(mz); + dd << QString("File extraction failed. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); } } } ok &= mz_zip_reader_end(mz); - mzScopeGuard.dismiss(); - ScopeGuard mzScopeGuard2([&] { - delete mz; - }); - if (!ok) { - dd << "Unzip error!"; + mz_zip_error err = mz_zip_get_last_error(mz); + dd << QString("Error: Failed to end zip reader, Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err));; + return Status(Status::FAIL, dd); } return Status::OK; } diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index f5faada162..2c8627392c 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -332,6 +332,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) if (isArchive) { + dd << "\n[Archiving diagnostics]\n"; QString sBackupFile = backupPreviousFile(sFileName); if (!sBackupFile.isEmpty()) { @@ -341,23 +342,26 @@ Status FileManager::save(const Object* object, const QString& sFileName) if (!saveOk) { return Status(Status::FAIL, dd, tr("Internal Error"), - tr("An internal error occurred. Your file may not be saved successfully.")); + tr("An internal error occurred. The project could not be saved.")); } - dd << "Miniz"; + dd << "Miniz: Zipping..."; Status stMiniz = MiniZ::compressFolder(sFileName, sTempWorkingFolder, filesToZip, "application/x-pencil2d-pclx"); if (!stMiniz.ok()) { dd.collect(stMiniz.details()); + dd << "\nError: Miniz failed to zip project"; return Status(Status::ERROR_MINIZ_FAIL, dd, tr("Miniz Error"), - tr("An internal error occurred. Your file may not be saved successfully.")); + tr("An internal error occurred. The project may not have been saved successfully.")); } - dd << "Zip file saved successfully"; + dd << "Miniz: Zip file saved successfully"; Q_ASSERT(stMiniz.ok()); - if (saveOk) + if (saveOk) { + dd << "Project saved successfully, deleting backup"; deleteBackupFile(sBackupFile); + } } progressForward(); @@ -366,7 +370,7 @@ Status FileManager::save(const Object* object, const QString& sFileName) { return Status(Status::FAIL, dd, tr("Internal Error"), - tr("An internal error occurred. Your file may not be saved successfully.")); + tr("An internal error occurred. The project may not have been saved successfully.")); } return Status::OK; From afa857fa93c87f15cef38d83aa1ff4a500a798dd Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 16:53:24 +0100 Subject: [PATCH 13/28] Add text explaining the importance of the bug report --- app/ui/errordialog.ui | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/ui/errordialog.ui b/app/ui/errordialog.ui index 315bf36b77..2c5b855fff 100644 --- a/app/ui/errordialog.ui +++ b/app/ui/errordialog.ui @@ -82,6 +82,16 @@ + + + + The following report contains vital information to figure out the cause of the error. Make sure to copy all of it, if you intend to report the bug. + + + true + + + From 05b03812a6ecfb1c1cf16bb0bb1bda0f211a5021 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 16:53:49 +0100 Subject: [PATCH 14/28] Add missing space --- app/src/mainwindow2.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/mainwindow2.cpp b/app/src/mainwindow2.cpp index 0fbd23d794..aba7dae7d2 100644 --- a/app/src/mainwindow2.cpp +++ b/app/src/mainwindow2.cpp @@ -773,7 +773,7 @@ bool MainWindow2::saveObject(QString strSavedFileName) ErrorDialog errorDialog(st.title(), st.description().append(tr("

An error has occurred and your file may not have saved successfully." - "If you believe that this error is an issue with Pencil2D, please create a new issue at:" + "\nIf you believe that this error is an issue with Pencil2D, please create a new issue at:" "
https://github.com/pencil2d/pencil/issues
" "Please be sure to include the following details in your issue:")), st.details().html()); errorDialog.exec(); From dcd70c599e82907c1152da219f81ec69f95cfa44 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 17:04:19 +0100 Subject: [PATCH 15/28] QMiniz: make sure to always close reader --- core_lib/src/qminiz.cpp | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index 74914ffcc2..82c7657cfa 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -83,6 +83,9 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q mz_zip_zero_struct(mz); mz_bool ok = mz_zip_writer_init_file(mz, zipFilePath.toUtf8().data(), 0); + ScopeGuard mzScopeGuard2([&] { + mz_zip_reader_end(mz); + }); if (!ok) { @@ -132,15 +135,6 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q return Status(Status::FAIL, dd); } - ok &= mz_zip_writer_end(mz); - - if (!ok) - { - mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Miniz writer end failed: error %1, %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); - return Status(Status::FAIL, dd); - } - return Status::OK; } @@ -173,6 +167,9 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) mz_zip_zero_struct(mz); mz_bool ok = mz_zip_reader_init_file(mz, zipFilePath.toUtf8().data(), 0); + ScopeGuard mzScopeGuard2([&] { + mz_zip_reader_end(mz); + }); if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); @@ -223,12 +220,8 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) } } - ok &= mz_zip_reader_end(mz); - if (!ok) { - mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Error: Failed to end zip reader, Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err));; return Status(Status::FAIL, dd); } return Status::OK; From 09deac93bda6899ea8b3d90a5b5add7e147a0be3 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 17:16:19 +0100 Subject: [PATCH 16/28] Layer: Add error diagnostics when failing to save one or more layers --- core_lib/src/structure/layer.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/core_lib/src/structure/layer.cpp b/core_lib/src/structure/layer.cpp index 2ac84e690a..9ae1a831fd 100644 --- a/core_lib/src/structure/layer.cpp +++ b/core_lib/src/structure/layer.cpp @@ -364,6 +364,7 @@ Status Layer::save(const QString& sDataFolder, QStringList& attachedFiles, Progr } if (!ok) { + dd << "\nError: Failed to save one or more files"; return Status(Status::FAIL, dd); } return Status::OK; From 66034b345a53cf4827c2179201adf9ac6b51f97e Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sat, 23 Nov 2024 17:29:59 +0100 Subject: [PATCH 17/28] FileManager:writeMainXML: Slightly safer way to close file --- core_lib/src/structure/filemanager.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 2c8627392c..c3b5040ce7 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -694,6 +694,9 @@ Status FileManager::writeMainXml(const Object* object, const QString& mainXmlPat dd << QString("Error: Failed to open Main XML at: %1, \n Reason: %2").arg(mainXmlPath).arg(file.errorString()); return Status(Status::ERROR_FILE_CANNOT_OPEN, dd); } + ScopeGuard fileScopeGuard([&] { + file.close(); + }); QDomDocument xmlDoc("PencilDocument"); QDomElement root = xmlDoc.createElement("document"); @@ -723,7 +726,6 @@ Status FileManager::writeMainXml(const Object* object, const QString& mainXmlPat QTextStream out(&file); xmlDoc.save(out, indentSize); out.flush(); - file.close(); dd << "Done writing main xml file: " << mainXmlPath; From 566ae76592b6a3f166390ce25662d20e2c4646da Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 24 Nov 2024 11:15:43 +0100 Subject: [PATCH 18/28] QMiniz: Make errors more explicit and fail quickly. --- core_lib/src/qminiz.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index 82c7657cfa..3286213980 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -68,10 +68,12 @@ size_t MiniZ::istreamReadCallback(void *pOpaque, mz_uint64 file_ofs, void * pBuf Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const QStringList& fileList, QString mimetype) { DebugDetails dd; + dd << "\n[Miniz COMPRESSION diagnostics]\n"; dd << QString("Creating Zip %1 from folder %2").arg(zipFilePath, srcFolderPath); if (!srcFolderPath.endsWith("/")) { + dd << "Adding / to path"; srcFolderPath.append("/"); } @@ -90,7 +92,8 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Miniz writer init failed. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("Error: Failed to init writer. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + return Status(Status::FAIL, dd); } // Add special uncompressed mimetype file to help with the identification of projects @@ -103,7 +106,8 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Cannot add mimetype. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("ERROR: Unable to add mimetype. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + return Status(Status::FAIL, dd); } } @@ -123,7 +127,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Cannot add %3. Error code: %1, reason: %2 - Aborting!").arg(static_cast(err)).arg(mz_zip_get_error_string(err), sRelativePath); + dd << QString("Error: Unable to add file: %3. Error code: %1, reason: %2 - Aborting!").arg(static_cast(err)).arg(mz_zip_get_error_string(err), sRelativePath); return Status(Status::FAIL, dd); } } @@ -131,7 +135,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Miniz finalize archive failed. Error code %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("Error: Failed to finalize archive. Error code %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); return Status(Status::FAIL, dd); } @@ -141,6 +145,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) { DebugDetails dd; + dd << "\n[Miniz EXTRACTION diagnostics]\n"; dd << QString("Unzip file %1 to folder %2").arg(zipFilePath, destPath); if (!QFile::exists(zipFilePath)) @@ -173,7 +178,7 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("Error: Failed to init the zip reader. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("Error: Failed to init reader. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); return Status(Status::FAIL, dd); } @@ -215,7 +220,7 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) { ok = false; mz_zip_error err = mz_zip_get_last_error(mz); - dd << QString("File extraction failed. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); + dd << QString("WARNING: Unable to extract file. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); } } } From 33eed046c4890885f2208e2a79fb269e352b44e2 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 24 Nov 2024 11:16:00 +0100 Subject: [PATCH 19/28] Add error keyword to writePalette --- core_lib/src/structure/filemanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index c3b5040ce7..7e55e01b6d 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -739,7 +739,7 @@ Status FileManager::writePalette(const Object* object, const QString& dataFolder if (paletteFile.isEmpty()) { DebugDetails dd; - dd << "Failed to save palette"; + dd << "\nError: Failed to save palette"; return Status(Status::FAIL, dd); } filesWritten.append(paletteFile); From b511d5dbc44e8d01f1cc76add4cbb20379dc1631 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 24 Nov 2024 11:26:45 +0100 Subject: [PATCH 20/28] FileManager: close file after recovery. --- core_lib/src/structure/filemanager.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 7e55e01b6d..7fa830a67d 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -888,6 +888,7 @@ Status FileManager::recoverObject(Object* object) xmlDoc.setContent(&file); root = xmlDoc.documentElement(); objectTag = root.firstChildElement("object"); + file.close(); } loadPalette(object); From 93e82131ee9b099628ec686a45f1e4f2e322d415 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 24 Nov 2024 11:35:52 +0100 Subject: [PATCH 21/28] Remove indentation --- core_lib/src/qminiz.cpp | 2 +- core_lib/src/structure/filemanager.cpp | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index 3286213980..f9b17b8c07 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -40,7 +40,7 @@ Status MiniZ::sanityCheck(const QString& sZipFilePath) if (!readOk || !closeOk) { DebugDetails dd; - dd << "\n [Miniz sanity check]\n"; + dd << "\n[Miniz sanity check]\n"; if (read_err != MZ_ZIP_NO_ERROR) { dd << QString("Found an error while reading the file. Error code: %2, reason: %3").arg(static_cast(read_err)).arg(mz_zip_get_error_string(read_err)); } diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 7fa830a67d..7cfc990a40 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -73,7 +73,7 @@ Object* FileManager::load(const QString& sFileName) // Let's check if we can read the file before we try to unzip. if (!sanityCheck.ok()) { dd.collect(sanityCheck.details()); - dd << "\n Error: Unable to extract project, miniz sanity check failed."; + dd << "\nError: Unable to extract project, miniz sanity check failed."; handleOpenProjectError(Status::ERROR_INVALID_XML_FILE, dd); return nullptr; } else { @@ -666,7 +666,7 @@ Status FileManager::writeKeyFrameFiles(const Object* object, const QString& data { saveLayersOK = false; dd.collect(st.details()); - dd << QString("\n Error: Failed to save Layer[%1] %2").arg(i).arg(layer->name()); + dd << QString("\nError: Failed to save Layer[%1] %2").arg(i).arg(layer->name()); } } @@ -675,9 +675,9 @@ Status FileManager::writeKeyFrameFiles(const Object* object, const QString& data auto errorCode = (saveLayersOK) ? Status::OK : Status::FAIL; if (saveLayersOK) { - dd << "\n All Layers saved"; + dd << "\nAll Layers saved"; } else { - dd << "\n Error: Unable to save all layers"; + dd << "\nError: Unable to save all layers"; } return Status(errorCode, dd); @@ -691,7 +691,7 @@ Status FileManager::writeMainXml(const Object* object, const QString& mainXmlPat QFile file(mainXmlPath); if (!file.open(QFile::WriteOnly | QFile::Text)) { - dd << QString("Error: Failed to open Main XML at: %1, \n Reason: %2").arg(mainXmlPath).arg(file.errorString()); + dd << QString("Error: Failed to open Main XML at: %1, \nReason: %2").arg(mainXmlPath).arg(file.errorString()); return Status(Status::ERROR_FILE_CANNOT_OPEN, dd); } ScopeGuard fileScopeGuard([&] { From 84806971c68be11e0856ced7bd281aa53e1a3e20 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 24 Nov 2024 12:03:21 +0100 Subject: [PATCH 22/28] FileManager: Check for Unzipping errors --- core_lib/src/structure/filemanager.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 7cfc990a40..81c878d57d 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -66,6 +66,7 @@ Object* FileManager::load(const QString& sFileName) { QString workingDirPath = obj->workingDir(); + dd << QString("Working dir: %1").arg(workingDirPath); dd << fileFormat.arg(".pclx"); Status sanityCheck = MiniZ::sanityCheck(sFileName); @@ -79,9 +80,15 @@ Object* FileManager::load(const QString& sFileName) } else { Status unzipStatus = unzip(sFileName, workingDirPath); dd.collect(unzipStatus.details()); - } - dd << QString("Project extracted to: %1 ").arg(workingDirPath); + if(unzipStatus.ok()) { + dd << QString("Unzipped at: %1 ").arg(workingDirPath); + } else { + dd << QString("Error: Unzipping failed: %1 ").arg(workingDirPath); + handleOpenProjectError(Status::ERROR_INVALID_XML_FILE, dd); + return nullptr; + } + } strMainXMLFile = QDir(workingDirPath).filePath(PFF_XML_FILE_NAME); strDataFolder = QDir(workingDirPath).filePath(PFF_DATA_DIR); @@ -285,8 +292,8 @@ Status FileManager::save(const Object* object, const QString& sFileName) { sTempWorkingFolder = object->workingDir(); + dd << QString("Working dir: %1").arg(sTempWorkingFolder); dd << fileFormat.arg(".pclx"); - dd << QString("Project TEMP location: %1").arg(sTempWorkingFolder); Q_ASSERT(QDir(sTempWorkingFolder).exists()); From 3447d678d47472921aae090cea3b67ed171554eb Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 24 Nov 2024 13:45:58 +0100 Subject: [PATCH 23/28] QMiniz: Fix typo... I'm blown away by the fact that this worked at all... wtf. The only one who reported a problem here was windows but technically the file should never have been closed on any of them. Frightening.. --- core_lib/src/qminiz.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index f9b17b8c07..bc272027b8 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -86,7 +86,7 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q mz_bool ok = mz_zip_writer_init_file(mz, zipFilePath.toUtf8().data(), 0); ScopeGuard mzScopeGuard2([&] { - mz_zip_reader_end(mz); + mz_zip_writer_end(mz); }); if (!ok) From d4bdc2369adae2624f289e51c5f8ba29a9b04fea Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 24 Nov 2024 14:11:26 +0100 Subject: [PATCH 24/28] Add missing QFile::close where a file has been opened. And move some closer to the open call to make sure it happens when the scope ends. --- core_lib/src/graphics/vector/vectorimage.cpp | 7 +++++++ core_lib/src/soundplayer.cpp | 5 +++++ core_lib/src/structure/filemanager.cpp | 8 +++++++- core_lib/src/structure/object.cpp | 8 ++++++-- 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/core_lib/src/graphics/vector/vectorimage.cpp b/core_lib/src/graphics/vector/vectorimage.cpp index 54a0b90fba..9bada65c5c 100644 --- a/core_lib/src/graphics/vector/vectorimage.cpp +++ b/core_lib/src/graphics/vector/vectorimage.cpp @@ -23,6 +23,7 @@ GNU General Public License for more details. #include #include #include "object.h" +#include "util.h" VectorImage::VectorImage() @@ -82,6 +83,9 @@ bool VectorImage::read(QString filePath) { return false; } + ScopeGuard fileScope([&] { + file.close(); + }); QDomDocument doc; if (!doc.setContent(&file)) return false; // this is not a XML file @@ -123,6 +127,9 @@ Status VectorImage::write(QString filePath, QString format) debugInfo << ("file.error() = " + file.errorString()); return Status(Status::FAIL, debugInfo); } + ScopeGuard fileScope([&] { + file.close(); + }); if (format != "VEC") { diff --git a/core_lib/src/soundplayer.cpp b/core_lib/src/soundplayer.cpp index 6120b67b03..208e51a8a0 100644 --- a/core_lib/src/soundplayer.cpp +++ b/core_lib/src/soundplayer.cpp @@ -19,6 +19,7 @@ GNU General Public License for more details. #include #include #include "soundclip.h" +#include "util.h" SoundPlayer::SoundPlayer() { @@ -41,6 +42,10 @@ void SoundPlayer::init(SoundClip* clip) QFile file(clip->fileName()); file.open(QIODevice::ReadOnly); + ScopeGuard fileScope([&] { + file.close(); + }); + mBuffer.setData(file.readAll()); mBuffer.open(QBuffer::ReadOnly); diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 81c878d57d..6ca13de3e1 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -114,6 +114,10 @@ Object* FileManager::load(const QString& sFileName) handleOpenProjectError(Status::ERROR_FILE_CANNOT_OPEN, dd); return nullptr; } + ScopeGuard fileScope([&] { + file.close(); + }); + dd << "Main XML exists: Yes"; QDomDocument xmlDoc; @@ -933,6 +937,9 @@ Status FileManager::rebuildMainXML(Object* object) { return Status::ERROR_FILE_CANNOT_OPEN; } + ScopeGuard fileScope([&] { + file.close(); + }); QDomDocument xmlDoc("PencilDocument"); QDomElement root = xmlDoc.createElement("document"); @@ -957,7 +964,6 @@ Status FileManager::rebuildMainXML(Object* object) QTextStream fout(&file); xmlDoc.save(fout, 2); fout.flush(); - file.close(); return Status::OK; } diff --git a/core_lib/src/structure/object.cpp b/core_lib/src/structure/object.cpp index bb25cdb1e0..053f78dac5 100644 --- a/core_lib/src/structure/object.cpp +++ b/core_lib/src/structure/object.cpp @@ -519,13 +519,15 @@ bool Object::exportPalette(const QString& filePath) const qDebug("Error: cannot export palette"); return false; } + ScopeGuard fileScope([&] { + file.close(); + }); if (file.fileName().endsWith(".gpl", Qt::CaseInsensitive)) exportPaletteGPL(file); else exportPalettePencil(file); - file.close(); return true; } @@ -665,6 +667,9 @@ bool Object::importPalette(const QString& filePath) { return false; } + ScopeGuard fileScope([&] { + file.close(); + }); if (file.fileName().endsWith(".gpl", Qt::CaseInsensitive)) { @@ -672,7 +677,6 @@ bool Object::importPalette(const QString& filePath) } else { importPalettePencil(file); } - file.close(); return true; } From 87007c57027e5c868ed0c3098d5422b6446f3164 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Thu, 28 Nov 2024 07:34:19 +0100 Subject: [PATCH 25/28] ErrorDialog: shorten description --- app/ui/errordialog.ui | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/ui/errordialog.ui b/app/ui/errordialog.ui index 2c5b855fff..8078968a47 100644 --- a/app/ui/errordialog.ui +++ b/app/ui/errordialog.ui @@ -85,7 +85,7 @@ - The following report contains vital information to figure out the cause of the error. Make sure to copy all of it, if you intend to report the bug. + This report contains vital information. Copy all of it when submitting a bug. true From 85b43e6d24f01621e3cc56958e43e0cc5b58f628 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Thu, 28 Nov 2024 07:59:06 +0100 Subject: [PATCH 26/28] QMiniz: Only close if we succeded to open it . --- core_lib/src/qminiz.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/core_lib/src/qminiz.cpp b/core_lib/src/qminiz.cpp index bc272027b8..51e4252005 100644 --- a/core_lib/src/qminiz.cpp +++ b/core_lib/src/qminiz.cpp @@ -85,9 +85,6 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q mz_zip_zero_struct(mz); mz_bool ok = mz_zip_writer_init_file(mz, zipFilePath.toUtf8().data(), 0); - ScopeGuard mzScopeGuard2([&] { - mz_zip_writer_end(mz); - }); if (!ok) { @@ -95,6 +92,9 @@ Status MiniZ::compressFolder(QString zipFilePath, QString srcFolderPath, const Q dd << QString("Error: Failed to init writer. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); return Status(Status::FAIL, dd); } + ScopeGuard mzScopeGuard2([&] { + mz_zip_writer_end(mz); + }); // Add special uncompressed mimetype file to help with the identification of projects { @@ -172,15 +172,14 @@ Status MiniZ::uncompressFolder(QString zipFilePath, QString destPath) mz_zip_zero_struct(mz); mz_bool ok = mz_zip_reader_init_file(mz, zipFilePath.toUtf8().data(), 0); - ScopeGuard mzScopeGuard2([&] { - mz_zip_reader_end(mz); - }); - if (!ok) { mz_zip_error err = mz_zip_get_last_error(mz); dd << QString("Error: Failed to init reader. Error code: %1, reason: %2").arg(static_cast(err)).arg(mz_zip_get_error_string(err)); return Status(Status::FAIL, dd); } + ScopeGuard mzScopeGuard2([&] { + mz_zip_reader_end(mz); + }); int num = mz_zip_reader_get_num_files(mz); From 685f96f7faa28f93dceaef8c40f44b1709fdfe29 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Fri, 29 Nov 2024 07:45:44 +0100 Subject: [PATCH 27/28] Remove explicit file.close because QFile already handles this --- core_lib/src/graphics/vector/vectorimage.cpp | 6 ------ core_lib/src/soundplayer.cpp | 4 ---- core_lib/src/structure/filemanager.cpp | 7 ------- core_lib/src/structure/object.cpp | 6 ------ 4 files changed, 23 deletions(-) diff --git a/core_lib/src/graphics/vector/vectorimage.cpp b/core_lib/src/graphics/vector/vectorimage.cpp index 9bada65c5c..3d5dd0598d 100644 --- a/core_lib/src/graphics/vector/vectorimage.cpp +++ b/core_lib/src/graphics/vector/vectorimage.cpp @@ -83,9 +83,6 @@ bool VectorImage::read(QString filePath) { return false; } - ScopeGuard fileScope([&] { - file.close(); - }); QDomDocument doc; if (!doc.setContent(&file)) return false; // this is not a XML file @@ -127,9 +124,6 @@ Status VectorImage::write(QString filePath, QString format) debugInfo << ("file.error() = " + file.errorString()); return Status(Status::FAIL, debugInfo); } - ScopeGuard fileScope([&] { - file.close(); - }); if (format != "VEC") { diff --git a/core_lib/src/soundplayer.cpp b/core_lib/src/soundplayer.cpp index 208e51a8a0..28495c7454 100644 --- a/core_lib/src/soundplayer.cpp +++ b/core_lib/src/soundplayer.cpp @@ -42,10 +42,6 @@ void SoundPlayer::init(SoundClip* clip) QFile file(clip->fileName()); file.open(QIODevice::ReadOnly); - ScopeGuard fileScope([&] { - file.close(); - }); - mBuffer.setData(file.readAll()); mBuffer.open(QBuffer::ReadOnly); diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 6ca13de3e1..3f2535a715 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -114,9 +114,6 @@ Object* FileManager::load(const QString& sFileName) handleOpenProjectError(Status::ERROR_FILE_CANNOT_OPEN, dd); return nullptr; } - ScopeGuard fileScope([&] { - file.close(); - }); dd << "Main XML exists: Yes"; @@ -899,7 +896,6 @@ Status FileManager::recoverObject(Object* object) xmlDoc.setContent(&file); root = xmlDoc.documentElement(); objectTag = root.firstChildElement("object"); - file.close(); } loadPalette(object); @@ -937,9 +933,6 @@ Status FileManager::rebuildMainXML(Object* object) { return Status::ERROR_FILE_CANNOT_OPEN; } - ScopeGuard fileScope([&] { - file.close(); - }); QDomDocument xmlDoc("PencilDocument"); QDomElement root = xmlDoc.createElement("document"); diff --git a/core_lib/src/structure/object.cpp b/core_lib/src/structure/object.cpp index 053f78dac5..0828cd9c70 100644 --- a/core_lib/src/structure/object.cpp +++ b/core_lib/src/structure/object.cpp @@ -519,9 +519,6 @@ bool Object::exportPalette(const QString& filePath) const qDebug("Error: cannot export palette"); return false; } - ScopeGuard fileScope([&] { - file.close(); - }); if (file.fileName().endsWith(".gpl", Qt::CaseInsensitive)) exportPaletteGPL(file); @@ -667,9 +664,6 @@ bool Object::importPalette(const QString& filePath) { return false; } - ScopeGuard fileScope([&] { - file.close(); - }); if (file.fileName().endsWith(".gpl", Qt::CaseInsensitive)) { From 055dd7fd989190d4a47e05868084cac8c1190e93 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Fri, 29 Nov 2024 19:52:29 +0100 Subject: [PATCH 28/28] Fix relative include path --- core_lib/src/structure/filemanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 3f2535a715..898ebb824b 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/core_lib/src/structure/filemanager.cpp @@ -24,7 +24,7 @@ GNU General Public License for more details. #include "fileformat.h" #include "object.h" #include "layercamera.h" -#include "util/util.h" +#include "util.h" FileManager::FileManager(QObject* parent) : QObject(parent) {