GNOME Bugzilla – Bug 746435
wayland: Use g_get_prgname as the xdg surface application id
Last modified: 2015-08-18 01:07:37 UTC
This makes some applications set the correct application id (for example Maps), but for some it doesn't include the org.gnome part when it probably should (for example Contacts). I didn't look into any reason why.
Created attachment 299790 [details] [review] wayland: Use g_get_prgname as the xdg surface application id The "app_id" of a xdg_surface should be the ID that can potentially be used to get the DBUS name or the .desktop file, i.e. the reverse DNS if available, otherwise often just the base name of the binary file. Usually g_get_prgname will provide us with that name. Still fallback to the gdk program class in case g_get_prgname returns NULL, even though this string usually doesn't conform to the expectations of xdg_surface.set_application_id.
Review of attachment 299790 [details] [review]: If you look at gdk_pre_parse, the two are more or less the same. I'm not sure this is sufficiently better. What we really need is the application id that gets passed as the second argument to gdk_wayland_window_set_dbus_properties_libgtk_only - but ware already using that, right ? At least if the application in question is using GtkApplication. Can you look at why this doesn't work in the cases you cite ? Maybe we just need to make a simplified version of that set_dbus_properties function (just setting the application_id) available for !GtkApplication apps ?
(In reply to Matthias Clasen from comment #2) > Review of attachment 299790 [details] [review] [review]: > > If you look at gdk_pre_parse, the two are more or less the same. I'm not > sure this is sufficiently better. Right, but GApplication sometimes override prgname which made it occasionally work. > > What we really need is the application id that gets passed as the second > argument to gdk_wayland_window_set_dbus_properties_libgtk_only - but ware > already using that, right ? At least if the application in question is using > GtkApplication. True. We call gdk_wayland_window_set_dbus_properties_libgtk_only too late though. We can make it call earlier (handle_window_realize instead of handle_window_map) but then we should maybe duplicate the data in gdkwindow-wayland.c and set it after mapping (as MetaWindow won't have been created in mutter yet otherwise). > > Can you look at why this doesn't work in the cases you cite ? Maybe we just > need to make a simplified version of that set_dbus_properties function (just > setting the application_id) available for !GtkApplication apps ? Where would this API be? I made a patch locally that adds a gdk_wayland_window_set_application_id () that is called from gtkapplication-wayland.c or should this be made platform generic? Regarding how well it works, testing random applications trying to get the application id first from the GtkApplication id, then g_get_prgname, and then gdk_get_program_class (): Correct (desktop file base name and application id): gnome-maps (org.gnome.Maps) gnome-contacts (org.gnome.Contacts) glade (glade) gnome-font-viewer (org.gnome.font-viewer) evince (evince) Half correct (desktop file name vs resulting application id): gucharmap (gucharmap.desktop vs org.gnome.Charmap) eog (eog.desktop vs org.gnome.eog.ApplicationService) ibus-setup (ibus-setup.desktop vs main.py) Using gdk_get_program_class () instead of g_get_prgname () makes the result slightly worse (glade and evince will no longer be correct)
(In reply to Jonas Ådahl from comment #3) > ... > ibus-setup (ibus-setup.desktop vs main.py) Um that should be Not correct. Anyhow, this is an issue with ibus-setup rather than GTK+ I assum. Regarding the half correct ones I assume its also something that can't be fixed in GTK+.
(In reply to Jonas Ådahl from comment #3) > (In reply to Matthias Clasen from comment #2) > > Review of attachment 299790 [details] [review] [review] [review]: > > > > If you look at gdk_pre_parse, the two are more or less the same. I'm not > > sure this is sufficiently better. > > Right, but GApplication sometimes override prgname which made it > occasionally work. > > > > > What we really need is the application id that gets passed as the second > > argument to gdk_wayland_window_set_dbus_properties_libgtk_only - but ware > > already using that, right ? At least if the application in question is using > > GtkApplication. > > True. We call gdk_wayland_window_set_dbus_properties_libgtk_only too late > though. We can make it call earlier (handle_window_realize instead of > handle_window_map) but then we should maybe duplicate the data in > gdkwindow-wayland.c and set it after mapping (as MetaWindow won't have been > created in mutter yet otherwise). Calling it in realize seems totally fine - it is where we do this under X. > > Regarding how well it works, testing random applications trying to get the > application id first from the GtkApplication id, then g_get_prgname, and > then gdk_get_program_class (): > > Correct (desktop file base name and application id): > gnome-maps (org.gnome.Maps) > gnome-contacts (org.gnome.Contacts) > glade (glade) > gnome-font-viewer (org.gnome.font-viewer) > evince (evince) > > Half correct (desktop file name vs resulting application id): > gucharmap (gucharmap.desktop vs org.gnome.Charmap) > eog (eog.desktop vs org.gnome.eog.ApplicationService) > ibus-setup (ibus-setup.desktop vs main.py) > > Using gdk_get_program_class () instead of g_get_prgname () makes the result > slightly worse (glade and evince will no longer be correct) One thing that plays into this is that the identity desktop file basename == busname is only mandatory for applications which have DBusActivatable=true in the desktop files, while desktop file basename == application id should always hold. Another thing to notice is that under X, the application id is not actually needed on the bus, from what I can see, since we also set _GTK_UNIQUE_BUS_NAME as a window property, and that seems to be what the shell uses (e.g. gucharmap does not take org.gnome.Charmap as a well-known name, yet its appmenu works fine).
Created attachment 300110 [details] [review] wayland: Set the correct xdg_surface application id The "app_id" of a xdg_surface should be the ID that can potentially be used to get the DBUS name or the .desktop file. For GtkApplication programs this is the ID passed when creating the GtkApplication object, so when available lets use that. As fallbacks, first try g_get_prgname as it often corresponds to the basename part of the .dektop file for non-GtkApplication programs. Otherwise use gdk_get_program_class, even though that string usually doesn't conform to the expectations of xdg_surface.set_application_id.
(In reply to Matthias Clasen from comment #5) > One thing that plays into this is that the identity desktop file basename == > busname is only mandatory for applications which have DBusActivatable=true > in the desktop files, while desktop file basename == application id should > always hold. So in other words, for those applications that doesn't have DBusActivatable=true, their .desktop files either need to be renamed, or they should change the string passed when creating the GtkApplication object? > > Another thing to notice is that under X, the application id is not actually > needed on the bus, from what I can see, since we also set > _GTK_UNIQUE_BUS_NAME as a window property, and that seems to be what the > shell uses (e.g. gucharmap does not take org.gnome.Charmap as a well-known > name, yet its appmenu works fine).
(In reply to Jonas Ådahl from comment #7) > (In reply to Matthias Clasen from comment #5) > > One thing that plays into this is that the identity desktop file basename == > > busname is only mandatory for applications which have DBusActivatable=true > > in the desktop files, while desktop file basename == application id should > > always hold. > > So in other words, for those applications that doesn't have > DBusActivatable=true, their .desktop files either need to be renamed, or > they should change the string passed when creating the GtkApplication object? Not sure. Somewhat unhelpful that the gtk-shell.xml protocol is not documented, so it is a bit hard to know what exactly is expected here.
In any case, I got confirmation that the application_id (as given to GApplication / GtkApplication) is always the same as the busname, and is what we should use here. And that is what GtkApplication passes down when it calls gdk_wayland_window_set_dbus_properties_libgtk_only. We should really arrange things so that call gives us what we need, when we needed - ie move it earlier if that is required.
Review of attachment 300110 [details] [review]: looks ok. I don't think falling back to gdk_get_program_class buys us anything, really. Could just leave that out.
Review of attachment 300110 [details] [review]: Looking at the documentation of set_app_id and at the mutter implementation, I actually have doubts that using the actual application id for this is going to do us much good. The spec is much to unclear to be useful; it appears to be a mixture between traditional wm_class and desktop id. And the mutter code calls set_wm_class.
(In reply to Matthias Clasen from comment #11) > Review of attachment 300110 [details] [review] [review]: > > Looking at the documentation of set_app_id and at the mutter implementation, > I actually have doubts that using the actual application id for this is > going to do us much good. > The spec is much to unclear to be useful; it appears to be a mixture between > traditional wm_class and desktop id. And the mutter code calls set_wm_class. The reason for writing this patch was me trying to improve the documentation of xdg_surface.set_app_id and make GTK+ behave more according to the intention. I've been explained that the intention is to allow the shell to find the .desktop file as well as the DBUS name if the .desktop file basename is the same as the DBUS name (for DBUS-activatable applications). mutter calls meta_window_set_wm_class with the app_id as both the name and instance. gnome-shell then tries to use these to find the appliation, either by its the StartupWMClass or the .desktop basename. Now gtk+ sets the app_id to the "program class" which is never the same as either the .desktop file basename or the DBUS name. gnome-shell will still be able to work with this since it reverses the change GTK+ did (by doing a upper -> lower case of the string), and it'll effectively work almost the same as the next method: Using g_get_prgname makes it succeed occasionally when the result happen to be the same as the .desktop name. Using the application ID from the GTK application object makes it succeed occasionally when the application is DBUS-activatable. If there is any API in GTK application to get the basename of the .desktop application, it'd probably be best to try that first and then fallback to g_get_prgname. I don't think we should keep using the GDK program class, as it'll never be correct.
> mutter calls meta_window_set_wm_class with the app_id as both the name and > instance. gnome-shell then tries to use these to find the appliation, either by > its the StartupWMClass or the .desktop basename. ah, so this goes into the whole application matching mess, where we try to guess which window relates to which application. > If there is any API in GTK application to get the basename of the .desktop > application, it'd probably be best to try that first and then fallback to > g_get_prgname. I don't think we should keep using the GDK program class, as > it'll never be correct. Sadly, there is no such api, so what you have is probably the best we can do.
Review of attachment 300110 [details] [review]: .
(In reply to Matthias Clasen from comment #13) > > mutter calls meta_window_set_wm_class with the app_id as both the name and > > instance. gnome-shell then tries to use these to find the appliation, either by > > its the StartupWMClass or the .desktop basename. > > ah, so this goes into the whole application matching mess, where we try to > guess which window relates to which application. > > > If there is any API in GTK application to get the basename of the .desktop > > application, it'd probably be best to try that first and then fallback to > > g_get_prgname. I don't think we should keep using the GDK program class, as > > it'll never be correct. > > Sadly, there is no such api, so what you have is probably the best we can do. Is this information available at all in there? There needs to be no gtk application API really, GTK application needs to provide the information to GDK.
Pushed with some minor tweaks to the commit message (its not correct yet, just more correct than before).
Created attachment 309435 [details] [review] wayland: Use g_get_prgname() to get the xdg_surface.set_app_id string Prior to this patch, the ID of the GtkApplication was always used for clients which were GtkApplications. This would only be guaranteed to be correct for D-Bus activatable programs. As a result, all non-D-Bus-activatable applications would set the wrong ID making the shell unable to find the corresponding .desktop file. This change makes it so that the GDK backend always uses the name passed to g_set_prgname, or the default value if not explicitly set, as this more often corresponds to the .desktop file. This means that in order to make D-Bus activatable applications set the correct application ID, they must, for now, manually call g_set_prgname() with their application ID (basename of the .desktop file). If g_get_prgname() returns NULL, fallback to gdk_get_program_class() even though it will most likely never be correct according to the xdg_surface.set_app_id specification.
Review of attachment 309435 [details] [review]: Ok.
Comment on attachment 309435 [details] [review] wayland: Use g_get_prgname() to get the xdg_surface.set_app_id string Attachment 309435 [details] pushed as e1fd877 - wayland: Use g_get_prgname() to get the xdg_surface.set_app_id string