[dconf: 6/7] engine: remove spurious local change notifications

[Daniel Playfair Cal <danielplayfaircal src gnome org>]

commit 5ed0724cd763cf79dcc8f6802a6aa093d635cac7
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 |  84 +++++++++++++++++++++++++-
 tests/engine.c        | 162 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 241 insertions(+), 5 deletions(-)
---
diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c
index cbb2a00e..6a1f2478 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,54 @@ dconf_engine_list (DConfEngine *engine,
   return list;
 }
 
+static gboolean
+dconf_engine_dir_has_writable_contents (DConfEngine *engine,
+                                        const gchar *dir)
+{
+  DConfChangeset *database;
+  GHashTable *current_state;
+
+  /* Read the on disk state */
+  if (engine->n_sources == 0 || !engine->sources[0]->writable)
+    // If there are no writable sources, there won't be any pending writes either
+    return FALSE;
+
+  dconf_engine_acquire_sources (engine);
+  database = dconf_gvdb_utils_changeset_from_table (engine->sources[0]->values);
+  dconf_engine_release_sources (engine);
+
+  /* Apply pending and in_flight changes to the on disk state */
+  dconf_engine_lock_queue (engine);
+
+  if (engine->in_flight != NULL)
+    dconf_changeset_change (database, engine->in_flight);
+
+  if (engine->pending != NULL)
+    {
+      /**
+       * We don't want to seal the pending changeset because it may still be changed,
+       * and sealing the changeset would be a side effect of passing engine->pending
+       * directly into dconf_changeset_change.
+       */
+      DConfChangeset *changes = dconf_changeset_filter_changes (database, engine->pending);
+      if (changes != NULL)
+        {
+          dconf_changeset_change (database, changes);
+          dconf_changeset_unref (changes);
+        }
+    }
+
+  dconf_engine_unlock_queue (engine);
+
+  /* Check if there are writable contents at the given directory in the current state */
+  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 +1178,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,
@@ -1274,6 +1351,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;
 
@@ -1298,7 +1379,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 fd2a3482..b0ad884e 100644
--- a/tests/engine.c
+++ b/tests/engine.c
@@ -1466,7 +1466,7 @@ test_watch_sync (void)
 static void
 test_change_fast (void)
 {
-  DConfChangeset *empty, *good_write, *bad_write, *very_good_write, *slightly_bad_write;
+  DConfChangeset *empty, *good_write, *good_write2, *bad_write, *very_good_write, *slightly_bad_write;
   GvdbTable *table, *locks;
   DConfEngine *engine;
   gboolean success;
@@ -1483,6 +1483,7 @@ test_change_fast (void)
 
   empty = dconf_changeset_new ();
   good_write = dconf_changeset_new_write ("/value", g_variant_new_string ("value"));
+  good_write2 = dconf_changeset_new_write ("/value2", g_variant_new_string ("value2"));
   bad_write = dconf_changeset_new_write ("/locked", g_variant_new_string ("value"));
   very_good_write = dconf_changeset_new_write ("/value", g_variant_new_string ("value"));
   dconf_changeset_set (very_good_write, "/to-reset", NULL);
@@ -1517,6 +1518,15 @@ test_change_fast (void)
   dconf_mock_dbus_assert_no_async ();
   g_assert_cmpstr (change_log->str, ==, "");
 
+  /* Verify that value is unset initially */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value");
+  g_assert (value == NULL);
+
+  /* Verify that value2 is unset initially */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2");
+  g_assert (value == NULL);
+
+  /* change /value */
   success = dconf_engine_change_fast (engine, good_write, NULL, &error);
   g_assert_no_error (error);
   g_assert (success);
@@ -1530,7 +1540,48 @@ test_change_fast (void)
   g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value");
   g_variant_unref (value);
 
-  /* Fail the attempted write.  This should cause a warning and a change. */
+  /* Repeat the same write for /value (which is already in the in_flight queue) */
+  success = dconf_engine_change_fast (engine, good_write, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (success);
+
+  /* Verify that /value is (still) set */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value");
+  g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value");
+  g_variant_unref (value);
+
+  /* That should not have emitted a synthetic change event, since the (local) value did not change */
+  g_assert_cmpstr (change_log->str, ==, "");
+  g_string_set_size (change_log, 0);
+
+  /* change /value2 */
+  success = dconf_engine_change_fast (engine, good_write2, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (success);
+
+  /* That should have emitted a synthetic change event */
+  g_assert_cmpstr (change_log->str, ==, "/value2:1::nil;");
+  g_string_set_size (change_log, 0);
+
+  /* Verify that /value2 is set */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2");
+  g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value2");
+  g_variant_unref (value);
+
+  /* change /value2 a second time */
+  success = dconf_engine_change_fast (engine, good_write2, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (success);
+
+  /* That should not have emitted a synthetic change event because the (local) value didn't change */
+  g_assert_cmpstr (change_log->str, ==, "");
+
+  /* Verify that /value2 is still set */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2");
+  g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value2");
+  g_variant_unref (value);
+
+  /* Fail the first attempted write.  This should cause a warning and a signal. */
   error = g_error_new_literal (G_FILE_ERROR, G_FILE_ERROR_NOENT, "something failed");
   dconf_mock_dbus_async_reply (NULL, error);
   g_clear_error (&error);
@@ -1539,8 +1590,25 @@ test_change_fast (void)
 
   assert_pop_message ("dconf", G_LOG_LEVEL_WARNING, "failed to commit changes to dconf: something failed");
 
-  /* Verify that the value became unset due to the failure */
-  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "value");
+  /* Verify that /value is still set (because the second write is in progress) */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value");
+  g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value");
+  g_variant_unref (value);
+
+  /* Verify that /value2 is still set because the write is in progress */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2");
+  g_assert_cmpstr (g_variant_get_string (value, NULL), ==, "value2");
+  g_variant_unref (value);
+
+  /* Now allow the second set of writes to succeed */
+  dconf_mock_dbus_async_reply (g_variant_new ("(s)", "tag"), NULL);
+
+  /* Verify that /value became unset due to the in flight queue clearing */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value");
+  g_assert (value == NULL);
+
+  /* Verify that /value2 became unset due to the in flight queue clearing */
+  value = dconf_engine_read (engine, DCONF_READ_FLAGS_NONE, NULL, "/value2");
   g_assert (value == NULL);
 
   /* Now try a successful write */
@@ -1619,6 +1687,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 +2240,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);