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

Set minimumWidth of WLabel according to the content's maximum possible width #11194

Merged
merged 11 commits into from
Feb 17, 2023

Conversation

daschuer
Copy link
Member

This fixes the wiggling off the knob position during adjusting the value.

A regression caused by the switch between value and label.

This fixes the wiggling off the horicontal knob position during adjusting the value.
@github-actions github-actions bot added the ui label Jan 10, 2023
@ronso0 ronso0 self-requested a review January 10, 2023 00:50
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.

LGTM, thanks for the fixup!

Just a question, and please remove the debug output (--amend)

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

ronso0 commented Jan 10, 2023

This screws up the label alignment in Tango and LateNight, though by looking at the Shade knobs xml I discovered how to simplify the Tango/LateNight knob templates.
fix: d639dad

@github-actions github-actions bot added the skins label Jan 15, 2023
@daschuer
Copy link
Member Author

Re-implementing the sizeHint() Function did finally the trick. Please test again.

@daschuer daschuer changed the base branch from main to 2.4 January 16, 2023 00:07
@ronso0 ronso0 linked an issue Jan 16, 2023 that may be closed by this pull request
@ronso0 ronso0 changed the title Set minimumWidth of th lable according to the maximum possible lable Set minimumWidth of WLabel according to the content's maximum possible width Jan 17, 2023
@ronso0
Copy link
Member

ronso0 commented Feb 2, 2023

Unfortunately it's not yet working as desired, and I couldn't figure why, yet.
I tried Graphic Eq in Shade and the label shrinks when it switches to show the value.

@ronso0
Copy link
Member

ronso0 commented Feb 2, 2023

I guess the minimumSizeHint doesn't consider the padding.
If I remove the padding the label is static

           padding-left: 3px;
           padding-right: 3px;

https://github.com/mixxxdj/mixxx/pull/11194/files#diff-e1924755bc957e48039a69bcc6c083ed94ae7d4570ef7f317595d370fd3ba5fbR26-R27

However, this is what happens:

  • start Mixxx with Shade, fx section is hidden, Graphic EQ is unit1, slot2
  • show fx
    Screenshot_2023-02-02_23-30-18
  • start to drag a parameter label, or reload the effect:
    the entire knob container shrinks in width:
    Screenshot_2023-02-02_23-30-27
    SizeHint for a specific label has changed.

Comment on lines 99 to 100
m_widthHint = metrics.size(0, m_longText).width() + frameWidth();
QString elidedText = metrics.elidedText(m_longText, m_elideMode, width() - frameWidth());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
m_widthHint = metrics.size(0, m_longText).width() + frameWidth();
QString elidedText = metrics.elidedText(m_longText, m_elideMode, width() - frameWidth());
m_widthHint = metrics.size(0, m_longText).width() + 2 * frameWidth();
QString elidedText = metrics.elidedText(m_longText, m_elideMode, width() - 2 * frameWidth());

Copy link
Member

@ronso0 ronso0 Feb 3, 2023

Choose a reason for hiding this comment

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

I think that was it.
At least now the size doesn't change anymore...

edit maybe add a comment that frameWidth() is the sum of margin, padding, border from stylesheets?

Though I think we should try to figure why the label shrinks when parameters are swapped.

Comment on lines 85 to 88
optimumWidth = math_max(
optimumWidth,
metrics.size(0, m_text).width());
m_widthHint = optimumWidth;
Copy link
Member

Choose a reason for hiding this comment

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

let's rename optimumWidth to valueWidth and

Suggested change
optimumWidth = math_max(
optimumWidth,
metrics.size(0, m_text).width());
m_widthHint = optimumWidth;
int optimumWidth = math_max(
valueWidth,
metrics.size(0, m_text).width());
m_widthHint = optimumWidth + 2 * frameWidth();

and remove the debug ouput below

Copy link
Member

Choose a reason for hiding this comment

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

with this the labels are okay, besides the fact that they are initially (before swapping parameters) expanded compared to 2.4

@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2023

Thank you for the good testing. For me it is working now. Also after parameter swapping.

@ronso0
Copy link
Member

ronso0 commented Feb 6, 2023

Yes, it works now.

It's just that something 'broke' the initial view in Shade, compared to 2.4:
initial view:
Screenshot_2023-02-06_01-18-38
swapped parameters:
Screenshot_2023-02-06_01-18-57

I think this actually looks better, but users may be confused / disappointed by the condensed view after the swap.
Though tbh I don't care too much about it. Since the other skins still work great this is ready.
If you're motivated to debug this we can wait, otherwise I'll merge it. Just let me know how you prefer it.

@ronso0
Copy link
Member

ronso0 commented Feb 10, 2023

@daschuer Ignore this "good"/harmless regression, or investiagte once more?

@daschuer
Copy link
Member Author

Let's just merge this. It is IMHO good enough.

@ronso0 ronso0 merged commit ea526e2 into mixxxdj:2.4 Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elide skin tag doesn't work if text is next to an expanding widget
2 participants