Add more details to FileManager error report (#1895)

* Fix case where saving with no undo changes would fail * QMiniz: Simplify logic * 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. * Improve the error detail log for readability And get rid of useless or confusing information. * ErrorDialog: Add copy to clipboard * Improve error details for readability - take 2 * Improve error details for readability - take 3 * Write a note when a backup has been made * 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. * Improve layer saving diagnostics * 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. * Improve diagnostics around qminiz and archiving * Add text explaining the importance of the bug report * Add missing space * QMiniz: make sure to always close reader * Layer: Add error diagnostics when failing to save one or more layers * FileManager:writeMainXML: Slightly safer way to close file * QMiniz: Make errors more explicit and fail quickly. * Add error keyword to writePalette * FileManager: close file after recovery. * Remove indentation * FileManager: Check for Unzipping errors * 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.. * 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. * ErrorDialog: shorten description * QMiniz: Only close if we succeded to open it . * Remove explicit file.close because QFile already handles this * Fix relative include path
pencil2d · Nov 30, 2024 · fe4f2bc · fe4f2bc
1 parent 62d0c8f
commit fe4f2bc
Show file tree

Hide file tree

Showing 16 changed files with 174 additions and 107 deletions.
diff --git 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 <QPushButton>
+#include <QClipboard>
+
 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( "<pre>%1</pre>" ).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
@@ -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;
 };

diff --git 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("<br><br>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:"
                                                            "<br><a href='https://github.com/pencil2d/pencil/issues'>https://github.com/pencil2d/pencil/issues</a><br>"
                                                            "Please be sure to include the following details in your issue:")), st.details().html());
         errorDialog.exec();

diff --git a/app/ui/errordialog.ui b/app/ui/errordialog.ui
@@ -82,6 +82,16 @@
      </item>
     </layout>
    </item>
+   <item>
+    <widget class="QLabel" name="label">
+     <property name="text">
+      <string>This report contains vital information. Copy all of it when submitting a bug.</string>
+     </property>
+     <property name="wordWrap">
+      <bool>true</bool>
+     </property>
+    </widget>
+   </item>
    <item>
     <widget class="QTextEdit" name="details">
      <property name="readOnly">

diff --git 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/graphics/vector/vectorimage.cpp b/core_lib/src/graphics/vector/vectorimage.cpp
@@ -23,6 +23,7 @@ GNU General Public License for more details.
 #include <QDebug>
 #include <QXmlStreamWriter>
 #include "object.h"
+#include "util.h"
 
 
 VectorImage::VectorImage()

diff --git 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;

diff --git 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<int>(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<int>(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<int>(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<int>(close_err)).arg(mz_zip_get_error_string(close_err));
         }
         return Status(Status::ERROR_MINIZ_FAIL, dd);
     }
@@ -66,28 +68,33 @@ 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("/");
     }
 
     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);
-        dd << QString("Miniz writer init failed: error %1, %2").arg(static_cast<int>(err)).arg(mz_zip_get_error_string(err));
+        dd << QString("Error: Failed to init writer. Error code: %1, reason: %2").arg(static_cast<int>(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
     {
@@ -99,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 %1").arg(static_cast<int>(err)).arg(mz_zip_get_error_string(err));
+            dd << QString("ERROR: Unable to add mimetype. Error code: %1, reason: %2").arg(static_cast<int>(err)).arg(mz_zip_get_error_string(err));
+            return Status(Status::FAIL, dd);
         }
     }
 
@@ -119,26 +127,15 @@ 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<int>(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<int>(err)).arg(mz_zip_get_error_string(err), sRelativePath);
+            return Status(Status::FAIL, dd);
         }
     }
     ok &= mz_zip_writer_finalize_archive(mz);
     if (!ok)
     {
         mz_zip_error err = mz_zip_get_last_error(mz);
-        dd << QString("Miniz finalize archive failed: error %1, %2").arg(static_cast<int>(err)).arg(mz_zip_get_error_string(err));
-        return Status(Status::FAIL, dd);
-    }
-
-    ok &= mz_zip_writer_end(mz);
-
-    mzScopeGuard.dismiss();
-    ScopeGuard mzScopeGuard2([&] { delete 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<int>(err)).arg(mz_zip_get_error_string(err));
+        dd << QString("Error: Failed to finalize archive. Error code %1, reason: %2").arg(static_cast<int>(err)).arg(mz_zip_get_error_string(err));
         return Status(Status::FAIL, dd);
     }
 
@@ -148,10 +145,12 @@ 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))
     {
+        dd << QString("Error: Zip file does not exist.");
         return Status::FILE_NOT_FOUND;
     }
 
@@ -166,17 +165,21 @@ 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 reader. Error code: %1, reason: %2").arg(static_cast<int>(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);
 
@@ -215,21 +218,15 @@ 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("WARNING: Unable to extract file. Error code: %1, reason: %2").arg(static_cast<int>(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!";
+        return Status(Status::FAIL, dd);
     }
     return Status::OK;
 }
diff --git a/core_lib/src/soundplayer.cpp b/core_lib/src/soundplayer.cpp
@@ -19,6 +19,7 @@ GNU General Public License for more details.
 #include <QMediaPlayer>
 #include <QFile>
 #include "soundclip.h"
+#include "util.h"
 
 SoundPlayer::SoundPlayer()
 {