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

Icons for "New", "Open" and "Save" are the same in KeepassXC #1294

Closed
marcbone opened this issue Mar 26, 2019 · 55 comments · Fixed by #1600
Closed

Icons for "New", "Open" and "Save" are the same in KeepassXC #1294

marcbone opened this issue Mar 26, 2019 · 55 comments · Fixed by #1600

Comments

@marcbone
Copy link

I think this issue actually belongs here.

Expected Behavior

In KeepassXC the icons for New, Open and Save should be different like in the adwaita theme

Actual Behavior

The icons are the same

Specifications

name: communitheme
summary: The next Ubuntu theme built by the community.
publisher: Didier Roche (didrocks)
license: unset
description: |
Yaru, formerly known as Communitheme, is the new Ubuntu theme built by the
community. Yaru will become the default Ubuntu theme in Ubuntu 18.10. This
package allows you to try out the theme on Ubuntu 18.04 LTS.

To try out the theme, install this package on Ubuntu 18.04 LTS, restart
your computer and select the "Ubuntu with communitheme snap" session from
the login screen.

More information is available at
https://community.ubuntu.com/t/faq-ubuntu-new-theme/1930.
snap-id: Yd6CISPIf6tEf3ZEJ0cqSoEg9rG2VkRi
tracking: stable
refresh-date: 13 days ago, at 19:41 CET
channels:
stable: 0.1 2019-03-13 (1768) 16MB -
candidate: ↑
beta: ↑
edge: 0.1 2019-03-22 (1775) 16MB -
installed: 0.1 (1768) 16MB -

@CDrummond
Copy link
Contributor

Yeah, I noticed this and mentioned in issue #831 As stated there, I believe the issue is that Yaru has a document icon, but no document-new and document-save icons. So, when Qt looks for document-new it can't find it and fallsback to document

@clobrano
Copy link
Member

clobrano commented Mar 27, 2019

Qt looks for document-new it can't find it and fallsback to document

how does this work? Yaru iconset inherits from Humanity and hicolor, so if an icon is not provided directly it should be given by one of the above icon sets

EDIT: now we also have document-new, but it's a symbolic icon

@CDrummond
Copy link
Contributor

To be honest, I don't really know. But seeing as the Yaru icon set has document I think Qt just uses that - ie. finds the closest matching icon within the current set. But, I could be wrong :-)

@clobrano
Copy link
Member

I looked at the code. The application actually looks for document-new while we have document-new-symbolic. If I change the name, removing the -symbolic part, it works. Preparing a fix, but maybe it'd be good to infor Kepassxc that the naming convention is changing in adwaita as well

clobrano added a commit that referenced this issue Mar 29, 2019
Some applications might use the (I guess) legacy names for the symbolic
icons document-new, document-open, document-edit. In yaru those files
are named document-new-symbolic, document-open-symbolic,
document-edit-symbolic (which is the new name convention)
and the application (see keepassxc) might not find them.

Adding new symlinks in symbolic/actions.list for fixing it

closes #1294
@clobrano
Copy link
Member

Transmission has the same use case, but it works fine

image

@chrisjbillington
Copy link
Contributor

Also seeing this in tortoisehg - it displays a document icon instead of a save icon, because it is looking for 'document-save'.

@chrisjbillington
Copy link
Contributor

Qt doesn't seem to know about fallback themes in this case:

import sys
from PyQt5 import QtWidgets, QtGui

app = QtWidgets.QApplication(sys.argv)

icon = QtGui.QIcon.fromTheme('document-save')

print("actual icon name:", repr(icon.name()))
print("theme name:", repr(icon.themeName()))
print("fallback theme name:", repr(icon.fallbackThemeName()))

button = QtWidgets.QToolButton()
button.resize(200, 200)
button.setIcon(icon)
button.show()
app.exec()

prints:

actual icon name: 'document'
theme name: 'Yaru'
fallback theme name: ''

and shows the document icon:
image

@CDrummond
Copy link
Contributor

@chrisjbillington is this not how the icon themes are supposed to work? I thought the convention was to strip off the trailing "-something" until a match was found. So that an icon set could have (e.g.) "input-mouse" which would be used if "input-mouse-usb" was not found.

See https://standards.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html this mentions:

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.

Seeing as "document" exists, but not "document-new", then "document" is being used - as its the more generic term, and in the current icon theme.

@chrisjbillington
Copy link
Contributor

@CDrummond, ah, I see! That makes sense. I had no clue how it was supposed to work and was just reported how it appears to be working. Good to know Qt appears to be following spec (even though the outcome might be not ideal in this case).

@leoheck
Copy link

leoheck commented Jul 31, 2019

