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

XDG icon specs implementation woes: Introduction to the missing icons #908

Open
Jajauma opened this issue May 14, 2016 · 29 comments
Open

XDG icon specs implementation woes: Introduction to the missing icons #908

Jajauma opened this issue May 14, 2016 · 29 comments
Assignees
Milestone

Comments

@Jajauma
Copy link
Contributor

Jajauma commented May 14, 2016

Judging by those parts of the Lua code which I have already got myself acquainted with, you maintain at least 3 (three!) competing facilities to map the icon names (possibly along with its screen size) into the file names on a disk. Let's introduce our players briefly:

  • icon_theme.find_icon_path() of lib/menubar/icon_theme.lua. Used solely to extract 10 (ten) paths to the categories icons (Office, Development, etc) in the menubar.
  • utils.lookup_icon() of lib/menubar/utils.lua. Used by the menubar implementation to extract the vast majority of application icons set in their respective *.desktop files.
  • util.geticonpath() of lib/awful/util.lua. Used by the naughty module to extract the full icon path using the icon name being received from a notification client over DBus.

They all three do work differently. Even worse they all three fail differently. This results in some application icons missing from the menubar and the icon missing from the notifications popup sent over DBus. I've tried switching them back and forth (using the menubar as a guinea pig) and from the three of those icon_theme.find_icon_path() being the most sophisticated seems to be a noticeable favorite as well. It was able to find most of the icons (with or without icon theme set in beautiful) compared to utils.lookup_icon(), albeit there are failures too (e.g. "nvidia-settings.png" - note the extension - could not be found while it is happily sitting under /usr/share/pixmaps).

Given that bestiary the real question reveals: is it worth to propose fixes for individual issues or is there already some undergoing black op plan of merging them/dropping them/adding 4th to the party/etc? From my personal point of view adding all the corner cases handling to the icon_theme.find_icon_path() and dropping (deprecating) the rest of a company would be the preferable and cleanest path but that might render a new problem - it is inherently slow. Recent making of the menubar asynchronous can mitigate it for some degree though.

Just in case you prefer leaving it all as it is I can open an issue for each and every missing icon I have, possibly along with a "fix".

@psychon
Copy link
Member

psychon commented May 15, 2016

I tried to stay out of this as much as possible...
I knew that the menubar had two similar implementations, but I wasn't aware that what naughty does is based on the same standard and somehow belongs to the rest.

Of course I'm all for "let's merge this into a single implementation". I'm not sure what's the best course of action here. Perhaps @actionless can say something?

@actionless
Copy link
Member

i wasn't really looking too much into desktop files parsing thing, mostly on multipage rendering, so unfortunately i don't have any strong opinion on that, but in general yes, it's usually better to generalize things, so i am voting up for merging that logic somehow together

@Jajauma
Copy link
Contributor Author

Jajauma commented May 15, 2016

Indeed, there are some nofitication clients known to request an icon by name. nm-applet for example requests its icons by their short name (nm-device-wired etc, see below, easily observable with dbus-monitor) hence notification clients on the wire are subject to the usual XDG-compatible mapping procedures so the user themes are working, legacy applications are working, and such.

Here is a test case regarding that nm-applet icon for all three implementations.

$ find /usr/share/icons/ -name 'nm-device-wired*'
/usr/share/icons/gnome/16x16/status/nm-device-wired.png
/usr/share/icons/gnome/22x22/status/nm-device-wired.png
/usr/share/icons/gnome/24x24/status/nm-device-wired.png
/usr/share/icons/gnome/256x256/status/nm-device-wired.png
/usr/share/icons/gnome/32x32/status/nm-device-wired.png
/usr/share/icons/gnome/48x48/status/nm-device-wired.png
/usr/share/icons/hicolor/16x16/apps/nm-device-wired.png
/usr/share/icons/hicolor/22x22/apps/nm-device-wired.png
/usr/share/icons/hicolor/32x32/apps/nm-device-wired.png
/usr/share/icons/hicolor/scalable/apps/nm-device-wired.svg

