Created attachment 289602 [details] [review]
patch: Add Glib::Binding

This is a first version of Glib::Binding. It has not been tested.
It should not be pushed before it has been tested. GBinding uses reference
counts in an unusual way, and that has complicated the wrapping in C++.

The patch is attached now, because I would appreciate comments. I'm not very
fond of the use of ValueBase. I hope they can be avoided in the C++ API
by some clever use of C++ templates, with the data types as template
parameters.

Review of attachment 289602 [details] [review]:

Thanks for taking time to tackle this! I read the patch while I was commuting, so this is only a very basic review/response.

Since you’re g_object_refing inside the bind_property and g_binding_unbind() does g_object_unref for you, wouldn’t doing

    Binding::reference() { inherited from ObjectBase (i.e. g_object_refs every time RefPtr is created) }
    Binding::unreference() { 
        ObjectBase::unreference(); 
        if(ref_count == 1 || ref_count == 2 && g_binding_source(binding)) g_object_unref(binding); 
    }

suffice? 

Reasoning: When there’s no more RefPtr’s left, you will always have either ref_count == 1 or ref_count == 2.

First case is when source/target have finalised and second one when they have not.

::: glib/src/binding.ccg
@@ +100,3 @@
+      (GBindingFlags)flags);
+
+    return Glib::RefPtr<Binding>(new Binding(binding));

In this function you return RefPtr<Binding>(new Binding(binding)) in two places. Here, and in line 125. Return in line 125 is accompanied by g_object_ref, while here it is not.

@@ +154,3 @@
+
+    // Tell GBinding to drop its reference, if it has not been done.
+    if (g_binding_get_source(binding))

This check is superfluous. The same check is done inside an unbind().

(In reply to comment #2)
> Since you’re g_object_refing inside the bind_property and g_binding_unbind()
> does g_object_unref for you, wouldn’t doing
>
>  Binding::reference() { inherited from ObjectBase (i.e. g_object_refs every time RefPtr is created) }
>  Binding::unreference() { 
>    ObjectBase::unreference();
>    if(ref_count == 1 || ref_count == 2 && g_binding_source(binding))
>      g_object_unref(binding); 
>  }
> suffice?

You're right, the extra ref counter in Glib::Binding is not necessary.
But it can't be done exactly like you suggest. Perhaps like so:
  if (gobject_->ref_count == 2 && g_binding_source(binding))
    g_object_unref(binding); 
  ObjectBase::unreference();

I'll probably remove m_impl.ref_count in the next version I'll attach.
I'm working on a better version, using templates. Then it will be possible to
use transformation functions without Glib::ValueBase or GValue parameters.

It's not nice to read ref_count, which is marked <private>. But it wouldn't be
the first time. It's done in Gtk::Object::set_manage(). An extra ref counter
in Glib::Binding is also ugly.

> In this function you return RefPtr<Binding>(new Binding(binding)) in two
> places. Here, and in line 125. Return in line 125 is accompanied by
> g_object_ref, while here it is not.

Yes, it's wrong. I'll fix it in the next version.

> This check is superfluous. The same check is done inside an unbind().

True. I think I added the check in unreference() before I realized that it must
be tested in unbind().

Created attachment 289966 [details] [review]
patch: Add Glib::Binding

A new version of Glib::Binding. This one is tested with the test case that I
will soon attach.

Created attachment 289967 [details]
Test case

This test case is a modified version of an example program in the gtkmm
tutorial, https://git.gnome.org/browse/gtkmm-documentation/tree/examples/book/range_widgets

Created attachment 291107 [details]
Test case 2

Modified test case, compatible with the pushed patch.

I have pushed
https://git.gnome.org/browse/glibmm/commit/?id=983dd287da4b00baedbd04ce0f777c043d93f5b0

I've come to realize that

 typedef sigc::slot<bool, const Glib::RefPtr<Binding>&, const GValue*, GValue*>
   BindingTransformSlot;

in Glib::Binding does not meet the (probably unwritten) design rules of
glibmm/gtkmm. It should be

  typedef sigc::slot<bool, const GValue*, GValue*> BindingTransformSlot;

In callback functions of this type it's customary to skip the first parameter
in the glib/gtk+ callback function. In my test case, the RefPtr<Binding>
parameter is not used in any of the callback functions, indicating that it's
seldom needed. In those cases where it is needed, an extra parameter can be
added by using sigc::bind() in the call to Binding::bind_property().

So far Glib::Binding exists only in the unstable glibmm version 2.43.
Unless someone objects soon, I will change BindingTransformSlot.
I will probably also change its name to SlotTransform. That would be more like
other slot typedefs.

Created attachment 294390 [details]
Test case 3

Modified test case, compatible with the pushed patch.

I have pushed
https://git.gnome.org/browse/glibmm/commit/?id=f02288579c548d29d9ce948c6057a61eb892036c

The slot typedef has been changed to

  typedef sigc::slot<bool, const GValue*, GValue*> SlotTransform;