group: make it comparable to ClutterBox
authorRobert Bragg <robert@linux.intel.com>
Tue, 9 Feb 2010 18:54:28 +0000 (18:54 +0000)
committerRobert Bragg <robert@linux.intel.com>
Fri, 12 Feb 2010 12:31:24 +0000 (12:31 +0000)
There is a lot of duplication between ClutterGroup and ClutterBox so
this makes the two files diff-able so that new fixes can easily be
ported to both and bug fixes missing in one or the other can be spotted
more easily. This doesn't change the behaviour of either actor; it's
really just a shuffle around of code and normalizes the coding style to
make the files comparable.

This has already uncovered one bug in ClutterBox, and also highlights
a bug in ClutterGroup + many other actors:

1) ClutterGroup::real_foreach was recently changed to use
   g_list_foreach instead of manually iterating the child list so it can
   safely handle calls like:
     clutter_container_foreach (container, clutter_actor_destroy);
   ClutterBox is still manually iterating the list.

2) In ClutterGroup we guard _queue_redraw() calls like this:
    if (CLUTTER_ACTOR_IS_VISIBLE (container))
clutter_actor_queue_redraw (CLUTTER_ACTOR (container));
   In ClutterBox we don't:
     I think ClutterBox is correct here because
     clutter_actor_queue_redraw already optimizes the case where the
     actor's not visible, but it also considers that the actor may be
     cloned and so the guard in ClutterGroup could break clones. This
     actually highlights a wider clutter bug since the same kinds of
     guards can be found in all other clutter actors.

clutter/clutter-box.c
clutter/clutter-group.c

