GNOME Bugzilla – Bug 671098
Please expose MetaPlugin to introspection users
Last modified: 2012-10-08 21:08:00 UTC
With a few modifications, it's possible to expose all of the MetaPlugin class to introspection, which allows for the MetaPlugin subclass to be implemented entirely in a non-C language. The two attached patches solve the two issues currently preventing this: 1. The g-ir-scanner can't determine the GI type of the XEvent argument to the MetaPlugin xevent_filter virtual method. It's not possible to directly annotate the virtual method function pointer type; instead you have to annotate the functional invoker for the virtual function. g-ir-scanner automatically copies the annotations on the invoker to the virtual function type as well. Therefore, I've added a meta_plugin_xevent_filter invoker, and taken advantage of it in MetaCompositor for some slight code simplification. 2. 5 of the methods in MetaPlugin return objects, so they need transfer mode annotations. With both of these changes, it's possible to remove the "skip" annotation on MetaPlugin and generate full introspection data for all of MetaPlugin and MetaPluginClass.
Created attachment 208737 [details] [review] [PATCH 1/2] Add a virtual invoker function for MetaPluginClass.xevent_filter
Created attachment 208738 [details] [review] [PATCH 2/2] Expose MetaPlugin to introspection
(In reply to comment #0) > With a few modifications, it's possible to expose all of the MetaPlugin class > to introspection, which allows for the MetaPlugin subclass to be implemented > entirely in a non-C language. > > The two attached patches solve the two issues currently preventing this: > > 1. The g-ir-scanner can't determine the GI type of the XEvent argument to the > MetaPlugin xevent_filter virtual method. It's not possible to directly annotate > the virtual method function pointer type; instead you have to annotate the > functional invoker for the virtual function. Not quite true anymore. With a recent enough g-ir-scanner (from git), you can directly annotate a vfunc. See this commit: http://git.gnome.org/browse/gobject-introspection/commit/?id=9b4185f88aa321b5160b100597d83d295b0af76e
Additionally, I think in the future we want to remove a lot of the MetaPlugin API - most of the entry points are just direct wrappers for other things. This is especially true if we want to have a plugin be able to control more than one screen, like people have been requesting for GNOME Shell.
Created attachment 208740 [details] [review] Expose MetaPlugin to introspection Thanks for that feedback. I was developing against an older version of the g-ir-scanner. The new syntax is much easier to work with. Since the new syntax lets me simplify the changes to the vfunc significantly, I've gone ahead and consolidated the changes into a single commit. I appreciate that the MetaPlugin API isn't perfect, but how quickly is it going away (or getting significantly reduced, or whatever)? If it's not happening immediately, can I convince you guys to at least adopt the patch until then? If the upcoming refactoring breaks the GI bindings, I would be more than happy to do whatever cleanup is required to expose the same functionality.
There wasn't any active work, but you reminded me of it, and I have a WIP patch in my local tree. The functions that the plugin API calls should all be introspectable, so see if you can mock something up that doesn't need the function annotations.
Removing the silly plugin API is tracked in bug 671103.
As far as I can tell, there are 3 removed API functions whose behavior I can't currently access through other exposed APIs: meta_plugin_set_stage_reactive, meta_plugin_get_xdisplay, and meta_plugin_set_stage_input_area. For the first, moving meta_set_stage_input_region and meta_empty_stage_input_region to the public headers should be sufficient to get it picked up - I don't expect any problems using that from the scanner. For the second, the function currently carries a (skip) annotation. Replacing that with Returns: (transfer none) should be sufficient (as my patch already does for meta_plugin_get_xdisplay) The last is more challenging. The xfixes-4.0 gir doesn't export XFixesCreateRegion or XFixesDestroyRegion, and xlib-2.0 doesn't include XRectangle, so there's no way to implement this method using the code currently exported to introspection. But while I'd like to see as much of the libmutter API exposed to introspection users, I am personally not using meta_plugin_set_stage_input_area at present, so I don't care as much if it gets dropped. However, even with the cleanup of these APIs, I'd still like the ability to subclass MetaPlugin from an introspection-based language, so I can then feed that plugin type back to the core compositor; the API cleanup feels largely orthogonal to the intent of my patch.
(In reply to comment #8) > As far as I can tell, there are 3 removed API functions whose behavior I can't > currently access through other exposed APIs: meta_plugin_set_stage_reactive, > meta_plugin_get_xdisplay, and meta_plugin_set_stage_input_area. > > For the first, moving meta_set_stage_input_region and > meta_empty_stage_input_region to the public headers should be sufficient to get > it picked up - I don't expect any problems using that from the scanner. My patch does that. > For the second, the function currently carries a (skip) annotation. Replacing > that with Returns: (transfer none) should be sufficient (as my patch already > does for meta_plugin_get_xdisplay) I don't think any Xlib types can be introspected correctly, so I'm not sure how useful of a method it is to use from a binding. > The last is more challenging. The xfixes-4.0 gir doesn't export > XFixesCreateRegion or XFixesDestroyRegion, and xlib-2.0 doesn't include > XRectangle, so there's no way to implement this method using the code currently > exported to introspection. But while I'd like to see as much of the libmutter > API exposed to introspection users, I am personally not using > meta_plugin_set_stage_input_area at present, so I don't care as much if it gets > dropped. We might be able to provide an API based on cairo_region_t, which should be usable from most bindings. > However, even with the cleanup of these APIs, I'd still like the ability to > subclass MetaPlugin from an introspection-based language, so I can then feed > that plugin type back to the core compositor; the API cleanup feels largely > orthogonal to the intent of my patch. Right, of course. I just didn't want you to waste any time fiddling with the plugin APIs that will probably go away soon.
(In reply to comment #9) > (In reply to comment #8) [...] > > For the second, the function currently carries a (skip) annotation. Replacing > > that with Returns: (transfer none) should be sufficient (as my patch already > > does for meta_plugin_get_xdisplay) > > I don't think any Xlib types can be introspected correctly, so I'm not sure how > useful of a method it is to use from a binding. There's an xlib-2.0 gir that includes types for Displays, Windows, and a handful of others. It's true that you can't really do anything with them from purely GI-based bindings like GJS and Python, but Vala has some custom bindings for Xlib functions that let you use the Display handles and Window XIDs with other Xlib functions. [...] > > However, even with the cleanup of these APIs, I'd still like the ability to > > subclass MetaPlugin from an introspection-based language, so I can then feed > > that plugin type back to the core compositor; the API cleanup feels largely > > orthogonal to the intent of my patch. > > Right, of course. I just didn't want you to waste any time fiddling with the > plugin APIs that will probably go away soon. Awesome, just wanted to make sure we were on the same page - I can definitely work around not having those functions in my code going forward, and I'm happy to adjust my patch in response to those changes. Would you prefer a patch that allows introspection of MetaPlugin, but instead adds (skip) annotations for the functions that aren't automatically introspectable (or even the ones that will go away when your branch lands)?
Created attachment 209659 [details] [review] Expose MetaPlugin to introspection I noticed that you pushed your API cleanup, so I've rebased my patch on top of that. Turns out it significantly reduces the changes required.
Review of attachment 209659 [details] [review]: > I noticed that you pushed your API cleanup, so I've rebased my patch on top of > that. Turns out it significantly reduces the changes required. Good to hear! ::: src/meta/meta-plugin.h @@ +100,3 @@ */ + /** + * MetaPluginClass::xevent_filter Missing a semicolon on the end -- should be: MetaPluginClass::xevent_filter: @@ +101,3 @@ + /** + * MetaPluginClass::xevent_filter + * @event: (type xlib.XEvent): Again, are we sure we can do anything interesting with the XEvent here?
(In reply to comment #12) > ::: src/meta/meta-plugin.h > @@ +100,3 @@ > */ > + /** > + * MetaPluginClass::xevent_filter > > Missing a semicolon on the end -- should be: > > MetaPluginClass::xevent_filter: Huh. You're absolutely correct, although the GIR scanner seems to be OK with my form as well, so I didn't notice it. I'll fix it and upload a new patch. > @@ +101,3 @@ > + /** > + * MetaPluginClass::xevent_filter > + * @event: (type xlib.XEvent): > > Again, are we sure we can do anything interesting with the XEvent here? In general, no - there's not actually much you can do with an XEvent from Python/GJS. Vala, however, includes a vapi file for xlib that lets you access the members of the XEvent union, so it's useful in that context. I'm not sure what we could do here to make this useful for Python/GJS/etc. Maybe if the vfunc was called with a GdkEvent? Although that would break ABI. It doesn't look like Gdk.Window.add_filter does much to make the XEvent argument useful to high-level GI consumers. I'm open to suggestions on this one.
Created attachment 210029 [details] [review] Expose MetaPlugin to introspection
Tested to run through introspection fine. I've pushed the patch, though it should be noted that the long-term plan is to get rid of MetaPlugin. (No concrete timeline.) Attachment 210029 [details] pushed as 99cbe76 - Expose MetaPlugin to introspection