GNOME Bugzilla – Bug 656554
Marshalling GVariants does not work for closures
Last modified: 2012-05-14 11:32:24 UTC
Created attachment 193853 [details] test case In bug 656325 we try to make Gio fully introspectable, for providing GDBus support in Python (bug 656330). The initial patch there used callbacks, which were rather easy to fix with GVariant arguments and return values (18a240cc492 and 5189b360c). However, now that bug 656325 requests using closures, we have the same problem there: calling a closure with a GVariant argument currently just yields a TypeError, from http://git.gnome.org/browse/pygobject/tree/gi/_gobject/pygtype.c?id=081dc2eb#n1153 Adding variant support there is surprisingly difficult, as this is using its own marshaller; _pygi_struct_new() is not available there, as _gobject.so does not link against _gi.so. I also tried to monkey-add an equivalent using the _gobject API: PyObject* pyg_variant_new (GVariant* variant) { PyGIStruct *self; /* PyTypeObject *type = (PyTypeObject *) _pygi_type_get_from_g_type(G_TYPE_VARIANT); */ PyTypeObject *type = (PyTypeObject *) pyg_type_wrapper_new(G_TYPE_VARIANT); printf("pyg_variant_new: got type %p\n", type); self = (PyGIStruct *) type->tp_alloc(type, 0); printf("pyg_variant_new: got self %p\n", self); if (self == NULL) { return NULL; } ( (PyGPointer *) self)->gtype = G_TYPE_VARIANT; ( (PyGPointer *) self)->pointer = variant; self->free_on_dealloc = TRUE; return (PyObject *) self; } But that just crashed, apparently pyg_type_wrapper_new() doesn't quite work here. John says that the big and best solution would be to get rid of all the marshalling in _gobject and just use the new one from gi/pygi-marshal-*, but all of this would need to move from _gi.so to gi/_glib/ first, together with quite some refactorization. I added a test method to g-i's Regress (in master already), and prepared a test case. It shows that closures with simple arguments do work, but not with GVariant arguments/return type.
I committed the test case with marking the gvariant part as EXFAIL. This ensures that the other part (passing int around) will keep working, and also makes it easier to test patches for this bug, by merely removing the @expectedFailure decorator.
Created attachment 213532 [details] [review] Support marshalling of GVariants for closures On today's plane ride I had a fresh look at this, and figured it out. There it is, review appreciated!
Review of attachment 213532 [details] [review]: The rest looks good though. ::: gi/_gobject/pygtype.c @@ +995,3 @@ + if (obj == Py_None) + g_value_set_variant(value, NULL); + else if (g_strcmp0(obj->ob_type->tp_name, "Variant") == 0) This is quite ugly. It'd be better to import the PyGVariant_Type from gi.repository.GLib and do a proper isinstance() here.
(In reply to comment #3) > + else if (g_strcmp0(obj->ob_type->tp_name, "Variant") == 0) > > This is quite ugly. I agree. > It'd be better to import the PyGVariant_Type from gi.repository.GLib We can duplicate the logic from _pygi_type_import_by_name() for GLib.Variant certainly, but that means having to import gi.repository.GLib into _gobject just for that. This seems much more expensive, and I'm not sure whether it can have bad side effects? (i. e. _gobject importing GLib and GLib importing _gobject)
Created attachment 213769 [details] [review] Support marshalling of GVariants for closures Here is a cleaner variant which imports GLib.Variant properly instead of just doing string comparison on the PyObject type.
Created attachment 213770 [details] [review] Support marshalling of GVariants for closures This cleans up the error handling. No need to manually set errors, PyImport_ImportModule() and PyObject_GetAttrString() already do that for us. This also fixes pyg_closure_marshal() to only set the generic "can't convert return value to desired type" error when we don't already have one. With that, you will now see test_variant (test_everything.TestClosures) ... Traceback (most recent call last):
+ Trace 230202
attr = super(DynamicGLibModule, self).__getattr__(name)
return getattr(self._introspection_module, name)
self.__name__, name))
ERROR if the import fails (I temporarily damaged the code to import "Variantt" to test this).
Argh, I accidentally pushed this already during bugzilla/patch review this morning. I think it's good, but it wasn't officially stamped by another developer.