Merge lp:~hikiko/unity-control-center/unity-control-center.replaced-gvariant-array-with-gvariant-builder into lp:unity-control-center

[Eleni Maria Stea]

In display add_dict_entry:
replaced the gvariant arrays with gvariant builder to simplify the code

In display add_dict_entry:
replaced the gvariant arrays with gvariant builder to simplify the code

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:12734
http://jenkins.qa.ubuntu.com/job/unity-control-center-ci/54/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-amd64-ci/54
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-armhf-ci/54
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-i386-ci/55

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-control-center-ci/54/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:12735
http://jenkins.qa.ubuntu.com/job/unity-control-center-ci/55/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-amd64-ci/55
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-armhf-ci/55
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-i386-ci/56

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-control-center-ci/55/rebuild

review: Approve (continuous-integration)

[Allison Karlitskaya (desrt)]

+ if (!key || !value)

 This kind of checking is normally done with g_return_val_if_fail() and common practice here is to return NULL (since typically you want things like this to cause crashes rather than be silently ignored). In any case, you should definitely use g_critical() instead of g_warning() for "programmer error" type situations. g_warning() is more like "something unexpected happened, but it was not the programmer's fault" -- like an invalidly-formatted D-Bus message or something.

You also attempt to deal with a NULL dictionary. That should not be possible. g_settings_get_value() will always return non-NULL (assuming you give it a valid GSettings object and key).

36 if (!dict)
37 {
38 + GVariant *pair[2];
39 + GVariant *dict_entry;
40 +
41 + pair[0] = g_variant_new_string (key);
42 + pair[1] = g_variant_new_int32 (value);
43 + dict_entry = g_variant_new_dict_entry (pair[0], pair[1]);
44 dict = g_variant_new_array (G_VARIANT_TYPE_DICT_ENTRY, &dict_entry, 1);
45 +
46 return dict;
47 }

As per the comment above (dict can never be NULL) you should delete this, but (just as an aside) note that a much easier way to do the same would be:

  return g_variant_new_parsed ("{ %s: %i }", key, value);

You should change the cases of:

  g_settings_get (..., "@a{si}", &value);

to

  value = g_settings_get_value(...);

and similarly:

  g_settings_set (..., "@a{si}", value);

to

  g_settings_set_value (..., value);

because they are more typesafe (and also more efficient).

  add_dict_entry (dict, monitor_name, value);
  g_settings_set (self->priv->desktop_settings, "scale-factor", "@a{si}", dict);

This code doesn't work because 'dict' is not updated in place, but rather a new value is returned (which you ignore). You must use the new value. You must also free the old value when you're done with it, so you're probably going to need two variables (unless you nest the call to add_dict_entry() inside of the GSettings call).

Floating GVariant references only come from functions named g_Variant_new_* in the name or g_variant_*_end(), so your call to g_variant_builder_end() gives you a floating reference which will be taken care of automatically when you pass it to g_settings_set_value(). The same is not true of the value you get from GSettings. You must free this one for yourself.

    g_settings_get (self->priv->desktop_settings, "scale-factor", "@a{si}", &dict);
    dict = add_dict_entry (dict, monitor_name, value);
    g_settings_set (self->priv->desktop_settings, "scale-factor", "@a{si}", dict);

This one does the update properly, but it leaks the old value of dict (and should be changed to g_settings_get_value/g_settings_set_value as mentioned above).

review: Needs Fixing

[Eleni Maria Stea (hikiko)]

fixed :)

[Allison Karlitskaya (desrt)]

This looks great to me. Assuming you tested it, +1.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:12736
http://jenkins.qa.ubuntu.com/job/unity-control-center-ci/57/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-amd64-ci/57
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-armhf-ci/57
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-control-center-trusty-i386-ci/58

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-control-center-ci/57/rebuild

review: Approve (continuous-integration)

