xdg-toplevel-icon-v1 support

[tokyo4j]

This PR allows clients to set icons of titlebar, OSD and client-list-combined-menu, via xdg-toplevel-icon-v1.

This PR also changes the semantics of scaled_icon_buffer: rather than calling scaled_icon_buffer_set_app_id() every time an app_id is set, we can now call scaled_icon_buffer_set_view() just once so that multiple scaled_icon_buffers bound to a window are automatically updated when an app_id is set or new icon is set via xdg-toplevel-icon-v1.

buffer sharing problem (worked around)

But this approach has a major problem: the buffer may not be redrawn on app_id or icon updates, due to buffer sharing mechanism of scaled_scene_buffer. Here's the reproducer of this problem:

• Open foot window
• Open another window, which deactivates the foot window and creates two instances of scaled_icon_buffers that are considered "equal" according to _equal() in scaled-icon-buffer.c.
• Start senpai in the foot window, which updates its app_id
• The titlebar icon is not updated to senpai's one

2025-05-25.00-12-03.mp4

A possible solution for this is storing app_id and wlr_xdg_toplevel_icon_v1 instance in scaled_icon_buffer and compare them in _equal(), but I'm not a fan of adding fields just for the sake of comparison.

While it may sound too radical, but I'm actually thinking of removing the buffer sharing mechanism altogether. As an alternative optimization, we can implement caches of lab_img so that SVG images are not parsed for every icons. Of course this means we will consume more memory for buffers in scaled_scene_buffers, but I guess its memory overhead is negligible (< 100KB) in most cases. This also significantly reduces the complexity of scaled_scene_buffer and prevents complicated bugs like #2485 and the one addressed by #2520. @Consolatis what do you think?

[tokyo4j]

Updated my branch to isolate scaled_icon_buffer and xdg-toplevel-icon so that we can easily add support for _NET_WM_ICON (#2760).

[johanmalm]

Thanks for grabbing this one 😄

I've not yet done any higher level thinking on this one, nor read through it all in detail so the below is just some initial low-level review comments:

1. We should use the prefix handle_ for signal-handler-functions, so suggest renaming xdg_toplevel_icon_handle_set_icon() Ref: https://github.com/labwc/labwc/blob/master/CONTRIBUTING.md#naming-conventions
2. I don't like the ownership model where we wl_array_init() in the signal-handler - and then call the matching wl_array_release() in view_set_icon(). Suggest trying to find something more balanced, for example call wl_array_release() after view_set_icon() in the signal-handler.
3. In view_set_icon() you can use xstrdup_replace()

   labwc/include/common/mem.h

   Line 51 in 8e9751e

   #define xstrdup_replace(ptr, str) do { \

4. It's slightly confusing having a view_set_icon() and a handle_view_set_icon() which do quite different things. I can live with it 😄 but I thought I'd mention it as someone reading it fresh.

[tokyo4j]

1. We should use the prefix handle_ for signal-handler-functions, so suggest renaming xdg_toplevel_icon_handle_set_icon() Ref: https://github.com/labwc/labwc/blob/master/CONTRIBUTING.md#naming-conventions

OK, let's rename other signal handlers in #2765 first.

2. I don't like the ownership model where we wl_array_init() in the signal-handler - and then call the matching wl_array_release() in view_set_icon(). Suggest trying to find something more balanced, for example call wl_array_release() after view_set_icon() in the signal-handler.

Agreed. I wrote that way because it was shorter, but it was indeed strange. Updated as suggested.

3. In view_set_icon() you can use xstrdup_replace()

No, it asserts when NULL is passed in the second argument. Perhaps we can update xstrdup_replace() or add another function to allow NULL, but it's not in the scope of this PR.

[Consolatis]

Looked at bit into the diff, some comments below

[tokyo4j]

I'm marking this PR as ready for final review since I've addressed the buffer sharing bug by adding view_app_id, view_icon_name and view_icon_buffers in struct scaled_icon_buffer.

[alexsch01]

@tokyo4j thank you for this PR, I think the description should be updated since the buffer sharing bug is addressed by you

[johanmalm]

Notwithstanding the review comments by @Consolatis above, I think this feels okay. Not tested.

I ponder whether we ought to provide an override to force the .desktop file icon, but could follow up with that if users want it. Am I right in thinking that the specified icon-theme will be used when looking up the icons specified by xdg-toplevel-icon-v1.

[tokyo4j]

Notwithstanding the review comments by @Consolatis above, I think this feels okay. Not tested.

I'm reluctant to merge this PR without @Consolatis's LGTM... can we wait for another week or so? I don't want to invade our codebase with codes that only I can follow (I admit ime.c is almost only understood by me, but it's more isolated from other parts unlike scaled_icon_buffer).

