Binding: Add set_manage() to not require a RefPtrwip/dboles/Binding-Issue#62

Allow users to call Glib::manage(RefPtr<Binding>) in order to defeat our
pre-existing and still default behaviour that a user must keep a RefPtr
to the Binding in order to keep it alive, instead letting it revert to
the C-style behaviour of staying alive as long as either object does.

We use Glib::manage() to avoid the possible pitfall of a user calling
->set_manage() on the result of bind_property*(), which might be empty.

https://gitlab.gnome.org/GNOME/glibmm/issues/62

3 files changed, 104 insertions, 19 deletions

diff --git a/glib/src/binding.ccg b/glib/src/binding.ccg
index dfad8a66..ddf59671 100644
--- a/glib/src/binding.ccg
+++ b/glib/src/binding.ccg
@@ -112,7 +112,8 @@ Binding::bind_property_value(const PropertyProxy_Base& source_property,
 
   // Take an extra ref. GBinding uses one ref itself, and drops it if
   // either the source object or the target object is finalized.
-  // The GBinding object must not be destroyed while there are RefPtrs around.
+  // The GBinding object must not be destroyed while there are RefPtrs around,
+  // unless set_manage() was called.
   g_object_ref(binding);
   return Glib::make_refptr_for_instance<Binding>(new Binding(binding));
 }
@@ -131,18 +132,36 @@ Binding::unbind()
 // It calls g_object_unref() itself, if either the source object or the
 // target object is finalized, almost like g_binding_unbind().
 // But the GBinding object shall be destroyed when and only when the last
-// reference from a Glib::RefPtr is dropped.
+// reference from a Glib::RefPtr is dropped, unless set_manage() was called.
 void
 Binding::unreference() const
 {
-  GBinding* const binding = const_cast<GBinding*>(gobj());
+  if (!m_manage)
+  {
+    GBinding* const binding = const_cast<GBinding*>(gobj());
 
-  // If the last Glib::RefPtr is being deleted, and the binding has not been unbound,
-  // then drop the extra reference that was added by bind_property_value().
-  if (gobject_->ref_count == 2 && g_binding_get_source(binding))
-    g_object_unref(binding);
+    // If the last Glib::RefPtr is being deleted, and the binding has not been unbound,
+    // then drop the extra reference that was added by bind_property_value().
+    if (gobject_->ref_count == 2 && g_binding_get_source(binding))
+      g_object_unref(binding);
+  }
 
   Object::unreference();
 }
 
+void
+Binding::set_manage()
+{
+  m_manage = true;
+}
+
+const Glib::RefPtr<Glib::Binding>&
+manage(const Glib::RefPtr<Glib::Binding>& binding)
+{
+  if (binding)
+    binding->set_manage();
+
+  return binding;
+}
+
 } // namespace Glib
diff --git a/glib/src/binding.hg b/glib/src/binding.hg
index f88e254e..39c1e654 100644
--- a/glib/src/binding.hg
+++ b/glib/src/binding.hg
@@ -91,12 +91,19 @@ namespace Glib
  * or g_signal_handler_block().
  *
  * The binding between properties is broken, and its resources freed, when the
- * Glib::Binding loses its last Glib::RefPtr, the source or target object is
- * deleted, or unbind() is called. If a Glib::RefPtr to the Glib::Binding
- * remains after the binding is broken another way, Glib::Binding::get_source()
- * and Glib::Binding::get_target() return an empty Glib::RefPtr. So, you must
- * keep a Glib::RefPtr to the Glib::Binding for as long as you want it to bind,
- * but doing that does not guarantee the source/target are still alive or bound.
+ * Glib::Binding loses its last Glib::RefPtr (by default), the source or target
+ * object is deleted, or unbind() is called. If a Glib::RefPtr to the
+ * Glib::Binding remains after the binding is broken another way,
+ * Glib::Binding::get_source() and Glib::Binding::get_target() return an empty
+ * Glib::RefPtr. So, by default, you must keep a Glib::RefPtr to the
+ * Glib::Binding for as long as you want it to bind, but doing that does not
+ * guarantee the source/target are still alive or bound.
+ *
+ * If it is not convenient to maintain a Glib::RefPtr to keep a Glib::Binding
+ * active, you can pass the Glib::Binding to Glib::manage() to specify that it
+ * should have its lifetime managed by the source/target objects and unbind()
+ * only. In that case, it will stay active as long as the source and target
+ * exist and unbind() is not called, even if no Glib::RefPtr to it is kept.
  *
  * @newin{2,44}
  */
@@ -406,6 +413,19 @@ public:
   _WRAP_PROPERTY("target", Glib::RefPtr<Glib::ObjectBase>, newin "2,44")
   _WRAP_PROPERTY("target-property", Glib::ustring, newin "2,44")
 
