Created attachment 148318 [details] [review]
Allow implementing vfuncs in python

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.

(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.

Created attachment 148723 [details] [review]
Allow implementing vfuncs in python

This is my latest patch, rebased against HEAD. Next I plan to rebase it in top of Zach's native closures once it lands.

Created attachment 148822 [details] [review]
Allow implementing vfuncs in python

Depends on https://bugzilla.gnome.org/show_bug.cgi?id=603095

Created attachment 148836 [details] [review]
Allow implementing vfuncs in python

Methods starting with do_ implement vfuncs in interfaces and virtual classes.

Created attachment 149022 [details] [review]
Allow implementing vfuncs in python

This patch adds support for NULL *out args

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?

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.

Created attachment 159009 [details] [review]
Implement vfuncs.

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.

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?

The following fix has been pushed:
86bc737 Implement vfuncs.

The following fix has been pushed:
86bc737 Implement vfuncs.