qt: Support macOS Dark mode #154

pull goums wants to merge 3 commits into bitcoin-core:master from goums:macos_color_icons changing 3 files +2 −7
  1. goums commented at 2:50 pm on December 17, 2020: none

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

    Rendering on macOS Big Sur before PR: macos-darkmode-before-pr

    Rendering on macOS Big Sur after PR: macos-darkmode-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 https://github.com/bitcoin/bitcoin/pull/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

  2. goums commented at 3:00 pm on December 17, 2020: none
    @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?
  3. jonasschnelli commented at 3:49 pm on December 17, 2020: contributor
    @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).
  4. jonasschnelli commented at 3:49 pm on December 17, 2020: contributor
    I think it would be good to post some screenshots how this looks in non-dark mode.
  5. goums commented at 4:52 pm on December 17, 2020: none

    Sure @jonasschnelli

    MacOS Big Sur - Light Mode - Before PR: macos-light-before-pr

    MacOS Big Sur - Light Mode - After PR: macos-light-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?

  6. goums commented at 5:03 pm on December 17, 2020: none

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

  7. Sjors commented at 6:47 pm on December 17, 2020: member
    Concept ACK. Just one boolean to fix all the things!
  8. prusnak approved
  9. hebasto commented at 8:13 pm on December 26, 2020: member
    @goums IIUC, you use the latest Homebrew’s Qt, right?
  10. hebasto commented at 8:25 pm on December 26, 2020: member

    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?

  11. goums commented at 9:22 pm on January 4, 2021: none

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

  12. jarolrod commented at 9:09 pm on January 14, 2021: member

    ACK c2db537b030dfcbacaff54d1a2768055fc3309f8

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

    Master: Dark Mode Screenshots master-dark

    PR: Dark and Light Mode Screenshots pr-light-and-dark

  13. hebasto commented at 9:07 pm on February 22, 2021: member

    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.

  14. laanwj commented at 8:42 am on February 23, 2021: member

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

  15. goums commented at 3:36 pm on February 25, 2021: none

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

    0$ make deploy
1$ open Bitcoin-Qt.app
    • When the user has dark mode settings: image
    • When the user has light mode settings: image

    However, there is still some glitches on the TabWidget: image

    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.

  16. goums renamed this:
    qt: Colorize icons on macOS for Dark mode support
    qt: Support macOS Dark mode
    on Feb 25, 2021
  17. jarolrod commented at 3:50 pm on February 25, 2021: member

    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.

  18. hebasto commented at 4:17 pm on February 25, 2021: member

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

  19. hebasto commented at 4:33 pm on February 25, 2021: member

    However, there is still some glitches on the TabWidget:

    That is orthogonal to this PR changes.

    See QTBUG-86513

  20. hebasto commented at 5:29 pm on February 25, 2021: member
    Tested 92369a176c99ff779ed809a99a2e12b79cbb20b0 with depends – no visual changes as expected.
  21. hebasto commented at 5:42 pm on February 25, 2021: member

    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.

  22. Allow icon colorization on mac os to better support dark mode 78f75a2d60
  23. allow darkmode on macos build 303cfc6227
  24. goums force-pushed on Feb 25, 2021
  25. goums commented at 9:32 pm on February 25, 2021: none

    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:

  26. hebasto approved
  27. hebasto commented at 10:31 pm on February 25, 2021: member
    ~ACK 303cfc62277efccf242dc5a51368e349dc2782e3~ retracted
  28. fanquake commented at 3:08 am on February 26, 2021: member

    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:

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

  29. hebasto commented at 8:46 pm on February 26, 2021: member

    ACK 303cfc6

    I retract my ACK.

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

    Screenshot from 2021-02-26 22-43-31

    UPDATE: probably these changes need to be documented properly?

  30. goums commented at 10:56 pm on February 26, 2021: none

    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

  31. goums commented at 11:27 pm on February 26, 2021: none

    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.

  32. jarolrod commented at 4:22 am on February 27, 2021: member

    @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): Screen Shot 2021-02-26 at 11 17 27 PM

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

  33. hebasto commented at 4:58 pm on February 27, 2021: member

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

    Screenshot from 2021-02-27 18-56-47

  34. hebasto commented at 5:04 pm on February 27, 2021: member
    Maybe document (in release notes?) that switching appearance Light/Dark while Bitcoin Core running is not supported for macOS Mojave?
  35. hebasto commented at 2:56 am on March 22, 2021: member

    @goums

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

  36. hebasto added the label macOS on Mar 22, 2021
  37. hebasto commented at 5:07 am on March 24, 2021: member

    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:

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

    ?

  38. fanquake commented at 6:58 am on March 24, 2021: member
    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.
  39. hebasto commented at 1:00 pm on March 24, 2021: member

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

  40. jarolrod commented at 1:29 am on March 25, 2021: member

    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.

    0-From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no
1-longer supported. Additionally, the appearance switching (light/dark) is
2-not supported for maOS 10.14.
3
4+From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no
5+longer supported. Additionally, switching between light and dark mode is
6+not supported on macOS 10.14.
  41. fanquake commented at 2:33 am on March 25, 2021: member

    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.

  42. hebasto commented at 2:46 am on March 25, 2021: member

    @goums

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

  43. hebasto added the label Waiting for author on Mar 25, 2021
  44. goums commented at 8:54 am on March 25, 2021: none

    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

  45. hebasto removed the label Waiting for author on Mar 25, 2021
  46. jarolrod commented at 1:25 pm on March 25, 2021: member

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

    0diff --git a/doc/release-notes.md b/doc/release-notes.md
1@@ -55,2 +55 @@ From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no
2+longer supported.
3^---- 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

  47. in doc/release-notes.md:55 in 02223fc8de outdated 
    51@@ -52,8 +52,7 @@ frequently tested on them.  It is not recommended to use Bitcoin Core on
52 unsupported systems.
53 
54 From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no
55-longer supported. Additionally, Bitcoin Core does not yet change appearance
56-when macOS "dark mode" is activated.
57+longer supported.
    hebasto commented at 1:27 pm on March 25, 2021:

    removing trailing space is enough :)

    0longer supported.
  48. jarolrod commented at 2:07 pm on March 25, 2021: member
    Don’t want to start with any bikeshedding but 56ed4ce87aa1704b2db16d71dbb69f1a8aa29a46 should be squashed onto 02223fc8de736b552c554e5bb9ecb884dd74c4c2, almost there 🥃
  49. hebasto approved
  50. hebasto commented at 2:11 pm on March 25, 2021: member

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

    • macOS Mojave 10.14.6 (18G8022):

    Screenshot from 2021-03-25 16-02-54

    • macOS Catalina 10.15.7 (19H524):

    Screenshot from 2021-03-25 15-54-22

    • macOS Big Sur 11.2.3 (20D91):

    Screenshot from 2021-03-25 15-43-42

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

  51. in doc/release-notes.md:54 in 56ed4ce87a outdated 
    50@@ -51,9 +51,7 @@ Core should also work on most other Unix-like systems but is not as
51 frequently tested on them.  It is not recommended to use Bitcoin Core on
52 unsupported systems.
53 
54-From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no
55-longer supported. Additionally, Bitcoin Core does not yet change appearance
56-when macOS "dark mode" is activated.
57+From Bitcoin Core 0.22.0 onwards, macOS versions earlier than 10.14 are no longer supported.
    hebasto commented at 2:13 pm on March 25, 2021:

    (see https://github.com/bitcoin/bitcoin/pull/20223)

    0From Bitcoin Core 22.0 onwards, macOS versions earlier than 10.14 are no longer supported.
  52. remove incompatibility release note for darkmode on macos dc4551c22c
  53. goums force-pushed on Mar 25, 2021
  54. goums commented at 2:33 pm on March 25, 2021: none
    ok, sorry about that ^^ git commits squashed into: dc4551c22c46104d18e8e7a8bf133cbebe4bc89f
  55. hebasto approved
  56. hebasto commented at 2:36 pm on March 25, 2021: member
    re-ACK dc4551c22c46104d18e8e7a8bf133cbebe4bc89f
  57. jarolrod commented at 2:36 pm on March 25, 2021: member
    ACK dc4551c22c46104d18e8e7a8bf133cbebe4bc89f
  58. MarcoFalke commented at 7:36 am on March 29, 2021: contributor
    @fanquake Anything left to do here?
  59. fanquake commented at 8:34 am on March 29, 2021: member

    Anything left to do here?

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

  60. MarcoFalke merged this on Mar 29, 2021
  61. MarcoFalke closed this on Mar 29, 2021
  62. sidhujag referenced this in commit a0c1fc775e on Mar 29, 2021
  64. hebasto commented at 11:38 am on May 27, 2021: member

    @goums

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

  65. gwillen referenced this in commit 256ce55913 on Jun 1, 2022
  66. bitcoin-core locked this on Aug 16, 2022


goums jonasschnelli Sjors prusnak hebasto jarolrod laanwj fanquake MarcoFalke

Labels
macOS

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 12:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me