Hi guys, what is the status of this issue?
Do you have any workaround for that? Just let me know cause I want to apply here, please.
I am really interested in fix this #1361

@clobrano
Copy link
Member

clobrano commented Aug 1, 2019

Hi @leoheck,

we didn't go deeper on this actually. I should be able to try again today with a VM

@leoheck
Copy link

leoheck commented Aug 1, 2019

@clobrano what do you want to try? Why you need a VM?
What should I do to help to move this forward? I thought it was just creating some symbolic links, isn't it?

@clobrano
Copy link
Member

clobrano commented Aug 1, 2019

See this for example

Transmission has the same use case, but it works fine

image

Icon sets have a "fallback" theme, for providing a wider range of icons, Yaru uses Humanity which should have document-new, and according to @chrisjbillington prototype above, the API QtGui.QIcon.fromTheme is expected to look for the fallback theme https://srinikom.github.io/pyside-docs/PySide/QtGui/QIcon.html#PySide.QtGui.PySide.QtGui.QIcon.fromTheme

I still wonder why it could not find it and of course what we can do to support it. If you have any idea of solution or testing, I will be glad to discuss it :)

@clobrano
Copy link
Member

clobrano commented Aug 1, 2019

Qt doesn't seem to know about fallback themes in this case:

import sys
from PyQt5 import QtWidgets, QtGui

app = QtWidgets.QApplication(sys.argv)

icon = QtGui.QIcon.fromTheme('document-save')

print("actual icon name:", repr(icon.name()))
print("theme name:", repr(icon.themeName()))
print("fallback theme name:", repr(icon.fallbackThemeName()))

button = QtWidgets.QToolButton()
button.resize(200, 200)
button.setIcon(icon)
button.show()
app.exec()

prints:

actual icon name: 'document'
theme name: 'Yaru'
fallback theme name: ''

and shows the document icon:
image

hi @chrisjbillington, with which pyqt5 version you tested this code? I installed the latest from pip (version 5.13.0) but it gives me errors like QIcon does not have fallbackThemeName attribute and the exec function is actually exec_

EDIT: nevermind, it actually works fine in 19.04, sorry

@leoheck
Copy link

leoheck commented Aug 1, 2019

I see. I tried many things here to test if how to fix this fallback issue but none of them was good.
So, I fixed it with this workaround since now since I dont want to wait more time for a fix.

sudo ln -fs /usr/share/icons/Humanity/actions/48/document-new.svg  /usr/share/icons/Yaru/scalable/actions/
sudo ln -fs /usr/share/icons/Humanity/actions/48/document-open.svg /usr/share/icons/Yaru/scalable/actions/
sudo ln -fs /usr/share/icons/Humanity/actions/48/document-save.svg /usr/share/icons/Yaru/scalable/actions/
sudo ln -fs /usr/share/icons/Humanity/actions/48/document-print.svg /usr/share/icons/Yaru/scalable/actions/

@Feichtmeier
Copy link
Member

Maybe we have an icon that is symlinked to all these icons
document-new.svg
document-open.svg
document-save.svg
document-print.svg
?

@Feichtmeier
Copy link
Member

@clobrano
Copy link
Member

clobrano commented Aug 1, 2019

The actual icon name found is "document", so Qt actually selects the "document" image before looking for an alternative in the fallback themes (which are used only if no match is found).
I think that, as per CDrummond comment, we are forced to provide the icons for:

document-new.svg
document-open.svg
document-save.svg
document-print.svg

EDIT: I am not sure we can safely symlink icons from the fallback theme, so @leoheck solution is valid only if made manually in its own system.

\cc @ubuntujaggers :)

@leoheck
Copy link

leoheck commented Aug 1, 2019

I guess numix does the same with symlinks, I found some there but I am not sure if they were related with these files. We just have to find an icon them with this working so we can ask/or see what are the differences.

I didn't have time to test hundred things this morning, but Freecad works with the default Adwaita theme. It is just a matter of comparing folders/files. The only issue I can see is the folder structure that slows down this process.

https://github.com/numixproject/numix-icon-theme

@Feichtmeier
Copy link
Member

Feichtmeier commented Aug 1, 2019

No, it's exactly what @clobrano said. We need that icons . The thing is: full color action icons where never intended to be part of Suru.
Since we no longer follow the development of Suru, we can for sure pic up that shitload of work. But for modern gnome apps, those full color action icons are not necessary.
Yet I understand that for those applications which use that legacy toolbars it's relevant.
But those 3 icons are just a tiny potion of the real work, yet stuart could for sure make those 3 icons and we could close this issue... But KDE Apps are really not high on our priority list

