recent-manager: Coalesce multiple changes

Since the ::changed implementation of GtkRecentManager implies a
synchronous write operation, when we receive multiple requests to emit a
::changed signal we might end up blocking.

This change coalesces multiple ::changed emission requests using the
following sequence:

  • the first request will install a timeout in 250 ms, which will
    emit the ::changed signal

  • each further request while the timeout has not been emitted
    will increase a counter

      ‣ if the counter reaches 250 before the timeout has been
        emitted, then the RecentManager will remove the timeout
        source and force a signal emission and reset the counter

This sequence should guarantee that frequent ::changed emission requests
are coalesced, and also guarantee that we don't let them dangle for too
long.

https://bugzilla.gnome.org/show_bug.cgi?id=616997

2 files changed, 143 insertions, 42 deletions

diff --git a/gtk/gtkrecentmanager.c b/gtk/gtkrecentmanager.c
index cb25542..7bd7572 100644
--- a/gtk/gtkrecentmanager.c
+++ b/gtk/gtkrecentmanager.c
@@ -99,6 +99,9 @@ struct _GtkRecentManagerPrivate
   GBookmarkFile *recent_items;
 
   GFileMonitor *monitor;
+
+  guint changed_timeout;
+  guint changed_age;
 };
 
 enum
@@ -338,12 +341,26 @@ gtk_recent_manager_get_property (GObject               *object,
 } 
 
 static void
-gtk_recent_manager_dispose (GObject *object)
+gtk_recent_manager_finalize (GObject *object)
 {
   GtkRecentManager *manager = GTK_RECENT_MANAGER (object);
   GtkRecentManagerPrivate *priv = manager->priv;
 
-  if (priv->monitor)
+  g_free (priv->filename);
+
+  if (priv->recent_items != NULL)
+    g_bookmark_file_free (priv->recent_items);
+
+  G_OBJECT_CLASS (gtk_recent_manager_parent_class)->finalize (object);
+}
+
+static void
+gtk_recent_manager_dispose (GObject *gobject)
+{
+  GtkRecentManager *manager = GTK_RECENT_MANAGER (gobject);
+  GtkRecentManagerPrivate *priv = manager->priv;
+
+  if (priv->monitor != NULL)
     {
       g_signal_handlers_disconnect_by_func (priv->monitor,
                                             G_CALLBACK (gtk_recent_manager_monitor_changed),
@@ -352,21 +369,21 @@ gtk_recent_manager_dispose (GObject *object)
       priv->monitor = NULL;
     }
 
-  G_OBJECT_CLASS (gtk_recent_manager_parent_class)->dispose (object);
-}
-
-static void
-gtk_recent_manager_finalize (GObject *object)
-{
-  GtkRecentManager *manager = GTK_RECENT_MANAGER (object);
-  GtkRecentManagerPrivate *priv = manager->priv;
+  if (priv->changed_timeout != 0)
+    {
+      g_source_remove (priv->changed_timeout);
+      priv->changed_timeout = 0;
+      priv->changed_age = 0;
+    }
 
-  g_free (priv->filename);
-  
-  if (priv->recent_items)
-    g_bookmark_file_free (priv->recent_items);
+  if (priv->is_dirty)
+    {
+      g_object_ref (manager);
+      g_signal_emit (manager, signal_changed, 0);
+      g_object_unref (manager);
+    }
 
-  G_OBJECT_CLASS (gtk_recent_manager_parent_class)->finalize (object);
+  G_OBJECT_CLASS (gtk_recent_manager_parent_class)->dispose (gobject);
 }
 
 static void
@@ -404,8 +421,6 @@ gtk_recent_manager_real_changed (GtkRecentManager *manager)
           else if (age == 0)
             {
               g_bookmark_file_free (priv->recent_items);
-              priv->recent_items = NULL;
-
               priv->recent_items = g_bookmark_file_new ();
             }
         }
