GNOME Bugzilla – Bug 787271
Make GListModel usable from G-I bindings
Last modified: 2017-09-26 12:44:26 UTC
I found that implementing the Gio.ListModel interface in Python was impossible, because the get_item() vfunc is marked as returning 'gpointer' which G-I treats as 'void'. See https://bugzilla.gnome.org/show_bug.cgi?id=786508 for the full story. It seems that we can fix this just by correcting the introspection annotations in glistmodel.c.
Created attachment 359112 [details] [review] Make GListModelInterface::get_item usable from GObject Introspection bindings Language bindings have so far been unable to implement the GListModel interface because the ::get_item virtual function returns a non-bindable type (gpointer). The `gpointer` type gets translated into `void` by G-I meaning that get_item() implementations can't return any items. We can set the return type of the get_item() vfunc explicitly to GObject, which fixes the issue. This patch also removes the existing (type GObject) annotation on g_list_model_get_item(), which is necessary because if its return type matches that of the get_item() vfunc, G-I connects the two and propagates the 'skip' annotation from one to the other resulting in the get_item() vfunc being hidden. There's no API break here because the 'skip' annotation makes g_list_model_get_item() invisible to G-I users anyway.
Created attachment 359113 [details] Testcase for implementing GListModel interface in Python Here is a testcase using pygobject. Without my patch, this will either fail at an assertion, or segfault some point before that. Testing this patch is a little tricky because the introspection data for GLib is copied into gobject-introspection and built there. If you are using jhbuild, try this (assuming you are in your source directory, and glib and gobject-introspection are both checked out there): jhbuild buildone gobject-introspection cd gobject-introspection/misc ./update-glib-annotations.py ../../glib ~/.cache/jhbuild/build/gobject-introspection/ jhbuild buildone gobject-introspection Run the testcase again under `jhbuild shell` and watch it succeed!
Review of attachment 359112 [details] [review]: This looks good to me. > This patch also removes the existing (type GObject) annotation on > g_list_model_get_item(), which is necessary because if its return type > matches that of the get_item() vfunc, G-I connects the two and > propagates the 'skip' annotation from one to the other resulting in the > get_item() vfunc being hidden. That sounds like a bug in gobject-introspection — it probably shouldn’t be connecting the two symbols if one of them is marked as (skip) and the other isn’t. Could you prepare a separate patch for that (in a new bug) please?
The idea was that bindings implement get_object() instead of get_item(), because get_item() returns a untyped pointer. The type is known from g_list_model_get_type(), but we cannot tell introspection that. Allison had some idea how to fix that in introspection. I think it came down to needing generic types. I suggest leaving it as is and using get_object() from bindings until generics are available.
(In reply to Lars Karlitski from comment #4) > The idea was that bindings implement get_object() instead of get_item(), > because get_item() returns a untyped pointer. The type is known from > g_list_model_get_type(), but we cannot tell introspection that. Allison had > some idea how to fix that in introspection. I think it came down to needing > generic types. > > I suggest leaving it as is and using get_object() from bindings until > generics are available. I think this bug is about implementing the GListModel interface on a class in a bound language. GListModelInterface has a get_item() vfunc but not a get_object() vfunc. Unless I’m misunderstanding, your suggestions all relate to the GListModel methods which consumers of a GListModel implementation should call — the patch doesn’t really touch those.
> GListModelInterface has a get_item() vfunc but not a get_object() vfunc. Ah, I missed that, sorry. It's been a while. The problem with returning GObject now is that we won't be able to change it to a broader type later on. But I don't know what the plan is for generic types and not being able to implement it at all from bindings is pretty bad. I've added Allison to this bug in case she knows more.
(In reply to Lars Karlitski from comment #6) > > GListModelInterface has a get_item() vfunc but not a get_object() vfunc. > > Ah, I missed that, sorry. It's been a while. > > The problem with returning GObject now is that we won't be able to change it > to a broader type later on. But I don't know what the plan is for generic > types and not being able to implement it at all from bindings is pretty bad. > > I've added Allison to this bug in case she knows more. Are you in favour of this patch being pushed or not?
> Are you in favour of this patch being pushed or not? I don't feel comfortable making this decision, as I haven't been active in GLib for a while. I lean towards yes, as I don't think we'll see much movement in the type system in the near future and it solves a problem people are having right now.
(In reply to Lars Karlitski from comment #8) > > Are you in favour of this patch being pushed or not? > > I don't feel comfortable making this decision, as I haven't been active in > GLib for a while. > > I lean towards yes, as I don't think we'll see much movement in the type > system in the near future and it solves a problem people are having right > now. Cool. I’ve pushed a load of introspection annotation changes recently, so in the interests of all the breakage (hopefully none) being discovered at the same time, I’d be in favour of pushing this now. Sam, can you please file a separate follow-up bug describing the situation around the conflation of the two gtk-doc comments mentioned in comment #3?
Attachment 359112 [details] pushed as bb26bc2 - Make GListModelInterface::get_item usable from GObject Introspection bindings
Created attachment 359720 [details] [review] glistmodel: Don't skip g_list_model_get_item from introspection Don't treat the vfunc and its wrapper differently, so keep "pretending" it always returns GLib.Object.
Brief writeup from a conversation on #introspection on IRC: for context, attachment #359112 [details] apparently breaks some of Vala’s expectations around GIRs, so there’s more work to be done here Emmanuele would prefer it if we changed get_item() to always return a GObject, rather than holding out for potentially eventually returning GBoxed types from it.
(In reply to Philip Withnall from comment #3) > > This patch also removes the existing (type GObject) annotation on > > g_list_model_get_item(), which is necessary because if its return type > > matches that of the get_item() vfunc, G-I connects the two and > > propagates the 'skip' annotation from one to the other resulting in the > > get_item() vfunc being hidden. > > That sounds like a bug in gobject-introspection — it probably shouldn’t be > connecting the two symbols if one of them is marked as (skip) and the other > isn’t. Could you prepare a separate patch for that (in a new bug) please? Hence it looks like we’re going to have to fix this as more of a priority. Sam, do you have time for that?
OK, I'll have a dig today and see if I can figure out the g-i issue. Thanks for pushing the patch through thus far!
With attachment #359112 [details] (my patch) merged in GLib, but without attachment #359720 [details] (Rico's patch), then if I do this... cd vala/vapi jhbuild run make gio-2.0 ...then I see a change in gio-2.0.vapi: `ListModel.get_item()` now returns a `void *` instead of a GObject, which is an API break for Vala code. I guess that's the issue that was discussed in IRC? This suggests `vapigen` is taking the return value from `g_list_model_get_item()` rather than the `ListModel->get_item()` vfunc, How else has it been marked as a `GObject *` up til now? That seems a bit wrong to me. However, we can fix g-ir-scanner to not associate methods with vfuncs where the method is skipped. That will allow Rico's patch to be merged without making the `get_item()` vfunc disappear completely from the .gir (which is what happens at present when it is applied).
Proposed g-ir-scanner fix: https://bugzilla.gnome.org/show_bug.cgi?id=787692
I just noticed that attachment #359720 [details] removes the (skip) annotation from g_list_model_get_item(). This doesn't break anything in practice, but I don't know if it's good practice to have a 'rename-to' without the corresponding 'skip'...
(In reply to Sam Thursfield from comment #17) > I just noticed that attachment #359720 [details] removes the (skip) > annotation from g_list_model_get_item(). This doesn't break anything in > practice, but I don't know if it's good practice to have a 'rename-to' > without the corresponding 'skip'... Yeah, Rico, why remove that (skip)?
Ok, leave it as it is.