Use "fusion" style on macOS Big Sur with old Qt

hebasto commented at 9:59 pm on January 7, 2021: member

The “macintosh” style is broken on macOS Big Sur:

hebasto commented at 10:02 pm on January 7, 2021: member

As the upcoming 0.21 release is the first one after macOS Big Sur release, do you mind considering to backport this change into the 0.21 branch in order to ship the usable binaries to Big Sur users?

prusnak commented at 11:23 pm on January 7, 2021: contributor

Why is there the version check for Qt? I see the same issue with Qt 5.12 on Big Sur.

Also checking for “macOS 11” is spurious.

Do we need the version checks at all? Why don’t just apply this always on macOS?

hebasto commented at 11:49 pm on January 7, 2021: member

Why is there the version check for Qt?

This change, actually, is intended to fix visual quality for builds with depends, including shipped releases.

I see the same issue with Qt 5.12 on Big Sur.

With the latest Homebrew’s Qt 5.15.2 the native macOS style looks much better.

Also checking for “macOS 11” is spurious.

Not sure if I understand you correctly. Do you mind elaborating your comment?

Do we need the version checks at all? Why don’t just apply this always on macOS?

Many (most?) macOS users prefer the native macOS style, and the “fusion” does not look native on macOS.

MarcoFalke added this to the milestone 0.21.1 on Jan 8, 2021

jarolrod commented at 2:04 pm on January 8, 2021: member

Concept ACK

I applied this patch to the rc5 source code to see how it would react on macOS Big Sur 11.1 with bitcoin-qt built with Qt 5.9.8.

The name check does not work: if (QSysInfo::prettyProductName().startsWith("macOS 11")) Removing this check successfully allows the Qt Fusion Style to appear. I’ll investigate what’s wrong with the naming.

Branch: 9f50395735097e0687daf99098c5f72ead8abc73 Qt Fusion Style fails to appear Screen Shot 2021-01-08 at 4 39 44 AM

Removing The Conditional Fusion Style appears Screen Shot 2021-01-08 at 8 53 06 AM

qt: Use "fusion" style on macOS Big Sur with old Qt

The "macintosh" style is broken on macOS Big Sur at least for Qt 5.9.8.

4e1154dfd1

hebasto force-pushed on Jan 8, 2021

hebasto commented at 2:25 pm on January 8, 2021: member

Updated 9f50395735097e0687daf99098c5f72ead8abc73 -> 4e1154dfd128cbada65e9ea08ee274cdeafc4c53 (pr177.01 -> pr177.02, diff):

addressed @jarolrod’s comment:

The name check does not work: if (QSysInfo::prettyProductName().startsWith("macOS 11"))

jarolrod commented at 3:32 pm on January 8, 2021: member

ACK 4e1154dfd128cbada65e9ea08ee274cdeafc4c53

Qt Fusion successfully shows. Tested with patched rc5 source code on macOS Big Sur with Qt 5.9.8

MarcoFalke assigned jonasschnelli on Jan 11, 2021

jonasschnelli commented at 1:45 pm on January 11, 2021: contributor

This PR macOS 11 dark mode (self compiled against Qt 5.14):

Master Dark Mode macOS 11:

I can’t see any major visual differences. Master already looks good on macOS BigSure. Is that due to Qt 5.14?

hebasto commented at 2:07 pm on January 11, 2021: member

@jonasschnelli

Is that due to Qt 5.14?

Yes: https://github.com/bitcoin-core/gui/blob/4e1154dfd128cbada65e9ea08ee274cdeafc4c53/src/qt/bitcoin.cpp#L470-L475

With Qt 5.14 the “macintosh” style looks not ideal, but usable. With Qt 5.9.8, that is shipped to users as gitian builds, it is unusable at all.

in src/qt/bitcoin.cpp:470 in 4e1154dfd1

466@@ -466,6 +467,13 @@ int GuiMain(int argc, char* argv[])
467     QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling);
468 #endif
469 
470+#if (QT_VERSION <= QT_VERSION_CHECK(5, 9, 8)) && defined(Q_OS_MACOS)

luke-jr commented at 4:52 pm on January 13, 2021:

Based on the linked Qt bug, this version check appears wrong?

jarolrod commented at 8:35 pm on January 13, 2021:

