pencil2d / pencil

Pencil2D is an easy, intuitive tool to make 2D hand-drawn animations. Pencil2D is open source and cross-platform.
http://pencil2d.org
GNU General Public License v2.0
1.45k stars 272 forks source link

Better error reporting for image import failure #1674

Closed scribblemaniac closed 1 year ago

scribblemaniac commented 2 years ago

Issue Summary

If an image import fails, the same dialog is shown for a variety of unrelated errors.

Actual Results

Currently if there is any issue with image importing, this dialog is displayed: Image Import Error Dialog

Expected Results

When image import fails, an error dialog is displayed similar to the one used for movie export errors detailing what exactly the error was, and providing debug details too if relevant.

Steps to reproduce

Layer type error:

  1. Select a vector layer
  2. Attempt to import an image

Image reading error:

  1. Download this corrupted image: pencil2d-corrupted
  2. Select a bitmap layer
  3. Attempt to import the downloaded image

File not found error:

  1. Select bitmap layer
  2. Attempt to import image but...
  3. When the import position dialog comes up, rename the file you selected for importing.
  4. Then confirm the position dialog.

Note: There are other errors as well. See https://doc.qt.io/qt-5/qimagereader.html#ImageReaderError-enum

System Information


Developer Notes

To implement this, Editor::importImage, Editor::importBitmapImage, and Editor::importVectorImage should return Status objects with error information, and MainWindow::importImage should read those objects and pass the Status data to an ErrorDialog if there's an issue.

victoriadealba commented 2 years ago

Hello I'm an undergraduate student looking to work on an open source project for one of my classes. I would like to know where in the files can I find this issue. I may be interested on taking this issue on

Jose-Moreno commented 2 years ago

@victoriadealba Hi Victoria. Thank you for your interest! I've informed the devs about your query. As far as I know the specific methods mentioned in the original report should be located as follows:

Method void MainWindow2::importImage() Location app/src/mainwindow2.cpp + app/src/mainwindow2.h

Method bool Editor::importImage(const QString& filePath) bool Editor::importBitmapImage(const QString& filePath, int space) bool Editor::importVectorImage(const QString& filePath) Location core_lib/src/interface/editor.cpp + core_lib/src/interface/editor.h


Additionally below I'll link the wiki that guides to most developer documents where you can learn how to compile the source code specifically for this project among other things.

https://github.com/pencil2d/pencil/wiki

This page sort of mirrors the wiki on the landing page, but more importantly it has the class references generated by doxygen (c++ source documentation tool) https://dev.pencil2d.org/

If you have questions please ask them here or visit us on discord in the #general-development channel (you need a role permission to let us know it's you so we can assign you the proper role)

victoriadealba commented 2 years ago

Thank you, Did you guys want the string of the error to be updated? I saw how the functions return booleans so was wondering if at under the MainWindow method to change the statement based off whether it's a bit, vector or regular image... If you wanted something else could you clarify the object oriented method and how to approach it with these boolean methods? I saw the original error mentioned something about having the statement similar to movie export errors.... Do you have a picture of what that error would look like?

scribblemaniac commented 2 years ago

This is mostly covered in the initial post:

To implement this, Editor::importImage, Editor::importBitmapImage, and Editor::importVectorImage should return Status objects with error information, and MainWindow::importImage should read those objects and pass the Status data to an ErrorDialog if there's an issue.

So you need to change the return types from bool to Status (defined in core_lib/src/util/pencilerror.h), and then pass that Status object to an ErrorDialog. The Status object can store the title, description, and debugging information if there is an error. See ActionCommands::exportMovie (in app/src/actioncommands.cpp) for an example of how the movie exporter calls functions that return Status, how it checks that status, and how it constructs and opens an error dialog if there is an error using information from status. See MovieExporter::executeFFmpeg (in core_lib/src/movie_exporter.cpp) for an example of how a function creates and adds information to a Status object.

The error dialog should produce messages that are unique for each of the types of errors described in the Steps to Reproduce section, and where relevant, should provide clear instructions to the user about what went wrong and how to correct the issue. The DebugDetails should at the very least contain the path to the image to import, and the format from QImageReader for bitmap image import, any other information specific to the error that might also be useful should be included (ex file permission flags when there is a file permission error).

You can trigger a movie export error by changing the camera width to an odd number (double click on the camera layer), and then exporting as a mp4 movie.

anpanring commented 1 year ago

Hello! I'm a college student interested in contributing to Pencil2D and I noticed this issue hasn't been active in a while. Would it be okay if I tried working on it?

J5lx commented 1 year ago

Sure! As you said, there hasn’t been any activity here in a while, so Victoria has probably moved on to something else. Scribble has already added some technical details above, but if anything is still unclear, feel free to ask!