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

Fix theme issues on macOS #5482

Merged
merged 2 commits into from
Oct 15, 2020
Merged

Fix theme issues on macOS #5482

merged 2 commits into from
Oct 15, 2020

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Sep 28, 2020

Screenshots

See below comments

Testing strategy

Tested on Windows and macOS

Type of change

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

src/gui/styles/dark/DarkStyle.cpp Outdated Show resolved Hide resolved
src/gui/styles/dark/DarkStyle.cpp Show resolved Hide resolved
@droidmonkey droidmonkey force-pushed the hotfix/mac-theme branch 2 times, most recently from a1c479b to 3841706 Compare October 3, 2020 03:23
@droidmonkey
Copy link
Member Author

@phoerious take another look please

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The classic theme will probably have resolution issues with the standard icons again, but I think it's worth it. This gets rid of so much unnecessary code.

The theming code is a bit redundant. Transparency should also work without dark mode available and we only need to check at all if the OS theme doesn't match to avoid a dark toolbar in light mode and vice versa. So isDarkMode and hasDarkMode can probably be merged into one.

@droidmonkey
Copy link
Member Author

droidmonkey commented Oct 7, 2020

I just totally disagree with using Qt::transparent in general, but especially on macOS < 10.14. Splitting the isDarkMode and hasDarkMode is necessary to ensure Qt::transparent is not used ever on lower macOS versions. Also using transparent on the toolbar causes the extended toolbar (when text is shown) to have no background.

@droidmonkey
Copy link
Member Author

@phoerious I completely removed the transparent logic, there is absolutely no need for it. The colors defined blend perfectly with the title bar of the window. I cannot see any difference when using transparent versus the colors. Transparent brings a host of issues.

@droidmonkey droidmonkey dismissed phoerious’s stale review October 8, 2020 01:25

Removing transparent

@droidmonkey
Copy link
Member Author

droidmonkey commented Oct 8, 2020

image

image

BEFORE WITH TRANSPARENT
image

@phoerious
Copy link
Member

phoerious commented Oct 8, 2020

There is a seam, which you can see when you zoom in a little and the toolbar looks awkwardly flat. And I don't see why transparent should not ever be used on old macOS versions. The only thing you need to check is whether the theme matches our theme and if macOS is too old, you assume light. That's also something we need to build into the theme auto detection (which otherwise assumes dark iirc).

droidmonkey and others added 2 commits October 14, 2020 21:25
* Include new icons for toolbar overflow to ensure they are tinted correctly and fit in with the rest of the UI.
* Replace custom code for clearing line edits by including a proper icon for the default action.
* Fix #5025 - Change edit entry widget title separator to the common bullet character • (U+2022)
* Fix #5307 and Fix #5347 - Remove transparent toolbar/window on macOS and properly color text in toolbar.
@droidmonkey droidmonkey merged commit 389899e into release/2.6.2 Oct 15, 2020
@droidmonkey droidmonkey deleted the hotfix/mac-theme branch October 15, 2020 03:55
phoerious added a commit that referenced this pull request Oct 21, 2020
Added

- Add option to keep window always on top to view menu [#5542]
- Move show/hide usernames and passwords to view menu [#5542]
- Add command line options and environment variables for changing the config locations [#5452]
- Include TOTP settings in CSV import/export and add support for ISO datetimes [#5346]

Changed

- Mask sensitive information in command execution confirmation prompt [#5542]
- SSH Agent: Avoid shortcut conflict on macOS by changing "Add key" to Ctrl+H on all platforms [#5484]

Fixed

- Prevent data loss with drag and drop between databases [#5536]
- Fix crash when toggling Capslock rapidly [#5545]
- Don't mark URL references as invalid URL [#5380]
- Reset entry preview after search [#5483]
- Set Qt::Dialog flag on database open dialog [#5356]
- Fix sorting of database report columns [#5426]
- Fix IfDevice matching logic [#5344]
- Fix layout issues and a stray scrollbar appearing on top of the entry edit screen [#5424]
- Fix tabbing into the notes field [#5424]
- Fix password generator ignoring settings on load [#5340]
- Restore natural entry sort order on application load [#5438]
- Fix paperclip and TOTP columns not saving state [#5327]
- Enforce fixed password font in entry preview [#5454]
- Add scrollbar when new database wizard exceeds screen size [#5560]
- Do not mark database as modified when viewing Auto-Type associations [#5542]
- CLI: Fix two heap-use-after-free crashes [#5368,#5470]
- Browser: Fix key exchange not working with multiple simultaneous users on Windows [#5485]
- Browser: Fix entry retrieval when "only best matching" is enabled [#5316]
- Browser: Ignore recycle bin on KeePassHTTP migration [#5481]
- KeeShare: Fix import crash [#5542]
- macOS: Fix toolbar theming and breadcrumb display issues [#5482]
- macOS: Fix file dialog randomly closing [#5479]
- macOS: Fix being unable to select OPVault files for import [#5341]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants