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

qt: Set AA_EnableHighDpiScaling attribute early #16254

Merged

Conversation

Projects
None yet
7 participants
@hebasto
Copy link
Member

commented Jun 20, 2019

Running bitcoin-qt compiled against Qt 5.12.4 causes a warning:

hebasto@bionic-qt:~/bitcoin$ src/qt/bitcoin-qt 
Attribute Qt::AA_EnableHighDpiScaling must be set before QCoreApplication is created.

This PR fixes this issue.

From Qt docs:

Enables high-DPI scaling in Qt on supported platforms (see also High DPI Displays). Supported platforms are X11, Windows and Android. Enabling makes Qt scale the main (device independent) coordinate system according to display scale factors provided by the operating system. This corresponds to setting the QT_AUTO_SCREEN​_SCALE_FACTOR environment variable to 1. This attribute must be set before QGuiApplication is constructed. This value was added in Qt 5.6.

Show resolved Hide resolved src/qt/bitcoin.cpp Outdated
Set AA_EnableHighDpiScaling attribute early
Qt docs: This attribute must be set before QGuiApplication is 
constructed.

@DrahtBot DrahtBot added the GUI label Jun 20, 2019

@hebasto hebasto force-pushed the hebasto:20190620-EnableHighDpiScaling-warning branch from 7b73b78 to 099e4b9 Jun 20, 2019

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

@MarcoFalke

I'd rather move this after all setAttribute. This shouldn't cause any issues, right?

Done.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Thanks for fixing this.

ACK 099e4b9

Before (see the broken icons in the lower right):
Screenshot from 2019-06-20 14-47-43

After:
Screenshot from 2019-06-20 14-46-30

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Is this for backport? @jonasschnelli

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Nice!
Finally someone figured this out and made it happen.

utACK 099e4b9
Will test on Win/linux soon.

It's def. a backport.

@laanwj laanwj added this to the 0.18.1 milestone Jun 20, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Before (see the broken icons in the lower left):

@MarcoFalke you mean on the lower right?

@hebasto can you run QT_FATAL_WARNINGS=1 src/qt/bitcoin-qt without this change?

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

@promag

can you run QT_FATAL_WARNINGS=1 src/qt/bitcoin-qt without this change?

hebasto@bionic-qt:~/bitcoin$ QT_FATAL_WARNINGS=1 src/qt/bitcoin-qt 
Attribute Qt::AA_EnableHighDpiScaling must be set before QCoreApplication is created.
Aborted (core dumped)
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Gitian builds for commit 9c95515 (master):

Gitian builds for commit 4460284 (master and this pull):

@promag

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

@hebasto thank you. I think reviewers should set QT_FATAL_WARNINGS=1 on their systems, can prevent incomplete changes like #16118.

@fanquake
Copy link
Member

left a comment

ACK 099e4b9. Did some testing on Bionic and Windows 10 (using VirtualBox). I couldn't see any obvious visual difference, but given Marco's screens above, this change is obviously better. I also checked that there wasn't any sort of regression on macOS.

master on Bionic using Qt 5.12.4:
master_ubuntu_bionic_qt_12_4

This PR on Bionic using Qt 5.12.4:
16254_ubuntu_bionic_qt_12_4

master on Windows 10 using depends:
master_windows_depends

This PR on Windows 10 using depends:
16254_windows_depends

This PR on macOS 10.14.5 using Qt 5.12.3:
16254_macos_qt_5_12_3

Confirmed that the below warning is no longer shown when running on Bionic:

Attribute Qt::AA_EnableHighDpiScaling must be set before QCoreApplication is created.

fanquake added a commit that referenced this pull request Jun 24, 2019

Merge #16263: qt: Use qInfo() if no error occurs
a2aabfb Use qInfo() if no error occurs (Hennadii Stepanov)