=== modified file 'panels/display/cc-display-panel.c'
--- panels/display/cc-display-panel.c	2014-02-24 16:53:59 +0000
+++ panels/display/cc-display-panel.c	2014-02-26 15:59:25 +0000
@@ -566,49 +566,23 @@
 static GVariant*
 add_dict_entry (GVariant *dict, const char *key, int value)
 {
-  GVariant *dict_entry;
-  GVariant **dict_entries;
-  GVariant *tmp;
-  GVariant *pair[2];
+  GVariantBuilder builder;
   GVariantIter iter;
-  unsigned long sz;
-  char str[512];
-  int i = 0;
-
-  pair[0] = g_variant_new_string (key);
-  pair[1] = g_variant_new_int32 (value);
-  dict_entry = g_variant_new_dict_entry (pair[0], pair[1]);
-
-  if (!dict)
-  {
-    dict = g_variant_new_array (G_VARIANT_TYPE_DICT_ENTRY, &dict_entry, 1);
-    return dict;
-  }
-
-  sz = g_variant_n_children (dict);
-  dict_entries = malloc (sizeof(GVariant*) * (sz + 1));
-
+
+  const gchar *k;
+  guint32 v;
+
+  g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{si}"));
   g_variant_iter_init (&iter, dict);
-  while (tmp = g_variant_iter_next_value (&iter))
+
+  while (g_variant_iter_next (&iter, "{&si}", &k, &v))
   {
-    int val = 0; char *str;
-    g_variant_get_child (tmp, 0, "s", &str);
-    if (!str)
-    {
-      g_warning ("Invalid dictionary entry.\n");
-      break;
-    }
-
-    if (strcmp (str, key) != 0)
-    {
-      dict_entries[i++] = tmp;
-    }
-    g_free(str);
+    if (!g_str_equal (k, key))
+      g_variant_builder_add (&builder, "{si}", k, v);
   }
-  dict_entries[i++] = dict_entry;
-  dict = g_variant_new_array (NULL, dict_entries, i);
+  g_variant_builder_add (&builder, "{si}", key, value);
 
-  return dict;
+  return g_variant_builder_end (&builder);
 }
 
 static void
@@ -618,12 +592,13 @@
   float t;
 
   GVariant *dict;
+  GVariant *new_dict;
 
   GtkAdjustment *adj = gtk_range_get_adjustment (GTK_RANGE(self->priv->ui_scale));
   const char *monitor_name = gnome_rr_output_info_get_name (self->priv->current_output);
   if (!monitor_name)
   {
-    g_warning("Failed to get monitor name.\n");
+    g_warning("Failed to get monitor name.");
     return;
   }
 
@@ -632,15 +607,16 @@
   gtk_scale_set_digits (GTK_SCALE(self->priv->ui_scale), 0);
   gtk_scale_add_mark (GTK_SCALE(self->priv->ui_scale), 8, GTK_POS_TOP, NULL);
 
-  g_settings_get (self->priv->desktop_settings, "scale-factor", "@a{si}", &dict);
+  dict = g_settings_get_value (self->priv->desktop_settings, "scale-factor");
   if (!g_variant_lookup (dict, monitor_name, "i", &value))
   {
     value = 8;
     self->priv->ui_prev_scale = value;
   }
-  add_dict_entry (dict, monitor_name, value);
-  g_settings_set (self->priv->desktop_settings, "scale-factor", "@a{si}", dict);
+  new_dict = add_dict_entry (dict, monitor_name, value);
+  g_settings_set_value (self->priv->desktop_settings, "scale-factor", new_dict);
   gtk_adjustment_set_value (adj, value);
+  g_variant_unref (dict);
 }
 
 static int
@@ -1050,17 +1026,19 @@
   if (value != self->priv->ui_prev_scale)
   {
     GVariant *dict;
+    GVariant *new_dict;
 
     monitor_name = gnome_rr_output_info_get_name (self->priv->current_output);
     if (!monitor_name)
     {
-      g_warning("Failed to get monitor name.\n");
+      g_warning("Failed to get monitor name.");
       return FALSE;
     }
 
-    g_settings_get (self->priv->desktop_settings, "scale-factor", "@a{si}", &dict);
-    dict = add_dict_entry (dict, monitor_name, value);
-    g_settings_set (self->priv->desktop_settings, "scale-factor", "@a{si}", dict);
+    dict = g_settings_get_value (self->priv->desktop_settings, "scale-factor");
+    new_dict = add_dict_entry (dict, monitor_name, value);
+    g_settings_set_value (self->priv->desktop_settings, "scale-factor", new_dict);
+    g_variant_unref (dict);
   }
 
   return FALSE;  /* gtk should still process this event */