GNOME Bugzilla – Bug 672727
Allow overriding the GObject base class
Last modified: 2012-11-07 10:28:50 UTC
It seems the GObject base class cannot be overridden via the python overrides mechanism due to it being implemented in C rather then being pulled in via introspection. Even so there are cases where overriding or augmenting behavior of this base class could ease development. For instance, the following tickets would be trivial as python overrides but would prove more difficult to implement in C: https://bugzilla.gnome.org/show_bug.cgi?id=332782 https://bugzilla.gnome.org/show_bug.cgi?id=672207 https://bugzilla.gnome.org/show_bug.cgi?id=672324 Furthermore, this could also allow a certain amount of refactoring of the C extension into python which would help minimize maintenance costs. The "props" accessor comes to mind as something that would be trivial in python but takes a large amount of C to get right. This however might want to stay in C if there are performance requirements. But these performance requirements should be proven through testing not speculation. After attempting to create a GObject override file I ran into issues with it not having an "__info__" member which I assume is pulled in through introspection.
Can you attach or paste an example script that illustrates the issue?
Created attachment 211965 [details] GObject override file Drop the attached file into gi/overrides and run "make check" Results in: TypeError: Can not override a type GObject, which is not in a gobject introspection typelib Which makes sense because the GObject class is a static binding not generated from gi. But still seems like we should be able to override aspects of it which might mean simulating it as a dynamic gi object by adding __info__?
Created attachment 228334 [details] [review] Add support for overriding GObject.Object Shift pygi module mechanics so the introspection generated 'Object' class becomes derived from the static GObject class. Add initial GObject.Object override which sets all static methods back essentially leapfrogging the introspection methods. This sets the stage for having the ability to remove static methods piecemeal in favor of introspection/python in future commits.
Created attachment 228335 [details] [review] Replace GObject notify methods with introspection and python Replace static context managers for freeze_notify and handler_block with python context managers. Remove notify, freeze_notify, thaw_notify static methods as the introspection versions work fine. This is a first pass at using introspection methods to replace static bindings for GObject.Object methods, much more can be done.
Created attachment 228336 [details] [review] Fix comments
Comment on attachment 228335 [details] [review] Replace GObject notify methods with introspection and python Ah, great to see that all the effort of dropping the static bindings now bear some fruits! Looks good to me (it depends on the previous patch of course, which I haven't reviewed yet).
Comment on attachment 228336 [details] [review] Fix comments Some questions about the patch: - def __getattr__(self, name): + def get_info(self, name): [...] + def __getattr__(self, name): + info = self.get_info(name) I don't see get_info() being called anywhere else in the patch. Why was this introduced? + get_data = _unsupported_method + set_data = _unsupported_method We dropped those in http://git.gnome.org/browse/pygobject/commit/?id=763534. I guess with building on top of the introspected GObject these come back into the namespace, so indeed we need to handle them. But I think they should instead throw a more specific error as they did in http://git.gnome.org/browse/pygobject/commit/?id=73531fd782 + new = _unsupported_method Hm, shouldn't this do the same as GObject() (which is the implicit GObject constructor)? It's not that important as it doesn't work right now either, but perhaps we want to make it work for consistency? Can you please add a test that trying to call an unsupported method raises a NotImplementedError? Thanks!
(In reply to comment #7) > (From update of attachment 228336 [details] [review]) > Some questions about the patch: > > - def __getattr__(self, name): > + def get_info(self, name): > [...] > + def __getattr__(self, name): > + info = self.get_info(name) > > I don't see get_info() being called anywhere else in the patch. Why was this > introduced? I think I added it at one point for interactive hacking/debugging convenience and it stuck. Actually it is probably a bad thing to have any public methods on IntrospectionModule and DynamicModule because they have the potential to mask the module attributes so I will put this back. > + get_data = _unsupported_method > + set_data = _unsupported_method > > We dropped those in http://git.gnome.org/browse/pygobject/commit/?id=763534. I > guess with building on top of the introspected GObject these come back into the > namespace, so indeed we need to handle them. But I think they should instead > throw a more specific error as they did in > http://git.gnome.org/browse/pygobject/commit/?id=73531fd782 How does this sound: RuntimeError('Object.get_data() and set_data() are not supported. Use normal Python attributes instead') > + new = _unsupported_method > > Hm, shouldn't this do the same as GObject() (which is the implicit GObject > constructor)? It's not that important as it doesn't work right now either, but > perhaps we want to make it work for consistency? So should this be an overridden classmethod which does something like the following? def new(cls): return cls() > Can you please add a test that trying to call an unsupported method raises a > NotImplementedError? Sure thing.
(In reply to comment #8) > How does this sound: RuntimeError('Object.get_data() and set_data() are not > supported. Use normal Python attributes instead') Sounds good! > > + new = _unsupported_method > > > > Hm, shouldn't this do the same as GObject() (which is the implicit GObject > > constructor)? It's not that important as it doesn't work right now either, but > > perhaps we want to make it work for consistency? > > So should this be an overridden classmethod which does something like the > following? > def new(cls): > return cls() Hm, I had thought of g_object_new() used through GI, but of course that doesn't work because of the varargs. So we could do that with an override, but it would need **kwargs for the properties. I'm just not sure how this would interact with e. g. bug 675581, as this .new() would get inherited to all subclasses. So on second thought this wasn't a very good idea, so let's just skip this for now. Thanks!
Created attachment 228344 [details] [review] Updates based on comments
Created attachment 228345 [details] [review] Add support for overriding GObject.Object (updates based on feedback)
Created attachment 228346 [details] [review] Replace GObject notify methods with introspection and python (rebased)
Comment on attachment 228344 [details] [review] Updates based on comments Looks nice, thanks! Just one nitpick: This test case calls bind_property_full() with an invalid signature: + with self.assertRaisesRegex(RuntimeError, 'This method is currently unsupported.*'): + obj.bind_property_full() It shouldn't matter much, but I think it's better to use force_floating() as that takes no arguments. + compat_control = _unsupported_method Where does that come from? It's not official GObject API.
(In reply to comment #13) > (From update of attachment 228344 [details] [review]) > Looks nice, thanks! > > Just one nitpick: This test case calls bind_property_full() with an invalid > signature: > > + with self.assertRaisesRegex(RuntimeError, 'This method is currently > unsupported.*'): > + obj.bind_property_full() Will do. > It shouldn't matter much, but I think it's better to use force_floating() as > that takes no arguments. > > + compat_control = _unsupported_method > > Where does that come from? It's not official GObject API. Not sure but it is in there: import gi.module info = gi.module.repository.find_by_name('GObject', 'Object') info.get_methods() (... <gi.FunctionInfo object (compat_control) at 0x0x7ff73c140bd8>, ...)
Created attachment 228347 [details] [review] Add support for overriding GObject.Object (test force_floating)
Comment on attachment 228347 [details] [review] Add support for overriding GObject.Object (test force_floating) Looks great now, thanks!