GNOME Bugzilla – Bug 644926
We need to support calling callbacks from python
Last modified: 2013-01-10 15:50:12 UTC
Created attachment 183546 [details] offscreen window example In order to properly support subclassing containers with custom internal widgets we need to support gtk_container_forall. This means supporting passing callbacks that can be invoked from python. These callback should be marked as scope call or scope async. I don't think scope destroy notify would work. Attach is a demo app which hooks up to forall. in the forall handler we should be doing callback(data) or something to that effect (you can look at the effects gtk3-demo code to see how forall is used in C). You will need the latests Gtk from head along with the patch in https://bugzilla.gnome.org/show_bug.cgi?id=644736 You will also need the patch from https://bugzilla.gnome.org/show_bug.cgi?id=644749 for GObject Introspection You might be able to get away without applying the patches if you just create a class which derives from Gtk.Container and overrides forall: class test(Gtk.Container): __gtype_name__="TestClass" def do_forall(self, include_internals, callback, data): callback(data)
Created attachment 183775 [details] [review] [gi] add a ccallback type which is used to invoke callbacks passed to a vfunc * Not done yet - this is just to show progress * Used when overriding methods like gtk_container_forall wich pass in a callback that needs to be executed on internal children: def do_forall(self, callback, userdata): callback(self.custom_child, userdata) * Currently can call callback but getting userdata requires we move the creation of the callback to invoke instead of arguments * it also requires a patch to gobject introspection which makes g_callable_info_invoke public * the code is kind of ugly because of the need to add this to invoke so the idea would be to move g_callable_info_invoke into pygi so that we can have full dubclassing abilities for the 2.28 branch. In master, I need to clean up the invoke rewrite, get that merged and then we can have our own simpler ffi_invoke layer - cleaning up the code quite a bit
Created attachment 183776 [details] [review] make g_callable_info_invoke public so we can call callbacks passed to a function
Not sure if I want to submit the GI patch since I want to move away from using the inefficient GI invoke calls and eventually marshal directly to ffi but for now this is the easiest way to test the pygi patch
There is an issue with converting the userdata since it is a gpointer. We need a way to wrap gpointers so that when it is passed to a gpointer parameter it is extracted as the raw pointer instead of passing the pyobject pointer. This is tough since if we are passing the pointer to a python callback we want it to be a pyobject but if it is a C callback it has to be the raw pointer (right now we pass the pyobject pointer always).
The other option is to store the userdata and pass it implicitly except there is no way to be 100% sure which item in a callback's type signature is the userdata parameter. We can use heuristics like checking if a gpointer parameter is called "data", "userdata" or "user_data" but that is a hack at best.
Created attachment 183948 [details] [review] [gi] add a ccallback type which is used to invoke callbacks passed to a vfunc * Not done yet - this is just to show progress * Used when overriding methods like gtk_container_forall wich pass in a callback that needs to be executed on internal children: def do_forall(self, callback, userdata): callback(self.custom_child, userdata) * Currently can call callback but getting userdata requires we move the creation of the callback to invoke instead of arguments * it also requires a patch to gobject introspection which makes g_callable_info_invoke public * the code is kind of ugly because of the need to add this to invoke so the idea would be to move g_callable_info_invoke into pygi so that we can have full dubclassing abilities for the 2.28 branch. In master, I need to clean up the invoke rewrite, get that merged and then we can have our own simpler ffi_invoke layer - cleaning up the code quite a bit
Ok, I'm not going to fix this right now but have attached a second version of the patch as an example of how we should be fixing this as I revert my changes locally so I can get back to working on the the invoke-rewrite branch which should make this change a bit easier.
Created attachment 200288 [details] [review] Add a ccallback type which is used to invoke callbacks passed to a vfunc Used when overriding methods like gtk_container_forall wich pass in a callback that needs to be executed on internal children: def do_forall(self, callback, userdata): callback(self.custom_child, userdata) https://bugzilla.gnome.org/show_bug.cgi?id=644926 Co-authored-by: Tomeu Vizoso <tomeu.vizoso@collabora.co.uk>
It works! \o/
Comment on attachment 183776 [details] [review] make g_callable_info_invoke public so we can call callbacks passed to a function It's in https://bugzilla.gnome.org/show_bug.cgi?id=663052
You rock tomeu. I'll test tonight. We are getting to the point where we can release an unstable for GNOME 3.4. Getting closer to feature complete with this patch.
Hi Tomeu, pygi-ccallback.h is missing from the patch. Can you please repost?
(In reply to comment #12) > Hi Tomeu, > pygi-ccallback.h is missing from the patch. Can you please repost? Sorry, must have lost it when I updated my distro, but I guess it can be easily reconstructed, or even it may be the same from John's original patch.
Created attachment 203270 [details] [review] Add a ccallback type which is used to invoke callbacks passed to a vfunc The matching .c is also missing from the patch. I took John's one from one of the earlier patches above and tweaked it to compile (please check). Unfortunately this doesn't work, it causes a segfault.
Created attachment 203271 [details] [review] gobject-introspection patch The above patch adds a test case, but this test case relies on a gobject-introspection change that was also lost. Here is my attempt to reconstruct it. With this applied, the segfault can be trivially reproduced with "make check". I've spent several hours running around on the dark here without success. John/Tomeu, I hope you can find a bit of time to see where I'm going wrong.
Created attachment 204175 [details] [review] Add a ccallback type which is used to invoke callbacks passed to a vfunc Used when overriding methods like gtk_container_forall wich pass in a callback that needs to be executed on internal children: def do_forall(self, callback, userdata): callback(self.custom_child, userdata) https://bugzilla.gnome.org/show_bug.cgi?id=644926 Co-authored-by: Tomeu Vizoso <tomeu.vizoso@collabora.co.uk>
Comment on attachment 203271 [details] [review] gobject-introspection patch These tests are now in https://bugzilla.gnome.org/show_bug.cgi?id=663052
I think this should work, we need reviews now (same for the patches in https://bugzilla.gnome.org/show_bug.cgi?id=663052 )
Review of attachment 204175 [details] [review]: Not the best one to review this as I don't really understand how pygi invokation works these days. ::: gi/pygi-ccallback.c @@ +75,3 @@ + self->destroy_notify_func = destroy_notify; + self->info = info; + g_base_info_ref( (GIBaseInfo *) info); This is never freed and the return value should be saved in self->info @@ +83,3 @@ +_ccallback_dealloc (PyGIBoxed *self) +{ + This might be a good place to free self->info. ::: gi/pygi-closure.c @@ +250,3 @@ + user_data = g_args[user_data_arg].v_pointer; + + else if ? And perhaps assert so that user_data_arg and destroy_notify_arg is never both != -1 ::: tests/test_gi.py @@ +1672,3 @@ + _object = SubObject() + _object.call_vfunc_with_callback() + Makes sense to try to call it more than once. This should probably be valgrinded, as it'll leak.
Created attachment 208119 [details] [review] Add a ccallback type which is used to invoke callbacks passed to a vfunc (2) I addressed the comments from Johan, comments in reply.
(In reply to comment #19) > Review of attachment 204175 [details] [review]: > > Not the best one to review this as I don't really understand how pygi > invokation works these days. > > ::: gi/pygi-ccallback.c > @@ +75,3 @@ > + self->destroy_notify_func = destroy_notify; > + self->info = info; > + g_base_info_ref( (GIBaseInfo *) info); > > This is never freed and the return value should be saved in self->info Did save the return value in self->info. > @@ +83,3 @@ > +_ccallback_dealloc (PyGIBoxed *self) > +{ > + > > This might be a good place to free self->info. Did unref self->info in there. > ::: gi/pygi-closure.c > @@ +250,3 @@ > + user_data = g_args[user_data_arg].v_pointer; > + > + > > else if ? > And perhaps assert so that user_data_arg and destroy_notify_arg is never both > != -1 Hmm, not sure about this one. In _callback_cache_new in pygi-cache.c we have the same code. If we error check we should agree on the intended behavior and do handle it in both cases. > ::: tests/test_gi.py > @@ +1672,3 @@ > + _object = SubObject() > + _object.call_vfunc_with_callback() > + > > Makes sense to try to call it more than once. I do call it twice now as a quick check improvement. > This should probably be valgrinded, as it'll leak.
Created attachment 210680 [details] [review] New patch which includes gi/pygi-ccallback.c and gi/pygi-ccallback.h Those files were present locally, why building did pass for me.
> I do call it twice now as a quick check improvement. I improved the test case to reset _object.worked to False before the second call, to ensure that it really works. I can't claim I fully understand this patch. I grok half of it, the other half at least looks reasonable to me. But it's been through a fair number of reviews, has tests, I ran some of our GI programs against it, and I can't break it. So let's get this in now, as we are basically on day 1 of the new GNOME cycle. Thanks!
Hmm there seem to be still an issue with this one, I am looking at forall from the Gtk.Container [1][2]. With the static bindings we did the following: {{{ def do_forall(self, include_internals, callback, callback_data): if self.child: print include_internals, callback, callback_data callback(self.child, callback_data) }}} The printed output is the following: "True <built-in function GtkContainer.do_forall callback> <PyCObject object at 0x9750cb0>" With the dynamic bindings if I do the above in my custom container I do get the error: "TypeError: do_forall() takes exactly 4 arguments (3 given)" If I remove one argument and do "do_forall(self, include_internals, callback)" the first two arguments are: "True <gi.CCallback object at 0xb714fc20>". So we miss the user data here that needs to be passed to the callback (the test in pygobject actually passes no data to the callback but that does not work here TEST_NAMES="test_gi.TestPythonGObject.test_callback_in_vfunc"). The annotation says: {{{ <virtual-method name="forall" invoker="forall"> <doc xml:whitespace="preserve">Invokes @callback on each child of @container, including children that are considered "internal" (implementation details of the container). "Internal" children generally weren't added by the user of the container, but were added by the container implementation itself. Most applications should use gtk_container_foreach(), rather than gtk_container_forall().</doc> <return-value transfer-ownership="none"> <type name="none" c:type="void"/> </return-value> <parameters> <parameter name="include_internals" transfer-ownership="none"> <type name="gboolean" c:type="gboolean"/> </parameter> <parameter name="callback" transfer-ownership="none" scope="call" closure="2"> <doc xml:whitespace="preserve">a callback</doc> <type name="Callback" c:type="GtkCallback"/> </parameter> <parameter name="callback_data" transfer-ownership="none"> <doc xml:whitespace="preserve">callback user data</doc> <type name="gpointer" c:type="gpointer"/> </parameter> </parameters> }}} and {{{ <method name="forall" c:identifier="gtk_container_forall"> <doc xml:whitespace="preserve">Invokes @callback on each child of @container, including children that are considered "internal" (implementation details of the container). "Internal" children generally weren't added by the user of the container, but were added by the container implementation itself. Most applications should use gtk_container_foreach(), rather than gtk_container_forall().</doc> <return-value transfer-ownership="none"> <type name="none" c:type="void"/> </return-value> <parameters> <parameter name="callback" transfer-ownership="none" scope="call" closure="1"> <doc xml:whitespace="preserve">a callback</doc> <type name="Callback" c:type="GtkCallback"/> </parameter> <parameter name="callback_data" transfer-ownership="none"> <doc xml:whitespace="preserve">callback user data</doc> <type name="gpointer" c:type="gpointer"/> </parameter> </parameters> </method> }}} Maybe someone has an idea. [1] http://developer.gnome.org/gtk/2.24/GtkContainer.html#gtk-container-forall [2] http://developer.gnome.org/gtk3/3.4/GtkContainer.html#gtk-container-forall
(In reply to comment #24) > Hmm there seem to be still an issue with this one, I am looking at forall from > the Gtk.Container [1][2]. > > With the static bindings we did the following: > {{{ > def do_forall(self, include_internals, callback, callback_data): > if self.child: > print include_internals, callback, callback_data > callback(self.child, callback_data) > }}} > > The printed output is the following: "True <built-in function > GtkContainer.do_forall callback> <PyCObject object at 0x9750cb0>" > > With the dynamic bindings if I do the above in my custom container I do get the > error: "TypeError: do_forall() takes exactly 4 arguments (3 given)" > > If I remove one argument and do "do_forall(self, include_internals, callback)" > the first two arguments are: "True <gi.CCallback object at 0xb714fc20>". > > So we miss the user data here that needs to be passed to the callback (the test > in pygobject actually passes no data to the callback but that does not work > here TEST_NAMES="test_gi.TestPythonGObject.test_callback_in_vfunc"). What do you mean by "does not work" and by "here"? As you can see, the test checks that the data is correct when the callback gets called: http://git.gnome.org/browse/gobject-introspection/tree/tests/gimarshallingtests.c#n3858 > Maybe someone has an idea. I would try to find what's the difference between gtk+ and the tests in g-i. Also may be a good idea to check with git-blame if the annotations in gtk+ have been changed since this was fixed in pygobject.
(In reply to comment #25) > (In reply to comment #24) > > Hmm there seem to be still an issue with this one, I am looking at forall from > > the Gtk.Container [1][2]. > > > > With the static bindings we did the following: > > {{{ > > def do_forall(self, include_internals, callback, callback_data): > > if self.child: > > print include_internals, callback, callback_data > > callback(self.child, callback_data) > > }}} > > > > The printed output is the following: "True <built-in function > > GtkContainer.do_forall callback> <PyCObject object at 0x9750cb0>" > > > > With the dynamic bindings if I do the above in my custom container I do get the > > error: "TypeError: do_forall() takes exactly 4 arguments (3 given)" > > > > If I remove one argument and do "do_forall(self, include_internals, callback)" > > the first two arguments are: "True <gi.CCallback object at 0xb714fc20>". > > > > So we miss the user data here that needs to be passed to the callback (the test > > in pygobject actually passes no data to the callback but that does not work > > here TEST_NAMES="test_gi.TestPythonGObject.test_callback_in_vfunc"). > > What do you mean by "does not work" and by "here"? In the do_forall method in my custom container example in GTK+ 3 there are 3 arguments passed. From printing them out it is the 'include_internals' boolean and the 'gi.CCallback'. The callback data which should be passed to the callback is not passed. > As you can see, the test checks that the data is correct when the callback gets > called: > > http://git.gnome.org/browse/gobject-introspection/tree/tests/gimarshallingtests.c#n3858 > > > Maybe someone has an idea. > > I would try to find what's the difference between gtk+ and the tests in g-i. > Also may be a good idea to check with git-blame if the annotations in gtk+ have > been changed since this was fixed in pygobject. I did check the annotations, the last edit was from John in 2011 annotating the forall vfunc. http://git.gnome.org/browse/gtk+/commit/?id=3938d3c2e44d4ab1ba32bac1055dd6a4c8d7f307
Created attachment 216650 [details] Simple one child container in GTK+ 3 Example of usage of the forall method.
Created attachment 217398 [details] [review] GTK+ patch I think this last issue rather lies in GTK+, I'm attaching a patch to mark the data in a GtkCallback as (closure) so it's transparent to forall(), but is still passed to the callback itself.
Created attachment 217399 [details] Updated testcase checking behavior Tomeu, I take it that's how forall() is meant to behave, right? should we reassing to GTK+? or better open a new bug? (this one seems to have some no longer relevant history :)
Comment on attachment 217398 [details] [review] GTK+ patch Thanks Carlos, this GTK+ patch looks fine to me. Please commit.
Comment on attachment 217398 [details] [review] GTK+ patch Apparently this was committed long ago.