@@ -1115,7 +1130,6 @@ gtk_recent_manager_add_full (GtkRecentManager     *manager,
    * will dump our changes
    */
   priv->is_dirty = TRUE;
-  
   gtk_recent_manager_changed (manager);
   
   return TRUE;
@@ -1176,7 +1190,6 @@ gtk_recent_manager_remove_item (GtkRecentManager  *manager,
     }
 
   priv->is_dirty = TRUE;
-
   gtk_recent_manager_changed (manager);
   
   return TRUE;
@@ -1400,7 +1413,6 @@ gtk_recent_manager_move_item (GtkRecentManager  *recent_manager,
     }
   
   priv->is_dirty = TRUE;
-
   gtk_recent_manager_changed (recent_manager);
   
   return TRUE;
@@ -1455,17 +1467,15 @@ purge_recent_items_list (GtkRecentManager  *manager,
 {
   GtkRecentManagerPrivate *priv = manager->priv;
 
-  if (!priv->recent_items)
+  if (priv->recent_items == NULL)
     return;
-  
+
   g_bookmark_file_free (priv->recent_items);
-  priv->recent_items = NULL;
-      
   priv->recent_items = g_bookmark_file_new ();
   priv->size = 0;
-  priv->is_dirty = TRUE;
-      
+
   /* emit the changed signal, to ensure that the purge is written */
+  priv->is_dirty = TRUE;
   gtk_recent_manager_changed (manager);
 }
 
@@ -1505,10 +1515,43 @@ gtk_recent_manager_purge_items (GtkRecentManager  *manager,
   return purged;
 }
 
+static gboolean
+emit_manager_changed (gpointer data)
+{
+  GtkRecentManager *manager = data;
+
+  manager->priv->changed_age = 0;
+  manager->priv->changed_timeout = 0;
+
+  g_signal_emit (manager, signal_changed, 0);
+
+  return FALSE;
+}
+
 static void
-gtk_recent_manager_changed (GtkRecentManager *recent_manager)
+gtk_recent_manager_changed (GtkRecentManager *manager)
 {
-  g_signal_emit (recent_manager, signal_changed, 0);
+  /* coalesce consecutive changes
+   *
+   * we schedule a write in 250 msecs immediately; if we get more than one
+   * request per millisecond before the timeout has a chance to run, we
+   * schedule an emission immediately.
+   */
+  if (manager->priv->changed_timeout == 0)
+    manager->priv->changed_timeout = gdk_threads_add_timeout (250, emit_manager_changed, manager);
+  else
+    {
+      manager->priv->changed_age += 1;
+
+      if (manager->priv->changed_age > 250)
+        {
+          g_source_remove (manager->priv->changed_timeout);
+          g_signal_emit (manager, signal_changed, 0);
+
+          manager->priv->changed_age = 0;
+          manager->priv->changed_timeout = 0;
+        }
+    }
 }
 
 static void
diff --git a/gtk/tests/recentmanager.c b/gtk/tests/recentmanager.c
index 6f0fa58..7f8f95b 100644
--- a/gtk/tests/recentmanager.c
+++ b/gtk/tests/recentmanager.c
@@ -19,6 +19,7 @@
  * Boston, MA 02111-1307, USA.
  */
 
+#include <glib/gstdio.h>
 #include <gtk/gtk.h>
 
 const gchar *uri = "file:///tmp/testrecentchooser.txt";
@@ -95,6 +96,69 @@ recent_manager_add (void)
   g_slice_free (GtkRecentData, recent_data);
 }
 
+typedef struct {
+  GMainLoop *main_loop;
+  gint counter;
+} AddManyClosure;
+
+static void
+check_bulk (GtkRecentManager *manager,
+            gpointer          data)
+{
+  AddManyClosure *closure = data;
+
+  if (g_test_verbose ())
+    g_print (G_STRLOC ": counter = %d\n", closure->counter);
+
+  g_assert_cmpint (closure->counter, ==, 100);
+
+  if (g_main_loop_is_running (closure->main_loop))
+    g_main_loop_quit (closure->main_loop);
+}
+
+static void
+recent_manager_add_many (void)
+{
+  GtkRecentManager *manager = g_object_new (GTK_TYPE_RECENT_MANAGER,
+                                            "filename", "recently-used.xbel",
+                                            NULL);
+  AddManyClosure *closure = g_new (AddManyClosure, 1);
+  GtkRecentData *data = g_slice_new0 (GtkRecentData);
+  gint i;
+
+  closure->main_loop = g_main_loop_new (NULL, FALSE);
+  closure->counter = 0;
+
+  g_signal_connect (manager, "changed", G_CALLBACK (check_bulk), closure);
+
+  for (i = 0; i < 100; i++)
+    {
+      gchar *new_uri;
+
+      data->mime_type = "text/plain";
+      data->app_name = "testrecentchooser";
+      data->app_exec = "testrecentchooser %u";
+
+      if (g_test_verbose ())
+        g_print (G_STRLOC ": adding item %d\n", i);
+
+      new_uri = g_strdup_printf ("file:///doesnotexist-%d.txt", i);
+      gtk_recent_manager_add_full (manager, new_uri, data);
+      g_free (new_uri);
+
+      closure->counter += 1;
+    }
+
+  g_main_loop_run (closure->main_loop);
+
+  g_main_loop_unref (closure->main_loop);
+  g_slice_free (GtkRecentData, data);
+  g_free (closure);
+  g_object_unref (manager);
+
+  g_assert_cmpint (g_unlink ("recently-used.xbel"), ==, 0);
+}
+
 static void
 recent_manager_has_item (void)
 {
@@ -234,20 +298,14 @@ main (int    argc,
 {
   gtk_test_init (&argc, &argv, NULL);
 
-  g_test_add_func ("/recent-manager/get-default",
-                   recent_manager_get_default);
-  g_test_add_func ("/recent-manager/add",
-                   recent_manager_add);
-  g_test_add_func ("/recent-manager/has-item",
-                   recent_manager_has_item);
-  g_test_add_func ("/recent-manager/move-item",
-                   recent_manager_move_item);
-  g_test_add_func ("/recent-manager/lookup-item",
-                   recent_manager_lookup_item);
-  g_test_add_func ("/recent-manager/remove-item",
-                   recent_manager_remove_item);
-  g_test_add_func ("/recent-manager/purge",
-                   recent_manager_purge);
+  g_test_add_func ("/recent-manager/get-default", recent_manager_get_default);
+  g_test_add_func ("/recent-manager/add", recent_manager_add);
+  g_test_add_func ("/recent-manager/add-many", recent_manager_add_many);
+  g_test_add_func ("/recent-manager/has-item", recent_manager_has_item);
+  g_test_add_func ("/recent-manager/move-item", recent_manager_move_item);
+  g_test_add_func ("/recent-manager/lookup-item", recent_manager_lookup_item);
+  g_test_add_func ("/recent-manager/remove-item", recent_manager_remove_item);
+  g_test_add_func ("/recent-manager/purge", recent_manager_purge);
 
   return g_test_run ();
 }