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

Allow to use the user/system theme icons for actions. #756

Closed
ghost opened this issue Jul 6, 2017 · 21 comments · Fixed by #2317
Closed

Allow to use the user/system theme icons for actions. #756

ghost opened this issue Jul 6, 2017 · 21 comments · Fixed by #2317

Comments

@ghost
Copy link

ghost commented Jul 6, 2017

Hi, i installed the .deb for the version 2.2.0 (i used to use the mono-based keepass2) and noticed some icons on the toolbar and menu used my user icon theme (breeze-dark) and others don't, for example the "document-new" icon uses the breeze theme but the "entry-new" don't. Then i downloaded the code just for curiosity and by deleting a "false" argument on the calls to the function that loads the icon i made all the action icons in that file to use my user/system theme. I just changed lines like:
m_ui->actionChangeMasterKey->setIcon(filePath()->icon("actions", "database-change-key", false));
to:
m_ui->actionChangeMasterKey->setIcon(filePath()->icon("actions", "database-change-key"));

Because the default KDE theme seems to cover well most if not all of the icons i changed i think it looks better now. So i mostly opened this bug to ask if there's any problem to allow the program to load the icons from the user theme at least on linux for future releases. Thanks in advance for your attention.

Before:
screenshot_20170706_183114

After:
screenshot_20170706_184646

This is the diff file:
icons.txt

KeePassXC - 2.2.0
Operating system: KDE Neon (Ubuntu 16.04)

@phoerious phoerious self-assigned this Jul 7, 2017
@phoerious phoerious added this to the v2.2.1 milestone Jul 7, 2017
@phoerious
Copy link
Member

phoerious commented Jul 7, 2017

I'm really not sure why KeePassXC is forcing custom icons for all those actions. Apparently some weird KeePassX legacy.
We generally need new icons, but using system icons where possible is of course a good idea, too. However, I'm not certain if the Breeze icons for "new entry", "edit entry" and "delete entry" are ideal. I believe our current icons are more or less custom-made icons which we took over from KeePassX. The rest are all Oxygen icons (i.e. KDE4), so it's no surprise the new KDE 5 Breeze theme covers most of them.

@ghost
Copy link
Author

ghost commented Jul 7, 2017

I see, thanks a lot for your answer. It's a matter of subjectivity, i personally like the breeze icons a lot more than the ones shipped with the program but i could understand if you guys prefer to use custom icons. Regarding the themes i guess if the user decides to use an icon theme with non-intuitive icons is up to that person, i guess in general the default themes from the main desktop environments should be sane. I personally would like to use the icons from my theme but it's up to you guys. Keep doing a great work!

@magkopian
Copy link
Contributor

magkopian commented Jul 7, 2017

I think there should be a setting so the user can decide whether they want to use the system icons, or the default icons included with the software. The reason for that, is because depending the icon theme you are using, some icons might not blend very well with the interface of the program or simply you might not like how your theme icons look on the interface of the program.

@phoerious
Copy link
Member

Made a few experiments. Kinda like it. For the password generator I used the Breeze equivalent of the Oxygen "roll" icon we are using at the moment.

screenshot_20170707_022134
screenshot_20170707_022109
screenshot_20170707_022122

@droidmonkey @TheZ3ro @weslly @louib What do you think? We could IMHO even replace the shipped icons with Breeze icons, so the default icons on non-KDE platforms look better out of the box. We would also solve HiDPI icon issues such as #548 then.

@ghost
Copy link
Author

ghost commented Jul 7, 2017

Yep, that's what i'm talking about. I like that a lot more.

@phoerious
Copy link
Member

phoerious commented Jul 7, 2017

BTW I'm against making it an option. Either we force users to use our icons or we rely on the system theme. Both are sane choices Having an option for that is rather silly.

@magkopian
Copy link
Contributor

magkopian commented Jul 7, 2017

@phoerious Relying on the system icon theme only is not a good idea in my opinion, because icons may be missing and this will result to a mixed icon interface which may look really bad, or like in my case cause confusion (I constantly mix up the save and open buttons because they look very similar).

If you believe that having an option for using either the system or the default icons is not a good idea, then I think the best would be to force the default icons of the program like you said. Which at least will guarantee that the interface is going to be consistent, instead of risking looking similar to what you see below.

keepassxc2

@droidmonkey
Copy link
Member

I like the breeze icons, but there are certainly other icon sets that are fantastic. Perhaps it would be wise to use different icons are Mac/Linux/Win since each platform has different ways to represent actions. Just thinking outside the box here. I am of the opinion that we could ship different icon themes that are all high quality but serve different system themes.

@ghost
Copy link
Author

ghost commented Jul 7, 2017

I consider the positive side of allowing the user/system icon theme is that it helps for the application to look integrated (or at least not alien) with the rest of the system but that's just my opinion of course.

@phoerious
Copy link
Member

phoerious commented Jul 7, 2017

