More detailed error reporting for image import failure #1750
Conversation
J5lx
added
Enhancement
UX
|
Thanks for your submission! We will review it when we get a chance, but I just want to warn you that PRs can move quite slowly in this project. We have very few developers and none of us are all that active right now so it can take some time before someone gets around to reviewing them. Just skimming the changes you've made, it looks just like what I had in mind for this enhancement. I don't anticipate there being any major issues with this. |
scribblemaniac
force-pushed
the
image-import-errors
branch
from
be03665
to
f3011d5
Compare
There was a problem hiding this comment.
Looks good. I've added a couple of further changes you can check out if you would like. Feel free to ask questions about anything I've done.
One of the main changes was adding the use of the tr function (https://doc.qt.io/qt-5/qobject.html#tr). We use this function throughout our code to add support for translating strings into other languages. When it comes to error dialogs specifically, we want to translate the title and description as those are meant to be read by the user. We don't translate the debug details as those are meant to be read by the developers and easily searched up in the English code.
The other noteworthy change was adding error dialogs to the image sequence, image predefined set, and animated GIF imports. This wasn't specified in the #1674, but they also should be using the same approach for returning and showing errors. And since they all use the importImage function which now returns a Status object, this was fairly simple to do.
|
Looks good, and thank you for your help with additional changes! I'll keep the tr function and other import methods in mind for the future. |
There was a problem hiding this comment.
Coding and style wise the changes looks good to me, the messages themselves however could be a bit more aligned.
In general I'd like to see the punctuation removed from the title. We don't use it anywhere else AFAIK and it's generally not common to do so.
The title should be short and concise, "couldn't" is just extra fluff that doesn't add anything, state the problem, simply put 😉
I've written a suggestion to one of them, the same title could be used for some of the other errors too.
Thanks for your contribution 😃
Let me know when you've addressed the mentioned changes, then i'll review again.
…o image-import-errors Merge scribblemaniac commit
Fixes #1674
I am new to contributing to Pencil2D and would really appreciate any feedback on these fixes, especially the debugging and error messages. Thank you!