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

Enable Mixxx's HDPI scaling for all versions QT < 5.6 #1741

Closed
wants to merge 1 commit into from

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Jul 6, 2018

This brings back Mixxx's own scaling solution for Ubuntu Trusty and Xenial.
Discussed in #1521

@daschuer daschuer changed the title Enable mixxx HID scaling for all versions QT < 5.6 Enable mixxx HDPI scaling for all versions QT < 5.6 Jul 6, 2018
@daschuer daschuer changed the title Enable mixxx HDPI scaling for all versions QT < 5.6 Enable Mixxx's HDPI scaling for all versions QT < 5.6 Jul 6, 2018
@Be-ing
Copy link
Contributor

Be-ing commented Jul 6, 2018

We should be removing this hack, not re-enabling it.

@daschuer
Copy link
Member Author

daschuer commented Jul 6, 2018

We can also use the preferences to set
QT_AUTO_SCREEN_SCALE_FACTOR
QT_SCALE_FACTOR
this requires that the user-config is loaded before the application is started.
It would be also nice to change it via skin load.
Unfortunately scale factors < 1 are working, but the preference window and other messages boxes are unusable. I use this for watching big skins on my small screen.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 6, 2018

this requires that the user-config is loaded before the application is started.

Which to me signals that Qt did not intend for applications to present this option to users.

@daschuer
Copy link
Member Author

daschuer commented Jul 6, 2018

It is not that uncommon to deal with it inside the application
https://github.com/search?l=C%2B%2B&q=QT_SCALE_FACTOR&type=Code
has 242 hits.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 6, 2018

Looking through those search results, I see lots of hacks for legacy Qt versions before the current API was introduced in Qt 5.6. Other search results look like hacks for specific devices and OS's and many are commented out.

@daschuer daschuer mentioned this pull request Aug 7, 2018
2 tasks
@ronso0
Copy link
Member

ronso0 commented Aug 7, 2018

works as before, thx!
qt 5.2.1 on trusty

@daschuer
Copy link
Member Author

daschuer commented Aug 8, 2018

There is an other use case for scaling, berry small screens:
https://www.mixxx.org/forums/viewtopic.php?f=8&t=12027&p=39236#p39236

Can we merge this?

@Be-ing
Copy link
Contributor

Be-ing commented Aug 8, 2018

No. Again, this hack should be removed. I don't think scaling should be exposed to users at all by applications, but if it is, the most appropriate way would be using Qt's scaling, not maintaining this elaborate hack for Qt4.

@daschuer
Copy link
Member Author

daschuer commented Aug 8, 2018

Remember: It is for Qt < 5.6 and we have no usable scaling solution for ≥ 5.6.
I consider setting the environment variables a unusable for the mayoruty if users.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 8, 2018

Going in circles discussing minimum Qt versions repeatedly on different PRs is not productive. If you want to propose supporting outdated systems, please take the discussion to Zulip.

@ronso0
Copy link
Member

ronso0 commented Aug 10, 2018

Even with a working automatic scaling solution, I think there are use cases where it's helpful to override:

  • sitting right in front of large HiDPI desktop screen:
    scale down so fonts and buttons are not ridiculously large
  • small screens:
    scale up so play indicators (Play button, Spinnies) are easier to recognize from a distance, accept squeezed skin parts in return

@daschuer
Copy link
Member Author

Ok, so we need a preferences option for ≥ 5.6 as well.
We can remove the guard here, to use our home made solution here as a short term solution, in addition to Qts scaling.
For long term, we might consider to tweak Qts scaling. Unfortunately this may interfere with the Os setting, for example if one sets the Qt environment variables system wide.

@ywwg
Copy link
Member

ywwg commented Aug 10, 2018

I also think it's best not to try to override OS settings with our own. For users with edge case screen sizes, they should be setting up their OS to their liking and Mixxx should respect those settings. If there are cases where the OS settings are truly broken, then I think it's ok to add new preferences as a workaround, with the expectation that we'd prefer to remove them once the OS is working correctly. We should only be doing things like setting "QApplication::setAttribute(Qt::AA_EnableHighDpiScaling)" if we detect the OS has highdpi enabled.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 10, 2018

We should only be doing things like setting "QApplication::setAttribute(Qt::AA_EnableHighDpiScaling)" if we detect the OS has highdpi enabled.

IIUC this code already won't do anything on non-high DPI screens. I don't think there's a need to enable it conditionally.

@ronso0
Copy link
Member

ronso0 commented Aug 10, 2018

Btw could someone explain
(1) what the preceivable difference is -for an unexperienced user- between Qt automatically scaling to let's say 175% and doing the same manually with our spinbox?
(2) how I as a skin designer without an HiDPI (not even FHD) screen would test for glitches in skins not at 100%?

@Be-ing
Copy link
Contributor

Be-ing commented Aug 10, 2018

Using Qt's scaling scales the entire GUI. Keeping this big pile of hacks for Qt4 does not scale everything, for example positioning in QSS files or arrows on spinboxes. With Qt's scaling there should be no need for skin designers to test at different scale factors once we fix the scaling issue we identified in #1772. If you do want to test anyway, you can set the QT_SCREEN_SCALE_FACTORS environment variable.

@ywwg
Copy link
Member

ywwg commented Aug 10, 2018

(2) how I as a skin designer without an HiDPI (not even FHD) screen would test for glitches in skins not at 100%?

Echoing what @Be-ing said, when the OS is in charge of doing the scaling it should be completely transparent to the application. Everything should be expanded equally and it shouldn't require any extra work. There has been a lot of work in browsers and OSes for converting virtual numbers in CSS files like "10px" into physical pixel values that may be totally different. That does explain why a minimum skin size of 1300x768 (or whatever) may not fit on a HiDPI screen - those are virtual pixel values that may get multiplied.

@daschuer
Copy link
Member Author

daschuer commented Aug 11, 2018

Ok, I understand the arguments not messing arround with the os/qt scaling, but I also see some convenient points and use cases to have scaling options in the preferences.

Unlike in preferences Mixxx does not use the standard Os widgets in the skin it is here more comparable to a web side, where scaling is a major feature of the browser.

How does the still enabled scaling of the library to this topic? Following the arguments to rely on the Os settings, this should also follow the Os text size settings.

The latest discussion is however a bit off topic because in the current state this PR fixes only a regression for Ubuntu Xenial and other < Qt 5.6 distros.
Users will loose all scaling after upgrading from Mixxx 2.1 to 2.2.

So I think we can merge this as it is, right?

@Be-ing
Copy link
Contributor

Be-ing commented Aug 11, 2018

Again you are imagining a user who:

  1. Has a high DPI screen
  2. Is using a 2 year old OS that does not support that high DPI screen
  3. Can't be bothered to update their OS
  4. Can't be bothered to set an environment variable
  5. Wants to use the latest version of Mixxx

I am highly sceptical any such user exists. Even if they do, I think the best way to support them is to tell them to update their OS. The cost of maintaining this elaborate hack that touches every part of the skin code for the sake of someone who can't be bothered to update their OS is IMO not worth it.

@daschuer
Copy link
Member Author

daschuer commented Aug 11, 2018

The case I imaging is not that exotic as it looks like from your latest post.

I am imaging a user, who

  1. uses Ubuntu Xenial, a fully supported OS, including HiDPI for gtk apps
  2. wants to use Mixxx 2.2 and keep the scaling settings from 2.1
  3. Can't be bothered to update their OS

I would like to stop maintaining the 2.1 branch after the 2.2 release. But unfortunately we cannot unconditionally recommend to update to Mixxx 2.2 without this PR because of this group of users.

@ywwg
Copy link
Member

ywwg commented Aug 11, 2018

It might help to pick a horizon for ubuntu version support and stick to it. We could choose to match Ubuntu's schedule, which supports Xenial in maintenance mode until 2021 (https://www.ubuntu.com/info/release-end-of-life), or we could say that since 18.04 has been released (the new LTS) we can safely drop support for 16.04.

This is a tough balance. It can take a lot of energy to support old releases, but Mixxx definitely has a substantial number of users on older, cheaper hardware with older OSes. Given our history of great compatibility with older machines, I'm ok with assuming this kind of edge-case user exists.

I think if we come up with a standard support policy it could make these discussions easier. The two main metrics I'm thinking of that come up a lot are screen size and OS version support. If we can write down a policy on the wiki then that will also help our users know what we're committed to supporting.

@ywwg
Copy link
Member

ywwg commented Aug 11, 2018

(In general, it's better to have written project policies than forcing everyone to rely on their memory of past discussions, or finding old email threads. I think it would be nice to have a goal of creating documents based on the output of the arguments that we have.)

@Be-ing
Copy link
Contributor

Be-ing commented Aug 11, 2018

I agree, I would rather have a consistent policy than drain my energy on ad-hoc discussions repeatedly on different PRs. The discussion is here on Zulip.

I have opened #1777 to elucidate the cost of merging this PR.

@daschuer
Copy link
Member Author

So what hat to do here. from our latest discussions it turns out that Xenial is among the supported distros. Without it we loose the HiDpi capability.
So I think we should merge it.
I also think that this patch shall not limit us for future skin widgets an exception small anoyanced using Xenial.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 17, 2018

Ubuntu 16.04 users can still set the QT_DEVICE_PIXEL_RATIO environment variable with Qt 5.5. Or they can upgrade to 18.04. I do not think there is a need to merge this.

@daschuer
Copy link
Member Author

Unfortunately this does not work for the waveforms:
bildschirmfoto vom 2018-08-17 22-37-10

@Be-ing
Copy link
Contributor

Be-ing commented Aug 17, 2018

Is that the case with all waveform renderers?

@daschuer
Copy link
Member Author

Only GL and GLSL renderer are effected. The Spinny is always effected.

@uklotzde
Copy link
Contributor

Another PR in an unclear state. Please close or update it.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 29, 2019

I am in favor of closing this. Maintaining a legacy hack for a 4 year old OS for hypothetical users who we have no evidence of existing in the first place makes no sense to me.

@daschuer
Copy link
Member Author

OK, since this seams to be an unwanted change we can close this.

A more important change would be to remove the requirement to deal with environment variables.
I guess the majority of user is not able to do this.

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.

None yet

5 participants