@Amr-Ibra
Copy link

Any news here?
I'm also affected by this, using the Qt apps: Kid3-qt and Octave.

@Amr-Ibra
Copy link

Many Qt apps appear broken in Yaru because of this issue.

@ubuntujaggers
Copy link
Contributor

Hi @Amr-Ibn, the icons render at 22px, so a multiple of 24 wouldn't scale sharply to 22px (which is important for a small button with details like a little "+"). If the icons can be displayed at 44px then we do need to export it at double size. If the icons can be rendered at multiple different sizes then we need multiple icons.

It may be the new folder didn't follow the right naming convention - I'll copy Adwaita exactly and see if that works, thanks for the tip :)

@ubuntujaggers
Copy link
Contributor

Okay, looks like Adwaita has these icons at 22px plus all the usual sizes.

Step one: I'll see if I can get the folders to work.

Step two: I'll draw the other sizes.

@ubuntujaggers
Copy link
Contributor

ubuntujaggers commented Oct 23, 2019

Okay, copying the 22x22/legacy folder structure doesn't work :(

Changing the icon size in Ubuntu doesn't seem to make these buttons render at different sizes. Nor does using the accessibility zoom feature (it just zooms in on the existing image assets, rather than adapting the UI to use any higher res assets that might be available).

Given that these are legacy icons, my suggestion is to only target the default 22px button size (presuming this is the same regardless of monitor size - but we can test that when I push). However, I will export the icons at 176px ( = 8x size) so they don't become blocky if there are edge cases where the UI renders them larger than 22px. They won't be 100% sharp in these cases (if they even exist) but at least there won't be visible pixellation.

@ubuntujaggers
Copy link
Contributor

Okay, frustratingly, that won't work. If I export a high res png for these buttons, Keepass scales it down to 22px (which is the desired behaviour) but Inkscape shows it at 100%, leading to a comically enormous button. So, really the only safe thing to do is to have a 22px button at 22px and then create the other sizes if they're ever used.

@clobrano
Copy link
Member

Thanks @ubuntujaggers, it seems good to me :)
I can't test right now, but I kinda remember to have found this icons also in some "action" folder, is that possible?

@ubuntujaggers
Copy link
Contributor

That was my first attempt, but there was no actions folder under full colour so I had to make it :(

Basically: if I make any new folders in the Yaru structure, the icons in them don't get used. But if I put them in existing folders they do.

@clobrano
Copy link
Member

Sorry for the silly question, but is there any list of "folders to be installed" that might need to be updated when you create a new one?

@ubuntujaggers
Copy link
Contributor

Good point, I'll see if I can find one 👍

@chrisjbillington
Copy link
Contributor

For what it's worth here in tortoisehg we've got document-save in 16x16:

image

@ubuntujaggers
Copy link
Contributor

Aha, good tip. Then we need to work out how to make new folders and do this properly.

@Feichtmeier
Copy link
Member

Should be fixed by #1600 (even though #1600 produced #1610 )

Could anyone confirm that it is fixed?

@chrisjbillington
Copy link
Contributor

I can confirm the icon in tortoisehg has been fixed.

@clobrano
Copy link
Member

clobrano commented Nov 7, 2019

Thanks!

@clobrano clobrano closed this as completed Nov 7, 2019
@amavlyanov
Copy link

amavlyanov commented Nov 8, 2019

Any chance it'll be ported to 19.04 / 19.10?

@clobrano
Copy link
Member

clobrano commented Nov 8, 2019

19.10 for sure

@mitya57
Copy link

mitya57 commented Dec 24, 2019

19.10 for sure

@clobrano Do you have any update for 19.10 backport? If you push the commits somewhere I can sponsor the upload for you. Though per the release policy, first we need to get this fixed in Focal.

Here is a downstream bug for this. To close that bug completely, a backport of #1639 would be nice too.

@leoheck
Copy link

leoheck commented Dec 25, 2019

@mitya57 This issue was fixed for me, I am using 19.10.

Look, Transmission and Freecad

image

@mitya57
Copy link

mitya57 commented Dec 25, 2019

Hmm, maybe this was fixed in snaps? I meant the deb package from Ubuntu repository.

@clobrano
Copy link
Member

There isn't any official release yet, but both snap and ppa have the update.

@mitya57, it's usually the desktop team that tell us that the package is about to be updated, then we just tag a commit here so that they can sign it and use it

@mitya57
Copy link

mitya57 commented Dec 27, 2019

I am not a member of desktop team, but I'm Qt maintainer in Ubuntu. If you tag some commit, I will happily upload it.

Or if you want Ack from desktop team first, I can ask on IRC.

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