Add system tray icon for "Toggle Window"

[wolframroesler]

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

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]

[droidmonkey]

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

[wolframroesler]

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:

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.

[droidmonkey]

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]

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]

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]

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]

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

[wolframroesler]

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

You mean this one?

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]

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]

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

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

[droidmonkey]

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]

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]

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

[wolframroesler]

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

We are. Another commit is coming up shortly.

[wolframroesler]

Here's how it looks on my machine.

[droidmonkey]

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