Fallback icon doesn't play nice with third-party panels

[ahigerd]

In openbox/client.c:client_update_icons(), if a window doesn't have an icon explicitly set and it doesn't have any parents to inherit an icon from, Openbox sets one:

        OBT_PROP_SETA32(self->window, NET_WM_ICON, CARDINAL, ldata, w*h+2);

I'm not entirely sure why this is necessary: client_startup initializes each client's icon to def_win_icon, so everything in Openbox itself will have something to render.

By contrast, if a window doesn't have an icon set on it, some other environments (I know GNOME in particular) can pull an icon out of the system theme. I haven't looked into exactly how GNOME does it, but in the panel I'm building I'm checking the WM_CLASS property to see if there's something matching in /usr/share/pixmaps or /usr/share/icons. Unfortunately, because Openbox is setting the _NET_WM_ICON for the window, nothing else will ever get the chance to notice that the icon is missing in order to supply one. (In particular, Slack doesn't set an icon for itself. Because it's not open-source, I can't fix it upstream.)

Experimentally removing the icon assignment doesn't appear to have any negative consequences.

Would you be willing to consider removing the icon-setting behavior? Is there anything that depends on that behavior?

If removing it isn't acceptable, could we discuss setting another property on the window to indicate that Openbox has overridden the icon? That wouldn't make third-party tools automatically work but at least it would make it possible to build Openbox compatibility.

[danakj]

There is a downside here which is that openbox (alt-tab, etc) will show a different icon than panels and other parts of the UI will. This is pretty confusing and inconsistent then.

I think since Openbox is the source of truth (after the application) it would be nicer to allow it to get an icon off disk.

I started adding code to parse application manifests many years ago but that’s more machinery than required if we could use per-app rules to specify an icon file path, if this is an uncommon issue.

automatically finding icons presents a lot more complexity and configuration questions but could always be added on later and wouldn’t conflict ideologically with the per-app setting.

[ahigerd]

Well, I'm not using Openbox's Alt+Tab, so that's not a problem for me, but I do see what you mean. That said, my intent was to replace the window's icon myself as the panel manager, which is why I would have been okay with setting another prop as an indication that it's okay for my panel to do it. A configuration option to decline to set a default icon would work, too, so that the default behavior still works for the default case of Openbox being in charge of the desktop instead of being the WM in a larger environment.

What manifests are you talking about? I'm not familiar with the notion; it might be helpful for me in my own stuff. I was considering parsing /usr/share/applications/*.desktop to map StartupWMClass to Icon -- that would have worked for Slack, but not for QTerminal.

Looking up WM_CLASS using the FreeDesktop.org Icon Theme Specification works just fine for both apps if you add /usr/share/pixmaps to the end of the search path.

EDIT: Also, you might not NEED to implement the logic -- it's already available in GTK, so there might be a convenient, self-contained implementation somewhere that you can use in Openbox.

[danakj]

Ah yes, memory is coming back to me. I was parsing both .desktop files (for a launcher) and Icon .theme files for icons. So you know about the same stuff then.

A config option to just not set any icons at all sounds reasonable - since the panel can take over responsibility and set the icons then. There may be assumptions in the code that there is some icon, though, and that would no longer always be true.

[ahigerd]

At least with my preliminary investigation, everything within Openbox's code uses client_icon(), which has a fallback to client_default_icon if no other icon has been set. The code that interacts with _NET_WM_ICON is expecting to be bootstrapping a window, so it's expecting the possibility of an icon not existing yet. As far as I can tell, the purpose of assigning to the _NET_WM_ICON property is to inform other applications such as panels and pagers about the icon in order to have consistent handling.

[ahigerd]

I've opened a PR with a proposed implementation of this configuration option: https://github.com/Mikachu/openbox/pull/22

[ahigerd]

Raising a heads-up on this thread: I've addressed the issues I encountered while testing my PR, so it's ready for review.