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

Add system tray icon for "Toggle Window" #3244

Merged
merged 3 commits into from Jun 10, 2019

Conversation

@wolframroesler
Copy link
Contributor

wolframroesler commented Jun 8, 2019

Fixes #3145
The system tray menu used to have three items (toggle window,
lock database, quit) of which only two had an icon, which
looked strange and unintended. This commit adds an icon for
the "Toggle window" menu item.

Type of change

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

Screenshots

toggle-icon

Testing strategy

Tested manually on Linux.

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]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
Fixes #3145
The system tray menu used to have three items (toggle window,
lock database, quit) of which only two had an icon, which
looked strange and unintended. This commit adds an icon for
the "Toggle window" menu item.
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 8, 2019

While you're at it, are you able to figure out why the lock icon is using the document symbol?

@wolframroesler

This comment has been minimized.

Copy link
Contributor Author

wolframroesler commented Jun 8, 2019

While you're at it, are you able to figure out why the lock icon is using the document symbol?

Sort of. The icon is loaded by the following line (line 924 in src/gui/MainWindow.cpp):

m_ui->actionLockDatabases->setIcon(filePath()->icon("actions", "document-encrypt"));

Why is it "document-encrypt" when we're not encrypting a document but locking a database? It turns out that KeePassXC uses FilePath::icon to load the icon, which in turn invokes QIcon::fromTheme("document-encrypt"), which for some reason (beyond my understanding of theming, which is essentially absent) ignores the "-encrypt" part and returns a "document" icon.

