qt: Support macOS Dark mode #154

goums · 2020-12-17T14:50:09Z

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.

~~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).~~
~~But once all glitches are fixed, we will be able to remove this temporary fix.~~
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

goums · 2020-12-17T15:00:31Z

@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?

jonasschnelli · 2020-12-17T15:49:15Z

@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).

jonasschnelli · 2020-12-17T15:49:58Z

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

goums · 2020-12-17T16:52:54Z

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 · 2020-12-17T17:03:27Z

@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 (bitcoin/bitcoin#5493 (comment)).

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

Sjors · 2020-12-17T18:47:24Z

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

hebasto · 2020-12-26T20:13:58Z

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

hebasto · 2020-12-26T20:25:24Z

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 · 2021-01-04T21:22:17Z

@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.

jarolrod

ACK c2db537

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

hebasto · 2021-02-22T21:07:12Z

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 · 2021-02-23T08:42:40Z

@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.

goums · 2021-02-25T15:36:25Z

@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.

jarolrod · 2021-02-25T15:50:41Z

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 · 2021-02-25T16:17:48Z

@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?

hebasto · 2021-02-25T16:33:20Z

However, there is still some glitches on the TabWidget:

That is orthogonal to this PR changes.

See QTBUG-86513

hebasto

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

hebasto

Approach ACK 92369a1, 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.

goums · 2021-02-25T21:32:21Z

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:

92369a1 is now 303cfc6

hebasto

~~ACK 303cfc6~~ retracted

fanquake · 2021-02-26T03:08:52Z

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). 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:

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).

hebasto · 2021-02-26T20:46:53Z

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?

goums · 2021-02-26T22:56:32Z

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

goums · 2021-02-26T23:27:27Z

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 · 2021-03-24T05:07:36Z

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
 ===============

?

fanquake · 2021-03-24T06:58:13Z

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.

hebasto · 2021-03-24T13:00:58Z

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 :)

jarolrod · 2021-03-25T01:29:49Z

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.

fanquake · 2021-03-25T02:33:47Z

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.

hebasto · 2021-03-25T02:46:51Z

@goums

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

goums · 2021-03-25T08:54:11Z

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

jarolrod · 2021-03-25T13:25:22Z

@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

hebasto · 2021-03-25T13:27:09Z

doc/release-notes.md

@@ -52,8 +52,7 @@ 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.
+longer supported. 


removing trailing space is enough :)

Suggested change

longer supported.

longer supported.

jarolrod · 2021-03-25T14:07:27Z

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

hebasto

ACK 56ed4ce, 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?

hebasto · 2021-03-25T14:13:12Z

doc/release-notes.md

-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 0.22.0 onwards, macOS versions earlier than 10.14 are no longer supported.


(see bitcoin/bitcoin#20223)

Suggested change

From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no longer supported.

From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no longer supported.

goums · 2021-03-25T14:33:29Z

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

hebasto

re-ACK dc4551c

jarolrod

ACK dc4551c

maflcko · 2021-03-29T07:36:03Z

@fanquake Anything left to do here?

fanquake · 2021-03-29T08:34:34Z

Anything left to do here?

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

hebasto · 2021-03-31T10:17:23Z

@goums @jarolrod

Do you mind testing bitcoin/bitcoin#19817 (in context of bitcoin/bitcoin#19817 (comment))?

hebasto · 2021-05-27T11:38:26Z

@goums

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

goums mentioned this pull request Dec 18, 2020

macOS 11 (Big Sur) visual bugs #136

Closed

2 tasks

prusnak approved these changes Dec 18, 2020

View reviewed changes

jarolrod reviewed Jan 14, 2021

View reviewed changes

goums changed the title ~~qt: Colorize icons on macOS for Dark mode support~~ qt: Support macOS Dark mode Feb 25, 2021

hebasto reviewed Feb 25, 2021

View reviewed changes

Uplab and others added 2 commits February 25, 2021 22:21

Allow icon colorization on mac os to better support dark mode

78f75a2

allow darkmode on macos build

303cfc6

goums force-pushed the macos_color_icons branch from 92369a1 to 303cfc6 Compare February 25, 2021 21:24

hebasto approved these changes Feb 25, 2021

View reviewed changes

hebasto added the Waiting for author label Mar 25, 2021

hebasto removed the Waiting for author label Mar 25, 2021

hebasto reviewed Mar 25, 2021

View reviewed changes

hebasto approved these changes Mar 25, 2021

View reviewed changes

hebasto reviewed Mar 25, 2021

View reviewed changes

remove incompatibility release note for darkmode on macos

dc4551c

goums force-pushed the macos_color_icons branch from 56ed4ce to dc4551c Compare March 25, 2021 14:31

hebasto approved these changes Mar 25, 2021

View reviewed changes

jarolrod reviewed Mar 25, 2021

View reviewed changes

maflcko merged commit 3bcd278 into bitcoin-core:master Mar 29, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 29, 2021

Merge bitcoin-core/gui#154: qt: Support macOS Dark mode

a0c1fc7

hebasto mentioned this pull request Mar 31, 2021

build: macOS toolchain bump bitcoin/bitcoin#19817

Merged

jarolrod mentioned this pull request Apr 3, 2021

macOS theme switching not supported #268

Closed

hebasto mentioned this pull request Apr 20, 2021

Support runtime appearance adjustment on macOS #275

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 3bcd278 into merged_master (Bitcoin PR bitcoin-core/gui#154)

256ce55

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

	From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no longer supported.
	From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no longer supported.

qt: Support macOS Dark mode #154

qt: Support macOS Dark mode #154

Conversation

goums commented Dec 17, 2020 • edited Loading

goums commented Dec 17, 2020

jonasschnelli commented Dec 17, 2020

jonasschnelli commented Dec 17, 2020

goums commented Dec 17, 2020

goums commented Dec 17, 2020

Sjors commented Dec 17, 2020

hebasto commented Dec 26, 2020

hebasto commented Dec 26, 2020

goums commented Jan 4, 2021

jarolrod left a comment

Choose a reason for hiding this comment

hebasto commented Feb 22, 2021

laanwj commented Feb 23, 2021

goums commented Feb 25, 2021

jarolrod commented Feb 25, 2021

hebasto commented Feb 25, 2021

hebasto commented Feb 25, 2021 • edited Loading

hebasto left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

goums commented Feb 25, 2021

hebasto left a comment • edited Loading

Choose a reason for hiding this comment

fanquake commented Feb 26, 2021

hebasto commented Feb 26, 2021 • edited Loading

goums commented Feb 26, 2021 • edited Loading

goums commented Feb 26, 2021 • edited Loading

hebasto commented Mar 24, 2021

fanquake commented Mar 24, 2021

hebasto commented Mar 24, 2021

jarolrod commented Mar 25, 2021

fanquake commented Mar 25, 2021

hebasto commented Mar 25, 2021

goums commented Mar 25, 2021

jarolrod commented Mar 25, 2021

hebasto Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

jarolrod commented Mar 25, 2021

hebasto left a comment • edited Loading

Choose a reason for hiding this comment

hebasto Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

goums commented Mar 25, 2021

hebasto left a comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

maflcko commented Mar 29, 2021

fanquake commented Mar 29, 2021

hebasto commented Mar 31, 2021

hebasto commented May 27, 2021

goums commented Dec 17, 2020 •

edited

Loading

hebasto commented Feb 25, 2021 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto commented Feb 26, 2021 •

edited

Loading

goums commented Feb 26, 2021 •

edited

Loading

goums commented Feb 26, 2021 •

edited

Loading

hebasto Mar 25, 2021 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto Mar 25, 2021 •

edited

Loading