[clutter/wip/apocalypses/apocalypse-1: 24/44] actor: Make Actor.add_child and Container.add_actor idempotent

[Emmanuele Bassi <ebassi src gnome org>]

commit e72ad56ec61886b6bdfbc72a792da6fbde862d7f
Author: Emmanuele Bassi <ebassi linux intel com>
Date:   Fri Dec 2 11:51:15 2011 +0000

    actor: Make Actor.add_child and Container.add_actor idempotent
    
    And make sure that overriding Container and calling
    clutter_actor_add_child() will result in the same sequence of operations
    as the current set_parent()+queue_relayout()+signal_emit pattern.
    
    Existing containers can continue using:
    
            clutter_actor_set_parent (child, CLUTTER_ACTOR (container));
            clutter_actor_queue_relayout (CLUTTER_ACTOR (container));
            g_signal_emit_by_name (container, "actor-added", child);
    
    and newly written containers overriding Container.add() can simply call:
    
            clutter_actor_add_child (CLUTTER_ACTOR (container), child);
    
    instead.

 clutter/clutter-actor.c           |  132 ++++++++++++++++++++++++++-----------
 tests/conform/test-actor-layout.c |   12 ++--
 2 files changed, 100 insertions(+), 44 deletions(-)
---
diff --git a/clutter/clutter-actor.c b/clutter/clutter-actor.c
index 7a9b7bf..1aeb71a 100644
--- a/clutter/clutter-actor.c
+++ b/clutter/clutter-actor.c
@@ -9194,6 +9194,8 @@ clutter_actor_add_child_internal (ClutterActor             *self,
  * This function will take into consideration the #ClutterActor:depth
  * of @child, and will keep the list of children sorted.
  *
+ * This function will emit the #ClutterContainer::actor-added signal.
+ *
  * Since: 1.10
  */
 void
@@ -9203,10 +9205,13 @@ clutter_actor_add_child (ClutterActor *self,
   g_return_if_fail (CLUTTER_IS_ACTOR (self));
   g_return_if_fail (CLUTTER_IS_ACTOR (child));
   g_return_if_fail (self != child);
+  g_return_if_fail (child->priv->parent_actor == NULL);
 
+  clutter_container_create_child_meta (CLUTTER_CONTAINER (self), child);
   clutter_actor_add_child_internal (self, child,
                                     insert_child_at_depth,
                                     NULL);
+  g_signal_emit_by_name (self, "actor-added", child);
 }
 
 /**
@@ -9224,6 +9229,8 @@ clutter_actor_add_child (ClutterActor *self,
  * This function will not take into consideration the #ClutterActor:depth
  * of @child.
  *
+ * This function will emit the #ClutterContainer::actor-added signal.
+ *
  * Since: 1.10
  */
 void
@@ -9233,12 +9240,15 @@ clutter_actor_insert_child_at_index (ClutterActor *self,
 {
   g_return_if_fail (CLUTTER_IS_ACTOR (self));
   g_return_if_fail (CLUTTER_IS_ACTOR (child));
+  g_return_if_fail (self != child);
   g_return_if_fail (child->priv->parent_actor == NULL);
   g_return_if_fail (index_ < self->priv->n_children);
 
+  clutter_container_create_child_meta (CLUTTER_CONTAINER (self), child);
   clutter_actor_add_child_internal (self, child,
                                     insert_child_at_index,
                                     GINT_TO_POINTER (index_));
+  g_signal_emit_by_name (self, "actor-added", child);
 }
 
 /**
@@ -9257,6 +9267,8 @@ clutter_actor_insert_child_at_index (ClutterActor *self,
  * This function will not take into consideration the #ClutterActor:depth
  * of @child.
  *
+ * This function will emit the #ClutterContainer::actor-added signal.
+ *
  * Since: 1.10
  */
 void
@@ -9266,14 +9278,18 @@ clutter_actor_insert_child_above (ClutterActor *self,
 {
   g_return_if_fail (CLUTTER_IS_ACTOR (self));
   g_return_if_fail (CLUTTER_IS_ACTOR (child));
+  g_return_if_fail (self != child);
+  g_return_if_fail (child != sibling);
   g_return_if_fail (child->priv->parent_actor == NULL);
   g_return_if_fail (sibling == NULL ||
                     (CLUTTER_IS_ACTOR (sibling) &&
                      sibling->priv->parent_actor == self));
 
+  clutter_container_create_child_meta (CLUTTER_CONTAINER (self), child);
   clutter_actor_add_child_internal (self, child,
                                     insert_child_above,
                                     sibling);
+  g_signal_emit_by_name (self, "actor-added", child);
 }
 
 /**
@@ -9292,6 +9308,8 @@ clutter_actor_insert_child_above (ClutterActor *self,
  * This function will not take into consideration the #ClutterActor:depth
  * of @child.
  *
+ * This function will emit the #ClutterContainer::actor-added signal.
+ *
  * Since: 1.10
  */
 void
@@ -9301,14 +9319,18 @@ clutter_actor_insert_child_below (ClutterActor *self,
 {
   g_return_if_fail (CLUTTER_IS_ACTOR (self));
   g_return_if_fail (CLUTTER_IS_ACTOR (child));
+  g_return_if_fail (self != child);
+  g_return_if_fail (child != sibling);
   g_return_if_fail (child->priv->parent_actor == NULL);
   g_return_if_fail (sibling == NULL ||
                     (CLUTTER_IS_ACTOR (sibling) &&
                      sibling->priv->parent_actor == self));
 
+  clutter_container_create_child_meta (CLUTTER_CONTAINER (self), child);
   clutter_actor_add_child_internal (self, child,
                                     insert_child_below,
                                     sibling);
+  g_signal_emit_by_name (self, "actor-added", child);
 }
 
 /**
@@ -9318,13 +9340,27 @@ clutter_actor_insert_child_below (ClutterActor *self,
  *
  * Sets the parent of @self to @parent.
  *
- * This function just calls clutter_actor_add_child().
+ * This function will result in @parent acquiring a reference on @self,
+ * eventually by sinking its floating reference first. The reference
+ * will be released by clutter_actor_unparent().
+ *
+ * This function should only be called by legacy #ClutterActor<!-- -->s
+ * implementing the #ClutterContainer interface.
+ *
+ * Newly written code should use clutter_actor_add_child() instead.
  */
 void
 clutter_actor_set_parent (ClutterActor *self,
 		          ClutterActor *parent)
 {
-  clutter_actor_add_child (parent, self);
+  g_return_if_fail (CLUTTER_IS_ACTOR (self));
+  g_return_if_fail (CLUTTER_IS_ACTOR (parent));
+  g_return_if_fail (self != parent);
+  g_return_if_fail (self->priv->parent_actor == NULL);
+
+  clutter_actor_add_child_internal (parent, self,
+                                    insert_child_at_depth,
+                                    NULL);
 }
 
 /**
@@ -9381,33 +9417,12 @@ invalidate_queue_redraw_entry (ClutterActor *self,
   return CLUTTER_ACTOR_TRAVERSE_VISIT_CONTINUE;
 }
 
-/**
- * clutter_actor_remove_child:
- * @self: a #ClutterActor
- * @child: a #ClutterActor
- *
- * Removes @child from the children of @self.
- *
- * This function will release the reference added by
- * clutter_actor_add_child().
- *
- * Since: 1.10
- */
-void
-clutter_actor_remove_child (ClutterActor *self,
-                            ClutterActor *child)
+static void
+clutter_actor_remove_child_internal (ClutterActor *self,
+                                     ClutterActor *child)
 {
   gboolean was_mapped;
 
-  g_return_if_fail (CLUTTER_IS_ACTOR (self));
-  g_return_if_fail (CLUTTER_IS_ACTOR (child));
-
-  if (child->priv->parent_actor == NULL)
-    return;
-
-  if (child->priv->parent_actor != self)
-    return;
-
   was_mapped = CLUTTER_ACTOR_IS_MAPPED (child);
 
   /* we need to unrealize *before* we set parent_actor to NULL,
@@ -9468,19 +9483,70 @@ clutter_actor_remove_child (ClutterActor *self,
 }
 
 /**
+ * clutter_actor_remove_child:
+ * @self: a #ClutterActor
+ * @child: a #ClutterActor
+ *
+ * Removes @child from the children of @self.
+ *
+ * This function will release the reference added by
+ * clutter_actor_add_child(), so if you want to keep using @child
+ * you will have to acquire a referenced on it before calling this
+ * function.
+ *
+ * This function will emit the #ClutterContainer::actor-removed
+ * signal.
+ *
+ * Since: 1.10
+ */
+void
+clutter_actor_remove_child (ClutterActor *self,
+                            ClutterActor *child)
+{
+  g_return_if_fail (CLUTTER_IS_ACTOR (self));
+  g_return_if_fail (CLUTTER_IS_ACTOR (child));
+  g_return_if_fail (self != child);
+  g_return_if_fail (child->priv->parent_actor != NULL);
+  g_return_if_fail (child->priv->parent_actor == self);
+
+  g_object_ref (child);
+
+  clutter_container_destroy_child_meta (CLUTTER_CONTAINER (self), child);
+
+  clutter_actor_remove_child_internal (self, child);
+
+  g_signal_emit_by_name (self, "actor-removed", child);
+
+  g_object_unref (child);
+}
+
+/**
  * clutter_actor_unparent:
  * @self: a #ClutterActor
  *
  * Removes the parent of @self.
  *
- * This function just calls clutter_actor_remove_child().
+ * This will cause the parent of @self to release the reference
+ * acquired when calling clutter_actor_set_parent(), so if you
+ * want to keep @self you will have to acquire a reference of
+ * your own, through g_object_ref().
+ *
+ * This function should only be called by legacy #ClutterActor<!-- -->s
+ * implementing the #ClutterContainer interface.
+ *
+ * Newly written code should use clutter_actor_remove_child() instead.
  *
  * Since: 0.1.1
  */
 void
 clutter_actor_unparent (ClutterActor *self)
 {
-  clutter_actor_remove_child (self->priv->parent_actor, self);
+  g_return_if_fail (CLUTTER_IS_ACTOR (self));
+
+  if (self->priv->parent_actor == NULL)
+    return;
+
+  clutter_actor_remove_child_internal (self->priv->parent_actor, self);
 }
 
 /**
@@ -10084,23 +10150,13 @@ container_add_actor (ClutterContainer *container,
                      ClutterActor     *actor)
 {
   clutter_actor_add_child (CLUTTER_ACTOR (container), actor);
-  clutter_actor_queue_relayout (CLUTTER_ACTOR (container));
-
-  g_signal_emit_by_name (container, "actor-added", actor);
 }
 
 static void
 container_remove_actor (ClutterContainer *container,
                         ClutterActor     *actor)
 {
-  g_object_ref (actor);
-
   clutter_actor_remove_child (CLUTTER_ACTOR (container), actor);
-  clutter_actor_queue_relayout (CLUTTER_ACTOR (container));
-
-  g_signal_emit_by_name (container, "actor-removed", actor);
-
-  g_object_unref (actor);
 }
 
 typedef struct {
diff --git a/tests/conform/test-actor-layout.c b/tests/conform/test-actor-layout.c
index eb9b031..f749eca 100644
--- a/tests/conform/test-actor-layout.c
+++ b/tests/conform/test-actor-layout.c
@@ -218,17 +218,17 @@ basic_layout (TestConformSimpleFixture *fixture,
   flower[0] = clutter_rectangle_new_with_color (CLUTTER_COLOR_Red);
   clutter_actor_set_size (flower[0], 100, 100);
   clutter_actor_set_name (flower[0], "Red Flower");
-  clutter_container_add_actor (CLUTTER_CONTAINER (vase), flower[0]);
+  clutter_actor_add_child (vase, flower[0]);
 
   flower[1] = clutter_rectangle_new_with_color (CLUTTER_COLOR_Yellow);
   clutter_actor_set_size (flower[1], 100, 100);
   clutter_actor_set_name (flower[1], "Yellow Flower");
-  clutter_container_add_actor (CLUTTER_CONTAINER (vase), flower[1]);
+  clutter_actor_add_child (vase, flower[1]);
 
   flower[2] = clutter_rectangle_new_with_color (CLUTTER_COLOR_Green);
   clutter_actor_set_size (flower[2], 100, 100);
   clutter_actor_set_name (flower[2], "Green Flower");
-  clutter_container_add_actor (CLUTTER_CONTAINER (vase), flower[2]);
+  clutter_actor_add_child (vase, flower[2]);
 
   clutter_actor_show_all (stage);
 
@@ -258,21 +258,21 @@ margin_layout (TestConformSimpleFixture *fixture,
   flower[0] = clutter_rectangle_new_with_color (CLUTTER_COLOR_Red);
   clutter_actor_set_size (flower[0], 100, 100);
   clutter_actor_set_name (flower[0], "Red Flower");
-  clutter_container_add_actor (CLUTTER_CONTAINER (vase), flower[0]);
+  clutter_actor_add_child (vase, flower[0]);
 
   flower[1] = clutter_rectangle_new_with_color (CLUTTER_COLOR_Yellow);
   clutter_actor_set_size (flower[1], 100, 100);
   clutter_actor_set_name (flower[1], "Yellow Flower");
   clutter_actor_set_margin_right (flower[1], 6);
   clutter_actor_set_margin_left (flower[1], 6);
-  clutter_container_add_actor (CLUTTER_CONTAINER (vase), flower[1]);
+  clutter_actor_add_child (vase, flower[1]);
 
   flower[2] = clutter_rectangle_new_with_color (CLUTTER_COLOR_Green);
   clutter_actor_set_size (flower[2], 100, 100);
   clutter_actor_set_name (flower[2], "Green Flower");
   clutter_actor_set_margin_top (flower[2], 6);
   clutter_actor_set_margin_bottom (flower[2], 6);
-  clutter_container_add_actor (CLUTTER_CONTAINER (vase), flower[2]);
+  clutter_actor_add_child (vase, flower[2]);
 
   clutter_actor_show_all (stage);