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

Snap: fix theming #3057

Merged

Conversation

3 participants
@joshirio
Copy link
Contributor

commented Apr 22, 2019

Use gtk3 file chooser dialogs, mouse coursor theme if available and force fallback icon theme, fixes issue #2966

Type of change

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

Description and Context

This is the continuation of PR #3028, now rebased on branch release/2.4.2 and with a patch to force the fallback icon theme to address inconsistencies and missing icon actions.

To recap this PR:

  • uses native GTK3 file chooser dialogs for a better user experience (bookmarks, easier directory navigation, home as default opening folder instead of custom revision based snap directory)
  • uses the correct mouse cursor theme when available (ie. contained in gtk-common-themes snap)
  • forces the fallback icon theme (the same as on Windows and macOS) instead of the system icon theme that may miss icons for some actions like (database locking, password generation)

Screenshots

Ubuntu 18.04/19.04
u1804-1904

elementary OS
elementary

Mint:
mint

Kubuntu:
kubuntu

Zorin OS:
zorin

Testing strategy

Ubuntu 18.04/19-04, Linux Mint, Zorin OS, Kubuntu, elementary OS.

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]

joshirio and others added some commits Apr 22, 2019

Snap: fix theming
Use gtk3 file chooser dialogs, mouse coursor theme if available and force fallback icon theme, fixes issue #2966
@droidmonkey

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Instead of using a patch method (which I do not prefer), I added the function to the bootstrap method.

@droidmonkey droidmonkey self-requested a review Apr 22, 2019

@droidmonkey droidmonkey added this to the v2.4.2 milestone Apr 25, 2019

@droidmonkey droidmonkey changed the title Snap: fix theming (rebased) Snap: fix theming Apr 25, 2019

@droidmonkey droidmonkey merged commit a2caa31 into keepassxreboot:release/2.4.2 Apr 25, 2019

3 checks passed

MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@joshirio joshirio deleted the joshirio:snap-gtk3-integration branch Apr 25, 2019

droidmonkey added a commit that referenced this pull request May 31, 2019

Release 2.4.2
- Improve resilience against memory attacks - overwrite memory before free [#3020]
- Prevent infinite save loop when location is unavailable [#3026]
- Attempt to fix quitting application when shutdown or logout issued [#3199]
- Support merging database custom data [#3002]
- Fix opening URL's with non-http schemes [#3153]
- Fix data loss due to not reading all database attachments if duplicates exist [#3180]
- Fix entry context menu disabling when using keyboard navigation [#3199]
- Fix behaviors when canceling an entry edit [#3199]
- Fix processing of tray icon click and doubleclick [#3112]
- Update group in preview widget when focused [#3199]
- Prefer DuckDuckGo service over direct icon download (increases resolution) [#2996]
- Remove apply button in application settings [#3019]
- Use winqtdeploy on Windows to correct deployment issues [#3025]
- Don't mark entry edit as modified when attribute selection changes [#3041]
- Use console code page CP_UTF8 on Windows if supported [#3050]
- Snap: Fix locking database with session lock [#3046]
- Snap: Fix theming across Linux distributions [#3057]
- Snap: Use SNAP_USER_COMMON and SNAP_USER_DATA directories [#3131]
- KeeShare: Automatically enable WITH_XC_KEESHARE_SECURE if quazip is found [#3088]
- macOS: Fix toolbar text when in dark mode [#2998]
- macOS: Lock database on switching user [#3097]
- macOS: Fix global Auto-Type when the database is locked [#3138]
- Browser: Close popups when database is locked [#3093]
- Browser: Add tests [#3016]
- Browser: Don't create default group if custom group is enabled [#3127]
@psvenk

This comment has been minimized.

Copy link

commented Jun 4, 2019

I am sorry if I have posted this in the wrong place, and I will be happy to split this into a separate issue if GitHub does not provide tools to easily do that.

Is there a way to apply the fixes from this pull request to non-Snap packages? I am still experiencing this issue after updating to 2.4.2 (all of my packages are up-to-date), with multiple icon themes. I am using Arch Linux (with KeePassXC from the official repository) with the MATE desktop environment, and I tested with the following icon themes:

  • Bluecurve (my default, and the default on Fedora many years ago)
  • Adwaita (GNOME 3's default)
  • GNOME (GNOME 2's default)
  • Breeze (KDE Plasma's default)
  • Humanity (Ubuntu's default)

With each of these icon themes, with the exception of Adwaita, KeePassXC's icons were inconsistent or ambigous. I will now go in depth about the issues that I noticed.

  • In Bluecurve, GNOME 2, and Humanity, the "lock database" icon is a generic file icon instead of a lock icon.
  • In Bluecurve, Breeze, and Humanity, the "copy username", "copy password", and "copy URL" icons are not consistent.
  • In Humanity, the "search" icon is a computer icon instead of a magnifying glass.
  • In Bluecurve and Humanity, "copy password" and "password generator" use the same system icon.

It would be nice if there were a way to detect an icon theme (e.g. Breeze) and use a set of icons that looks more native to that icon theme. In the example of Breeze, it seems like most of the work is already done in the icon theme. Of course, I am not aware of how much work this would take, and I am not an icon designer. If this is not feasible, or as a stopgap solution, I would like KeePassXC to impose its own icon set for all themes so that the interface looks more consistent.

Here are some screenshots:

Bluecurve:
Bluecurve

Adwaita:
Adwaita

GNOME 2:
GNOME 2

Breeze:
Breeze

Humanity:
Humanity

@joshirio

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@psvenk I think this would be better tracked on a new issue specific to linux. The reason for this behavior can be found here.

The code on linux is actively looking for the icon theme. Usually Qt apps tend to ship custom icons (what keepassxc is already doing) and then load this directly from the resource system. On windows and mac there's no system icon theme concept so there the default behavior is to load the fallback and custom shipped icons (from the resource system), on linux because of all the theme integration, Qt looks for an appropriate icon from the default system icon theme, if none is found, a fallback icon is loaded just like on windows.

I think a quick workaround would be to use the snap icon patch for all linux (also for appimage) targets. But that's just a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.