@magkopian if you are having trouble distingusinging those icons you should probably change your icon theme. Icon names in Linux are more or less standardized and if we don't abuse icons for purposes they were not made for, we should get consistent results. Especially standard options such as save and open SHOULD IMHO adhere to platform standards and not come with looks the user isn't used to.

Therefore I also strongly agree with @droidmonkey upon using different icons for different platforms (if we can find appropriate open source themes or manage to use the system's native icons, e.g from shell.dll). If we really change the shipped default icons, we should defer this to 2.3.0, though.

@magkopian
Copy link
Contributor

@phoerious I'm using the Papirus icon theme which I believe is one of the most popular icon themes out there, and I really like how the application icons look in combination with the Arc theme on Gnome. The interface of KeePassXC is honestly the only thing I'm having issues with, so in my case I'd much rather use the default icons of the program than the system icons.

I believe that @droidmonkey has a point here though, shipping with different icon themes and allowing the user which one to choose is the best way to go in my opinion. This is also exactly what Filezilla does.

filezilla_theme

@weslly
Copy link
Contributor

weslly commented Jul 7, 2017

Made a few experiments. Kinda like it. For the password generator I used the Breeze equivalent of the Oxygen "roll" icon we are using at the moment.
screenshot_20170707_022134

@phoerious It looks so much better! Would be nice to replace the current shipped icons with those.

And while we are at it, the search icon should have an small v caret to indicate you can click it to change search settings.

@phoerious phoerious modified the milestones: v2.3.0, v2.2.1 Jul 7, 2017
@phoerious
Copy link
Member

phoerious commented Jul 7, 2017

I know there are apps which allow the user to change the icon theme. But I still find it kind of silly. Every other platform comes with standard icons, which apps are expected to use (especially macOS). Custom icons are only used for things for which there is no default icon or if the whole app somehow as a custom design (Adobe products, for instance). Why should that be different on Linux? The fact that the Papirus icons are barely recognizable is definitely a problem of that icon set and I really suggest you file a bug report there. I do agree that the save and open icon are rather bad whereas the rest looks decent. But those are standard icons and should serve as such. If they don't, the icon theme is bad.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Jul 7, 2017

My two cents here: I think we should ship one set of icon (the KDE-Qt default like we are doing right now) and let the OS replace all the icons if the user is using a different icon theme.
Shipping >1 icon set is just adding complexity to the application. I don't like the system/user-icon option neither

@phoerious
Copy link
Member

I don't think we actually need to ship more than one icon theme. We only need a more intelligent platform-specific icon lookup. On Linux it works out of the box on most systems, only macOS and Windows always fall back to our default icons. However, even those platforms come with standard icons which applications can use.

@weslly
Copy link
Contributor

weslly commented Jul 7, 2017

macOS standard icon set is actually quite limited https://developer.apple.com/macos/human-interface-guidelines/icons-and-images/system-icons/

@phoerious
Copy link
Member

Well, there is probably no New/Open/Save, because those actions tend to be in the menu without an icon. But there are other icons that are quite common and users expect them instead of something custom-made.

@hifi
Copy link
Member

hifi commented Jul 9, 2017

My opinion is that having a consistent icon theme between platforms helps users to navigate the program and keeps it familiar. Even more so when you mix them in your daily routine.

It's also easier to make or read instructions when the screenshots have the distinct icons everywhere.

As it has been said no platform provide enough platform specific icons that would be immediately understood by the user it would be better to stick with custom ones but not have unique set for each platform.

@k4r7u
Copy link

k4r7u commented Oct 6, 2017

I think hardcoded icons are actually a really bad idea (at least on a Linux system).
Maybe check for icons in the hicolor icons folder and if no are present use hardcoded fallback icons.
Or place the needed icons in the system icons folder during the installation like a lot other applications are doing it.
Below are screenshots of KeePassX with overwritten icons and KeePassXC on elementary OS.
keepassx
keepassxc

The same issue exists for me with the tray icon, which looks totally out of place.
Maybe provide an option for "power users/experts" to overwrite the icons in some way.
If adding an option to the settings menu is not wanted, what about a command line flag like --use-system-icons?
At the moment, while I prefer the advanced feature set of KeePassXC, not being able to modify the used icons is "forcing" me to still use KeePassX.

This is not meant as criticism as I totally honor the effort put into KeePassXC, but more as an idea to make KeePassXC more usable and better integrated in different distros.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Oct 6, 2017

Seems strange to me that KeePassX is working and KeePassXC isn't... I need to dive more into the code

@droidmonkey
Copy link
Member

The whole icon loading/use code needs an overhaul

@phoerious phoerious modified the milestones: v2.3.0, v2.4.0 Jan 28, 2018
fishman pushed a commit to fishman/keepassxc that referenced this issue Sep 22, 2018
KeepassXC tries to load the theme icon first and then falls back to the internal icon unless the check is explicitely disabled. Remove the check from most icons

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

Successfully merging a pull request may close this issue.

7 participants