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

Convert all application icons to Material Design #4066

Merged
merged 4 commits into from
Jan 11, 2020

Conversation

wolframroesler
Copy link
Contributor

Type of change

  • ✅ Refactor (significant modification to existing code)

... if you consider icon files to be "code". Only trivial changes to the C++ code were done.

Description and Context

Convert all application icons (open database, create entry, database settings, etc. etc.) to "Material Design". Icons are taken from the Material Design repo (https://github.com/Templarian/MaterialDesign).

That means that all application icons are SVG now, rather than PNG as before. All fixed-size application icons have been replaced.

Includes a shell script to easily re-create the icon files from the Material Design repo (to be used e. g. when the Material Design repo has undergone changes that we want to incorporate into KeePassXC).

Database icons (the "default icons" you chose for entries in your password database) are unchanged.

Fixes #475

Screenshots

Screenshot from 2019-12-27 15-26-04
Screenshot from 2019-12-27 15-26-28
Screenshot from 2019-12-27 15-26-43
Screenshot from 2019-12-27 15-26-52

Testing strategy

Tested manually. To facilitate testing, introduced a new environment variable that makes KeePassXC ignore the system icon theme and uses built-in icons only:

$ KEEPASSXC_IGNORE_ICON_THEME=1 keepassxc

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
Copy link
Member

Does this also completely fix fuzzy icons on HiDPI? That is another bug we have open.

@droidmonkey
Copy link
Member

droidmonkey commented Dec 27, 2019

You also need to update the COPYING file with the new sources for the icons.

@wolframroesler
Copy link
Contributor Author

Does this also completely fix fuzzy icons on HiDPI? That is another bug we have open.

@droidmonkey I hope so since we're using nothing but SVG now, however I can't try it out since I've got a regular monitor only.

@droidmonkey
Copy link
Member

I'll be able to test that

@louib
Copy link
Member

louib commented Dec 27, 2019

@wolframroesler really nice, thanks for that PR!

@droidmonkey
Copy link
Member

Similar to the group icons, can we use the non-filled in icons for the entry menu? It looks a little off balance.

@droidmonkey
Copy link
Member

droidmonkey commented Dec 28, 2019

The icons looks fuzzy on high DPI, perhaps we are missing a Qt setting somewhere.

image

image

@droidmonkey droidmonkey added pr: refactoring Pull request that refactors code user interface labels Dec 28, 2019
@droidmonkey droidmonkey added this to the v2.6.0 milestone Dec 28, 2019
@droidmonkey
Copy link
Member

droidmonkey commented Dec 28, 2019

Simple fix, we need to set AA_UseHighDpiPixmaps everywhere else now. Please remove the #ifdef Q_OS_LINUX guards in main.cpp, line 53-55.

image

wolframroesler added a commit to wolframroesler/keepassxc that referenced this pull request Dec 28, 2019
@wolframroesler
Copy link
Contributor Author

By the way, there's nothing we can do to avoid that "issue" found by CodeFactor (nothing that's easy and not too convoluted I mean). Is there a way to say "this is fine, just ignore it"? Or do we just live with the message?

@wolframroesler
Copy link
Contributor Author

Similar to the group icons, can we use the non-filled in icons for the entry menu? It looks a little off balance.

Most icons come with an "outline" version, which I'm now using consistently. Looks much better, less heavy.

toolbar

entries

@droidmonkey
Copy link
Member

I can ignore the code factor issue, no biggie

@phoerious phoerious added EPIC high-dpi ux and removed pr: refactoring Pull request that refactors code labels Dec 29, 2019
@varjolintu
Copy link
Member

Tried this briefly today with macOS High Sierra in a non-HiDPI enviroment. The icons look great, but sometimes it's hard to distinguish them from each other. Especially the View or Edit Entry and Delete Entry icons. Open Database icon is also a bit strange. So multiple times I noticed I had to see what the icon tooltip says (I hardly use the icons so I don't remember the exact position and function of them).

Use the following to run KeePassXC with the icons from the
source code, ignoring the operating system's Qt icon theme:

```
KEEPASSXC_IGNORE_ICON_THEME=1 keepassxc
```

The patch further adds a script `makeicons.sh` that re-creates KeePassXC
icons from the Material Design icon set and can be used for easily
updating icons in the future. Instructions are in the script.

Fixes keepassxreboot#475
for the "Toggle Window" menu item. It matches the other
(Material Design) icons much better than the colored
icon.

Fixes keepassxreboot#475
... not only on Linux, in order to prevent icons from being fuzzy.

Fixes keepassxreboot#475
It's used extremely rarely, having it in such a prominent position
in the tool bar isn't justified. Also, with the Material Design
icons, its tool bar icon can easily be confused with "create new
entry".

Fixes keepassxreboot#475
@phoerious phoerious merged commit 84e3925 into keepassxreboot:develop Jan 11, 2020
@droidmonkey
Copy link
Member

