[glib/gdbus-codegen] gdbus-codegen: Don't send out PropertiesChanged if value ends up not changing

[David Zeuthen <davidz src gnome org>]

commit 6bccc46d152079d69cf8aebef08433b1ec6055c7
Author: David Zeuthen <davidz redhat com>
Date:   Fri Apr 15 15:53:28 2011 -0400

    gdbus-codegen: Don't send out PropertiesChanged if value ends up not changing
    
    A fairly typical pattern is to have code that does
    
     foo_set_bar (object, "");
     if (some_condition)
       {
         foo_set_bar (object, "yes");
       }
    
    where some_condition is often true every time @object is updated.
    
    With this code, bar is essentially always "yes" but because of how
    gdbus-codegen works, useless PropertiesChanged events got scheduled
    and sent out. With this patch, we avoid that by always keeping the
    original value around and comparing it only when we deem it's time to
    send out the ::PropertiesChanged signal (typically in an idle but can
    be forced by the user via flush()).
    
    Also add a test case for this.
    
    Signed-off-by: David Zeuthen <davidz redhat com>

 gio/gdbus-codegen/codegen.py   |   49 ++++++++++++++++++++++-----------
 gio/tests/gdbus-test-codegen.c |   59 +++++++++++++++++++++++++++++++++++++++-
 gio/tests/test-codegen.xml     |    2 +
 3 files changed, 93 insertions(+), 17 deletions(-)
---
diff --git a/gio/gdbus-codegen/codegen.py b/gio/gdbus-codegen/codegen.py
index 7a685f8..d86147d 100644
--- a/gio/gdbus-codegen/codegen.py
+++ b/gio/gdbus-codegen/codegen.py
@@ -96,14 +96,14 @@ class CodeGenerator:
         self.c.write('typedef struct\n'
                      '{\n'
                      '  const _ExtendedGDBusPropertyInfo *info;\n'
-                     '  GParamSpec *pspec;\n'
-                     '  GValue value;\n'
+                     '  guint prop_id;\n'
+                     '  GValue orig_value; /* the value before the change */\n'
                      '} ChangedProperty;\n'
                      '\n'
                      'static void\n'
                      '_changed_property_free (ChangedProperty *data)\n'
                      '{\n'
-                     '  g_value_unset (&data->value);\n'
+                     '  g_value_unset (&data->orig_value);\n'
                      '  g_free (data);\n'
                      '}\n'
                      '\n')
@@ -1685,6 +1685,18 @@ class CodeGenerator:
         self.c.write('}\n'
                      '\n')
         if len(i.properties) > 0:
