GNOME Bugzilla – Bug 168718
libwnck skimps on an access to XClassHint components
Last modified: 2011-08-12 10:58:00 UTC
'man XClassHint' says: ... Note that the name set in this property may differ from the name set as WM_NAME. That is, WM_NAME specifies what should be displayed in the title bar and, therefore, can contain temporal information (for example, the name of a file cur- rently in an editor's buffer). On the other hand, the name specified as part of WM_CLASS is the formal name of the application that should be used when retrieving the application's resources from the resource database. That distinction above is often crucial as can be easily seen with a help of 'xprop' utility. XClassHint structure looks like this typedef struct { char *res_name; char *res_class; } XClassHint; and libwnck provides a function, declared in libwnck/class-group.h and called wnck_class_group_get_res_class(), to get that second string. One would expect that wnck_class_group_get_name() from the same header will give an access to 'res_name' above. Unfortunately this is not the case. This function seems to return the same string as wnck_application_get_name() which is less then useful. An internal _wnck_window_get_resource_name() function will actually get a desired string but this is not a public interface. It seems to me that something went askew in an implementation phase. OTOH libwnck goes to a great length to warn that its interface is not stable to it does not look like a great deal to fix that.
Created attachment 88447 [details] [review] Add wnck_class_group_get_res_name() Here's a patch that adds wnck_class_group_get_res_name(). It also contain some other fixes (like: don't take into account fallback names). Note that the patch adds wnck_application_has_name() and wnck_application_has_icon_name() for consistency with the similar WnckWindow functions.
> Here's a patch that adds wnck_class_group_get_res_name(). It does that? A string like "wnck_class_group_get_res_name" does not seem to show up anywhere in that patch. Is it complete?
Created attachment 88448 [details] [review] Real patch That was the wrong patch :-)
This is totally needed. And should be confirmed. The Ubuntu unity project called BAMF (application matching framework) can't live without a correct implementation of this feature. With the current implementation is very hard to group correctly applications, since we need the correct (res_name, class_name) tuple. The bad implementation of libwnck caused also that other projects would follow badly the ICCCM standard using the "class name" as "resource name of class" (chromium is an example: http://code.google.com/p/chromium/issues/detail?id=20587). I didn't check yet if the previously attached patch merges correctly or needs some tuning, but I'm available for working on it, if needed. However, please, follow the standards correctly.
Created attachment 190480 [details] [review] Added APIs for getting WM_CLASS name and instance for a wnkWindow I've re-implemented this for the current code. However not touching the class-group, that in fact is well defined as it is (a group must be shared by all the applications with the same class name, it doesn't care about their instance name). I've preferred to add public getters to the Window res_name and res_class parameters that are, in fact, what people would need (also if, while the res_class was accessible, the res_name wasn't). This make possible to get take a wnck class_group and iterate over its sub-windows to get the ones with a particular instance class. Patches attaced, with various fixes and support for wnckprop.
Review of attachment 190480 [details] [review]: Thanks for the patch, see the small comments. ::: libwnck/window.c @@ +1178,3 @@ + * + * Gets class name parameter of <ulink + * url="http://tronche.com/gui/x/icccm/sec-4.html#WM_CLASS">WM_CLASS Property</ulink> for the window @window. Please correctly wrap the comment :-) Also, it'd be nice to mention that this is the same thing as some API called on the WnckClassGroup from this WnckWindow. @@ +1195,3 @@ + * + * Gets class instance parameter of <ulink + * url="http://tronche.com/gui/x/icccm/sec-4.html#WM_CLASS">WM_CLASS Property</ulink> for the window @window. Same here about wrapping. ::: libwnck/window.h @@ +303,3 @@ + +const char* wnck_window_get_wmclass_name (WnckWindow *window); +const char* wnck_window_get_wmclass_instance (WnckWindow *window); My main doubt is about the name of those API. It used to be resource_name and resource_instance, but talking about class instead of resource might be clearer -- except that the WM_CLASS doc mentions the RESOURCE_NAME environment variable... And I think I'd prefer to just talk about class instead of wmclass. So for me, it should either be: get_resource_{name,instance} or get_class_{name,instance}. Do you have any strong opinion about one or the other? (If we move to "class", then it might make sense to rename variables to class_* instead of res_*) Also, we'll want to deprecate wnck_class_group_get_res_class() and give it a more appropriate name too. ::: libwnck/wnckprop.c @@ +1514,3 @@ + buf = _("<unset>"); + + g_print ("Class Instance: %s\n", buf); You should use _(). Also, it'd be nice to add a translators comment to explain what instance means here (look at the comment for class group).
Review of attachment 190480 [details] [review]: About the API name, I'd avoid to use get_class_{name,instance} because it could be confusing, in fact the "class" term could be related also to the WnckWindow class, and who is reading the code using this API could think that we're retrieving the gobject class name or class instance name or something... using get_resource_{name,instance} could be fine, but it's not that easily explicative... Without reading the docs you wouldn't have thought that is referred to a WM_CLASS attribute. Also, the the parameters we're trying to get here are related to the Window Manager, so using get_wmclass_* seems reasonable to me. However, just let me know which you'd prefer and I'll update the patch. Thanks for your review!
Created attachment 192894 [details] [review] Added APIs to get WM_CLASS group name and instance name of a wnkWindow Patch upgraded to be conform to what Vunts prefers according to our discussion in IRC ;) APIs now are: - wnck_window_get_class_group_name - wnck_window_get_class_instance_name
Thanks, pushed!