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

add workaround for cut off buffer delay text #2291

Closed

Conversation

djfun
Copy link
Contributor

@djfun djfun commented Jan 26, 2022

Short description of changes

When choosing a non-predefined buffer delay, the text is cut off. Appending
2 space characters seems to fix the problem.

Context: Fixes an issue?

Fixes: #1430

Does this change need documentation? What needs to be documented and how?

no

Status of this Pull Request
Waiting on other approach mentioned here: #1430 (comment)

What is missing until this pull request can be merged?

nothing

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie
Copy link
Member

hoffie commented Jan 27, 2022

Meta: CodeQL CI check failed because the PR is not based on the very latest master, which contains a CodeQL-relevant fix. Should be fixed after rebasing (no conflicts expected).

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

I know @pljones doesn‘t like this fix. Is it still acceptable or should we check if there’s a way to fix it in Qt designer? Increasing space for the label?

@softins
Copy link
Member

softins commented Feb 7, 2022

I know @pljones doesn‘t like this fix. Is it still acceptable or should we check if there’s a way to fix it in Qt designer? Increasing space for the label?

It looks to me like the label space is actually getting auto-sized according to the length of the text already, but that the metrics of the font are being slightly miscalculated (at least on that platform), resulting in the calculated width of the bounding box being slightly too small. This looks like a platform or Qt bug to me, and probably the proposed workaround is the easiest way to solve it.

@hoffie
Copy link
Member

hoffie commented Feb 7, 2022

I was about to suggest merging as-is if there's no other obvious fix (maybe some Qt css margin or padding could help, but it would be a workaround nevertheless). However, I can't reproduce this on Arch/Wayland/Sway, so I'd tend towards checking if this is a general issue at all.

@dzpex Does this affect a fresh Ubuntu profile or could it be related to some non-standard Qt layout or font setting?

@pljones
Copy link
Collaborator

pljones commented Feb 8, 2022

Needs rebasing to current master, I think.

When choosing a non-predefined buffer delay, the text is cut off. Appending
2 space characters seems to fix the problem.

Fixes jamulussoftware#1430

Signed-off-by: Martin Kaistra <[email protected]>
@djfun djfun force-pushed the bugfix/buffer-delay-cut-off branch from 884af89 to 573dae6 Compare February 8, 2022 17:34
@@ -823,7 +823,8 @@ void CClientSettingsDlg::UpdateSoundCardFrame()
else
{
// special title text with buffer size information added
grbSoundCrdBufDelay->setTitle ( tr ( "Buffer Delay: " ) + GenSndCrdBufferDelayString ( iCurActualBufSize ) );
// append 2 whitespace characters, so that the text doesn't get cut off
grbSoundCrdBufDelay->setTitle ( tr ( "Buffer Delay: " ) + GenSndCrdBufferDelayString ( iCurActualBufSize ) + " " );
Copy link
Member

Choose a reason for hiding this comment

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

Although it doesn’t look like the cause is fixed with that, it’s a workaround which doesn’t seem to have negative side effects. I‘d say it’s ready?

Copy link
Member

Choose a reason for hiding this comment

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

Can anyone confirm this original issue at all? And therefore, confirm the fix? If yes, I'd say go for it (maybe add a TODO comment to work out something better one day).

If anyone wants to dive deeper (I can't as I don't see this issue on my system). GammaRay might help.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn’t occur for me under Gnome 3.38.5 X11 or Wayland either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am currently using Ubuntu 20.10 with Gnome 40.4.0 and Wayland. Did you test on Ubuntu? It might depend on the UI font, which is "Ubuntu Regular, 11" for me (you can set that with gnome-tweaks)

Copy link
Member

Choose a reason for hiding this comment

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

See also #1430 (comment)
(I chose to post the original issue again as the reproducability is more issue-related than PR-related, but it's... not entirely clear. ;))

Copy link
Collaborator

Choose a reason for hiding this comment

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

The PR should include "after" screenshots if they're not included in the issue (which they're not here).

@ann0see
Copy link
Member

ann0see commented Mar 5, 2022

I think a decision should be made if we merge this.

Pro:
Doesn't seem to have a big impact
Might solve an issue for a minority (?) of users
Con:
Includes code not needed by many people
Might be fixed by a later version of Qt (maybe even Qt 6.something)?

@ann0see
Copy link
Member

ann0see commented Apr 11, 2022

@djfun @pljones @hoffie How can we proceed here?

See my comment above: #2291 (comment)

@ann0see
Copy link
Member

ann0see commented Apr 30, 2022

Closing this PR in favor of #1430 (comment)

@djfun maybe you could work on the alternative approach?

@ann0see ann0see closed this Apr 30, 2022
@hoffie
Copy link
Member

hoffie commented May 1, 2022

I've got some half-ready code for that, but it hasn't been fully tested which is why I haven't submitted a PR yet.

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

Successfully merging this pull request may close these issues.

Buffer delay text cut off
5 participants