Use g_list_insert_sorted() instead of our custom function

The clutter_timeout_pool_insert_sorted() function caused an inversion of
the timeout sources in the pool; this led to a wrong behaviour in the
execution of the timeout functions. See bug 471.

This patch drops clutter_timeout_pool_insert_sorted() in favour of the
standard g_list_insert_sorted(), which produces identical behaviours with
and without the pool.

A new test, written by Rob Bradford, has been added to the regression test
suite in order to identify sorting issues with the timeout pools.

clutter/clutter-timeout-pool.c

@@ -95,12 +95,21 @@ clutter_timeout_sort (gconstpointer a,
   gint comparison;
 
   /* Keep 'ready' timeouts at the front */
-  return (comparison = TIMEOUT_READY (t_a) - TIMEOUT_READY (t_b))
-    ? -comparison
-    /* Otherwise sort by expiration time */
-    : ((comparison = t_a->expiration.tv_sec - t_b->expiration.tv_sec)
-       ? comparison
-       : t_a->expiration.tv_usec - t_b->expiration.tv_usec);
+  if (TIMEOUT_READY (t_a))
+    return -1;
+
+  if (TIMEOUT_READY (t_b))
+    return 1;
+  
+  /* Otherwise sort by expiration time */
+  comparison = t_a->expiration.tv_sec - t_b->expiration.tv_sec;
+  if (comparison < 0)
+    return -1;
+
+  if (comparison > 0)
+    return 1;
+
+  return (t_a->expiration.tv_usec - t_b->expiration.tv_usec);
 }
 
 static gint
@@ -320,54 +329,13 @@ clutter_timeout_pool_check (GSource *source)
   return (pool->ready > 0);
 }
 
-static GList *
-clutter_timeout_pool_insert_sorted (GList *node, GList *list)
-{
-  GList *l, *last = NULL, *next;
-
-  /* Find the first timeout which expires after this one */
-  for (l = list; l; l = l->next)
-    {
-      ClutterTimeout *t_a = node->data;
-      ClutterTimeout *t_b = l->data;
-
-      last = l;
-      next = l->next;
-
-      if (clutter_timeout_sort (t_a, t_b) < 0)
-	break;
-    }
-
-  /* If we didn't find an entry then add it at the end */
-  if (!l)
-    {
-      node->prev = last;
-      node->next = NULL;
-
-      if (last)
-	last->next = node;
-    }
-  else
-    {
-      /* Otherwise insert it before the found entry */
-      node->next = l;
-      node->prev = l->prev;
-      l->prev = node;
-
-      if (node->prev)
-	node->prev->next = node;
-    }
-
-  return node->prev ? list : node;
-}
-
 static gboolean
 clutter_timeout_pool_dispatch (GSource     *source,
                                GSourceFunc  func,
                                gpointer     data)
 {
   ClutterTimeoutPool *pool = (ClutterTimeoutPool *) source;
-  GList *l;
+  GList *dispatched_timeouts;
 
   /* the main loop might have predicted this, so we repeat the
    * check for ready timeouts.
@@ -383,6 +351,7 @@ clutter_timeout_pool_dispatch (GSource     *source,
   while (pool->timeouts && pool->timeouts->data && pool->ready-- > 0)
     {
       ClutterTimeout *timeout = pool->timeouts->data;
+      GList *l;
 
       /* One of the ready timeouts may have been removed during dispatch,
        * in which case pool->ready will be wrong, but the ready timeouts
@@ -436,16 +405,22 @@ clutter_timeout_pool_dispatch (GSource     *source,
     }
 
   /* Re-insert the dispatched timeouts in sorted order */
-  while (pool->dispatched_timeouts)
+  dispatched_timeouts = pool->dispatched_timeouts;
+  while (dispatched_timeouts)
     {
-      GList *next = pool->dispatched_timeouts->next;
+      ClutterTimeout *timeout = dispatched_timeouts->data;
+      GList *next = dispatched_timeouts->next;
 
-      pool->timeouts = clutter_timeout_pool_insert_sorted (pool->dispatched_timeouts,
-							   pool->timeouts);
+      if (timeout)
+        pool->timeouts = g_list_insert_sorted (pool->timeouts, timeout,
+                                               clutter_timeout_sort);
 
-      pool->dispatched_timeouts = next;
+      dispatched_timeouts = next;
     }
 
+  g_list_free (pool->dispatched_timeouts);
+  pool->dispatched_timeouts = NULL;
+
   pool->ready = 0;
 
   clutter_threads_leave ();

tests/Makefile.am

@@ -1,7 +1,7 @@
 noinst_PROGRAMS = test-textures test-events test-offscreen test-scale \
                   test-actors test-behave test-text test-entry test-project \
 		  test-boxes test-perspective test-rotate test-depth \
-		  test-threads
+		  test-threads test-timeline
 
 INCLUDES = -I$(top_srcdir)/
 LDADD = $(top_builddir)/clutter/libclutter-@CLUTTER_FLAVOUR@-@CLUTTER_MAJORMINOR@.la
@@ -22,5 +22,6 @@ test_perspective_SOURCES = test-perspective.c
 test_rotate_SOURCES    = test-rotate.c
 test_depth_SOURCES     = test-depth.c
 test_threads_SOURCES   = test-threads.c
+test_timeline_SOURCES  = test-timeline.c
 
 EXTRA_DIST = redhand.png

tests/test-timeline.c

@@ -0,0 +1,55 @@
+#include <clutter/clutter.h>
+
+static void
+timeline_1_new_frame_cb (ClutterTimeline *timeline, gint frame_no)
+{
+  g_debug ("1: Doing frame %d.", frame_no);
+}
+
+static void
+timeline_2_new_frame_cb (ClutterTimeline *timeline, gint frame_no)
+{
+  g_debug ("2: Doing frame %d.", frame_no);
+}
+
+static void
+timeline_3_new_frame_cb (ClutterTimeline *timeline, gint frame_no)
+{
+  g_debug ("3: Doing frame %d.", frame_no);
+}
+
+int
+main (int argc, char **argv)
+{
+  ClutterTimeline *timeline_1;
+  ClutterTimeline *timeline_2;
+  ClutterTimeline *timeline_3;
+
+  clutter_init (&argc, &argv);
+
+  timeline_1 = clutter_timeline_new (100, 50);
+  timeline_2 = clutter_timeline_clone (timeline_1);
+  timeline_3 = clutter_timeline_clone (timeline_1);
+
+  g_signal_connect (timeline_1,
+                    "new-frame", G_CALLBACK (timeline_1_new_frame_cb),
+                    NULL);
+  g_signal_connect (timeline_2,
+                    "new-frame", G_CALLBACK (timeline_2_new_frame_cb),
+                    NULL);
+  g_signal_connect (timeline_3,
+                    "new-frame", G_CALLBACK (timeline_3_new_frame_cb),
+                    NULL);
+
+  clutter_timeline_start (timeline_1);
+  clutter_timeline_start (timeline_2);
+  clutter_timeline_start (timeline_3);
+
+  clutter_main ();
+
+  g_object_unref (timeline_1);
+  g_object_unref (timeline_2);
+  g_object_unref (timeline_3);
+
+  return;
+}