Nice squash job phoerious

@phoerious
Copy link
Member

Thank you. Here's a screenshot for you:

image

I noticed that the "Open" icon is not very suited for low-DPI screens, particularly the version in the "Database" menu, which is even smaller. Can we find something better and more in line with the pixel grid?

image

@droidmonkey
Copy link
Member

Perhaps it needs to be tweaked from the source?

@phoerious
Copy link
Member

The folder tab section looks blurry and needs to be adjusted to fit a regular raster.

@wolframroesler
Copy link
Contributor Author

Maybe it's related not to the icon (which is SVG) but to scaling? How about other icons, are they blurry as well?

Alignment is OK for me:

Screenshot from 2020-01-11 19 10 09

@phoerious
Copy link
Member

phoerious commented Jan 11, 2020

Quite possible. That's why I asked for PNGs to be provided with the correct resolution (for icons of which we do not have SVGs) instead of relying on Qt to scale them properly. Sometimes there is even a dedicated 16x16 version of an icon and a different one for 32x32 or 48x48 etc. I had hoped the SVG rendering works better, but maybe it doesn't and we also need to compile PNGs from them.

That being said, you can redesign SVG icons to improve low-res pixel rasterisation by aligning them with a grid, avoiding different stroke widths (like the one used for the folder tab), and lines with angles other than 90 or 45 degrees.

@wolframroesler
Copy link
Contributor Author

Hm, all application icons are SVG now, because that's all we get from the Material Design repo. Maybe it would help to pre-create PNGs in various resolutions with Image Magick or a similar tool and ship these instead of the SVGs. It would be quite easy to do that in a shell script.

How does that go with your Qt theme, @phoerious?

@phoerious
Copy link
Member

Wouldn't be any different, you only have to add the PNGs to the qrc file.

@wolframroesler
Copy link
Contributor Author

Working on a prototype, will submit a first version soon.

#475 is closed, is there a more suitable issue to reference in the new commits?

wolframroesler added a commit to wolframroesler/keepassxc that referenced this pull request Jan 11, 2020
This is an experimental commit that makes KeePassXC use fixed-size
PNG icons instead of relying on Qt's SVG scaling. Note that the
result is not completely satisfactory (and may be system-dependent).

It comes with a shell script (utils/makepng.sh) that converts the
SVG files to PNG. The script contains three implementations of the
SVG->PNG conversion routine, two with ImageMagick and one with
Inkscape. Instructions for use are in the script's header comment.

Fixes keepassxreboot#475/keepassxreboot#4066 (after some more work).
@wolframroesler
Copy link
Contributor Author

I wrote a shell script that converts the SVGs to PNGs in various resolutions. I'm not quite satisfied, however, because the result looks more blurry than the SVG version, at least on my system:

SVG version
with-svg

PNG version
with-png

The script contains three implementations of SVG-to-PNG conversions, none of which seem to work perfectly. However, judging if the PNGs are crisp or blurry pushes my eyesight (or my monitor, or both) to the limits. @droidmonkey maybe you can pull it and have a look yourself. I really hope we find a solution that doesn't require the icons to be hand-tailored for low resolutions.

And probably this should go into a new issue.

@droidmonkey
Copy link
Member

I am not a fan of prebaking PNG. If an SVG renders incorrectly, fix the SVG.

@wolframroesler
Copy link
Contributor Author

@phoerious

That being said, you can redesign SVG icons to improve low-res pixel rasterisation by aligning them with a grid, avoiding different stroke widths (like the one used for the folder tab), and lines with angles other than 90 or 45 degrees.

I'm afraid this is beyond my area of expertise. Maybe you can report it to the Material Design guys (https://github.com/Templarian/MaterialDesign)? They have been extremely responsive. The name of the icon in question is folder-open-outline.

@phoerious
Copy link
Member

phoerious commented Jan 12, 2020

@wolframroesler How do we handle dark themes and selected states of icons? In my PR #4110 I have an ugly intermediary solution where I convert the SVGs to 128x128 pixmaps and then recolour them a) for inverted icons in selection highlights and b) for dark themes (which I try to detect based on the standard palette).

I think there might be a more elegant way to achieve this which does not include creating fixed-size pixmaps nor the need for two different icon sets.

Also an interesting observation: Providing only 128x128 icons and then letting Qt do the rest works better than creating multiple sizes beforehand. Perhaps that's because some icons are not exactly 16x16 or so and scaling them to the correct size from 128 produces better results than scaling from the nearest already-low resolution, who knows.

@wolframroesler
Copy link
Contributor Author

@phoerious replying in #4110.

@OLLI-S
Copy link

OLLI-S commented Jan 18, 2020

@wolframroesler Regarding these icons in #4066 (comment):

image

Are these icons final?
The icons "Add", "Edit" and "Delete" have a "Speech Bubble" icon, but you are programming a password manager, not a chat client (I did not find any chat features yet 😉 ).

