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 handle size for sliders, and set consistent handle style #7528

Open
wants to merge 1 commit into
base: master
from

Conversation

5 participants
@TryTwo
Contributor

TryTwo commented Oct 28, 2018

Slider handles had no size set, so were super tiny. Also set all sliders to have ticks below them, so the handles can all look the same. This look is nicer than square handles.

Novice at Qt, but I think I did it correctly..

@JosJuice

This comment has been minimized.

Contributor

JosJuice commented Oct 28, 2018

Could you post screenshots for comparison?

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Oct 28, 2018

Comparison

@JosJuice

This comment has been minimized.

Contributor

JosJuice commented Oct 29, 2018

That does look better! Though for some reason, the slider handles showed up fine on my system (Windows 10) even before this PR...

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Oct 29, 2018

That's odd. I even tried portable mode and it still looks like that. I also can't seem to get a custom style to affect it, if that's any clue.

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Oct 29, 2018

Removed global tick interval from GraphicsSlider and corrected the values for each Slider that uses it. Fixed Game Config Slider. Do we want to show the specific value for any more sliders? Like how CPU override does.

@Techjar

This comment has been minimized.

Contributor

Techjar commented Oct 29, 2018

These commits should be squashed and the message changed to match the PR.

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Oct 29, 2018

Added audio sliders.
I realized I'm actually setting the height for the entire slider element, and then Qt must be fixing the handle height from that. I also figured out how to change the handles in the style sheets - you have to set handle.background, handle.height, and handle.margin for it to take the change. This still won't work if the height isn't defined (for me).

Sorry, still learning how to use github stuff. Messages- do you mean the name of the changes?

@Techjar

This comment has been minimized.

Contributor

Techjar commented Oct 29, 2018

Commits don't have a "name", only a message. The message should be a description of the commit's changes just like PR itself. They can be multi-line as well.

@TryTwo TryTwo force-pushed the TryTwo:Slider branch 2 times, most recently from e8f5067 to 57fb654 Oct 29, 2018

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Nov 1, 2018

Fixed audio slider dynamically resizing as the digits in the value next to it changed. Also lined things up better. It seems tickdown style kind of makes the slider look off center when right next to text.

Can I rename the branch I'm PRing? I didn't know the name became a part of the PR itself. Could be more descriptive.

@JosJuice

This comment has been minimized.

Contributor

JosJuice commented Nov 1, 2018

No, the branch name of a PR can't be changed. But it doesn't need to be super descriptive, that's what the commit message is for. I think the current branch name is fine.

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Nov 5, 2018

I think this PR is done unless someone doesn't like my methods of setting a fixed height to widget elements .

@lioncash

This comment has been minimized.

Member

lioncash commented Nov 5, 2018

You should likely be sizing elements based off screen metrics, rather than hardcoding fixed pixel heights/widths (which is explicitly advised against in Qt's documentation on Hi-DPI displays), otherwise the elements can show up too small on varying Hi-DPI monitor types.

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Nov 5, 2018

I found the issue of my QSlider being too small and not styled correctly (top pic) is due to some Windows 10 customization I did. Not sure exactly what customization. It may relate to BhaaLseN's referenced PR above.

It's possible setfixedheight is invalidating a part of the default slider styling in dolphin, or becoming a fallback when the default styling can't figure something out. In the pic I saw it looked like the QSlider Handle could be colored by the windows theme, is that correct? Does my PR have colored handles? I can't test this

/edit It could relate to this: http://doc.qt.io/qt-5/stylesheet-examples.html (search for " happened " and read that section.

I couldn't find if QSize will resize settings for high DPI. Unfortunately I can't test it myself. Maybe I can run something to get the widget height then feed that back into setheight and it'll fix the bug. I don't know much about sizing with screen metrics, if that's the only answer.

Fix Qt Sliders: Set Minimum height for Qt Slider elements, set consis…
…tent tick mark handle style. Fixed audio slider having a dynamic length. Fixes slider handle sometimes defaulting to 0 height.

@TryTwo TryTwo force-pushed the TryTwo:Slider branch from 57fb654 to 82bc1b7 Nov 6, 2018

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Nov 6, 2018

I changed it to setMinimumHeight so it could still become larger if needed. If that's not good enough then I'd need some help. It really just needs to be set to the height of all the other layout elements, so it can't bug out and decide on 0px.

@DXGLdotinfo

This comment has been minimized.

Contributor

DXGLdotinfo commented Nov 6, 2018

For me, the skinny sliders occurred when I added a compatibility manifest to Dolphin.exe enabling themes in High Contrast Mode. Whether or not my PR gets merged rests on this PR being implemented in a fully sane manner, as unthemed High Contrast Mode doesn't cause the issue.

@TryTwo

This comment has been minimized.

Contributor

TryTwo commented Nov 6, 2018

I managed to force VirtualBox to both 3840x2160 and 800x600 then have it rescale to my monitor. There were no issues with slider sizing when using either setminimumheight or setfixedheight.

QSize also mentioned it uses "integer point precision" so these settings are not pixel based, and are expected to scale correctly. I'm going to call DPI a non-issue. Let's commit this and in the unlikely event it makes sliders look weird on some platform, we can always fix it after hearing about the specifics.

@lioncash

This comment has been minimized.

Member

lioncash commented Nov 7, 2018

setMinimumHeight is fine (in this instance), since it doesn't clamp the ability for the controls to grow.

I don't even see how QSize is relevant here, since it's just a struct that holds integer values. What the values for a QSize mean can vary depending on which particular API of the Qt libraries they're used with. They can still represent ranges of pixels. The only reason integer-point precision is used in its description is to differentiate it from QSizeF, which uses floating-point precision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment