-
Notifications
You must be signed in to change notification settings - Fork 269
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
Support runtime appearance adjustment on macOS #275
Conversation
Concept ACK 3696ebc Wow! Looks great! MacOS 10.14.6 (18G8022) Light ThemeMacOS 10.14.6 (18G8022) Dark Theme |
Leaving it as is because have no nice design solution for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Spent all day playing around with this and have some findings. These are the main points summarized:
The State of macOS Dark Mode Support:
- This PR does allow for runtime appearance adjustment
- Dark mode does not work when compiled through depends natively on macOS 11 and 10.15. It does work on macOS 10.14 (out of scope of this PR)
- The Linux cross-compiles work, this is important as the release binaries are cross-compiled from linux
- There is a weird bug with rows in a table, they have their borders colored in white when in dark mode.
- The macOS tool bump would break dark mode again (out of scope of this PR)
What this PR should fix:
- Should fix the bug with tables when cross-compiled from linux
Below are pictures of this PR [natively compiled, native depends, cross-compile, cross-compile with macOS toolchain bump (19817)] on macOS [10.14, 10.15, 11]
macOS 10.14 Mojave
Native Compile
Start: Dark Mode | Switch: Light Mode |
---|---|
Native Depends Build
Start: Dark Mode | Switch: Light Mode |
---|---|
Linux Cross-Compile for macOS
Start: Dark Mode | Switch: Light Mode |
---|---|
This PR + macOS Toolchain Bump
Start: Dark Mode | Switch: Light Mode |
---|---|
macOS 10.15 Catalina
Native Compile
Start: Dark Mode | Switch: Light Mode |
---|---|
Native Depends Build
Start: Dark Mode | Switch: Light Mode |
---|---|
Linux Cross-Compile for macOS
Start: Dark Mode | Switch: Light Mode |
---|---|
This PR + macOS Toolchain Bump
Start: Dark Mode | Switch: Light Mode |
---|---|
macOS 11 Big Sur
Native Compile
Start: Dark Mode | Switch: Light Mode |
---|---|
Native Depends Build
Start: Dark Mode | Switch: Light Mode |
---|---|
Linux Cross-Compile for macOS
Start: Dark Mode | Switch: Light Mode |
---|---|
This PR + macOS Toolchain Bump
Start: Dark Mode | Switch: Light Mode |
---|---|
Accessibility is out of this PR scope, as its only goal is the smooth runtime switching between dark and light appearances (note that the dark appearance is supported since #154). I'll be happy to review dedicated prs that improves accessibility. Probably, it'd be nice to start from some kind of a design guide. |
Many thanks for your extensive testing and reviewing!
Sorry, still investigating this weird issue.
Is this our bug or Qt's one (upstream)? Is this bug of the switching between modes? Or it was present in #154? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quickly tested ACK 5737301. I've tested this rebased with bitcoin/bitcoin#21793.
This change is a prerequisite to support changeable appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
The ThemedLabel class correctly handles appearance changes on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
This change fixes the GUI when changing appearance on macOS.
Rebased 5737301 -> c231254 (pr275.01 -> pr275.02) on top of the merged bitcoin/bitcoin#21793 -- the dark mode works for every build :) |
Is this still true after the latest dependency bumps? |
Good catch! Removed. |
tACK c231254 on macOS 11.4 The code changes look sane, but I didn't study them in detail. This should also be tested on Linux and Windows, since not everything is wrapped in I tested with and without |
ACK c231254 Tested on mac OS Big Sur (11.2.3) Screen capture for those who are not testing on macOS: macos-light-dark-mode-switching.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK c231254
Tested over all possible configurations (as i did here: #275 (review)). Also tested on Linux and Windows to ensure there are no introduced visual glitches. This looks good to merge.
On Linux Mint 20.1 (Qt 5.12.8) with installed |
Tested ACK c231254 on macOS Big Sur arm64. |
On macOS switching appearance (Light -> Dark or Dark -> Light) when Bitcoin Core is running makes the GUI pretty unusable.
This bug is especially important when a user chose the "Auto" mode to adjust appearance automatically.
This PR fixes Bitcoin Core behavior.
This is an alternative to #268.