A test program (run it e.g. within vim as :!awesome-client <%):

local naughty = require("naughty")
local menubar_icon_theme = require("menubar.icon_theme")
local menubar_utils = require("menubar.utils")
local awful_util = require("awful.util")

naughty.notify({
    title = "Version 1/3",
    text = "Calling menubar.icon_theme():find_icon_path() ... ",
    icon = menubar_icon_theme():find_icon_path("nm-device-wired"),
    timeout = 0
})

naughty.notify({
    title = "Version 2/3",
    text = "Calling menubar.utils.lookup_icon() ...",
    icon = menubar_utils.lookup_icon("nm-device-wired"),
    timeout = 0
})

naughty.notify({
    title = "Version 3/3",
    text = "Calling awful.util.geticonpath() ...",
    icon = awful_util.geticonpath("nm-device-wired"),
    timeout = 0
})

And a result:
test3

Apparently the awful.util implemenation has failed to find anything at all. That's way the popup in #899 has no shiny icon when I plug an RJ45 in while it is actually supposed to be there.

@Elv13
Copy link
Member

Elv13 commented May 20, 2016

Just a quick note that I personally use GTK/GDK for this. I know @psychon wont allow a dependency on GTK because it cannot be trusted not to implement new features that might accidently deaklock Awesome when requiring GTK, but it give a very fast, asynchronous and compliant implementation. As a big plus, it also support thumbnails.

https://github.com/Elv13/awesome-configs/blob/master/utils/fd_async.lua#L410

@osleg
Copy link
Contributor

osleg commented May 20, 2016

If it's so slow (can't check right now and noone here provided benchmarks) than maybe it's better to use C for icons lookup?

@Elv13
Copy link
Member

Elv13 commented May 20, 2016

It's not slow because of Lua, it is slow because of all the I/O operations and inodes checks. KDE, for example, keep a service with some SHM memory to share this burden across all applications. I am not sure how GTK/GDK handle this, but they probably have some caching too.

@Jajauma
Copy link
Contributor Author

Jajauma commented May 20, 2016

@osleg: I'm sorry, there are no real benchmarks ATM, it is merely my personal opinion. I have been using the patched menubar which uses icon_theme.find_icon_path() implementation for several months and it took about 1,5-2 s for the menu just to pop up from the cold, while the original menubar using utils.lookup_icon() pops up instantly from the cold. I bet that was the primary reason to keep those two implementations under lib/menubar/ from the beginning, like "the fast (i.e. optimized) one" and "the robust (i.e. conformant) one". Since the menubar building process was recently made asynchronous the lag after hitting Mod+p has gone, and the (somewhat empty) menubar pops up instantly and is populated with the entries gradually, so the task of a lookup optimization is of low importance, if any, I guess.


As a sidenote regarding the language: I believe the verbatim implementation of the algorithm from the standard text (like the icon_theme.find_icon_path()) is doomed to be suboptimal because it introduces a heavy random non-uniform access process on an HDD, being repeated scores of hundreds times while being spread across the disk partitions (see those file masks in a tight loop). The standard text advises to walk the whole /icons/, ~/.icons directory trees at once and use the cached result for the file checking, thus evading the random access. The gtk icon theme cache binary files could be consulted first as well, but ... we cannot mmap(2) from Lua, right?

@Jajauma
Copy link
Contributor Author

Jajauma commented May 20, 2016

@Elv13: Thanks for the heads up, I'm going try first that external implementation and see if it works for me. It probably could serve us as a good reference.

@psychon
Copy link
Member

psychon commented May 23, 2016

Just a quick note that I personally use GTK/GDK for this. I know @psychon wont allow a dependency on GTK because it cannot be trusted not to implement new features that might accidently deaklock Awesome when requiring GTK, but it give a very fast, asynchronous and compliant implementation. As a big plus, it also support thumbnails.

https://github.com/Elv13/awesome-configs/blob/master/utils/fd_async.lua#L410

Sigh. Apparently we'll have to use GTK for this. This bug provides a good reason and my unspecific fear that something might go wrong is weaker than that...

