[window] Prefer GTK icon theme over NET_WM_ICON #1702

Kafva · 2022-09-10T07:59:39Z

I have noticed that the window mode does not pick up certain icons from the icon theme set in gtk-3.0.ini. Looking at window.c I can see that this is to be expected since the class name that is used to look up the icon name is not guaranteed to be correct.

However, currently the NETWM icon seems to override any GTK icons that are found, this patch is meant to address this.

Before/after patch using:

[Settings]
gtk-icon-theme-name=Papirus-Dark

# rofi -show window

(theme)

DaveDavenport · 2022-09-10T08:09:55Z

I don't think this patch implements correct behavior.
The icon the application explicitly sets for itself should get priority.
(some) Applications use this to relay extra information to the user.

Kafva · 2022-09-10T08:30:56Z

Oh ye, I assume you mean like an email app setting "X unread messages" in the corner of the icon?
I would personally prefer that the icons always try to follow the theme but I guess that behaviour is worth preserving for other use cases.

DaveDavenport · 2022-09-10T08:56:14Z

maybe get the applications to use the correct icon theme instead?

Kafva · 2022-09-10T09:12:45Z

Hmmm ye, I guess the source of the problem isn't really rofi but how my environment sets (or does not set) the application icons that are sent. Definitely seems better to fix that instead if that is the case.

Kafva · 2022-09-10T13:11:23Z

I tried my patch in a different window manager (awesomewm instead of bspwm) but saw the same behaviour as shown in my prior screenshots.

Were you implying earlier that each individual application (kitty, obs etc.) would need to be patched to provide an icon from the active GTK theme (in queries for their NET_WM_ICON) or is this something that the WM/DE has control over/should handle?

DaveDavenport · 2022-09-10T13:15:18Z

I have no idea what a window manager has to do with what icon an application set for itself.

I don't know how applications exactly do this, I guess it depends on the application.

But I am not going to change rofi default behavior, that seems correct, because a user finds it 'ugly'.

Kafva · 2022-09-10T13:49:33Z

Ye, the window manager is probably unrelated 😅

My main gripe with the current behaviour is that the drun mode does show icons from the GTK theme, so it feels inconsistent that another mode shows different icons.

But I am fine with using my fork instead if this is not desirable as the default.

DaveDavenport · 2022-09-10T14:06:39Z

I don't think you read my reply right. I am not going to change the default behaviour, this does not rule out an config option to change the default behaviour.

Kafva · 2022-09-10T14:23:12Z

Oh ok, I guess I will reopen the issue then.

DaveDavenport · 2022-09-10T15:18:27Z

I think gtk icons is not the right name, -window-prefer-icon-theme is better.

DaveDavenport · 2022-09-10T16:06:50Z

doc/rofi.1.markdown

@@ -585,6 +585,9 @@ Default: *"wmctrl -i -R {window}"*

 Show window thumbnail (if available) as icon in the window switcher.

+`-window-prefer-icon-theme`
+
+Use icons from the current GTK theme in window mode.


It is not 'gtk theme'. It is the icon theme set, this is not directly dependent on gtk.
Rofi f.e. has it own set icon-theme option, and this will be used when set.

I changed it to 'icon theme', does it need to be more verbose in some other way?

DaveDavenport · 2022-09-10T16:14:14Z

source/modes/window.c

+  } else if (c->icon == NULL && c->icon_checked == FALSE) {
+    c->icon = get_net_wm_icon(rmpd->ids->array[selected_line], size);
+    c->icon_checked = TRUE;
+  }


Is this logic correct? when config.window_prefer_icon_theme is not set, it will never fall back to the 'class icon' when the application has no net_wm_icon set.
It now always requests this icon, even when not used. This seems wasteful.

Tried to address this in 4bd7d3b

Only fetch the NET_WM_ICON and theme icon when their counterpart is NULL as a fallback for each state of the `-window-prefer-icon-theme` flag.

DaveDavenport · 2022-09-19T16:14:10Z

Going over the patch, there is a small possible glitching issue. It might show the application set by the application, then the one from the theme.
To fix this an extension needs to be made to rofi_icon_fetcher. I will look at this.

DaveDavenport · 2022-10-22T09:05:03Z

Pushed the extension to rofi_icon_fetcher and directly implemented this request (this exposed another small issue, so decided to commit the whole).

Can you test?

Thanks to Kafva (https://github.com/Kafva) for the original patch. Issue: #1702

DaveDavenport · 2022-10-22T09:16:32Z

   You can prefer the icon theme above the window set icon with the 'prefer-icon-theme' setting:

          configuration {
            window {
                prefer-icon-theme: true;
            }
          }

   or pass -window-prefer-icon-theme true on command line.

Kafva · 2022-10-22T10:08:23Z

Ye, tested the new option via CLI and from the config file, both seem to work as expected 🙂

github-actions · 2022-11-23T00:02:16Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

[window] Prefer GTK icon theme over NET_WM_ICON

93d2595

Kafva closed this Sep 10, 2022

Kafva reopened this Sep 10, 2022

[window] Add -window-prefer-gtk-icons option

55aa4dc

[window] Rename option to -window-prefer-icon-theme

05b06ab

DaveDavenport reviewed Sep 10, 2022

View reviewed changes

Kafva added 2 commits September 10, 2022 20:05

[docs] Change description of -window-prefer-icon-theme

77826f6

[window] Fix logic for -window-prefer-icon-theme

4bd7d3b

Only fetch the NET_WM_ICON and theme icon when their counterpart is NULL as a fallback for each state of the `-window-prefer-icon-theme` flag.

DaveDavenport added this to the 1.7.6 milestone Oct 3, 2022

DaveDavenport closed this in 96a69a5 Oct 22, 2022

DaveDavenport reopened this Oct 22, 2022

DaveDavenport added a commit that referenced this pull request Oct 22, 2022

[Window] Small fixes to prefer-icon-theme option

60fcbbb

Thanks to Kafva (https://github.com/Kafva) for the original patch. Issue: #1702

Kafva closed this Oct 22, 2022

github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2022

[window] Prefer GTK icon theme over NET_WM_ICON #1702

[window] Prefer GTK icon theme over NET_WM_ICON #1702

Uh oh!

Conversation

Kafva commented Sep 10, 2022

Uh oh!

DaveDavenport commented Sep 10, 2022

Uh oh!

Kafva commented Sep 10, 2022

Uh oh!

DaveDavenport commented Sep 10, 2022

Uh oh!

Kafva commented Sep 10, 2022

Uh oh!

Kafva commented Sep 10, 2022

Uh oh!

DaveDavenport commented Sep 10, 2022

Uh oh!

Kafva commented Sep 10, 2022

Uh oh!

DaveDavenport commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kafva commented Sep 10, 2022

Uh oh!

DaveDavenport commented Sep 10, 2022

Uh oh!

DaveDavenport Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

Kafva Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

DaveDavenport Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

Kafva Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

DaveDavenport commented Sep 19, 2022

Uh oh!

DaveDavenport commented Oct 22, 2022

Uh oh!

DaveDavenport commented Oct 22, 2022

Uh oh!

Kafva commented Oct 22, 2022

Uh oh!

github-actions bot commented Nov 23, 2022

Uh oh!

Uh oh!

DaveDavenport commented Sep 10, 2022 •

edited

Loading