MeanEYE / Sunflower

Small and highly customizable twin-panel file manager for Linux with support for plugins.
GNU General Public License v3.0
428 stars 42 forks source link

Remove deprecated set_wmclass functions #474

Closed joshas closed 3 years ago

joshas commented 3 years ago

Removing deprecated set_wmclass functions as recommended in GTK docs:

Don’t use this function. It sets the X Window System “class” and “name” hints for a window. According to the ICCCM, you should always set these to the same value for all windows in an application, and GTK+ sets them to that value by default, so calling this function is sort of pointless.

jtojnar commented 3 years ago

Unfortunately, GTK determines the file name from the executable name so it will be whatever the file name is. For example, sunflower instead of Sunflower. Or in case of NixOS, where we need to wrap the program, .sunflower-wrapped.

This will make GNOME Shell show the filename as a window title when running Sunflower without having it installed:

wrong window title in GNOME dash

xprop output:

WM_CLASS(STRING) = "Sunflower", ".sunflower-wrapped"
joshas commented 3 years ago

@jtojnar Sorry for regression. I presumed that GTK will handle naming by itself. Do you have a .destkop file for Sunflower installed?

@MeanEYE Should I return set_wmclass() function for main application window?

jtojnar commented 3 years ago

It will get it from the program name https://gitlab.gnome.org/GNOME/gtk/-/blob/b46f50079bd80eb6f572a6524ee6b9040ecab7ff/gtk/gtkwindow.c#L1674-1675, which is not reliable for us when we rename the executable to wrap it.

Weirdly, the program class should be resolved from g_get_prgname

https://gitlab.gnome.org/GNOME/gtk/-/blob/b46f50079bd80eb6f572a6524ee6b9040ecab7ff/gdk/gdk.c#L300-302

but it is different:

(gdb) call (char *) gdk_get_program_class()
$1 = 0x20f4240 ".sunflower-wrapped"
(gdb) call (char *) g_get_prgname()
$2 = 0x1fc8be0 "Sunflower"

Maybe program class in GDK is set before GApplication is initialised

but according to https://developer.gnome.org/glib/unstable/glib-Miscellaneous-Utility-Functions.html#g-set-prgname, I would expect the initialization to happen first.

MeanEYE commented 3 years ago

@jtojnar does setting wm_class solve the issue for now? If so, we can safely revert the changes since I am guessing there are many other distributions out there and window managers who rely on this information.

jtojnar commented 3 years ago

Yes, before this patch, the value was:

WM_CLASS(STRING) = "Sunflower", "Sunflower"
MeanEYE commented 3 years ago

And the issue was non-existent?

jtojnar commented 3 years ago

Yes, GNOME Shell showed this:

image

Maybe there is a better way to go about this, I will check GNOME Shell code to determine why it uses class name from WM_CLASS for the app title, when desktop file is not found.

MeanEYE commented 3 years ago

Legacy support most likely. Either way, this is not only Gnome Shell we are talking about. Am assuming same behavior is in Mate and Gnome 2.x since they were originally based on GTK2 and depend on that. Probably same Xfce and LXDE.

@joshas please revert the patch.

jtojnar commented 3 years ago

Maybe GNOME Shell just expects all apps to have desktop files installed, in which case they would not care about people testing software without installation. While Nix allows running software without being installed globally or into the user profile, this feature is still pretty rare in other Linux distros.