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 787271 - Make GListModel usable from G-I bindings
Make GListModel usable from G-I bindings
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: introspection
2.53.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on: 787692
Blocks:
 
 
Reported: 2017-09-04 22:05 UTC by Sam Thursfield
Modified: 2017-09-26 12:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make GListModelInterface::get_item usable from GObject Introspection bindings (2.29 KB, patch)
2017-09-04 22:05 UTC, Sam Thursfield
committed Details | Review
Testcase for implementing GListModel interface in Python (973 bytes, text/plain)
2017-09-04 22:15 UTC, Sam Thursfield
  Details
glistmodel: Don't skip g_list_model_get_item from introspection (1.15 KB, patch)
2017-09-13 13:12 UTC, Rico Tzschichholz
none Details | Review

Description Sam Thursfield 2017-09-04 22:05:14 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.
Comment 1 Sam Thursfield 2017-09-04 22:05:57 UTC
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.
Comment 2 Sam Thursfield 2017-09-04 22:15:53 UTC
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!
Comment 3 Philip Withnall 2017-09-08 15:23:16 UTC
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?
Comment 4 Lars Karlitski 2017-09-12 10:10:35 UTC
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.
Comment 5 Philip Withnall 2017-09-12 10:23:32 UTC
(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.
Comment 6 Lars Karlitski 2017-09-12 10:46:55 UTC
> 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.
Comment 7 Philip Withnall 2017-09-12 11:03:39 UTC
(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?
Comment 8 Lars Karlitski 2017-09-12 11:23:10 UTC
> 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.
Comment 9 Philip Withnall 2017-09-12 11:31:42 UTC
(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?
Comment 10 Philip Withnall 2017-09-12 11:32:52 UTC
Attachment 359112 [details] pushed as bb26bc2 - Make GListModelInterface::get_item usable from GObject Introspection bindings
Comment 11 Rico Tzschichholz 2017-09-13 13:12:14 UTC
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.
Comment 12 Philip Withnall 2017-09-13 13:19:15 UTC
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.
Comment 13 Philip Withnall 2017-09-13 13:19:52 UTC
(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?
Comment 14 Sam Thursfield 2017-09-14 09:26:09 UTC
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!
Comment 15 Sam Thursfield 2017-09-14 18:12:14 UTC
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).
Comment 16 Sam Thursfield 2017-09-14 18:28:44 UTC
Proposed g-ir-scanner fix: https://bugzilla.gnome.org/show_bug.cgi?id=787692
Comment 17 Sam Thursfield 2017-09-14 18:32:02 UTC
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'...
Comment 18 Philip Withnall 2017-09-15 10:43:21 UTC
(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)?
Comment 19 Rico Tzschichholz 2017-09-26 12:44:26 UTC
Ok, leave it as it is.