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

Musicbrainz: Elide long error messages in the progress bar #13673

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

daschuer
Copy link
Member

And put the full text into a tooltip, like we do in the library table.
This fixes #13664

@daschuer daschuer added this to the 2.4.2 milestone Sep 20, 2024
@daschuer daschuer changed the title Misicbrainz: Elide long error messages in the progress bar Musicbrainz: Elide long error messages in the progress bar Sep 20, 2024
Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 540 to 542
QString cantConnect = tr("Can't connect to %1: %2").arg(app, message);
QFontMetrics metrics(loadingProgressBar->font());
QString elidedCantConnect = metrics.elidedText(
Copy link
Member

Choose a reason for hiding this comment

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

All are const, right?

@ronso0
Copy link
Member

ronso0 commented Sep 20, 2024

I wonder if we need the full error string/url at all.
Wouldn't it suffice to show "Can't connect to MusicBrainz" and log the full string?
Or "Can't connect to MusicBrainz. See log for details."

Besides, I wouldn't expect a tooltip on a progressbar.
(is the tooltip always shown, regardless the tooltip setting in Interface preferences?)

@ronso0 ronso0 linked an issue Sep 20, 2024 that may be closed by this pull request
Qt::ElideRight,
loadingProgressBar->width() - 4);
loadingProgressBar->setFormat(elidedCantConnect);
if (cantConnect.size() != elidedCantConnect.size()) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there the possibility of the edge case that the elided text has the same length as the non-elided text? Could this happen when the text gets elided but then the added ellipses pads it back out to the same length again? That would cause information to be missing (not much sure, but still a character or two) and no tooltip being shown.
Unless we know this is a performance problem, I would just always set the tooltip. makes the code simpler and avoids the edge case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that is likely, but no gurantee, because the elipsis can be either the … (https://www.compart.com/de/unicode/U+2026) or ... (three dots) depending on the font.

@daschuer
Copy link
Member Author

I wonder if we need the full error string/url at all.
Wouldn't it suffice to show "Can't connect to MusicBrainz" and log the full string?

We show the translated error string form the Qt backend that is intendend to be displayed to the user. So I consider it a good idea to show it. The errror message is long if it contains a long path auto generated path, so eliding will probably not cut off sigificant data and we may consider to remove the tooltip.

is the tooltip always shown, regardless the tooltip setting in Interface preferences?

Only if the original message is too long and independent froem the preferendes.

This was just a quick workaround. If you prefer I will remove the tooltip. Please give a hint.

Copy link
Member

@Swiftb0y Swiftb0y 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 Author

Merge? @ronso0 Is that OK for now?

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

Yup, LGTM.
Thank you!

@ronso0 ronso0 merged commit feb320a into mixxxdj:2.4 Sep 23, 2024
14 checks passed
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.

Musicbrainz error is too long for progress bar
4 participants