Pull request description:

  [Warning and Debugging Messages](https://doc.qt.io/qt-5/debug.html#warning-and-debugging-messages):
  > - `qInfo()` is used for informational messages.
  > - `qWarning()` is used to report warnings and recoverable errors in your
  application.
  >
  > If the `QT_FATAL_WARNINGS` environment variable is set, `qWarning()` exits after printing the warning message. This makes it easy to obtain a backtrace in the debugger.

  [`qWarning()`](https://doc.qt.io/qt-5/qtglobal.html#qWarning):
  > Calls the message handler with the warning message message... This function does nothing if `QT_NO_WARNING_OUTPUT` was defined during compilation; it exits if at the nth warning corresponding to the counter in environment variable `QT_FATAL_WARNINGS`.

  This PR allows more productive debugging using the environment variable `QT_FATAL_WARNINGS`.

  Examples:
  - #16118 (comment)
  - #16254 (comment)

  The behavior, when option `-debug=qt` is set/unset, remains unchanged.

ACKs for commit a2aabf:
  promag:
    ACK a2aabfb, I also have this change locally.
  Empact:
    ACK a2aabfb
  laanwj:
    ACK a2aabfb
  fanquake:
    ACK a2aabfb.

Tree-SHA512: b4df300c9c00a1705b0d3a10227e3deaac19a98b0a898bb60d5a88872cf450fb131eba150d9dd6c29e021566ee04b3b86b7d486bbe28bd894743c128d2309155
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Tested on Win10... much much better HiDPI experience.

Master:
Bildschirmfoto 2019-06-24 um 14 33 24
Bildschirmfoto 2019-06-24 um 14 33 14
Bildschirmfoto 2019-06-24 um 14 34 47
Bildschirmfoto 2019-06-24 um 14 37 50

This PR:
Bildschirmfoto 2019-06-24 um 14 34 01
Bildschirmfoto 2019-06-24 um 14 37 36
Bildschirmfoto 2019-06-24 um 14 35 02
Bildschirmfoto 2019-06-24 um 14 34 28

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jun 24, 2019

Merge bitcoin#16254: qt: Set AA_EnableHighDpiScaling attribute early
099e4b9 Set AA_EnableHighDpiScaling attribute early (Hennadii Stepanov)

Pull request description:

  Running `bitcoin-qt` compiled against Qt 5.12.4 causes a warning:
  ```
  hebasto@bionic-qt:~/bitcoin$ src/qt/bitcoin-qt
  Attribute Qt::AA_EnableHighDpiScaling must be set before QCoreApplication is created.
  ```

  This PR fixes this issue.

  From Qt docs:
  - [Qt::AA_EnableHighDpiScaling](https://doc.qt.io/qt-5/qt.html#ApplicationAttribute-enum):
  > Enables high-DPI scaling in Qt on supported platforms (see also High DPI Displays). _Supported platforms are X11, Windows and Android._ Enabling makes Qt scale the main (device independent) coordinate system according to display scale factors provided by the operating system. This corresponds to setting the `QT_AUTO_SCREEN​_SCALE_FACTOR` environment variable to 1. This attribute must be set before `QGuiApplication` is constructed. This value was added in Qt 5.6.

  - [QCoreApplication::setAttribute()](https://doc.qt.io/qt-5/qcoreapplication.html#setAttribute)

ACKs for commit 099e4b:
  MarcoFalke:
    ACK 099e4b9
  jonasschnelli:
    utACK 099e4b9
  fanquake:
    ACK 099e4b9. Did some testing on `Bionic` and `Windows 10` (using VirtualBox). I couldn't see any obvious visual difference, but given Marco's screens above, this change is obviously better. I also checked that there wasn't any sort of regression on macOS.

Tree-SHA512: 1965a427ee14ffb3871bac317685032406cf02d1fa2b2dc11c8b643bfe4ba09195674d149d1e41752f14c0d000446b35e142f3ce60d987ba97082fd7ee39a094

@MarcoFalke MarcoFalke merged commit 099e4b9 into bitcoin:master Jun 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jun 24, 2019

Set AA_EnableHighDpiScaling attribute early
Qt docs: This attribute must be set before QGuiApplication is
constructed.

Github-Pull: bitcoin#16254
Rebased-From: 099e4b9

@hebasto hebasto deleted the hebasto:20190620-EnableHighDpiScaling-warning branch Jun 24, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Though it seems not to resolve the HiDPI issues on Ubuntu Gnome:

Bildschirmfoto 2019-06-24 um 15 38 09

Bildschirmfoto 2019-06-24 um 15 38 06

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Hmm, that's odd. I was testing on fedora 30 gnome and the issue was fixed there.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Gitian builds for commit 2cbcc55 (master):

Gitian builds for commit 6969aa8 (master and this pull):

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

It's glitchy... sometimes it works. Especially if I pass in --resetguisettings. Haven't figured out why exactly.

@fanquake

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Being backported in 16035.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.