@sorairolake sorairolake mentioned this pull request Jan 22, 2020
droidmonkey added a commit that referenced this pull request Jul 7, 2020
Added

- Custom Light and Dark themes [#4110, #4769, #4791, #4796, #4892, #4915]
- Compact mode to use classic Group and Entry line height [#4910]
- View menu to quickly switch themes, compact mode, and toggle UI elements [#4910]
- Search for groups and scope search to matched groups [#4705]
- Save Database Backup feature [#4550]
- Sort entries by "natural order" and move lines up/down [#4357]
- Option to launch KeePassXC on system startup/login [#4675]
- Caps Lock warning on password input fields [#3646]
- Add "Size" column to entry view [#4588]
- Browser-like tab experience using Ctrl+[Num] (Alt+[Num] on Linux) [#4063, #4305]
- Password Generator: Define additional characters to choose from [#3876]
- Reports: Database password health check (offline) [#3993]
- Reports: HIBP online service to check for breached passwords [#4438]
- Auto-Type: DateTime placeholders [#4409]
- Browser: Show group name in results sent to browser extension [#4111]
- Browser: Ability to define a custom browser location (macOS and Linux only) [#4148]
- Browser: Ability to change root group UUID and inline edit connection ID [#4315, #4591]
- CLI: `db-info` command [#4231]
- CLI: Use wl-clipboard if xclip is not available (Linux) [#4323]
- CLI: Incorporate xclip into snap builds [#4697]
- SSH Agent: Key file path env substitution, SSH_AUTH_SOCK override, and connection test [#3769, #3801, #4545]
- SSH Agent: Context menu actions to add/remove keys [#4290]

Changed

- Complete replacement of default database icons [#4699]
- Complete replacement of application icons [#4066, #4161, #4203, #4411]
- Complete rewrite of documentation and manpages using Asciidoctor [#4937]
- Complete refactor of config files; separate between local and roaming [#4665]
- Complete refactor of browser integration and proxy code [#4680]
- Complete refactor of hardware key integration (YubiKey and OnlyKey) [#4584, #4843]
- Significantly improve performance when saving and opening databases [#4309, #4833]
- Remove read-only detection for database files [#4508]
- Overhaul of password fields and password generator [#4367]
- Replace instances of "Master Key" with "Database Credentials" [#4929]
- Change settings checkboxes to positive phrasing for consistency [#4715]
- Improve UX of using entry actions (focus fix) [#3893]
- Set expiration time to Now when enabling entry expiration [#4406]
- Always show "New Entry" in context menu [#4617]
- Issue warning before adding large attachments [#4651]
- Improve importing OPVault [#4630]
- Improve AutoOpen capability [#3901, #4752]
- Check for updates every 7 days even while still running [#4752]
- Improve Windows installer UI/UX [#4675]
- Improve config file handling of portable distribution [#4131, #4752]
- macOS: Hide dock icon when application is hidden to tray [#4782]
- Browser: Use unlock dialog to improve UX of opening a locked database [#3698]
- Browser: Improve database and entry settings experience [#4392, #4591]
- Browser: Improve confirm access dialog [#2143, #4660]
- KeeShare: Improve monitoring file changes of shares [#4720]
- CLI: Rename `create` command to `db-create` [#4231]
- CLI: Cleanup `db-create` options (`--set-key-file` and `--set-password`) [#4313]
- CLI: Use stderr for help text and password prompts [#4086, #4623]
- FdoSecrets: Display existing secret service process [#4128]

Fixed

- Fix changing focus around the main window using tab key [#4641]
- Fix search field clearing while still using the application [#4368]
- Improve search help widget displaying on macOS and Linux [#4236]
- Return keyboard focus after editing an entry [#4287]
- Reset database path after failed "Save As" [#4526]
- Use SHA256 Digest for Windows code signing [#4129]
- Improve handling of ccache when building [#4104, #4335]
- macOS: Properly re-hide application window after browser integration and Auto-Type usage [#4909]
- Auto-Type: Fix crash when performing on new entry [#4132]
- Browser: Send legacy HTTP settings to recycle bin [#4589]
- Browser: Fix merging browser keys [#4685]
- CLI: Fix encoding when exporting database [#3921]
- SSH Agent: Improve reliability and underlying code [#3833, #4256, #4549, #4595]
- FdoSecrets: Fix crash when editing settings before service is enabled [#4332]
@Saroumane
Copy link

Hello, my distro just upgraded to KeepassXC 2.6.x and I come here to see what happened.
I don't understand the rationale behind going from colorful application icons to monochrome icons.
As previous users have highlighted in this thread, it causes an extra effort to avoid confusion between monochrome icons.
That's why I think it's a step backwards in terms of usability.

Could you please provide a setting to restore colorful unambiguous applications icons ?

@phoerious
Copy link
Member

Not planned, so no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve application icons [$50]
7 participants