This version check is correct because #136 concerns Qt compiled on macOS with depends. Compiling with depends builds bitcoin-qt with Qt 5.9.8. See: Qt Depends Package

hebasto commented at 7:53 pm on January 15, 2021:

Based on the linked Qt bug, this version check appears wrong?

The link to Qt bug removed from the OP. It is possible that version check could be improved, but I do not know for sure from which Qt version widgets are quite usable (I do not even say about fixing all bugs).

The recent Homebrew’s Qt versions look decent (but not ideal). And this PR goal is to fix GUI look while building with depends (including gitian binary).

in src/qt/bitcoin.cpp:472 in 4e1154dfd1

466@@ -466,6 +467,13 @@ int GuiMain(int argc, char* argv[])
467     QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling);
468 #endif
469 
470+#if (QT_VERSION <= QT_VERSION_CHECK(5, 9, 8)) && defined(Q_OS_MACOS)
471+    const auto os_name = QSysInfo::prettyProductName();
472+    if (os_name.startsWith("macOS 11") || os_name.startsWith("macOS 10.16")) {

luke-jr commented at 4:54 pm on January 13, 2021:

Perhaps it should apply to 12+ as well, though probably the issue will be long-gone by then

jarolrod commented at 8:23 pm on January 13, 2021:

I think this is fine as is. This is meant to be a patch until the Qt version is bumped up from 5.9.8, not a permanent fix. Presumably, we will get the Qt version bumped up before there is a macOS 12.

hebasto commented at 8:03 pm on January 15, 2021:

Perhaps it should apply to 12+ as well, though probably the issue will be long-gone by then

Did not test 10.12, but there are no such visual issues on Catalina 10.15.7 (19H114) with the recent 0.21.0 release.

I believe the “macintosh” style is broken on macOS Big Sur only.

UPDATE: v0.21.0 looks on Mojave 10.14 fine.

luke-jr changes_requested

hebasto commented at 0:43 am on January 21, 2021: member

Btw, there is an alternative to this PR. It is reverting of https://github.com/bitcoin/bitcoin/pull/16578. Then it would be possible to pass -style Fusion as a command line argument.

cc @achow101

achow101 commented at 2:31 am on January 21, 2021: member

Btw, there is an alternative to this PR. It is reverting of bitcoin/bitcoin#16578

NACK. The details will become clear soon.

luke-jr commented at 3:21 pm on January 21, 2021: member

Btw, there is an alternative to this PR. It is reverting of bitcoin/bitcoin#16578. Then it would be possible to pass -style Fusion as a command line argument.

Users should not need to pass options.

MarcoFalke added the label macOS on Jan 21, 2021

MarcoFalke commented at 8:10 pm on January 21, 2021: contributor

run appveyor (will need to be rerun before merge)

MarcoFalke closed this on Jan 21, 2021

MarcoFalke reopened this on Jan 21, 2021

MarcoFalke commented at 3:01 pm on January 25, 2021: contributor

review ACK 4e1154dfd128cbada65e9ea08ee274cdeafc4c53 can’t test

laanwj commented at 8:49 am on January 26, 2021: member

So nice to see we’re finally going to support MacOS dark mode. (Concept ACK, can’t test this)

jonasschnelli approved

jonasschnelli commented at 9:03 am on January 28, 2021: contributor

Tested ACK 4e1154dfd128cbada65e9ea08ee274cdeafc4c53

jonasschnelli merged this on Jan 28, 2021

jonasschnelli closed this on Jan 28, 2021

MarcoFalke referenced this in commit 6dc58e9945 on Jan 28, 2021

MarcoFalke commented at 9:17 am on January 28, 2021: contributor

Backported in https://github.com/bitcoin/bitcoin/pull/20901

hebasto deleted the branch on Jan 28, 2021

sidhujag referenced this in commit 8f9dacb966 on Jan 28, 2021

MarcoFalke referenced this in commit 8e6532053f on Mar 16, 2021

apoelstra referenced this in commit 900078b305 on Sep 21, 2021

apoelstra referenced this in commit ab153f9555 on Sep 21, 2021

apoelstra referenced this in commit 9e6c08d514 on Sep 22, 2021

bitcoin-core locked this on Aug 16, 2022

Use “fusion” style on macOS Big Sur with old Qt #177