+  /** Sets the binding as having its lifetime managed by the lifetimes of its
+   * source and target objects, rather than requiring a Glib::RefPtr to be kept.
+   *
+   * See the class description for full information on the lifetimes of bindings.
+
+   * To manage the result of bind_property() or bind_property_value(), which can
+   * return an empty Glib::RefPtr on error, pass the result to Glib::manage(),
+   * as that specifically avoids calling set_manage() on an empty Glib::RefPtr.
+   *
+   * @newin{2,64}
+   */
+  void set_manage() override final;
+
 #ifndef DOXYGEN_SHOULD_SKIP_THIS
   /** Decrement the reference count for this object.
    * You should never need to do this manually - use the object via a RefPtr instead.
@@ -414,6 +434,8 @@ public:
 #endif /* DOXYGEN_SHOULD_SKIP_THIS */
 
 private:
+  bool m_manage = false;
+
   // The functor TransformProp can be implicitly converted to a SlotTransform
   // and used in a call to bind_property_value().
   template <typename T_from, typename T_to>
@@ -443,4 +465,27 @@ private:
   };
 };
 
+/** Sets a Glib::Binding as having its lifetime managed by the lifetimes of its
+ * source and target objects, rather than requiring a Glib::RefPtr to be kept.
+ *
+ * See the class description of Glib::Binding for full information on the
+ * lifetimes of bindings.
+ *
+ * For instance:
+ * @code
+ * void bind_my_properties(const Glib::RefPtr<Source>& source,
+ *                         const Glib::RefPtr<Target>& target)
+ * {
+ *   Glib::manage( Glib::Binding::bind_property( source->property_foo(),
+ *                                               target->property_bar() ) );
+ *   // don’t keep RefPtr to new Binding as source & target control its lifetime
+ * }
+ * @endcode
+ *
+ * @newin{2,64}
+ * @param binding the Glib::Binding to set as managed, or an empty Glib::RefPtr.
+ * @return the same Glib::Binding or empty Glib::RefPtr.
+ */
+const Glib::RefPtr<Glib::Binding>& manage(const Glib::RefPtr<Glib::Binding>& binding);
+
 } // namespace Glib
diff --git a/tests/glibmm_binding/main.cc b/tests/glibmm_binding/main.cc
index 1f86cea2..08025d98 100644
--- a/tests/glibmm_binding/main.cc
+++ b/tests/glibmm_binding/main.cc
@@ -56,15 +56,20 @@ test()
   auto source = StringSource{};
   auto target = IntTarget{};
 
+  auto create_binding = [&source, &target](Glib::Binding::Flags flags)
+  {
+    return Glib::Binding::bind_property(
+      source.property_string(), target.property_int(),
+      flags, &transform_string_to_int);
+  };
+
   // We should obviously not change the target before it has been bound!
   target.property_int() = 7;
   source.property_string() = "42";
   g_assert_cmpint(target.property_int(), ==, 7);
 
   {
-    auto binding = Glib::Binding::bind_property(
-      source.property_string(), target.property_int(),
-      Glib::Binding::Flags::DEFAULT, &transform_string_to_int);
+    auto binding = create_binding(Glib::Binding::Flags::DEFAULT);
 
     // Without SYNC_CREATE, only changes after bound will be synced
     g_assert_cmpint(target.property_int(), ==, 7);
@@ -92,13 +97,29 @@ test()
   g_assert_cmpint(target.property_int(), ==, 47);
 
   {
-    auto binding = Glib::Binding::bind_property(
-      source.property_string(), target.property_int(),
-      Glib::Binding::Flags::SYNC_CREATE, &transform_string_to_int);
+    auto binding = create_binding(Glib::Binding::Flags::SYNC_CREATE);
 
     // With SYNC_CREATE, value of source must sync to target on bind
     g_assert_cmpint(target.property_int(), ==, 89);
   }
+
+  // Ensure the binding was released when its RefPtr went out of scope
+  source.property_string() = "97";
+  g_assert_cmpint(target.property_int(), ==, 89);
+
+  // Ensure that a manage()d binding...
+  {
+    auto binding = create_binding(Glib::Binding::Flags::DEFAULT);
+    Glib::manage(binding);
+
+    // (a) still syncs when the source value changes
+    source.property_string() = "1999";
+    g_assert_cmpint(target.property_int(), ==, 1999);
+  }
+
+  // and (b) still binds the properties after our RefPtr to it goes out of scope
+  source.property_string() = "2001";
+  g_assert_cmpint(target.property_int(), ==, 2001);
 }
 
 } // namespace