Created attachment 161850 [details] [review]
Proof of concept for wrapping GVariant and GSettings

Forgot to mention that this assumes that the GVariant struct is marked as foreign, which I don't think is correct.

(In reply to comment #2)
> Forgot to mention that this assumes that the GVariant struct is marked as
> foreign, which I don't think is correct.

Colin and Johan think this could be correct after all.

Extracted from the overview of changes from GLib 2.25.7 to GLib 2.25.8:

* GVariant
 - added introspection annotations

* GSettings:
 - added introspection annotiations

What's the next steps to fully support it in pygi?

Thanks!

Created attachment 163181 [details] [review]
Add support for GVariant

* gi/pygi-foreign*.[hc]: Add wrapper functions for GVariant and
  remove the *release_arg functions in favor of the simpler
  gboolean (*PyGIArgOverrideReleaseFunc) (GIBaseInfo *base_info,
                                          gpointer struct_).

* gi/pygi-invoke.c: Use pygi_struct_foreign_convert_from_g_argument
  when a function returns a foreign struct.

* gi/pygi-struct.c: properly release foreign structs.

(In reply to comment #4)
> Extracted from the overview of changes from GLib 2.25.7 to GLib 2.25.8:
> 
> * GVariant
>  - added introspection annotations
> 
> * GSettings:
>  - added introspection annotiations
> 
> What's the next steps to fully support it in pygi?
Note this merely means "add some introspection annotations" to these objects. I only added a few things we needed about _get_strv() and _set_strv() functions, so many more annotations may be needed.

(Though for GSettings at least, basic support works fine.)

Comment on attachment 163181 [details] [review]
Add support for GVariant


>diff --git a/gi/pygi-struct.c b/gi/pygi-struct.c
>index 2f1ce42..e8f12d6 100644
>--- a/gi/pygi-struct.c
>+++ b/gi/pygi-struct.c
>@@ -29,11 +29,17 @@
> static void
> _struct_dealloc (PyGIStruct *self)
> {
>+    GIBaseInfo *info = _pygi_object_get_gi_info (
>+                           (PyObject *) ( (PyObject *) self)->ob_type,
>+                           &PyGIStructInfo_Type);
>+
>     PyObject_GC_UnTrack ( (PyObject *) self);
> 
>     PyObject_ClearWeakRefs ( (PyObject *) self);
> 
>-    if (self->free_on_dealloc) {
>+    if (info != NULL && g_struct_info_is_foreign ( (GIStructInfo *) info)) {
>+        pygi_struct_foreign_release (info, ( (PyGPointer *) self)->pointer);
>+    } else if (self->free_on_dealloc) {
>         g_free ( ( (PyGPointer *) self)->pointer);
>     }
> 

info most likely needs to be released here

The rest of the patch looks good.  It would be nice to have a GVariant, gdbus and GSettings demos to go along with the tests.  Also the tests should most likely check the whole GVariant typelib to make sure marshalling happens as we expect it.  Otherwise I would say fix up the missing unref and commit with some comments in the code about expanding the tests.

Could you have a look at bug 622294 to check that the annotations I added seem right/useful? With them, most of GVariant should be annotated.

(In reply to comment #9)
> Could you have a look at bug 622294 to check that the annotations I added seem
> right/useful? With them, most of GVariant should be annotated.

Could someone take this patch and run with it? I'm too behind with other stuff :(

Hi Tomeu

Seems that this commit did not include all the files in the patch, and broke my jhbuild process:
http://git.gnome.org/browse/pygobject/commit/?id=e65275bc57f345c111eb12a6b4476ff1ddc3bc24

Could you commit the gi/pygi-foreign-gvariant.c and gi/pygi-foreign-gvariant.h files too?

They're there now.