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

Made error dialog with "Show Details" so wide, that there are no unnecessary line breaks. #4738

Merged
merged 11 commits into from
May 29, 2022

Conversation

JoergAtGithub
Copy link
Member

Without this PR a ErrorDialog with ShowDetails looked like this (many uneccessary linebreaks and a scrollbar):
grafik

Now it looks like this:
grafik

Because it's not possible to react on the Show Details button (the event is not public) of QMessageBox, it is neccessary to position the dialog considering the expanded size. Therefore Error Dialogs with details, are openend off center:
grafik

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for taking care.
I have found a typo.

I consider the automatic size changing, that makes the buttons move, a usability issue.
Did you consider to just resize the whole message box to avoid this?

src/errordialoghandler.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented May 1, 2022

I appreciate this is finally taken care of, thanks!
Though I'd like to haggle over the size: 1000px is huuuge, this will definitely create issues on R'Pi setups.
Would 800px be sufficient to at least make the most [controller mapping] file paths fit?

@JoergAtGithub
Copy link
Member Author

I could easily limit it to the real screen width.
But for the given font size and the length of the messages, it needs so much pixels.

src/errordialoghandler.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented May 1, 2022

I could easily limit it to the real screen width.

👍

@Swiftb0y
Copy link
Member

Swiftb0y commented May 2, 2022

Wouldn't the better solution be to make the textbox which contains the error not break lines but instead horizontally scrollable?

@ronso0
Copy link
Member

ronso0 commented May 2, 2022

Wouldn't the better solution be to make the textbox which contains the error not break lines but instead horizontally scrollable?

Good idea, though I find horizontal scrolling cumbersome.
What about a combination of

  • expanding the details box in width and height so that expectable file paths fit in (we can estimate the max length of configDir/controllers/Some-Controller-Mapping-file)
  • adjust the error message to break after the file path so that the actual error is on a separate line

?

@JoergAtGithub
Copy link
Member Author

I fixed the typo and limited the dialog size in expanded mode to the screen size, to support also devices with tiny low resolution screend like Raspberry Pi.

The objective of this PR is to make the message easier readable. Horizontal scrollbars would do the opposite, because than the interesting part of the message on th right would be invisible by default and would always require scrolling.

Reformatting or analyzing the message text is out of scope of this PR. Feel free to start a follow up PR.

Than there was the remark, if it wouldn't be nicer to show also the collapsed dialog in full width,to prevent the off center dialog. IMHO this is a matter of taste, I think the current solution is the nicer one, because all collapsed dialogs (with or without details) lookinitially the same - just teh position is different.
But I could easily change this, if the majority opinion is, that the collapsed dialog should have also full width.

Copy link
Member

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

@daschuer
Copy link
Member

daschuer commented May 5, 2022

@ronso0 Do you want to have a final look?

@ronso0
Copy link
Member

ronso0 commented May 6, 2022

For me this crashes after getScreen returns nullptr (dunno why)

src/errordialoghandler.cpp Show resolved Hide resolved
src/errordialoghandler.cpp Show resolved Hide resolved
src/errordialoghandler.cpp Outdated Show resolved Hide resolved
@JoergAtGithub JoergAtGithub requested a review from ronso0 May 15, 2022 16:38
src/errordialoghandler.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented May 28, 2022

Works good, also with Qt5.12. Thank you!

@ronso0 ronso0 added this to the 2.3.3 milestone May 28, 2022
@ronso0
Copy link
Member

ronso0 commented May 29, 2022

LGTM, thank you for taking care of this!

I'll continue with the line-breaks PR.

@ronso0 ronso0 merged commit ef3739d into mixxxdj:main May 29, 2022
@ronso0 ronso0 modified the milestones: 2.3.3, 2.4.0 May 29, 2022
@ronso0
Copy link
Member

ronso0 commented May 29, 2022

somehow I assumed this would be a UX improvement for 2.3.3 but I didn't check.

Does anyone else consider this worth backporting? Is it possible without too much hazzle?
(I would do the work if there's an agreement)

Edit
won't hapen soonish, if at all.
There's more important (and more fun) stuff on the table..

@JoergAtGithub
Copy link
Member Author

From my side backporting is Ok!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants