Skip to content
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

Don't rely on AppleInterfaceStyle for theme switching #8615

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

opa334
Copy link
Contributor

@opa334 opa334 commented Oct 22, 2022

Fixes #7615

I discovered that in very specific conditions (when interface style is on automatic and the switch happens while the Mac is turned off, as outlined in the original issue), the AppleInterfaceStyle preference value in .GlobalPreferences is wrong. I worked around this issue by instead relying on the proper system API to get the appearance state.

Additionally there was a second issue, originally the code was registering for the AppleInterfaceThemeChangedNotification to know when the appearance changed, unfortunately this event fires before the system API returns the new appearance state, so I had to switch it to registering a KVO on the effectiveAppearance property as done in Apple sample code.

Screenshots

Before:
before

After:
after

Testing strategy

Reproduce the specific conditions outlined above and open the old build and the build with this change after another (you will see one looks wrong and one looks right). Instead of reproducing the conditions (very time consuming), you can also manually give the AppleInterfaceStyle key the wrong value: defaults write NSGlobalDomain AppleInterfaceStyle -string "<Dark/Light>". (Note that you have to use defaults, manually writing this key using a plist editor does not work).

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

…e key for dark mode detection, as it's not always correct
@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.28%. Comparing base (e3a3734) to head (a373df3).
Report is 354 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #8615   +/-   ##
========================================
  Coverage    64.28%   64.28%           
========================================
  Files          341      341           
  Lines        44330    44330           
========================================
+ Hits         28494    28496    +2     
+ Misses       15836    15834    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@droidmonkey
Copy link
Member

Has a bug report been submitted to Apple regarding their breaking their own api contract with the policy not changing values?

@opa334
Copy link
Contributor Author

opa334 commented Oct 22, 2022

Has a bug report been submitted to Apple regarding their breaking their own api contract with the policy not changing values?

Not yet, I'm not too familiar with how bug reporting to Apple works 😅. This change is still needed to work around the issue on older macOS versions either way.

@opa334
Copy link
Contributor Author

opa334 commented Oct 22, 2022

Well before reporting it, I made sure to test it on Ventura RC2 in a VM first and it seems like the issue has already been fixed, at least I wasn't able to reproduce it

src/gui/osutils/macutils/AppKitImpl.mm Outdated Show resolved Hide resolved
src/gui/osutils/macutils/AppKitImpl.mm Outdated Show resolved Hide resolved
src/gui/osutils/macutils/AppKitImpl.mm Outdated Show resolved Hide resolved
src/gui/osutils/macutils/AppKitImpl.mm Outdated Show resolved Hide resolved
@opa334
Copy link
Contributor Author

opa334 commented Oct 30, 2022

Applied the requested changes, let me know if there is anything else.

@opa334 opa334 requested a review from phoerious October 31, 2022 23:29
@opa334
Copy link
Contributor Author

opa334 commented Dec 19, 2022

Actually this is not fixed in Ventura, just wasn't able to replicate it in a VM I assume.

@droidmonkey droidmonkey changed the title Fix #7615 - Don't rely on AppleInterfaceStyle preference key for dark… Don't rely on AppleInterfaceStyle for theme switching Jan 14, 2023
@droidmonkey droidmonkey dismissed phoerious’s stale review January 30, 2023 01:13

Changes were made

@droidmonkey droidmonkey added this to the v2.7.5 milestone Jan 30, 2023
@droidmonkey droidmonkey merged commit 1e770e3 into keepassxreboot:develop Jan 30, 2023
dmaslenko pushed a commit to dmaslenko/keepassxc that referenced this pull request Jan 30, 2023
…#8615)

* Fix keepassxreboot#7615 - Don't rely on AppleInterfaceStyle preference key for dark mode detection, as it's not always correct
pull bot pushed a commit to tigerwill90/keepassxc that referenced this pull request Jan 30, 2023
…#8615)

* Fix keepassxreboot#7615 - Don't rely on AppleInterfaceStyle preference key for dark mode detection, as it's not always correct
pull bot pushed a commit to contropist/keepassxc that referenced this pull request Jan 30, 2023
…#8615)

* Fix keepassxreboot#7615 - Don't rely on AppleInterfaceStyle preference key for dark mode detection, as it's not always correct
droidmonkey pushed a commit that referenced this pull request Feb 18, 2023
* Fix #7615 - Don't rely on AppleInterfaceStyle preference key for dark mode detection, as it's not always correct
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Feb 18, 2023
Perlover added a commit to Perlover/keepassxc that referenced this pull request May 18, 2023
Release 2.7.5

- Add menu option to allow screenshots [keepassxreboot#8841]
- Add support for Botan 3 [keepassxreboot#9388]
- Increase max TOTP step to 24 hours [keepassxreboot#9149]
- Improve HTML export layout [keepassxreboot#8987]
- Turn search reset off by default [keepassxreboot#9153]
- Use QClipboard::clear() instead of setting blank text [keepassxreboot#9148]
- Hide group column header choice when not in search [keepassxreboot#9171]
- Improve look of KeePassXC logo and icons [keepassxreboot#9355]
- Add keyboard shortcuts for app and database settings [keepassxreboot#9007]
- Hide rename button from attachments preview panel [keepassxreboot#8842]
- Linux: Set SingleMainWindow in .desktop file [keepassxreboot#7430]

- Fix crash when search clears while creating new entry [keepassxreboot#9230]
- Fix crash when using Windows Hello in a Remote Desktop session [keepassxreboot#9006]
- Fix crash in Group Edit after enabling Browser Integration [keepassxreboot#8778]
- Fix canceling quick unlock when it is unavailable [keepassxreboot#9034]
- Set password input field font correctly [keepassxreboot#8732]
- Greatly improve performance when rendering entry view [keepassxreboot#9398]
- Fix various accessibility issues [keepassxreboot#9138]
- Fix arrows size when expand/collapse a group [keepassxreboot#9096]
- Select the clone instead of the original after cloning an entry [keepassxreboot#9070]
- Fix bugs with preview widget [keepassxreboot#9170]
- Fix status bar update when switching to other DB [keepassxreboot#9073]
- Fix database settings spin box bug [keepassxreboot#9101]
- Fix Ctrl+Tab shortcut to cycle databases in unlock dialog [keepassxreboot#8839]
- Fix TOTP QR code maintaining square ratio [keepassxreboot#9027]
- Fix Auto-Type configuration page on custom sequence selection [keepassxreboot#8752]
- Fix unexpected behavior of `--lock` when KeePassXC is not running [keepassxreboot#8889]
- Make open folder icon exempt from "Apply group icon to entry" [keepassxreboot#9205]
- Allow setting default file open directory with env var [keepassxreboot#9192]
- SSH Agent: Fix support for AES-256/GCM openssh keys [keepassxreboot#8968]
- Browser: Fix Native Messaging script path with BSD OS's [keepassxreboot#8835]
- MacOS: Fix text selection for Auto-Type clear field [keepassxreboot#9066]
- MacOS: Don't rely on AppleInterfaceStyle for theme switching [keepassxreboot#8615]
- Windows: Remove registry detection of desktop shortcut [keepassxreboot#9380]
@nobrowser
Copy link

By the way, #9423 also does happen to me on Big Sur. (Latest update: 11.7.7, intel arch, system theme is set to automatic).

@phoerious phoerious removed the bug label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: macOS pr: backported Pull request backported to previous release pr: bugfix Pull request that fixes a bug theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeePassXC doesn't properly switch between light and dark appearance on macOS
5 participants