Add more details to FileManager error report #1895

MrStevns · 2024-11-24T10:53:01Z

The purpose of this PR has been to make the error report cover more steps, introduce "Error" to make it more explicit what's a information, a warning and an error.

I've also tried to make it fail faster rather than spitting out all kinds of irrelevant information when the actual error happened 4 lines above.

Here's an example of what our current report looks like:

Raw file path: /Users/mrstevns/Desktop/p2d testing/test-save-corruption.pclx
Resolved file path: /Users/mrstevns/Desktop/p2d testing/test-save-corruption.pclx
  File name: /Users/mrstevns/Desktop/p2d testing/test-save-corruption.pclx
  Recognized New zipped Pencil2D File Format (*.pclx) !
    Miniz found an error while reading the file. - 7, failed finding central directory
    Miniz found an error while closing file file. - 24, invalid parameter
  XML file: /private/var/folders/5c/2bmlz9fx2kzbd56j81ktpghr0000gn/T/Pencil2D/test-save-corruption_Y2xD_cplu6r9q/main.xml
  Data folder: /private/var/folders/5c/2bmlz9fx2kzbd56j81ktpghr0000gn/T/Pencil2D/test-save-corruption_Y2xD_cplu6r9q/data
  Working folder: /private/var/folders/5c/2bmlz9fx2kzbd56j81ktpghr0000gn/T/Pencil2D/test-save-corruption_Y2xD_cplu6r9q/
  Main XML file does not exist
System Info
Pencil2D version: 0.7.0.911 (stable)
Build ABI: x86_64-little_endian-lp64
Kernel: darwin, 21.6.0
Operating System: macOS 12.7
end

And here's the new one:

Raw file path: /Users/mrstevns/Desktop/p2d testing/test-save-corruption.pclx
Resolved file path: /Users/mrstevns/Desktop/p2d testing/test-save-corruption.pclx
  
[Project LOAD diagnostics]

  File name: /Users/mrstevns/Desktop/p2d testing/test-save-corruption.pclx
  Working dir: /private/var/folders/5c/2bmlz9fx2kzbd56j81ktpghr0000gn/T/Pencil2D/test-save-corruption_Y2xD_719f2v7t/
  Project format: .pclx
    
[Miniz sanity check]

    Found an error while reading the file. Error code: 7, reason: failed finding central directory
    Found an error while closing the file. Error code: 24, reason: invalid parameter
  
Error: Unable to extract project, miniz sanity check failed.

[System Info]

  Pencil2D version: 0.0.0.0 (dev)
  Build ABI: x86_64-little_endian-lp64
  Kernel: darwin, 21.6.0
  Operating System: macOS 12.7
  Language: en_GB

In addition the error dialog now also has a "Copy to clipboard" button, so the user can more easily paste the error report.

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.

And get rid of useless or confusing information.

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.

…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.

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..

And move some closer to the open call to make sure it happens when the scope ends.

app/ui/errordialog.ui

core_lib/src/graphics/vector/vectorimage.cpp

chchwy · 2024-11-27T11:36:57Z

core_lib/src/qminiz.cpp

        srcFolderPath.append("/");
    }

    mz_zip_archive* mz = new mz_zip_archive;
+    ScopeGuard mzScopeGuard([&] {


OnScopeExit(delete mz);

I knew about OnScopeExit when I made this change, I just prefer using ScopeGuard directly rather than a macro

core_lib/src/qminiz.cpp

core_lib/src/soundplayer.cpp

core_lib/src/structure/filemanager.cpp

chchwy · 2024-11-27T11:48:53Z

Thanks for thoroughly revising all the project load/save error logs and adding the missing parts. This is very important. Apart from the explicit QFile::close calls, everything looks great to me!

.

* 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 # Conflicts: # core_lib/src/managers/undoredomanager.cpp # core_lib/src/structure/filemanager.cpp

MrStevns added 24 commits November 22, 2024 19:11

Fix case where saving with no undo changes would fail

741a56d

QMiniz: Simplify logic

0d9bf8a

QMiniz: Make sure to abort when failing to compress file

871ccc9

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

33115d4

And get rid of useless or confusing information.

ErrorDialog: Add copy to clipboard

466f6be

Improve error details for readability - take 2

335d88d

Improve error details for readability - take 3

422cce5

Write a note when a backup has been made

8b705f8

Improve layer saving diagnostics

e106ac5

Improve diagnostics around qminiz and archiving

fb0b24e

Add text explaining the importance of the bug report

afa857f

Add missing space

05b0381

QMiniz: make sure to always close reader

dcd70c5

Layer: Add error diagnostics when failing to save one or more layers

09deac9

FileManager:writeMainXML: Slightly safer way to close file

66034b3

QMiniz: Make errors more explicit and fail quickly.

566ae76

Add error keyword to writePalette

33eed04

FileManager: close file after recovery.

b511d5d

Remove indentation

93e8213

FileManager: Check for Unzipping errors

8480697

QMiniz: Fix typo...

3447d67

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.

d4bdc23

And move some closer to the open call to make sure it happens when the scope ends.

MrStevns force-pushed the improvement/saving-error-handling branch from bb18358 to d4bdc23 Compare November 24, 2024 13:11

MrStevns changed the title ~~Improve FileManager error handling for readability~~ Add more details to FileManager error report Nov 24, 2024