After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 602736 - Allow implementing vfuncs in python
Allow implementing vfuncs in python
Status: RESOLVED FIXED
Product: pygi
Classification: Deprecated
Component: general
unspecified
Other All
: Normal normal
: 0.6
Assigned To: pygi-maint
pygi-maint
Depends on:
Blocks:
 
 
Reported: 2009-11-23 14:54 UTC by Tomeu Vizoso
Modified: 2010-04-18 17:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow implementing vfuncs in python (16.68 KB, patch)
2009-11-23 14:54 UTC, Tomeu Vizoso
none Details | Review
Allow implementing vfuncs in python (23.99 KB, patch)
2009-11-30 10:20 UTC, Tomeu Vizoso
none Details | Review
Allow implementing vfuncs in python (27.19 KB, patch)
2009-12-01 12:50 UTC, Tomeu Vizoso
none Details | Review
Allow implementing vfuncs in python (26.77 KB, patch)
2009-12-01 17:13 UTC, Tomeu Vizoso
none Details | Review
Allow implementing vfuncs in python (26.79 KB, patch)
2009-12-03 17:50 UTC, Tomeu Vizoso
none Details | Review
Allow implementing vfuncs in python (only interfaces are supported now) (4.18 KB, patch)
2010-03-08 17:26 UTC, Abderrahim Kitouni
none Details | Review
Implement vfuncs. (15.22 KB, patch)
2010-04-18 14:35 UTC, Tomeu Vizoso
committed Details | Review

Description Tomeu Vizoso 2009-11-23 14:54:45 UTC
Rough patch attached, comments on the approach and other generalities most welcome.
Comment 1 Tomeu Vizoso 2009-11-23 14:54:48 UTC
Created attachment 148318 [details] [review]
Allow implementing vfuncs in python
Comment 2 Abderrahim Kitouni 2009-11-29 19:47:22 UTC
Review of attachment 148318 [details] [review]:

I haven't tested this because it didn't apply, do I need any other patch?
(a few comments on the idea)

::: configure.ac
@@ +48,3 @@
 AC_SUBST(INTROSPECTION_COMPILER) 
 
+PKG_CHECK_MODULES(FFI, libffi >= 3.0)

is this really necessary? doesn't using g-i automatically pull this (because of girffi)

::: gi/gimodule.c
@@ +255,3 @@
+
+static PyObject *
+_wrap_pyg_hook_up_interface_implementor (PyObject *self, PyObject *args)

I feel the initialization here should just go in initialize_interface (which is empty right now) rather than making a new function.
Comment 3 Tomeu Vizoso 2009-11-30 10:11:46 UTC
(In reply to comment #2)
> Review of attachment 148318 [details] [review]:
> 
> I haven't tested this because it didn't apply, do I need any other patch?
> (a few comments on the idea)

Yeah sorry, need to rebase it. I will have to refactor the code around Zach's stuff in https://bugzilla.gnome.org/show_bug.cgi?id=603095

> ::: configure.ac
> @@ +48,3 @@
>  AC_SUBST(INTROSPECTION_COMPILER) 
> 
> +PKG_CHECK_MODULES(FFI, libffi >= 3.0)
> 
> is this really necessary? doesn't using g-i automatically pull this (because of
> girffi)

But we still need the CFLAGS, etc, right?

> ::: gi/gimodule.c
> @@ +255,3 @@
> +
> +static PyObject *
> +_wrap_pyg_hook_up_interface_implementor (PyObject *self, PyObject *args)
> 
> I feel the initialization here should just go in initialize_interface (which is
> empty right now) rather than making a new function.

The other day decided to only wrap the functions that have a python implementation, because otherwise we were marshalling calls C->Python->C when no marshalling was really needed (vfuncs implemented by a parent C class, for example).

So instead of _wrap_pyg_hook_up_interface_implementor we'll have _wrap_pyg_hook_up_vfunc_implementation.
Comment 4 Tomeu Vizoso 2009-11-30 10:20:46 UTC
Created attachment 148723 [details] [review]
Allow implementing vfuncs in python
Comment 5 Tomeu Vizoso 2009-11-30 10:22:19 UTC
This is my latest patch, rebased against HEAD. Next I plan to rebase it in top of Zach's native closures once it lands.
Comment 6 Tomeu Vizoso 2009-12-01 12:50:10 UTC
Created attachment 148822 [details] [review]
Allow implementing vfuncs in python

Depends on https://bugzilla.gnome.org/show_bug.cgi?id=603095
Comment 7 Tomeu Vizoso 2009-12-01 17:13:34 UTC
Created attachment 148836 [details] [review]
Allow implementing vfuncs in python

Methods starting with do_ implement vfuncs in interfaces and virtual classes.
Comment 8 Tomeu Vizoso 2009-12-03 17:50:37 UTC
Created attachment 149022 [details] [review]
Allow implementing vfuncs in python

This patch adds support for NULL *out args
Comment 9 Abderrahim Kitouni 2010-03-08 17:26:17 UTC
Created attachment 155569 [details] [review]
Allow implementing vfuncs in python (only interfaces are supported now)

(I beleive classes could be done similarily using pyg_register_class_init)

This is what I meant in comment #2, I prefer this to adding a new function, and binding it to python since this is the pygobject way of doing things.

Thoughts?
Comment 10 Abderrahim Kitouni 2010-03-08 17:36:35 UTC
btw, I reworked argument handling and made a patch to bug 602736. This patch depends on the three patches there (that is, attachment 153710 [details] [review], attachment 155360 [details] [review] and attachment 155567 [details] [review]). So this patch contains only changes needed for interfaces.
Comment 11 Tomeu Vizoso 2010-04-18 14:35:54 UTC
Created attachment 159009 [details] [review]
Implement vfuncs.
Comment 12 Colin Walters 2010-04-18 15:48:22 UTC
Review of attachment 159009 [details] [review]:

::: gi/gimodule.c
@@ +177,3 @@
+    } else
+        struct_info = g_object_info_get_class_struct ((GIObjectInfo*)ancestor_info);
+

struct_info is leaked.

@@ +187,3 @@
+
+        field_info = g_struct_info_get_field (struct_info, i);
+

field_info gets leaked here.  We should probably add g_struct_info_load_field.

@@ +192,3 @@
+            continue;
+
+        type_info = g_field_info_get_type (field_info);

type_info is leaked; same for above.

@@ +193,3 @@
+
+        type_info = g_field_info_get_type (field_info);
+        interface_info = g_type_info_get_interface (type_info);

interface_info is leaked.  Also you should check that:

if (g_type_info_get_tag (type_info) != GI_TYPE_TAG_INTERFACE)
  continue;

otherwise if you did do_foo where foo was actually an integer field, we'd assert and crash.

@@ +201,3 @@
+
+        closure = _pygi_make_native_closure((GICallableInfo*)callback_info,
+            GI_SCOPE_TYPE_NOTIFIED, py_function, NULL);

The closure here just lives forever then?  Probably OK in the short term, but we should tie it to the Python class' lifetime I think.

@@ +209,3 @@
+
+    if (!is_interface)
+        g_type_class_unref (implementor_class);

Why is the unref conditional on is_interface?  It looks like it always needs to be unref'd.
Comment 13 Zach Goldberg 2010-04-18 16:07:01 UTC
Review of attachment 159009 [details] [review]:

::: gi/gimodule.c
@@ +201,3 @@
+
+        closure = _pygi_make_native_closure((GICallableInfo*)callback_info,
+            GI_SCOPE_TYPE_NOTIFIED, py_function, NULL);

Also, I don't think you plan to ever actually use the destroy notify, right?  Perhaps we should implement GI_SCOPE_TYPE_INVALID as if it were scope type infinity?
Comment 14 Tomeu Vizoso 2010-04-18 17:13:29 UTC
The following fix has been pushed:
86bc737 Implement vfuncs.
Comment 15 Tomeu Vizoso 2010-04-18 17:13:47 UTC
The following fix has been pushed:
86bc737 Implement vfuncs.