index 67c52d0..6cc39ec 100644 (file)
@@ -149,7 +149,6 @@ clutter_box_real_foreach (ClutterContainer *container,
                           gpointer          user_data)
 {
   ClutterBoxPrivate *priv = CLUTTER_BOX (container)->priv;
-  GList *l;
 
   for (l = priv->children; l != NULL; l = l->next)
     (* callback) (l->data, user_data);
index 960b82f..ac908ff 100644 (file)
 #include "clutter-container.h"
 #include "clutter-fixed-layout.h"
 #include "clutter-main.h"
-#include "clutter-private.h"
 #include "clutter-debug.h"
-#include "clutter-marshal.h"
 #include "clutter-enum-types.h"
+#include "clutter-marshal.h"
+#include "clutter-private.h"
 
 #include "cogl/cogl.h"
 
-enum
-{
-  ADD,
-  REMOVE,
-
-  LAST_SIGNAL
-};
-
-static void clutter_container_iface_init (ClutterContainerIface *iface);
-
-G_DEFINE_TYPE_WITH_CODE (ClutterGroup,
-                         clutter_group,
-                         CLUTTER_TYPE_ACTOR,
-                         G_IMPLEMENT_INTERFACE (CLUTTER_TYPE_CONTAINER,
-                                                clutter_container_iface_init));
-
 #define CLUTTER_GROUP_GET_PRIVATE(obj) \
-(G_TYPE_INSTANCE_GET_PRIVATE ((obj), CLUTTER_TYPE_GROUP, ClutterGroupPrivate))
+  (G_TYPE_INSTANCE_GET_PRIVATE ((obj), CLUTTER_TYPE_GROUP, ClutterGroupPrivate))
 
 struct _ClutterGroupPrivate
 {
@@ -88,159 +72,51 @@ struct _ClutterGroupPrivate
   ClutterLayoutManager *layout;
 };
 
-
-static void
-clutter_group_paint (ClutterActor *actor)
-{
-  ClutterGroupPrivate *priv = CLUTTER_GROUP (actor)->priv;
-  GList               *child_item;
-
-  CLUTTER_NOTE (PAINT, "ClutterGroup paint enter '%s'",
-                clutter_actor_get_name (actor) ? clutter_actor_get_name (actor)
-                                               : "unknown");
-
-  for (child_item = priv->children;
-       child_item != NULL;
-       child_item = child_item->next)
-    {
-      ClutterActor *child = child_item->data;
-
-      g_assert (child != NULL);
-
-      clutter_actor_paint (child);
-    }
-
-  CLUTTER_NOTE (PAINT, "ClutterGroup paint leave '%s'",
-                clutter_actor_get_name (actor) ? clutter_actor_get_name (actor)
-                                               : "unknown");
-}
-
-static void
-clutter_group_pick (ClutterActor       *actor,
-                   const ClutterColor *color)
-{
-  ClutterGroupPrivate *priv = CLUTTER_GROUP (actor)->priv;
-  GList               *child_item;
-
-  /* Chain up so we get a bounding box pained (if we are reactive) */
-  CLUTTER_ACTOR_CLASS (clutter_group_parent_class)->pick (actor, color);
-
-  for (child_item = priv->children;
-       child_item != NULL;
-       child_item = child_item->next)
-    {
-      ClutterActor *child = child_item->data;
-
-      g_assert (child != NULL);
-
-      clutter_actor_paint (child);
-    }
-}
-
-static void
-clutter_group_get_preferred_width (ClutterActor *self,
-                                   gfloat        for_height,
-                                   gfloat       *min_width_p,
-                                   gfloat       *natural_width_p)
-{
-  ClutterContainer *container = CLUTTER_CONTAINER (self);
-  ClutterGroupPrivate *priv = CLUTTER_GROUP (self)->priv;
-
-  clutter_layout_manager_get_preferred_width (priv->layout, container,
-                                              for_height,
-                                              min_width_p,
-                                              natural_width_p);
-}
-
-static void
-clutter_group_get_preferred_height (ClutterActor *self,
-                                    gfloat        for_width,
-                                    gfloat       *min_height_p,
-                                    gfloat       *natural_height_p)
-{
-  ClutterContainer *container = CLUTTER_CONTAINER (self);
-  ClutterGroupPrivate *priv = CLUTTER_GROUP (self)->priv;
-
-  clutter_layout_manager_get_preferred_height (priv->layout, container,
-                                               for_width,
-                                               min_height_p,
-                                               natural_height_p);
-}
-
-static void
-clutter_group_allocate (ClutterActor           *self,
-                        const ClutterActorBox  *box,
-                        ClutterAllocationFlags  flags)
+enum
 {
-  ClutterContainer *container = CLUTTER_CONTAINER (self);
-  ClutterGroupPrivate *priv = CLUTTER_GROUP (self)->priv;
+  ADD,
+  REMOVE,
 
-  /* chain up to set actor->allocation */
-  CLUTTER_ACTOR_CLASS (clutter_group_parent_class)->allocate (self, box, flags);
+  LAST_SIGNAL
+};
 
-  if (priv->children == NULL)
-    return;
+static void clutter_container_iface_init (ClutterContainerIface *iface);
 
-  clutter_layout_manager_allocate (priv->layout, container, box, flags);
-}
+G_DEFINE_TYPE_WITH_CODE (ClutterGroup, clutter_group, CLUTTER_TYPE_ACTOR,
+                         G_IMPLEMENT_INTERFACE (CLUTTER_TYPE_CONTAINER,
+                                                clutter_container_iface_init));
 
-static void
-clutter_group_dispose (GObject *object)
+static gint
+sort_by_depth (gconstpointer a,
+               gconstpointer b)
 {
-  ClutterGroup *self = CLUTTER_GROUP (object);
-  ClutterGroupPrivate *priv = self->priv;
-
-  if (priv->children)
-    {
-      g_list_foreach (priv->children, (GFunc) clutter_actor_destroy, NULL);
-      g_list_free (priv->children);
-
-      priv->children = NULL;
-    }
-
-  if (priv->layout)
-    {
-      g_object_unref (priv->layout);
-      priv->layout = NULL;
-    }
+  gfloat depth_a = clutter_actor_get_depth (CLUTTER_ACTOR(a));
+  gfloat depth_b = clutter_actor_get_depth (CLUTTER_ACTOR(b));
 
-  G_OBJECT_CLASS (clutter_group_parent_class)->dispose (object);
-}
+  if (depth_a < depth_b)
+    return -1;
 
