[dconf/patch/engine-check: 4/4] engine: remove spurious local change notifications

[Daniel Playfair Cal <danielplayfaircal src gnome org>]

commit c54df02ff750a27084393d75ec6499a72de5c791
Author: Daniel Playfair Cal <daniel playfair cal gmail com>
Date:   Tue Dec 12 00:18:18 2017 +1100

    engine: remove spurious local change notifications
    
    When used with the "fast" (optimistic concurrency) API, the engine library
    emits a change notification local to a process after a change is initiated
    from that process. Previously, it would emit this notification even if the
    newly written value was the same as the previous value (according to that
    process's view of the state). After this change, the local change
    notification is not sent unless the new value is different from the
    current value (as seen by that process).

 engine/dconf-engine.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 tests/engine.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 172 insertions(+), 1 deletion(-)
---
diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c
index 18b8aa5..24f86a9 100644
--- a/engine/dconf-engine.c
+++ b/engine/dconf-engine.c
@@ -24,6 +24,7 @@
 
 #include "../common/dconf-enums.h"
 #include "../common/dconf-paths.h"
+#include "../common/dconf-gvdb-utils.h"
 #include "../gvdb/gvdb-reader.h"
 #include <string.h>
 #include <stdlib.h>
@@ -822,6 +823,57 @@ dconf_engine_list (DConfEngine *engine,
   return list;
 }
 
+static gboolean
+dconf_engine_dir_has_writable_contents (DConfEngine *engine,
+                                        const gchar *dir)
+{
+  DConfEngineSource *first_source;
+  DConfChangeset *database;
+  GHashTable *current_state;
+
+  dconf_engine_acquire_sources (engine);
+
+  first_source = engine->sources[0];
+  if (!first_source->writable || first_source->values == NULL)
+    {
+      dconf_engine_release_sources (engine);
+      return FALSE;
+    }
+
+  database = dconf_changeset_new_from_gvdb_table (first_source->values);
+
+  dconf_engine_release_sources (engine);
+
+  // Apply pending and in_flight changes to the on disk state
+  if (engine->in_flight != NULL)
+    {
+      DConfChangeset *changes = dconf_changeset_filter_changes (database, engine->in_flight);
+      if (changes != NULL)
+        {
+          dconf_changeset_change (database, changes);
+          dconf_changeset_unref (changes);
+        }
+    }
+
+  if (engine->pending != NULL)
+    {
+      DConfChangeset *changes = dconf_changeset_filter_changes (database, engine->pending);
+      if (changes != NULL)
+        {
+          dconf_changeset_change (database, changes);
+          dconf_changeset_unref (changes);
+        }
+    }
+
+  current_state = dconf_gvdb_utils_table_from_changeset (database);
+
+  gboolean result = g_hash_table_contains (current_state, dir);
+
+  g_hash_table_unref (current_state);
+  dconf_changeset_unref (database);
+  return result;
+}
+
 typedef void (* DConfEngineCallHandleCallback) (DConfEngine  *engine,
                                                 gpointer      handle,
                                                 GVariant     *parameter,
@@ -1129,6 +1181,34 @@ dconf_engine_prepare_change (DConfEngine     *engine,
  */
 static void dconf_engine_manage_queue (DConfEngine *engine);
 
+/**
+ * a #DConfChangesetPredicate which determines whether the given path and
+ * value is already present in the given engine. "Already present" means
+ * that setting that path to that value would have no effect on the
+ * engine, including for directory resets.
+ */
+static gboolean
+dconf_engine_path_has_value_predicate (const gchar *path,
+                                      GVariant *new_value,
+                                      gpointer user_data)
+{
+  DConfEngine *engine = user_data;
+
+  // Path reset are handled specially
+  if (g_str_has_suffix (path, "/"))
+    return !dconf_engine_dir_has_writable_contents (engine, path);
+
+  g_autoptr(GVariant) current_value = dconf_engine_read (
+    engine,
+    DCONF_READ_USER_VALUE,
+    NULL,
+    path
+  );
+  return ((current_value == NULL && new_value == NULL) ||
+          (current_value != NULL && new_value != NULL &&
+           g_variant_equal (current_value, new_value)));
+}
+
 static void
 dconf_engine_emit_changes (DConfEngine    *engine,
                            DConfChangeset *changeset,
@@ -1273,6 +1353,10 @@ dconf_engine_change_fast (DConfEngine     *engine,
   if (dconf_changeset_is_empty (changeset))
     return TRUE;
 
+  gboolean has_no_effect = dconf_changeset_all (changeset,
+                                                dconf_engine_path_has_value_predicate,
+                                                engine);
+
   if (!dconf_engine_changeset_changes_only_writable_keys (engine, changeset, error))
     return FALSE;
 
@@ -1297,7 +1381,8 @@ dconf_engine_change_fast (DConfEngine     *engine,
   dconf_engine_unlock_queue (engine);
 
   /* Emit the signal after dropping the lock to avoid deadlock on re-entry. */
-  dconf_engine_emit_changes (engine, changeset, origin_tag);
+  if (!has_no_effect)
+    dconf_engine_emit_changes (engine, changeset, origin_tag);
 
   return TRUE;
 }
diff --git a/tests/engine.c b/tests/engine.c
index fd2a348..c629572 100644
--- a/tests/engine.c
+++ b/tests/engine.c
@@ -1619,6 +1619,91 @@ test_change_fast (void)
   change_log = NULL;
 }
 
+/**
+ * Tests that dconf_engine_change_fast() emits local optimistic change
+ * notifications in the right circumstances
+ */
+static void
+test_change_fast_redundant (void)
+{
+  DConfChangeset *change;
+  DConfEngine *engine;
+  change_log = g_string_new (NULL);
+
+  // Initialise an empty engine
+  engine = dconf_engine_new (SRCDIR "/profile/dos", NULL, NULL);
+
+  // Send an empty changeset, which has no effect
+  change = dconf_changeset_new ();
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "");
+
+  // Reset the root directory, which has no effect since the database is empty
+  change = dconf_changeset_new_write ("/", NULL);
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "");
+
+  // Set apple to NULL, which has no effect because it was already unset
+  change = dconf_changeset_new_write ("/apple", NULL);
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "");
+
+  // Set apple to apple
+  change = dconf_changeset_new_write ("/apple", g_variant_new_string ("apple"));
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;");
+  g_string_set_size (change_log, 0);
+
+  // Set apple to apple, which has no effect because it is the same as the old value
+  change = dconf_changeset_new_write ("/apple", g_variant_new_string ("apple"));
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "");
+  g_string_set_size (change_log, 0);
+
+  // Set apple to orange, which has an effect because it is different to the old value
+  change = dconf_changeset_new_write ("/apple", g_variant_new_string ("orange"));
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;");
+  g_string_set_size (change_log, 0);
+
+  // Set apple to NULL, which has an effect because it was previously set
+  change = dconf_changeset_new_write ("/apple", NULL);
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;");
+  g_string_set_size (change_log, 0);
+
+  // Set apple to apple
+  change = dconf_changeset_new_write ("/apple", g_variant_new_string ("apple"));
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "/apple:1::nil;");
+  g_string_set_size (change_log, 0);
+
+  // Reset the root directory, which has an effect since the database is not empty
+  change = dconf_changeset_new_write ("/", NULL);
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "/:1::nil;");
+  g_string_set_size (change_log, 0);
+
+  // Reset the root directory again, which has no effect since the database is empty
+  change = dconf_changeset_new_write ("/", NULL);
+  dconf_engine_change_fast (engine, change, NULL, NULL);
+  dconf_changeset_unref (change);
+  g_assert_cmpstr (change_log->str, ==, "");
+
+  dconf_engine_unref (engine);
+  g_string_free (change_log, TRUE);
+  change_log = NULL;
+}
+
 static GError *change_sync_error;
 static GVariant *change_sync_result;
 
@@ -2087,6 +2172,7 @@ main (int argc, char **argv)
   g_test_add_func ("/engine/watch/fast/short_lived", test_watch_fast_short_lived_subscriptions);
   g_test_add_func ("/engine/watch/sync", test_watch_sync);
   g_test_add_func ("/engine/change/fast", test_change_fast);
+  g_test_add_func ("/engine/change/fast_redundant", test_change_fast_redundant);
   g_test_add_func ("/engine/change/sync", test_change_sync);
   g_test_add_func ("/engine/signals", test_signals);
   g_test_add_func ("/engine/sync", test_sync);