+            # if property is already scheduled then re-use entry.. though it could be
+            # that the user did
+            #
+            #  foo_set_prop_bar (object, "");
+            #  foo_set_prop_bar (object, "blah");
+            #
+            # say, every update... In this case, where nothing happens, we obviously
+            # don't want a PropertiesChanged() event. We can easily check for this
+            # by comparing against the _original value_ recorded before the first
+            # change event. If the latest value is not different from the original
+            # one, we can simply ignore the ChangedProperty
+            #
             self.c.write('static gboolean\n'
                          '_%s_emit_changed (gpointer user_data)\n'
                          '{\n'
@@ -1699,9 +1711,15 @@ class CodeGenerator:
                          '    {\n'
                          '      ChangedProperty *cp = l->data;\n'
                          '      GVariant *variant;\n'
-                         '      variant = g_dbus_gvalue_to_gvariant (&cp->value, G_VARIANT_TYPE (cp->info->parent_struct.signature));\n'
-                         '      g_variant_builder_add (&builder, "{sv}", cp->info->parent_struct.name, variant);\n'
-                         '      g_variant_unref (variant);\n'
+                         '      const GValue *cur_value;\n'
+                         '\n'
+                         '      cur_value = &skeleton->priv->properties->values[cp->prop_id - 1];\n'
+                         '      if (!_g_value_equal (cur_value, &cp->orig_value))\n'
+                         '        {\n'
+                         '          variant = g_dbus_gvalue_to_gvariant (cur_value, G_VARIANT_TYPE (cp->info->parent_struct.signature));\n'
+                         '          g_variant_builder_add (&builder, "{sv}", cp->info->parent_struct.name, variant);\n'
+                         '          g_variant_unref (variant);\n'
+                         '        }\n'
                          '    }\n'
                          '  g_dbus_connection_emit_signal (g_dbus_interface_skeleton_get_connection (G_DBUS_INTERFACE_SKELETON (skeleton)),\n'
                          '                                 NULL, g_dbus_interface_skeleton_get_object_path (G_DBUS_INTERFACE_SKELETON (skeleton)),\n'
@@ -1719,9 +1737,8 @@ class CodeGenerator:
             self.c.write('  return FALSE;\n'
                          '}\n'
                          '\n')
-            # if property is already scheduled then re-use entry
             self.c.write('static void\n'
-                         '_%s_schedule_emit_changed (%sSkeleton *skeleton, const _ExtendedGDBusPropertyInfo *info, GParamSpec *pspec, const GValue *value)\n'
+                         '_%s_schedule_emit_changed (%sSkeleton *skeleton, const _ExtendedGDBusPropertyInfo *info, guint prop_id, const GValue *orig_value)\n'
                          '{\n'
                          '  ChangedProperty *cp;\n'
                          '  GList *l;\n'
@@ -1732,19 +1749,19 @@ class CodeGenerator:
                          '      if (i_cp->info == info)\n'
                          '        {\n'
                          '          cp = i_cp;\n'
-                         '          g_value_unset (&cp->value);\n'
                          '          break;\n'
                          '        }\n'
                          '    }\n'
-                         '  if (cp == NULL)\n'
+                         %(i.name_lower, i.camel_name))
+            self.c.write('  if (cp == NULL)\n'
                          '    {\n'
                          '      cp = g_new0 (ChangedProperty, 1);\n'
-                         '      cp->pspec = pspec;\n'
+                         '      cp->prop_id = prop_id;\n'
                          '      cp->info = info;\n'
                          '      skeleton->priv->changed_properties = g_list_prepend (skeleton->priv->changed_properties, cp);\n'
+                         '      g_value_init (&cp->orig_value, G_VALUE_TYPE (orig_value));\n'
+                         '      g_value_copy (orig_value, &cp->orig_value);\n'
                          '    }\n'
-                         '  g_value_init (&cp->value, G_VALUE_TYPE (value));\n'
-                         '  g_value_copy (value, &cp->value);\n'
                          '  if (skeleton->priv->changed_properties_idle_source == NULL)\n'
                          '    {\n'
                          '      skeleton->priv->changed_properties_idle_source = g_idle_source_new ();\n'
@@ -1755,7 +1772,7 @@ class CodeGenerator:
                          '    }\n'
                          '}\n'
                          '\n'
-                         %(i.name_lower, i.camel_name, i.name_lower))
+                         %(i.name_lower))
         self.c.write('static void\n'
                      '%s_skeleton_set_property (GObject      *object,\n'
                      '  guint         prop_id,\n'
@@ -1767,10 +1784,10 @@ class CodeGenerator:
                          '  g_assert (prop_id - 1 >= 0 && prop_id - 1 < %d);\n'
                          '  if (!_g_value_equal (value, &skeleton->priv->properties->values[prop_id - 1]))\n'
                          '    {\n'
+                         '      if (g_dbus_interface_skeleton_get_connection (G_DBUS_INTERFACE_SKELETON (skeleton)) != NULL)\n'
+                         '        _%s_schedule_emit_changed (skeleton, _%s_property_info_pointers[prop_id - 1], prop_id, &skeleton->priv->properties->values[prop_id - 1]);\n'
                          '      g_value_copy (value, &skeleton->priv->properties->values[prop_id - 1]);\n'
                          '      g_object_notify_by_pspec (object, pspec);\n'
-                         '      if (g_dbus_interface_skeleton_get_connection (G_DBUS_INTERFACE_SKELETON (skeleton)) != NULL)\n'
-                         '        _%s_schedule_emit_changed (skeleton, _%s_property_info_pointers[prop_id - 1], pspec, value);\n'
                          '    }\n'
                          %(i.camel_name, i.ns_upper, i.name_upper, len(i.properties), i.name_lower, i.name_lower))
         self.c.write('}\n'
diff --git a/gio/tests/gdbus-test-codegen.c b/gio/tests/gdbus-test-codegen.c
index de3d3b0..0b8ff15 100644
--- a/gio/tests/gdbus-test-codegen.c
+++ b/gio/tests/gdbus-test-codegen.c
@@ -212,6 +212,26 @@ on_handle_request_multi_property_mods (FooBar                 *object,
   return TRUE;
 }
 
+static gboolean
+on_handle_property_cancellation (FooBar                 *object,
+                                 GDBusMethodInvocation  *invocation,
+                                 gpointer                user_data)
+{
+  guint n;
+  n = foo_bar_get_n (object);
+  /* This queues up a PropertiesChange event */
+  foo_bar_set_n (object, n + 1);
+  /* this modifies the queued up event */
+  foo_bar_set_n (object, n);
+  /* this flushes all PropertiesChanges event (sends the D-Bus message right
+   * away, if any - there should not be any)
+   */
+  g_dbus_interface_skeleton_flush (G_DBUS_INTERFACE_SKELETON (object));
+  /* this makes us return the reply D-Bus method */
+  foo_bar_complete_property_cancellation (object, invocation);
+  return TRUE;
+}
+
 /* ---------------------------------------------------------------------------------------------------- */
 
 static gboolean
@@ -455,6 +475,10 @@ on_bus_acquired (GDBusConnection *connection,
                     "handle-request-multi-property-mods",
                     G_CALLBACK (on_handle_request_multi_property_mods),
                     NULL);
+  g_signal_connect (exported_bar_object,
+                    "handle-property-cancellation",
+                    G_CALLBACK (on_handle_property_cancellation),
+                    NULL);
 
   exported_bat_object = foo_bat_skeleton_new ();
   g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (exported_bat_object),
@@ -641,6 +665,23 @@ on_test_signal (FooBar              *proxy,
 }
 
 static void
+on_property_cancellation_cb (FooBar        *proxy,
+                             GAsyncResult  *res,
+                             gpointer       user_data)
+{
+  ClientData *data = user_data;
+  gboolean ret;
+  GError *error = NULL;
+
+  error = NULL;
+  ret = foo_bar_call_property_cancellation_finish (proxy, res, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_main_loop_quit (data->thread_loop);
+}
+
+static void
 check_bar_proxy (FooBar    *proxy,
                  GMainLoop *thread_loop)
 {
@@ -949,10 +990,26 @@ check_bar_proxy (FooBar    *proxy,
   _g_assert_property_notify (proxy, "n");
   g_assert_cmpint (foo_bar_get_n (proxy), ==, 10042);
   g_assert_cmpint (data->num_notify_n, ==, 2);
-
   /* Checks that u didn't change at all */
   g_assert_cmpint (data->num_notify_u, ==, 3);
 
+  /* Now we check that if the service does
+   *
+   *   guint n = foo_bar_get_n (foo);
+   *   foo_bar_set_n (foo, n + 1);
+   *   foo_bar_set_n (foo, n);
+   *
+   *  then no PropertiesChanged() signal is emitted!
+   */
+  error = NULL;
+  foo_bar_call_property_cancellation (proxy,
+                                      NULL, /* GCancellable */
+                                      (GAsyncReadyCallback) on_property_cancellation_cb,
+                                      data);
+  g_main_loop_run (thread_loop);
+  /* Checks that n didn't change at all */
+  g_assert_cmpint (data->num_notify_n, ==, 2);
+
   /* cleanup */
   g_free (data);
 }
diff --git a/gio/tests/test-codegen.xml b/gio/tests/test-codegen.xml
index cec5714..162d850 100644
--- a/gio/tests/test-codegen.xml
+++ b/gio/tests/test-codegen.xml
@@ -64,6 +64,8 @@
 
     <method name="UnimplementedMethod"/>
 
+    <method name="PropertyCancellation"/>
+
     <signal name="TestSignal">
       <annotation name="org.gtk.GDBus.DocString" value="Signal documentation."/>
       <arg type="i" name="val_int32">