-static void
-clutter_group_real_show_all (ClutterActor *actor)
-{
-  clutter_container_foreach (CLUTTER_CONTAINER (actor),
-                             CLUTTER_CALLBACK (clutter_actor_show),
-                             NULL);
-  clutter_actor_show (actor);
-}
+  if (depth_a > depth_b)
+    return 1;
 
-static void
-clutter_group_real_hide_all (ClutterActor *actor)
-{
-  clutter_actor_hide (actor);
-  clutter_container_foreach (CLUTTER_CONTAINER (actor),
-                             CLUTTER_CALLBACK (clutter_actor_hide),
-                             NULL);
+  return 0;
 }
 
 static void
 clutter_group_real_add (ClutterContainer *container,
                         ClutterActor     *actor)
 {
-  ClutterGroup *group = CLUTTER_GROUP (container);
-  ClutterGroupPrivate *priv = group->priv;
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (container)->priv;
 
   g_object_ref (actor);
 
   priv->children = g_list_append (priv->children, actor);
-  clutter_actor_set_parent (actor, CLUTTER_ACTOR (group));
+  clutter_actor_set_parent (actor, CLUTTER_ACTOR (container));
 
   /* queue a relayout, to get the correct positioning inside
    * the ::actor-added signal handlers
    */
-  clutter_actor_queue_relayout (CLUTTER_ACTOR (group));
+  clutter_actor_queue_relayout (CLUTTER_ACTOR (container));
 
   g_signal_emit_by_name (container, "actor-added", actor);
 
@@ -253,8 +129,7 @@ static void
 clutter_group_real_remove (ClutterContainer *container,
                            ClutterActor     *actor)
 {
-  ClutterGroup *group = CLUTTER_GROUP (container);
-  ClutterGroupPrivate *priv = group->priv;
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (container)->priv;
 
   g_object_ref (actor);
 
@@ -264,7 +139,7 @@ clutter_group_real_remove (ClutterContainer *container,
   /* queue a relayout, to get the correct positioning inside
    * the ::actor-removed signal handlers
    */
-  clutter_actor_queue_relayout (CLUTTER_ACTOR (group));
+  clutter_actor_queue_relayout (CLUTTER_ACTOR (container));
 
   /* at this point, the actor passed to the "actor-removed" signal
    * handlers is not parented anymore to the container but since we
@@ -272,8 +147,8 @@ clutter_group_real_remove (ClutterContainer *container,
    */
   g_signal_emit_by_name (container, "actor-removed", actor);
 
-  if (CLUTTER_ACTOR_IS_VISIBLE (CLUTTER_ACTOR (group)))
-    clutter_actor_queue_redraw (CLUTTER_ACTOR (group));
+  if (CLUTTER_ACTOR_IS_VISIBLE (CLUTTER_ACTOR (container)))
+    clutter_actor_queue_redraw (CLUTTER_ACTOR (container));
 
   g_object_unref (actor);
 }
@@ -283,8 +158,7 @@ clutter_group_real_foreach (ClutterContainer *container,
                             ClutterCallback   callback,
                             gpointer          user_data)
 {
-  ClutterGroup *group = CLUTTER_GROUP (container);
-  ClutterGroupPrivate *priv = group->priv;
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (container)->priv;
 
   /* Using g_list_foreach instead of iterating the list manually
      because it has better protection against the current node being
@@ -298,8 +172,7 @@ clutter_group_real_raise (ClutterContainer *container,
                           ClutterActor     *actor,
                           ClutterActor     *sibling)
 {
-  ClutterGroup *self = CLUTTER_GROUP (container);
-  ClutterGroupPrivate *priv = self->priv;
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (container)->priv;
 
   priv->children = g_list_remove (priv->children, actor);
 
@@ -317,11 +190,9 @@ clutter_group_real_raise (ClutterContainer *container,
     }
   else
     {
-      gint pos;
+      gint index_ = g_list_index (priv->children, sibling) + 1;
 
-      pos = g_list_index (priv->children, sibling) + 1;
-
-      priv->children = g_list_insert (priv->children, actor, pos);
+      priv->children = g_list_insert (priv->children, actor, index_);
     }
 
   /* set Z ordering a value below, this will then call sort
@@ -364,11 +235,9 @@ clutter_group_real_lower (ClutterContainer *container,
     }
   else
     {
-      gint pos;
-
-      pos = g_list_index (priv->children, sibling);
+      gint index_ = g_list_index (priv->children, sibling);
 
-      priv->children = g_list_insert (priv->children, actor, pos);
+      priv->children = g_list_insert (priv->children, actor, index_);
     }
 
   /* See comment in group_raise for this */
@@ -382,66 +251,165 @@ clutter_group_real_lower (ClutterContainer *container,
     clutter_actor_queue_redraw (CLUTTER_ACTOR (container));
 }
 
-static gint
-sort_z_order (gconstpointer a,
-              gconstpointer b)
+static void
+clutter_group_real_sort_depth_order (ClutterContainer *container)
 {
-  float depth_a, depth_b;
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (container)->priv;
 
-  depth_a = clutter_actor_get_depth (CLUTTER_ACTOR(a));
-  depth_b = clutter_actor_get_depth (CLUTTER_ACTOR(b));
+  priv->children = g_list_sort (priv->children, sort_by_depth);
 
-  if (depth_a < depth_b)
-    return -1;
+  if (CLUTTER_ACTOR_IS_VISIBLE (container))
+    clutter_actor_queue_redraw (CLUTTER_ACTOR (container));
+}
 
-  if (depth_a > depth_b)
-    return 1;
+static void
+clutter_container_iface_init (ClutterContainerIface *iface)
+{
+  iface->add = clutter_group_real_add;
+  iface->remove = clutter_group_real_remove;
+  iface->foreach = clutter_group_real_foreach;
+  iface->raise = clutter_group_real_raise;
+  iface->lower = clutter_group_real_lower;
+  iface->sort_depth_order = clutter_group_real_sort_depth_order;
+}
 
-  return 0;
+static void
+clutter_group_real_paint (ClutterActor *actor)
+{
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (actor)->priv;
+
+  CLUTTER_NOTE (PAINT, "ClutterGroup paint enter '%s'",
+                clutter_actor_get_name (actor) ? clutter_actor_get_name (actor)
+                                               : "unknown");
+
+  g_list_foreach (priv->children, (GFunc) clutter_actor_paint, NULL);
+
+  CLUTTER_NOTE (PAINT, "ClutterGroup paint leave '%s'",
+                clutter_actor_get_name (actor) ? clutter_actor_get_name (actor)
+                                               : "unknown");
 }
 
 static void
-clutter_group_real_sort_depth_order (ClutterContainer *container)
+clutter_group_real_pick (ClutterActor       *actor,
+                         const ClutterColor *pick)
 {
-  ClutterGroup *self = CLUTTER_GROUP (container);
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (actor)->priv;
+
+  /* Chain up so we get a bounding box pained (if we are reactive) */
+  CLUTTER_ACTOR_CLASS (clutter_group_parent_class)->pick (actor, pick);
+
+  g_list_foreach (priv->children, (GFunc) clutter_actor_paint, NULL);
+}
+
+static void
+clutter_group_real_get_preferred_width (ClutterActor *actor,
+                                        gfloat        for_height,
+                                        gfloat       *min_width,
+                                        gfloat       *natural_width)
+{
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (actor)->priv;
+
+  clutter_layout_manager_get_preferred_width (priv->layout,
+                                              CLUTTER_CONTAINER (actor),
+                                              for_height,
+                                              min_width, natural_width);
+}
+
+static void
+clutter_group_real_get_preferred_height (ClutterActor *actor,
+                                         gfloat        for_width,
+                                         gfloat       *min_height,
+                                         gfloat       *natural_height)
+{
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (actor)->priv;
+
+  clutter_layout_manager_get_preferred_height (priv->layout,
+                                               CLUTTER_CONTAINER (actor),
+                                               for_width,
+                                               min_height, natural_height);
+}
+
+static void
+clutter_group_real_allocate (ClutterActor           *actor,
+                             const ClutterActorBox  *allocation,
+                             ClutterAllocationFlags  flags)
+{
+  ClutterGroupPrivate *priv = CLUTTER_GROUP (actor)->priv;
+  ClutterActorClass *klass;
+
+  klass = CLUTTER_ACTOR_CLASS (clutter_group_parent_class);
+  klass->allocate (actor, allocation, flags);
+
+  if (priv->children == NULL)
+    return;
+
+  clutter_layout_manager_allocate (priv->layout,
+                                   CLUTTER_CONTAINER (actor),
+                                   allocation, flags);
+}
+
+static void
+clutter_group_dispose (GObject *object)
+{
+  ClutterGroup *self = CLUTTER_GROUP (object);
   ClutterGroupPrivate *priv = self->priv;
 
-  priv->children = g_list_sort (priv->children, sort_z_order);
+  if (priv->children)
+    {
+      g_list_foreach (priv->children, (GFunc) clutter_actor_destroy, NULL);
+      g_list_free (priv->children);
+
+      priv->children = NULL;
+    }
+
+  if (priv->layout)
+    {
+      g_object_unref (priv->layout);
+      priv->layout = NULL;
+    }
 
-  if (CLUTTER_ACTOR_IS_VISIBLE (self))
-    clutter_actor_queue_redraw (CLUTTER_ACTOR (self));
+  G_OBJECT_CLASS (clutter_group_parent_class)->dispose (object);
+}
 
+static void
+clutter_group_real_show_all (ClutterActor *actor)
+{
+  clutter_container_foreach (CLUTTER_CONTAINER (actor),
+                             CLUTTER_CALLBACK (clutter_actor_show),
+                             NULL);
+  clutter_actor_show (actor);
 }
 
 static void
-clutter_container_iface_init (ClutterContainerIface *iface)
+clutter_group_real_hide_all (ClutterActor *actor)
 {
-  iface->add = clutter_group_real_add;
-  iface->remove = clutter_group_real_remove;
-  iface->foreach = clutter_group_real_foreach;
-  iface->raise = clutter_group_real_raise;
-  iface->lower = clutter_group_real_lower;
-  iface->sort_depth_order = clutter_group_real_sort_depth_order;
+  clutter_actor_hide (actor);
+  clutter_container_foreach (CLUTTER_CONTAINER (actor),
+                             CLUTTER_CALLBACK (clutter_actor_hide),
+                             NULL);
 }
 
+
+
+
 static void
 clutter_group_class_init (ClutterGroupClass *klass)
 {
-  GObjectClass *object_class = G_OBJECT_CLASS (klass);
+  GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
   ClutterActorClass *actor_class = CLUTTER_ACTOR_CLASS (klass);
 
-  g_type_class_add_private (object_class, sizeof (ClutterGroupPrivate));
+  g_type_class_add_private (klass, sizeof (ClutterGroupPrivate));
 
-  object_class->dispose = clutter_group_dispose;
+  actor_class->get_preferred_width = clutter_group_real_get_preferred_width;
+  actor_class->get_preferred_height = clutter_group_real_get_preferred_height;
+  actor_class->allocate = clutter_group_real_allocate;
+  actor_class->paint = clutter_group_real_paint;
+  actor_class->pick = clutter_group_real_pick;
+  actor_class->show_all = clutter_group_real_show_all;
+  actor_class->hide_all = clutter_group_real_hide_all;
 
-  actor_class->paint           = clutter_group_paint;
-  actor_class->pick            = clutter_group_pick;
-  actor_class->show_all        = clutter_group_real_show_all;
-  actor_class->hide_all        = clutter_group_real_hide_all;
+  gobject_class->dispose = clutter_group_dispose;
 
-  actor_class->get_preferred_width  = clutter_group_get_preferred_width;
-  actor_class->get_preferred_height = clutter_group_get_preferred_height;
-  actor_class->allocate             = clutter_group_allocate;
 }
 
 static void