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.

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

[Run] Improve error MessageBox #34564

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

davidegiacometti
Copy link
Collaborator

@davidegiacometti davidegiacometti commented Sep 3, 2024

Summary of the Pull Request

This is not a fix to the error, but an improvement to MessageBox error title since it's not clear to the user that the it's related to PT Run.
This was partially addressed by #25095, but not for all the errors.

image

image

image

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Assessed the MessageBox.Show(...) errors that could be triggered on startup, detecting which need to explicit call out "PT Run"
  • Aligned loading/initialization error messages.
  • Don't think that IPublicAPI.ShowMsg(...) dialogs need "PT Run" in title since they are usually showed as a response to a user interaction (example click on a result or action).

Validation Steps Performed

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 4, 2024

@davidegiacometti
There are two plugin init error msg boxes. Did you check if the title is used in both of them?

@davidegiacometti
Copy link
Collaborator Author

I'll check. Thanks for pointing out this.

@htcfreek

This comment has been minimized.

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 4, 2024

Screenshot_20240904-190429_GitHub.jpg

@davidegiacometti
Copy link
Collaborator Author

I have verified and one plugin error dialog was addressed in #25095.
This PR aim to fix the remaining and another related to the hotkey.

@davidegiacometti davidegiacometti force-pushed the users/davidegiacometti/run-error-msgbox branch from 9ee52f5 to 67be391 Compare September 5, 2024 20:30
@htcfreek
Copy link
Collaborator

htcfreek commented Sep 6, 2024

@davidegiacometti
Thoughts regarding the init error message:

  1. We should update the content of the init error message to be aligned with the title. (Loading vs initializing.)
  2. Can we improve the text to be in line with the loading error text? (First list the affected plugins and in the next paragraph give some advice what to do.) That makes it easier to understand what plugins are affected, I think.
  3. I think we should use the same advice like in the loading error message.

Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

LGTM . ❤️ This is a big quality of life improvement.

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Sep 23, 2024
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

@jaimecbernardo jaimecbernardo merged commit 035d70d into main Sep 23, 2024
14 checks passed
@davidegiacometti davidegiacometti deleted the users/davidegiacometti/run-error-msgbox branch October 3, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants