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 746893 - gtk_list_box_bind_model is not introspectable
gtk_list_box_bind_model is not introspectable
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
3.16.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 726450
 
 
Reported: 2015-03-27 14:28 UTC by David Shea
Modified: 2015-06-02 15:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GtkListBox: fix model binding refcount issue (1.70 KB, patch)
2015-03-27 15:59 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description David Shea 2015-03-27 14:28:50 UTC
Please quit implementing features that are only usable in C.
Comment 1 Emmanuele Bassi (:ebassi) 2015-03-27 14:33:04 UTC
Please, instead of snappy remarks, could you please explain why is not bindable? Is it because of missing type information on the GtkListBoxCreateWidgetFunc closure?
Comment 2 Matthias Clasen 2015-03-27 14:46:20 UTC
In the gir, I see:

      <method name="bind_model"
              c:identifier="gtk_list_box_bind_model"
              version="3.16"
              introspectable="0">

It might be interesting to figure out if we can add 'bindability' tests to our testsuite.
Comment 3 Emmanuele Bassi (:ebassi) 2015-03-27 14:59:26 UTC
Likely because GtkListBoxCreateWidgetFunc is also marked as not introspectable:

    <callback name="ListBoxCreateWidgetFunc"
              c:type="GtkListBoxCreateWidgetFunc"
              version="3.16"
              introspectable="0">
      <doc xml:space="preserve">Called for list boxes that are bound to a #GListModel with
gtk_list_box_bind_model() for each item that gets added to the model.</doc>
      <return-value>
        <doc xml:space="preserve">a #GtkWidget that represents @item</doc>
        <type name="Widget" c:type="GtkWidget*"/>
      </return-value>
      <parameters>
        <parameter name="item" transfer-ownership="none">
          <doc xml:space="preserve">the item from the model for which to create a widget for</doc>
          <type name="gpointer" c:type="gpointer"/>
        </parameter>
        <parameter name="user_data" transfer-ownership="none" closure="1">
          <doc xml:space="preserve">user data</doc>
          <type name="gpointer" c:type="gpointer"/>
        </parameter>
      </parameters>
    </callback>
Comment 4 Emmanuele Bassi (:ebassi) 2015-03-27 15:02:38 UTC
Indeed, with this:

diff --git a/gtk/gtklistbox.h b/gtk/gtklistbox.h
index 4ff8383..a648ac8 100644
--- a/gtk/gtklistbox.h
+++ b/gtk/gtklistbox.h
@@ -171,13 +171,13 @@ typedef void (*GtkListBoxUpdateHeaderFunc) (GtkListBoxRow *row,
 
 /**
  * GtkListBoxCreateWidgetFunc:
- * @item: the item from the model for which to create a widget for
+ * @item: (type GObject): the item from the model for which to create a widget for
  * @user_data: (closure): user data
  *
  * Called for list boxes that are bound to a #GListModel with
  * gtk_list_box_bind_model() for each item that gets added to the model.
  *
- * Returns: a #GtkWidget that represents @item
+ * Returns: (transfer full): a #GtkWidget that represents @item
  *
  * Since: 3.16
  */

Both bind_model() and GtkListBoxCreateWidgetFunc are introspectable.

The transfer rules of the delegate function are fuzzy, though; the GtkListBox will just take the pointer, but it does not state whether or not the ownership of the returned pointer is meant to be transferred fully.
Comment 5 Emmanuele Bassi (:ebassi) 2015-03-27 15:31:14 UTC
So, it's apparently clear that the API (along with GListModel) is not really meant to be bindable through introspection — and minimally bindable only by open coding the callable trampoline.

I suspect this is going to be a WONTFIX, or it's going to be made dependant of an eventual bug for making introspection work with collections.
Comment 6 David Shea 2015-03-27 15:36:40 UTC
I'm not sure if this matches the implementation, but transfer full makes the most sense to me. The expectation is that the widget returned by the create func will be immediately added as a child of the ListBox (or added to a ListBoxRow and added to the ListBox), so it would be taking ownership of the reference initially held in the create func.
Comment 7 Allison Karlitskaya (desrt) 2015-03-27 15:59:19 UTC
Created attachment 300471 [details] [review]
GtkListBox: fix model binding refcount issue

As it is, GtkListBox model binding will work nicely as long as your
create_widget_func returns a floating reference on the newly-created
widget.

If you try to return a full reference (as any higher-level language
would do) then you will leak that reference.

Fix that up by converting any floating references into full references
and then unconditionally releasing the full reference after adding to
the box.
Comment 8 Emmanuele Bassi (:ebassi) 2015-03-27 16:02:28 UTC
Review of attachment 300471 [details] [review]:

Looks good. Thanks for the big comment on top.
Comment 9 Allison Karlitskaya (desrt) 2015-03-27 16:06:37 UTC
(In reply to Ryan Lortie (desrt) from comment #7)
> Created attachment 300471 [details] [review] [review]

This is technically an API break since we will change the behaviour in the case that the user gives us a full ref.  Previously, we would leak that ref, but now we will release it ("properly").  The only problem is if the user was somehow arranging to have that ref release later themselves.

Let's backport this to the stable branch before anyone notices.... it's only been a released stable API for a few days.
Comment 10 Allison Karlitskaya (desrt) 2015-03-27 16:07:50 UTC
Comment on attachment 300471 [details] [review]
GtkListBox: fix model binding refcount issue

Attachment 300471 [details] pushed as 6e03e7e - GtkListBox: fix model binding refcount issue
Comment 11 Allison Karlitskaya (desrt) 2015-03-27 16:08:33 UTC
...and pushed to gtk-3-16.
Comment 12 David Shea 2015-03-27 16:47:42 UTC
Hold up, though. I appreciate the refcount change, but GtkListBoxCreateWidgetFunc is still marked as introspectable=0 without something like the change in comment 4
Comment 13 Allison Karlitskaya (desrt) 2015-03-29 16:16:00 UTC
I'm sorry -- git-bz marked the bug as closed by accident.

I didn't mean to imply that it was fixed.

fwiw, I think the approach in comment 4 is approximately correct.
Comment 14 Debarshi Ray 2015-06-01 16:07:07 UTC
Review of attachment 300471 [details] [review]:

::: gtk/gtklistbox.c
@@ +3618,3 @@
+       * Finally, we'll release this full reference below, leaving only
+       * the one held by the box.
+       */

Actual use from gjs disagrees with this assessment. For example, see the WIP patch in bug 726450

Without this, the GtkListBoxRows are definitely not being leaked because I can see gtk_list_box_row_finalize being invoked by the GC.

On the other hand, this patch leads to a crash with "Finalizing proxy for an already freed object of type: GtkListBoxRow" when the GC runs.
Comment 15 Debarshi Ray 2015-06-01 16:07:33 UTC
Reopened.
Comment 16 Debarshi Ray 2015-06-02 15:20:07 UTC
(In reply to Debarshi Ray from comment #14)
> Actual use from gjs disagrees with this assessment. For example, see the WIP
> patch in bug 726450
> 
> Without this, the GtkListBoxRows are definitely not being leaked because I
> can see gtk_list_box_row_finalize being invoked by the GC.
> 
> On the other hand, this patch leads to a crash with "Finalizing proxy for an
> already freed object of type: GtkListBoxRow" when the GC runs.

After talking to desrt and Jasper in #gtk+ on GIMPNet, I realized that this is the right thing to do. The fault lies with gjs (bug 750286) in this case.

Closing.