Am I right in thinking that the specified icon-theme will be used when looking up the icons specified by xdg-toplevel-icon-v1.

That's right.

I think icon themes or buffers explicitly offered by clients should be usually prioritized for better out-of-box experience of applications like wine and gamescope. But I understand that your concern that this PR deprives users of the ability to set arbitrary icons with .desktop files. We can discuss it later (e.g. disable client-provided icons via <theme clientSideIcon="no"> as I mentioned in #2760 (comment), or adding new window rule fields).

[Consolatis]

'm reluctant to merge this PR unless @Consolatis's LGTM... can we wait for another week or so? I don't want to invade our codebase with codes that only I can follow.

Appreciate that. I was intending to give a final review yesterday but RL stuff happened, will do tonight or tomorrow at the latest.

[Sunderland93]

Hello. Thank you for this PR! Tested with RetroArch (which is recently added support for this protocol) and it works great!

[Consolatis]

I've read through the diff again and tried to test it nested. Other than the minor nitpick with the view_from_xdg_toplevel() wrapper including asserts this LGTM.

I don't really have any client which sets a icon using the protocol, foot only seems to set the icon name which is always a copy of the app-id foot is using so I can't really test it properly. But based on the code and feedback above that part seems to work well.

Something we might want to consider as follow up PR is how to prioritize the different icon options. E.g. "that application X icon looks bad, I want the one from my icon theme" vs "this application spawns different windows and each has its own window icon but its always using the one from the theme for all the windows". My personal preference would be to provide a window rule attribute with some kind of enum to influence the priority selection. That approach could be used to express a global preference or app specific ones.

Great work!

[Sunderland93]

I don't really have any client which sets a icon using the protocol,

RetroArch uses both icon name and icon buffer

[tokyo4j]

With all the threads resolved, I'll merge this PR.

Something we might want to consider as follow up PR is how to prioritize the different icon options. E.g. "that application X icon looks bad, I want the one from my icon theme" vs "this application spawns different windows and each has its own window icon but its always using the one from the theme for all the windows". My personal preference would be to provide a window rule attribute with some kind of enum to influence the priority selection. That approach could be used to express a global preference or app specific ones.

That sounds sensible, but I'm not sure how the new window rule attribute should look like. If the goal is to allow users to set arbitrary icons, passing xdg-spec-reliant icon names (which also allows absolute paths) like <windowRule app_id="..." icon="[icon name]" /> will be simple, easy and extensible. But if the user wants to just ignore icons set via xdg-toplevel-icon and use the icon fetched from app_id, he will need to manually set the icon name in the new attribute.

[Consolatis]

That sounds sensible, but I'm not sure how the new window rule attribute should look like. If the goal is to allow users to set arbitrary icons, passing xdg-spec-reliant icon names (which also allows absolute paths) like <windowRule app_id="..." icon="[icon name]" /> will be simple, easy and extensible. But if the user wants to just ignore icons set via xdg-toplevel-icon and use the icon fetched from app_id, he will need to manually set the icon name in the new attribute.

I thought more about something like <windowRule identifier="xterm" preferIcon="client|server" />, similar to the decorations. client would then be _NET_WM_ICON / xdg-toplevel-icon and server would be app-id / WM_CLASS via libsfdo.

[tokyo4j]

Wait, this PR turns the window icons to the fallback one after Reconfigure. I'll investigate and fix it soon.

EDIT: fixed by #2794