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 HiDPI support with Qt5 #1521

Merged
merged 1 commit into from
May 23, 2018
Merged

enable HiDPI support with Qt5 #1521

merged 1 commit into from
May 23, 2018

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Feb 25, 2018

This will automatically use the system scaling settings as documented at http://doc.qt.io/qt-5/highdpi.html

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 25, 2018

Could someone test this on Windows and macOS? @JosepMaJAZ, @esbrandt?

@JosepMaJAZ
Copy link
Contributor

AppVeyor build is still with QT4.

I have QT 5.7 (as downloaded from QTs site when it was the current release) with which I made some mixxx build tests quite some time ago and I could try how it works out now.
If I don't get too many problems to build it, I should be able to report about it today.

@Be-ing Be-ing added this to the 2.2.0 milestone Mar 19, 2018
@@ -169,6 +169,7 @@ DlgPrefInterface::DlgPrefInterface(QWidget * parent, MixxxMainWindow * mixxx,
checkBoxScaleFactorAuto->hide();
comboBoxScaleFactor->hide();
labelScaleFactor->hide();
QApplication::setAttribute(Qt::AA_EnableHighDpiScaling);
Copy link
Member

Choose a reason for hiding this comment

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

This attribute must be set before Q(Gui)Application is constructed.
https://doc.qt.io/qt-5/qt.html
Looks like this should go in main.cpp before creating MixxxApplication.

Copy link
Contributor Author

@Be-ing Be-ing May 22, 2018

Choose a reason for hiding this comment

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

Good catch, thanks! Fixed (and force pushed to replace old commit).

@Be-ing Be-ing force-pushed the qt5_hidpi branch 3 times, most recently from 51bd204 to 0c7e432 Compare May 22, 2018 23:39
This will automatically use the system scaling settings as
documented at http://doc.qt.io/qt-5/highdpi.html
@rryan
Copy link
Member

rryan commented May 23, 2018

LGTM

@rryan
Copy link
Member

rryan commented May 23, 2018

macOS CI looks broken, but unrelated:

[CXX] src/library/autodj/autodjprocessor.cpp
In file included from src/library/analysisfeature.cpp:7:
In file included from src/library/library.h:18:
In file included from src/library/analysisfeature.h:16:
src/library/dlganalysis.h:64:18: error: field has incomplete type 'QButtonGroup'
    QButtonGroup m_songsButtonGroup;
                 ^
/usr/local/Cellar/qt/5.11.0/Frameworks/QtWidgets.framework/Headers/qabstractbutton.h:53:7: note: forward declaration of 'QButtonGroup'

@Be-ing Be-ing merged commit 4a76ade into mixxxdj:2.1 May 23, 2018
@daschuer
Copy link
Member

daschuer commented Jun 1, 2018

Unfortunately this is a regression for Ubuntu Trusty and Xenial, which have a Qt version < Qt 5.6

@rryan
Copy link
Member

rryan commented Jun 1, 2018

Unfortunately this is a regression for Ubuntu Trusty and Xenial, which have a Qt version < Qt 5.6

This change only affects >=5.6 so how can it affect those?

@daschuer
Copy link
Member

daschuer commented Jun 1, 2018

Right, this PR itself is not a regression. Due to moving to QT5 we have lost the HIDPI hacks of QT4, this PR beings HIDPI back, but only for QT 5.6 builds and up. So there is a gap that have still no HIDPI.

This can be fixed by either backport the QT 5.6 changes to detect the OS scaling or show the scaling slider in preferences and let the user choose the QT 5 scaling (not the QT4 Hack of cause)

@rryan
Copy link
Member

rryan commented Jun 1, 2018

This can be fixed by either backport the QT 5.6 changes to detect the OS scaling or show the scaling slider in preferences and let the user choose the QT 5 scaling (not the QT4 Hack of cause)

Or do nothing :).

Since there is an LTS available that fixes the problem and users can always stick with MIxxx 2.1 it seems pretty low severity.

@daschuer
Copy link
Member

daschuer commented Jun 1, 2018

Yes of cause. But It will be surprising for Xenial users, to loose HIDPI after update to 2.2.

@uklotzde
Copy link
Contributor

uklotzde commented Jul 4, 2018

The auto-scaling does not work well on my T460p on the built-in display with a resolution of 2560x1440. The Mixxx UI should select a scaling factor of 1x instead of 2x to be usable.

We need an option to customize the scaling in the preferences. Until then I have to remove this line from my custom Qt5 build.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 4, 2018

You can use Qt's standard environment variables if the autodetection doesn't work well for your screen. I have not seen any other Qt apps provide a GUI for adjusting this. I do not think Mixxx should be an exception. Qt's autodetection works great on my 3840 x 2160 14 inch diagonal screen.

@daschuer
Copy link
Member

daschuer commented Jul 4, 2018

We already have a GUI to reanable the individual scaling. Let's bring it back.
If Uwe want's it, there are probably many users out there with the same demand but without the knowledge to set environment variables.
There is also the regression for legacy qt5 user without auto adjusting.

@uklotzde
Copy link
Contributor

uklotzde commented Jul 5, 2018

Confirmed: The auto scaling can be overruled by setting QT_AUTO_SCREEN_SCALE_FACTOR=0.

But this workaround is cumbersome for the casual user. We should at least offer an option to disable auto scaling in the DisplayInterface preferences.

Those intermediate resolutions between FullHD and 4K do not work well with the default 2x upscaling. Same issue with GNOME where I use a Scaling Factor of 1,0 even on a 14" screen.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 5, 2018

I do not think it is an application's place to offer a UI for adjusting this system setting.

@uklotzde
Copy link
Contributor

uklotzde commented Jul 5, 2018

I disagree. We are now unconditionally setting Qt::AA_EnableHighDpiScaling which works for the majority of uses cases, but fails for some as outlined. Conversely we could also remove this invocation entirely and tell the user to simply set QT_AUTO_SCREEN_SCALE_FACTOR=1 if they really need auto scaling? No, not really ;)

Programmatically enabling auto scaling by default is ok, as long as we offer an option that allows to disable it (i.e. not enable it explicitly). Otherwise I expect support requests by users that are faced with an unusable UI. We cannot expect them to read the Qt developer documentation.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 5, 2018

I presume if a user is having a problem with Mixxx's scaling, every Qt application is affected too. So why should any one application be responsible for working around it?

@uklotzde
Copy link
Contributor

uklotzde commented Jul 5, 2018

Not every Qt application. If we explicitly invoke QApplication::setAttribute(Qt::AA_EnableHighDpiScaling) because we assume that this is a sensible default we should also offer the user to opt-out easily if needed and skip setting this attribute.

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.

5 participants