Well, theming aside, the reason "document-encrypt" was used for the "lock database" icon in the first place probably was that there is an icon file in the share directory that displays a nice padlock, share/icons/application/*/actions/database-lock.png, which I suppose the original author wanted to use despite its name. Seems like theming just got in the way.

I implemented the following solution: I copied the database-lock.png icon (all three of them, in different resolutions) to database-lock.png and loaded that one for the Lock Database menu item:

    m_ui->actionLockDatabases->setIcon(filePath()->icon("actions", "database-lock"));

Now the padlock is displayed correctly:

lock-icon

Someone with more insight into the theming and icon philosophy of KeePassXC may decide if this a good solution. Another would be to turn theming off for the Lock Database icon, like this:

m_ui->actionLockDatabases->setIcon(filePath()->icon("actions", "document-encrypt", false));

which also works but which I don't really like.

Originally, icon `document-encryt.png` was used, however theming seems
to be getting in the way by sometimes displaying a plain "document"
icon instead. Copied the icon files to `database-lock.png` and used
that one for the "lock database" tray menu item instead.

Reference: #3244
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 8, 2019

sweet! "document-encrypt" never makes sense in any context. I think themes (or Qt) try to be smart and when they cannot find an explicit "document-encrypt" they fall back to "document". This occurs elsewhere in our icon library and needs to be corrected. See #3231

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 8, 2019

From the FreeDesktop.org docs:

The dash “-” character is used to separate levels of specificity in icon names, for all contexts other than MimeTypes. For instance, we use “input-mouse” as the generic item for all mouse devices, and we use “input-mouse-usb” for a USB mouse device. However, if the more specific item does not exist in the current theme, and does exist in a parent theme, the generic icon from the current theme is preferred, in order to keep consistent style.

@wolframroesler

This comment has been minimized.

Copy link
Contributor Author

wolframroesler commented Jun 8, 2019

Well, document-encrypt is used somewhere else so I introduced a new name instead of renaming the existing icon:

src/gui/dbsettings/DatabaseSettingsDialog.cpp:80:    m_ui->categoryList->addCategory(tr("Security"), FilePath::instance()->icon("actions", "document-encrypt"));

That line also feels a little fishy now.

As for the new name, do you think database-lock is a good choice? I originally named it simply lock but again theming kicked in and turned it into something like a "lock workstation" icon. Is there a list of icon names supported by the theming system, so we could pick one that definitely and across all platforms does what we want?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 8, 2019

database-lock is a perfect name. There is no standard icon for it so we can (and should) use a custom, but relevant, name.

@droidmonkey droidmonkey added this to the v2.5.0 milestone Jun 9, 2019
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 9, 2019

Please update the database settings dialog icon as well to database-lock

@wolframroesler

This comment has been minimized.

Copy link
Contributor Author

wolframroesler commented Jun 9, 2019

Please update the database settings dialog icon as well to database-lock

You mean this one?

security

Can do that easily, but are you sure we want to use identical icons for "set database security properties" and "lock database"? Note that the exact same icon would be visible on screen twice at the same time, each doing different things. Not that there was a more suitable icon to use for "Security", though ... and the padlock is still a lot better than the sheet-of-paper icon that's currently being displayed. Something like a shield would be nice IMHO, maybe https://www.iconfinder.com/icons/282458/security_shield_icon with the colours edited a bit. Your choice :)

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jun 9, 2019

The icon does not really need fixing. I hate to put the blame on you, but it's your icon set that is lacking this specific icon.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 9, 2019

Actually @wolframroesler you are correct. We should use the same icon that is used for security in the application settings.

image

This is FilePath::instance()->icon("status", "security-high")

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 9, 2019

The icon does not really need fixing. I hate to put the blame on you, but it's your icon set that is lacking this specific icon.

This may be true, but the root problem is we have used an icon that is meant for "documents" and when the icon is lacking the fallback is the document icon. That is not the correct fallback and is the result of using a poorly named icon in the first place.

@wolframroesler

This comment has been minimized.

Copy link
Contributor Author

wolframroesler commented Jun 9, 2019

Actually @wolframroesler you are correct. We should use the same icon that is used for security in the application settings.

Agreed. Wasn't aware of that one :) Will take care of it.

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jun 9, 2019

Is it? I thought we were using the "security-high" icon.

@wolframroesler

This comment has been minimized.

Copy link
Contributor Author

wolframroesler commented Jun 9, 2019

Is it? I thought we were using the "security-high" icon.

We are. Another commit is coming up shortly.

@wolframroesler

This comment has been minimized.

Copy link
Contributor Author

wolframroesler commented Jun 9, 2019

Here's how it looks on my machine.

dbsettings

Previously, the "document-encrypt" icon was used, which should be
something like a padlock but which, due to theming, somethings
fell back to a generic document icon (page of paper).

The "document-encrypt" icon is no longer used and was removed.
@droidmonkey droidmonkey merged commit 293ef35 into keepassxreboot:develop Jun 10, 2019
4 checks passed
4 checks passed
CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 10, 2019

Thank you for your contributions, would love to see some more PR's from you!

@wolframroesler wolframroesler deleted the wolframroesler:feature/trayicon-for-toggle branch Jun 10, 2019
phoerious added a commit that referenced this pull request Oct 26, 2019
Added

- Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)]
- Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)]
- Add offline user manual accessible via the 'Help' menu [[#3274](#3274)]
- Add support for importing 1Password OpVault files [[#2292](#2292)]
- Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)]
- Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)]
- Add group sorting feature [[#3282](#3282)]
- Add feature to download favicons for all entries at once [[#3169](#3169)]
- Add word case option to passphrase generator [[#3172](#3172)]
- Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)]
- Add UNIX man page for main program [[#3665](#3665)]
- Add 'Monospaced font' option to the notes field [[#3321](#3321)]
- Add support for key files in auto open [[#3504](#3504)]
- Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)]
- Complete usernames based on known usernames from other entries [[#3300](#3300)]
- Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)]
- Allow abbreviation of field names in entry search [[#3440](#3440)]
- Allow setting group icons recursively [[#3273](#3273)]
- Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)]
- Drop to background after copying a password to the clipboard [[#3253](#3253)]
- Add 'Lock databases' entry to tray icon menu [[#2896](#2896)]
- Add option to minimize window after unlocking [[#3439](#3439)]
- Add option to minimize window after opening a URL [[#3302](#3302)]
- Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)]
- Browser: Add initial support for multiple URLs [[#3558](#3558)]
- Browser: Add entry-specific browser integration settings [[#3444](#3444)]
- CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)]
- CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)]
- CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)]
- CLI: Add XML import [[#3572](#3572)]
- CLI: Add CSV export to the 'export' command [[#3278](#3278)]
- CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)]
- CLI: Add `--dry-run` option for merging databases [[#3254](#3254)]
- CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)].
- CLI: Add interactive shell mode command `open` [[#3224](#3224)]

Changed

- Redesign database unlock dialog [ [#3287](#3287)]
- Rework the entry preview panel [ [#3306](#3306)]
- Move notes to General tab on Group Preview Panel [[#3336](#3336)]
- Enable entry actions when editing an entry and cleanup entry context menu  [[#3641](#3641)]
- Improve detection of external database changes  [[#2389](#2389)]
- Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)]
- Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)]
- Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)]
- Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)]
- Merge custom data between databases only when necessary [[#3475](#3475)]
- Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)]
- Add 'New Entry' context menu when no entries are selected [[#3671](#3671)]
- Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)]
- Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)]
- Browser: Show database name when pairing with a new browser [[#3638](#3638)]
- Browser: Show URL in allow access dialog [[#3639](#3639)]
- CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)]
- CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)]
- CLI: Rename command `extract` to `export`. [[#3277](#3277)]

Fixed

- Improve accessibility for assistive technologies [[#3409](#3409)]
- Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)]
- Fix password generator issues with special characters [[#3303](#3303)]
- Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)]
- Fix password visibility toggle button state on unlock dialog [[#3312](#3312)]
- Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)]
- Fix hard-coded background color in search help popup [[#3001](#3001)]
- Fix font choice for password preview [[#3425](#3425)]
- Fix handling of read-only files when autosave is enabled [[#3408](#3408)]
- Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)]
- Enable HighDPI icon scaling on Linux [[#3332](#3332)]
- Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))]
- Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)]
- Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)]
- Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)]
- Fix crash when removing custom data [[#3508](#3508)]
- Fix placeholder resolution in URLs [[#3281](#3281)]
- Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)]
- Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)]
- Browser: Fix password entropy calculation [[#3107](#3107)]
- Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.