If someone wants, feel free to make awesome use GTK. How/where do we call gtk_init? I think "the docs" say it has to be called before using GTK.

@actionless
Copy link
Member

also i remember you were mentioning what we can use some gtk function to have resizeable svg-s

@osleg
Copy link
Contributor

osleg commented May 25, 2016 via email

@psychon
Copy link
Member

psychon commented May 25, 2016

@osleg Sorry, but Awesome already depends on cairo, pango, gdk-pixbuf, xcb, xkbcommon, startup-notification, lua, xdg-basedir, dbus, glib and some other stuff. Also, I don't think that loading libgtk automatically makes something not-lightweight.
@actionless I think the "SVG stuff" is "more or less" in gdk-pixbuf, which we already use. I don't know if GTK adds anything "nice" here.

@actionless
Copy link
Member

ah, right, i found that discussion: #390 (comment)

@osleg
Copy link
Contributor

osleg commented May 25, 2016

@psychon, adding a non-lightweight toolkit making it non-light weight.
There is no much choices that are not using Qt or GTK, and awesome
is one of them, don't let it become just another "fuck i hate that shit" monster.

The issue is:

  1. Awesome have 3 different functions for fetching icons
  2. None of them is consitent
  3. They are slow since have no caching.

So basically all is required is to write an icon fetcher with cacher.... I know that's not a five minute task, but wouldn't it be better to add this functionality only instead of pulling an entire toolkit just for that?

@actionless
Copy link
Member

actionless commented May 25, 2016

There is no much choices that are not using Qt or GTK

are you able to run awesome without gtk?

UPD: my bad, lgi don't require it

@osleg
Copy link
Contributor

osleg commented May 25, 2016

$ apt-cache depends awesome
awesome
Depends: libc6
Depends: libcairo2
Depends: libdbus-1-3
Depends: libgdk-pixbuf2.0-0
Depends: libglib2.0-0
Depends: liblua5.1-0
Depends: libstartup-notification0
Depends: libx11-6
Depends: libxcb-cursor0
Depends: libxcb-icccm4
Depends: libxcb-keysyms1
Depends: libxcb-randr0
Depends: libxcb-shape0
Depends: libxcb-util1
Depends: libxcb-xinerama0
Depends: libxcb-xtest0
Depends: libxcb1
Depends: libxdg-basedir1
Depends: menu
Depends: dbus-x11
Depends: lua-lgi
Depends: gir1.2-freedesktop
Depends: gir1.2-pango-1.0
Recommends: x11-xserver-utils
Recommends: rlwrap
Recommends: feh
Yes I can

And I actually do it on RasPi
And I really don't want to pull GTK to not-really-computer
Anyway.... If you guys will decide to go with GTK - I'll just switch to something else.
But if you want to come with own solution (or at least some less bloated tool) - I'm up to help with all the free time i've got (plenty usually) :) Just point me where to start :)

@actionless
Copy link
Member

actionless commented Nov 24, 2016

is anybody going to take the decision which implementation of icon path routine to leave?

from this screenshot it seems what at least awful implementation of it is lacking: #908 (comment)

unless awesome support SVG scaling i would suggest modifying iconpath function to also receive "desired_size" argument and choose the icon which is the same or closest (but bigger) to the requested size and if that arg is not set then use some beautiful.default_icon_size

@psychon
Copy link
Member

psychon commented Nov 24, 2016

is anybody going to take the decision which implementation of icon path routine to leave?

Currently: No, doesn't look like it. Ultimately it does not really matter which of the routines is used, it only matters that only one remains and that it is standards compliant. I guess it should then end up being in gears? ;-)

unless awesome support SVG scaling i would suggest modifying iconpath function to also receive "desired_size" argument and choose the icon which is the same or closest (but bigger) to the requested size

Depends on what level of SVG support you want. "Load this SVG into a surface of size 42x30" is easily implementable (I think GdkPixbuf provides this functionality, and only this), but of course means that at loading-time we need to know which size we should end up with. Thus, desired_size is a good idea.

@actionless
Copy link
Member

actionless commented Nov 24, 2016

Depends on what level of SVG support you want. "Load this SVG into a surface of size 42x30" is easily implementable (I think GdkPixbuf provides this functionality, and only this), but of course means that at loading-time we need to know which size we should end up with. Thus, desired_size is a good idea.

i think if we would be able to do the first the we can implement the latter with some special type of widget. which will be reloading svg each time when widget geometry changes

@psychon
Copy link
Member

psychon commented Nov 25, 2016

Just if you want something to play with:
Load a file (not only a SVG) at a given size:

local lgi = require("lgi")
local GdkPixbuf = lgi.GdkPixbuf
local Gdk = lgi.Gdk
Gdk.init({}) -- Sigh :-(
local file = "/usr/share/weston/wayland.svg"
local _, width, height = GdkPixbuf.Pixbuf.get_file_info(file)
print("Native size is", width, height)
local surface = Gdk.cairo_surface_create_from_pixbuf(GdkPixbuf.Pixbuf.new_from_file_at_size(file, 300, 300), 1, nil)
print(surface, surface.status)

Edit: Nice, last time I looked this was harder to do. Gdk.cairo_surface_create_from_pixbuf must be new. I like it.

@Elv13
Copy link
Member

Elv13 commented Nov 25, 2016

Also, note that librsvg needs to be installed or bad things will happen

@psychon
Copy link
Member

psychon commented Nov 25, 2016

"Bad things" being GdkPixbuf telling you that it does not support the file format?

@Elv13
Copy link
Member

Elv13 commented Nov 25, 2016

I have seen crashes in the past in some GTK apps, but the apps were probably to blame. The autogen often forget to check for librsvg because it is implicit and Gentoo doesn't install it unless there is a dependency or an USE flag. So this is a common issue for Gentoo users. So "bad things" is "if you don't handle it, then it will eventually print a nil error"

@Veratil
Copy link
Contributor

Veratil commented Feb 13, 2017

Since this was mentioned in #1549, I took a look over the three:

Currently icon_theme.lua implements the freedesktop icon theme specification:

-- This implementation is based on the specifications:
--  Icon Theme Specification 0.12
--  http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-0.12.html

It does have some caching based on the theme name and the directories searched. There might be some room for improvement, but it's good.

menubar/utils.lua is very similar in the search pattern it takes, but doesn't search as exhaustively as icon_theme.lua does. It also implements caching based on the parameter passed to lookup_icon(). It almost appears that it tried to implement the icon theme specification, but very loosely.

awful/util.lua:geticonpath() just does a loop through the parameters (or defaults) and returns if found (doesn't even return nil if nothing found).

I'm for going only to the freedesktop standard (since it is a standard after).

Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jul 3, 2017
@Oblomov Oblomov mentioned this issue Jul 11, 2017
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 13, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 13, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 13, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 13, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 13, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 16, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 17, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 18, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 18, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 22, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 22, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Feb 25, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Feb 25, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue May 28, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue May 29, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Aug 6, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Aug 6, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Nov 7, 2018
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 4, 2019
Elv13 added a commit to Elv13/awesome-1 that referenced this issue Jan 4, 2019
@selurvedu
Copy link

util.geticonpath() of lib/awful/util.lua. Used by the naughty module to extract the full icon path using the icon name being received from a notification client over DBus.

Unfortunately, it only considers non-scalable icon sizes like 32x32, 48x48, 64x64 etc., while naughty itself has no trouble with displaying SVG icons. I tested it with naughty.config.icon_formats = { "png", "gif", "svg" } by running notify-send -i parcellite test and notify-send -i retroarch test (the icon paths are /usr/share/pixmaps/parcellite.svg and /usr/share/pixmaps/retroarch.svg).

Does this count as a separate feature request or is it a part of this issue?

@Elv13 Elv13 added this to the v4.4 milestone Oct 14, 2021
@Elv13 Elv13 self-assigned this Oct 14, 2021
@Elv13 Elv13 modified the milestones: v4.4, v4.5 Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants