qt: Support macOS Dark mode #154

Allow icons to be colorized on macOS to support native Dark mode color scheme.

Rendering on macOS Big Sur before PR:

Rendering on macOS Big Sur after PR:

Light mode stay visually unchanged.

<del>Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see https://github.com/bitcoin/bitcoin/pull/14593). </del> <del>But once all glitches are fixed, we will be able to remove this temporary fix. </del> Edit: this PR is know including the removal of NSRequiresAquaSystemAppearance on Info.plist file so that the color fix is apply to every build.

Linked issues: #68 #136

@laanwj you wrote this code 6 years ago, maybe you could review this PR. I didn't figure out why the colorizeIcons options was set to false on macosx platform?

@goums IIRC, the icon colouring looked bad on MacOS. The discussion is probably somewhere back in 2014: (I think the discussion happened somewhere here: https://github.com/bitcoin/bitcoin/pull/5493).

I think it would be good to post some screenshots how this looks in non-dark mode.

Sure @jonasschnelli

MacOS Big Sur - Light Mode - Before PR:

MacOS Big Sur - Light Mode - After PR:

It looks exactly the same to me, with icon colorization active/inactive on light mode. Maybe older version of MacOs were behaving differently?

@goums IIRC, the icon colouring looked bad on MacOS. The discussion is probably somewhere back in 2014: (I think the discussion happened somewhere here: bitcoin/bitcoin#5493).

Thanks, I've read the discussion, it seems MacOS had no theme styling options at that time, as you mentioned it (https://github.com/bitcoin/bitcoin/pull/5493#issuecomment-67459610).

That's probably why the colorizeIcons options was set to false for macosx platform on first commit (https://github.com/bitcoin/bitcoin/pull/6487) and never changed since then.

Concept ACK. Just one boolean to fix all the things!

@goums IIUC, you use the latest Homebrew's Qt, right?

Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see bitcoin/bitcoin#14593).

I don't think that all macOS users ignore make deploy while building from source. Why make such a difference?

@goums IIUC, you use the latest Homebrew's Qt, right?

I installed QT Creator from their website, as mentioned in the readme: https://github.com/bitcoin-core/gui/tree/master/src/qt#using-qt-creator-as-ide

I don't think that all macOS users ignore make deploy while building from source. Why make such a difference?

Yes sure, my idea was to fix the glitches step by step. Then we could revert the use of NSRequiresAquaSystemAppearance option in the plist file so that everyone can enjoy the Dark Mode in macos.

ACK c2db537b030dfcbacaff54d1a2768055fc3309f8

The PR Colorizes icons as White on macOS using Dark Mode. Icons are Black when using Light Mode.

Master: Dark Mode Screenshots

PR: Dark and Light Mode Screenshots

I don't think that all macOS users ignore make deploy while building from source. Why make such a difference?

Yes sure, my idea was to fix the glitches step by step.

Both steps could be done in this pr as two different commits, no?

Then we could revert the use of NSRequiresAquaSystemAppearance option in the plist file so that everyone can enjoy the Dark Mode in macos.

For the second commit? Although, it will require to test builds with depends.

@laanwj you wrote this code 6 years ago, maybe you could review this PR. I didn't figure out why the colorizeIcons options was set to false on macosx platform?

Yes, for the reason @jonasschnelli says. Somehow it didn't look good on MacOS—which was all-white at the time—so black icons looked best.

This may have changed now, thank you for working on dark mode support.

@hebasto, as the plist file is not in the src/qt folder, I wasn't sure if it should be updated from this GUI repo or from the main repo.

Anyway, I've updated it so that all deploy can benefit from darkmode now, depending on user "System Preferences".

$ make deploy
$ open Bitcoin-Qt.app

• When the user has dark mode settings:
• When the user has light mode settings:

However, there is still some glitches on the TabWidget:

I don't know if this is acceptable or not?

This bug is coming directly from QMacStyle, and won't be fixed until QT 6, as discussed here: #136 (comment) The only workaround I can think of is to force the use fusion QT style on macos.

However, there is still some glitches on the TabWidget:

The only workaround I can think of is to force the use fusion QT style on macos.

#177 forces Fusion qt style on older versions of Qt. If we are sticking with Qt 5.9.8 as the depends package for the next release, then the next release will use Fusion Qt style.

@hebasto, as the plist file is not in the src/qt folder, I wasn't sure if it should be updated from this GUI repo or from the main repo.

I think this is fine for such a change.

Going to test. @goums Do you mind sorting out this PR commits by dropping unrelated one?

However, there is still some glitches on the TabWidget:

That is orthogonal to this PR changes.

See QTBUG-86513

Tested 92369a176c99ff779ed809a99a2e12b79cbb20b0 with depends -- no visual changes as expected.

Approach ACK 92369a176c99ff779ed809a99a2e12b79cbb20b0, tested on macOS Big Sur 11.2.1 (20D74):

• Homebrew's Qt 5.15.2:
  • running bitcoin-qt -- dark mode works fine
  • installing from Bitcoin-Core.dmg -- dark mode works fine
• cross-compiling with depends -- no changes, as expected

I think only commits must be sorted out (or squashed) before this PR gets ready to be merged.

Thanks for the full review @hebasto

I think only commits must be sorted out (or squashed) before this PR gets ready to be merged.

Sorry about that, I've removed the merge commit and rebase the branch instead, should be ok now. However commit tags have changed:

• https://github.com/bitcoin-core/gui/commit/92369a176c99ff779ed809a99a2e12b79cbb20b0 is now 303cfc6

~ACK 303cfc62277efccf242dc5a51368e349dc2782e3~ retracted

Can the commit message in 303cfc62277efccf242dc5a51368e349dc2782e3 be rewritten so it explains why this is possible. For release builds we are still using a version of Qt that doesn't properly support dark mode (as far as I'm aware support landed in 5.12.x). It also wasn't really the case that dark mode wasn't allowed before, you just had to build using a newer Qt. i.e this is master using dark mode:

<img width="846" alt="master" src="https://user-images.githubusercontent.com/863730/109249399-950fe480-7822-11eb-8b9a-dbf890d3ee13.png">

If we now properly support dark mode, this note should be removed from doc/release-notes.md as part of this change:

Additionally, Bitcoin Core does not yet change appearance when macOS "dark mode" is activated.

Also, the PR body needs to be updated it's included in the merge), because it's no longer correct. i.e:

Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see bitcoin/bitcoin#14593).

ACK 303cfc6

I retract my ACK.

These changes do not work on macOS Mojave 10.14.6 (18G8022 ) + Homebrew's Qt 5.15.2:

UPDATE: probably these changes need to be documented properly?

These changes do not work on macOS Mojave 10.14.6 (18G8022 ) + Homebrew's Qt 5.15.2: @hebasto, I'm running on macOS BigSur, and I'm struggling to find a way to test on macOS Mojave... Do you use VirtualBox to run multiple macOS versions?

UPDATE: probably these changes need to be documented properly?

yes sure

Can the commit message in 303cfc6 be rewritten so it explains why this is possible. For release builds we are still using a version of Qt that doesn't properly support dark mode (as far as I'm aware support landed in 5.12.x).

Ok, thanks for your explanations @fanquake . So theoretically, users downloading release builds will still run QT 5.9.8, right? As it's the version specified in qt.mk I'll try to run some tests with QT 5.9.8

If we now properly support dark mode, this note should be removed from doc/release-notes.md as part of this change:

Additionally, Bitcoin Core does not yet change appearance when macOS "dark mode" is activated.

Also, the PR body needs to be updated it's included in the merge), because it's no longer correct. i.e:

Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see bitcoin/bitcoin#14593).

Yes, I'll update that too.

@hebasto What's going on is that if you start off with light mode, then switch to dark mode; the icons do not get re-colorized into white. If you start off with dark mode enabled, and switch to light mode; the icons do not get re-colorized into black. @goums You can't get 10.14.6 from apple anymore so you'll have to do a quick google search for it. As for running it, I personally virtualize it. This can be done with VirtualBox or Parallels.

Command Line this kind of works on macOS 10.14.6 Qt 5.15.2 for me (from command-line):

.DMG EDIT: It still kind of works from .dmg after make deploy. Still experiences the same bug arising from switching between dark and light mode.

@goums

Do you use VirtualBox to run multiple macOS versions?

Yes. Join to irc ##bitcoin-core-gui channel on freenode to discuss technical questions in this repo :) @jarolrod

What's going on is that if you start off with light mode, then switch to dark mode; the icons do not get re-colorized into white. If you start off with dark mode enabled, and switch to light mode; the icons do not get re-colorized into black.

You are right. Another screenshot from macOS Mojave:

Maybe document (in release notes?) that switching appearance Light/Dark while Bitcoin Core running is not supported for macOS Mojave?

@goums

Has https://github.com/bitcoin/bitcoin/pull/21376, which was merged recently, any impact on this PR?

If we now properly support dark mode, this note should be removed from doc/release-notes.md as part of this change:

Additionally, Bitcoin Core does not yet change appearance when macOS "dark mode" is activated.

Maybe document (in release notes?) that switching appearance Light/Dark while Bitcoin Core running is not supported for macOS Mojave? @fanquake

Will the following work:

--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -51,9 +51,9 @@ Core should also work on most other Unix-like systems but is not as
 frequently tested on them.  It is not recommended to use Bitcoin Core on
 unsupported systems.
 
-From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no
-longer supported. Additionally, Bitcoin Core does not yet change appearance
-when macOS "dark mode" is activated.
+From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no
+longer supported. Additionally, the appearance switching (light/dark) is
+not supported for maOS 10.14.
 
 Notable changes
 ===============

?

So is it just 10.14 it doesn't work on now? Release builds work correctly on 10.15+? I'm not sure what's wrong with "Bitcoin Core does not yet change appearance when macOS "dark mode" is activated.", but I'm happy for this to be rewritten/updated however.

With this PR:

Release builds work correctly on 10.15+?

Yes.

So is it just 10.14 it doesn't work on now?

Partially. Appearance switching while Bitcoin Core is running does not cause automatic switching of icons.

what's wrong with "Bitcoin Core does not yet change appearance when macOS "dark mode" is activated."

On 10.14 only icons do not change appearance.

Being a non-English native speaker, I'll appreciate better wording suggestions to express the ideas above :)

Being a non-English native speaker, I'll appreciate better wording suggestions to express the ideas above :) @hebasto this is how I would put it, just a suggestion.

-From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no
-longer supported. Additionally, the appearance switching (light/dark) is
-not supported for maOS 10.14.

+From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no
+longer supported. Additionally, switching between light and dark mode is
+not supported on macOS 10.14.

On 10.14 only icons do not change appearance.

In that case I'd just drop the note entirely. It's no longer worth-while to call out in the release notes.

@goums

Could you implement @fanquake's suggestion to get this PR ready to merge?

Thanks @hebasto and @jarolrod for the updates and suggestions.

On 10.14 only icons do not change appearance.

In that case I'd just drop the note entirely. It's no longer worth-while to call out in the release notes.

I agree with @fanquake comment: as user usually don't switch light/dark mode to test the application, it will work just fine for most people by rendering correctly on start time depending on their settings. I've dropped the line from the release note.

Also, the PR body needs to be updated it's included in the merge), because it's no longer correct. i.e:

Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see bitcoin/bitcoin#14593).

Done

@goums linter is failing: https://cirrus-ci.com/task/4605693853433856?command=lint#L916

diff --git a/doc/release-notes.md b/doc/release-notes.md
@@ -55,2 +55 @@ From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no
+longer supported.
^---- failure generated from test/lint/lint-whitespace.sh

You can just place this warning on one line now. Doesn't have to go on two separate lines just to accommodate longer supported

Don't want to start with any bikeshedding but 56ed4ce87aa1704b2db16d71dbb69f1a8aa29a46 should be squashed onto 02223fc8de736b552c554e5bb9ecb884dd74c4c2, almost there 🥃

ACK 56ed4ce87aa1704b2db16d71dbb69f1a8aa29a46, gitian builds (this pr merged onto the master) tested:

• macOS Mojave 10.14.6 (18G8022):

• macOS Catalina 10.15.7 (19H524):

• macOS Big Sur 11.2.3 (20D91):

@goums On purpose to get a nice commit history in the git, do you mind squashing your commits, at least the last two ones?

ok, sorry about that ^^ git commits squashed into: dc4551c22c46104d18e8e7a8bf133cbebe4bc89f

re-ACK dc4551c22c46104d18e8e7a8bf133cbebe4bc89f

ACK dc4551c22c46104d18e8e7a8bf133cbebe4bc89f

@fanquake Anything left to do here?

Anything left to do here?

I haven't tested this, but I think it can be merged.

@goums @jarolrod

Do you mind testing https://github.com/bitcoin/bitcoin/pull/19817 (in context of https://github.com/bitcoin/bitcoin/pull/19817#issuecomment-810949389)?

@goums

Kindly asking to test #275 as it completes the task you started here.