Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Publisher: Enhance KnownPublishError visualisation #373

Closed
wants to merge 15 commits into from
Closed

Publisher: Enhance KnownPublishError visualisation #373

wants to merge 15 commits into from
+52 −14

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Apr 4, 2024 •
edited
Loading

Changelog Description

Tweaking KnownPublishError to have more user friendly error handling.
Also, slightly changing error report.

Here's some demo from Houdini - mantra ifd product type

Animation_44

Testing notes:

  1. Change a RuntimeError to KnownPublishError and use better message and add a label similar to the provided example.
  2. A good practice to add self.log.error(....) with more detailed error message.
  3. Raise the error intentionally
  4. Telling if the new error reporting is more user friendly.

@MustafaJafar MustafaJafar linked an issue Apr 4, 2024 that may be closed by this pull request
Enhance KnownPublishError visualisation #348
Closed
2 tasks
@MustafaJafar MustafaJafar mentioned this pull request Apr 4, 2024
Closed
2 tasks
@BigRoy
Copy link
Collaborator

BigRoy commented Apr 4, 2024

image

With the "error" wording above the buttons I'm currently unsure what "Save to disk" would save because the text feels less focused on 'reporting it' I wouldn't immediately think that "Save to disk" would be talking about the report. But maybe that's just me.

MustafaJafar and others added 2 commits April 4, 2024 16:04
@MustafaJafar @BigRoy
add missing quotes
ebf9a3d 
Co-authored-by: Roy Nieterau <[email protected]>
@MustafaJafar @BigRoy
fix misspelled word
902d7a4 
Co-authored-by: Roy Nieterau <[email protected]>
@MustafaJafar
Copy link
Contributor Author

With the "error" wording above the buttons I'm currently unsure what "Save to disk" would save because the text feels less focused on 'reporting it' I wouldn't immediately think that "Save to disk" would be talking about the report. But maybe that's just me.

I'm not sure about how to rephrase it in a better way.
My goal was to write something informative. click for more info, report the error, etc.
what do you think could be a suitable informative message ?

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 4, 2024

I'm not sure about how to rephrase it in a better way.
My goal was to write something informative. click for more info, report the error, etc.
what do you think could be a suitable informative message ?

Maybe re-reading it again I have less of an issue with it. Just my brain being dumb sometimes. Removing the words "You can" might also put more focus on "Report the error".

@MustafaJafar MustafaJafar marked this pull request as draft April 5, 2024 14:33
@iLLiCiTiT iLLiCiTiT changed the title Tests: Enhance knownpublisherror to have more user friendly errors Publisher: Enhance KnownPublishError visualisation Apr 5, 2024
@iLLiCiTiT
Copy link
Member

Moving some of internal conversation here.

This is current state of the labels:
image

I don't think user should look at the frame at the bottom to find out what happened, if the information is for user. The bottom panel is short technical information.

First, my "feeling" is that both titles might be Error happened. The frame at the bottom would be unchanged. Instead of label would KnownPublishError had user_message which would be by default. This is not your fault. Please report the error using one of the options below. which would be shown as message at the top, but the developer could change it with the error?

This will cause that the message "Please report the error using one of the options below." could be overriden, and question is if we should put the message elsewhere (e.g. next to buttons). Or change labels of buttons to Copy report to clipboard and Save report to disk?

I think @mkolar came up with current design so he should have opinion on this.

@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Apr 5, 2024 •
edited
Loading

I've just figured out that I forgot to push my example.
Here's what this PR does:

I've added an example in Mantra IFD houdini product type. to run it:

  • Create a mantra IFD product type, switch off farm rendering and publish

Error message in my example is not accurate. it was badly copied/pasted. I was more concerned about showing what does this PR do

Animation_42

image

image

@MustafaJafar MustafaJafar requested review from mkolar and BigRoy April 5, 2024 15:30
BigRoy
BigRoy reviewed Apr 9, 2024
View reviewed changes
client/ayon_core/hosts/houdini/api/lib.py Show resolved Hide resolved
client/ayon_core/hosts/houdini/api/lib.py Outdated Show resolved Hide resolved
client/ayon_core/hosts/houdini/plugins/publish/extract_mantra_ifd.py Outdated Show resolved Hide resolved
client/ayon_core/tools/publisher/widgets/report_page.py
@@ -1459,7 +1459,7 @@ class CrashWidget(QtWidgets.QWidget):
def __init__(self, controller, parent):
super(CrashWidget, self).__init__(parent)

main_label = QtWidgets.QLabel("This is not your fault", self)
main_label = QtWidgets.QLabel("Error Occurred!", self)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if Error Occurred! is too unfriendly/scary? I recall that being a huge design decision as to why once it was chosen to report with This is not your fault. So may be one for @mkolar to comment on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, I don't know what should be the message.
I only changed it to open a discussion about it.

@MustafaJafar MustafaJafar requested a review from BigRoy April 23, 2024 13:30
@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Apr 24, 2024 •
edited
Loading

As I'm more and more thinking about it I guess we'll need different exception so we know if it is something which is caused by user or it is really a bug. It also affects UI which has messages related to actions and their buttons. Question is if the report buttons should be visible in case it is not user fault etc. That is UX desicion, and I not good at it, so we need someone who can tell.

@MustafaJafar
Copy link
Contributor Author

As I'm more and more thinking about it I guess we'll need different exception so we know if it is something which is caused by user or it is really a bug. It also affects UI which has messages related to actions and their buttons. Question is if the report buttons should be visible in case it is not user fault etc. That is UX desicion, and I not good at it, so we need someone who can tell.

Cool!
I think we can close this PR or leave it as it is and continue the discussion in this issue #348

@MustafaJafar
Copy link
Contributor Author

Closing this PR and moving the discussion to #348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antirotor antirotor Awaiting requested review from antirotor

@iLLiCiTiT iLLiCiTiT Awaiting requested review from iLLiCiTiT

@moonyuet moonyuet Awaiting requested review from moonyuet

@mkolar mkolar Awaiting requested review from mkolar

@BigRoy BigRoy Awaiting requested review from BigRoy

Assignees

@iLLiCiTiT iLLiCiTiT

Labels
host: Houdini size/XS
Projects
PR reviewing
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhance KnownPublishError visualisation
4 participants
@MustafaJafar @BigRoy @iLLiCiTiT @ynbot