Merge lp:~attente/unity-gtk-module/1243413 into lp:unity-gtk-module/14.04

[William Hua]

Add a GSettings schema that allows the user to set a blacklist and whitelist for unity-gtk-module (LP: #1243413).

Add a GSettings schema that allows the user to set a blacklist and whitelist for unity-gtk-module (LP: #1243413).

[Charles Kerr (charlesk)]

Thank you for doing this!

[Charles Kerr (charlesk)]

Leaks

- in is_blacklisted(), the GSettings and GVariants are leaked if the code returns while looping through g_variant_iter_next()

Tweaks

- in is_blacklisted(), we reload the GSetting from disk each time is_blacklisted() is called -- sometimes even twice per call. Seems like we'd have less overhead if we loaded it once and kept it as a private field.

- in is_blacklisted(), code duplication where we get a GSettings value as a variant & walk it looking for a key. That duplication would go away if it was extracted into its own method.

review: Needs Fixing

[lp:~attente/unity-gtk-module/1243413]

311. By William Hua on 2013-11-15

Fix leaks and remove duplicate code.

[William Hua (attente)]

Thanks Charles!

is_blacklisted is only called once when the module is first loaded. I'm thinking it's probably better to unref the GSettings object as soon as it's done with it rather than keeping it around for the life of the process. If you still disagree though, I'll fix it.

But agreed on your other points, hopefully those are fixed now.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:311
http://jenkins.qa.ubuntu.com/job/unity-gtk-module-ci/18/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/unity-gtk-module-trusty-armhf-ci/4/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-gtk-module-ci/18/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Thanks for clarifying when is_blacklisted is called(), sounds good.

review: Approve

=== modified file 'configure.ac'
--- configure.ac	2013-07-02 16:17:36 +0000
+++ configure.ac	2013-11-15 20:04:12 +0000
@@ -23,6 +23,8 @@
 
 AM_PATH_PYTHON
 
+GLIB_GSETTINGS
+
 AC_SUBST([GTK_VERSION], [$with_gtk])
 AC_SUBST([GTK_MODULE_DIR], [$with_gtk_module_dir])
 

=== modified file 'data/Makefile.am'
--- data/Makefile.am	2013-07-02 16:17:36 +0000
+++ data/Makefile.am	2013-11-15 20:04:12 +0000
@@ -1,3 +1,6 @@
+gsettings_SCHEMAS = com.canonical.unity-gtk-module.gschema.xml
+@GSETTINGS_RULES@
+
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = unity-gtk$(GTK_VERSION)-parser.pc
 

=== added file 'data/com.canonical.unity-gtk-module.gschema.xml'
--- data/com.canonical.unity-gtk-module.gschema.xml	1970-01-01 00:00:00 +0000
+++ data/com.canonical.unity-gtk-module.gschema.xml	2013-11-15 20:04:12 +0000
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<schemalist>
+  <schema id="com.canonical.unity-gtk-module" path="/com/canonical/unity-gtk-module/">
+    <key name="blacklist" type="as">
+      <summary>Application blacklist</summary>
+      <description>List of applications where unity-gtk-module should be disabled.</description>
+      <default>[]</default>
+    </key>
+    <key name="whitelist" type="as">
+      <summary>Application whitelist</summary>
+      <description>List of applications where unity-gtk-module should be enabled.</description>
+      <default>[]</default>
+    </key>
+  </schema>
+</schemalist>

=== modified file 'debian/control'
--- debian/control	2013-07-16 10:42:29 +0000
+++ debian/control	2013-11-15 20:04:12 +0000
@@ -32,7 +32,7 @@
 Description: Common files for GtkMenuShell D-Bus exporter
  This GTK+ module exports GtkMenuShells over D-Bus.
  .
- This package contains common Xsession.d scripts.
+ This package contains common data files.
 
 Package: libunity-gtk2-parser0
 Architecture: any

=== modified file 'debian/unity-gtk-module-common.install'
--- debian/unity-gtk-module-common.install	2013-07-02 16:21:42 +0000
+++ debian/unity-gtk-module-common.install	2013-11-15 20:04:12 +0000
@@ -1,1 +1,2 @@
+usr/share/glib-2.0/schemas/*
 usr/share/upstart/sessions/*

=== modified file 'src/main.c'
--- src/main.c	2013-10-31 21:15:16 +0000
+++ src/main.c	2013-11-15 20:04:12 +0000
@@ -21,6 +21,30 @@
 #include <gdk/gdkx.h>
 #include "unity-gtk-parser.h"
 
+/*
+ * Default list of apps which should not be patched.
+ * Use xprop | grep CLASS to find the name to use.
+ */
+static const char * const BLACKLIST[] =
+{
+  "Eclipse",
+  "acroread",
+  "emacs",
+  "emacs23",
+  "emacs23-lucid",
+  "emacs24",
+  "emacs24-lucid",
+  "freeciv",
+  "freeciv-gtk2",
+  "glade",
+  "gwyddion",
+  NULL
+};
+
+#define UNITY_GTK_MODULE_SCHEMA "com.canonical.unity-gtk-module"
+#define BLACKLIST_KEY           "blacklist"
+#define WHITELIST_KEY           "whitelist"
+
 #define _GTK_UNIQUE_BUS_NAME     "_GTK_UNIQUE_BUS_NAME"
 #define _UNITY_OBJECT_PATH       "_UNITY_OBJECT_PATH"
 #define _GTK_MENUBAR_OBJECT_PATH "_GTK_MENUBAR_OBJECT_PATH"
@@ -90,22 +114,59 @@
                                                                       gint           *natural_height);
 #endif
 
-/* Crude blacklist to avoid patching innocent apps.
-   Use xprop | grep CLASS to find the name to use */
+static gboolean
+is_string_in_array (const gchar *string,
+                    GVariant    *array)
+{
+  GVariantIter iter;
+  const gchar *element;
+
+  g_return_val_if_fail (array != NULL, FALSE);
+  g_return_val_if_fail (g_variant_is_of_type (array, G_VARIANT_TYPE ("as")), FALSE);
+
+  g_variant_iter_init (&iter, array);
+  while (g_variant_iter_next (&iter, "&s", &element))
+    {
+      if (g_strcmp0 (element, string) == 0)
+        return TRUE;
+    }
+
+  return FALSE;
+}
+
+static gboolean
+is_listed (const gchar *name,
+           const gchar *key)
+{
+  GSettings *settings;
+  GVariant *array;
+  gboolean listed;
+
+  settings = g_settings_new (UNITY_GTK_MODULE_SCHEMA);
+  array = g_settings_get_value (settings, key);
+  listed = is_string_in_array (name, array);
+
+  g_variant_unref (array);
+  g_object_unref (settings);
+
+  return listed;
+}
+
 static gboolean
 is_blacklisted (const gchar *name)
 {
-  return g_strcmp0 (name, "Eclipse") == 0 ||
-         g_strcmp0 (name, "emacs") == 0 ||
-         g_strcmp0 (name, "emacs23") == 0 ||
-         g_strcmp0 (name, "emacs23-lucid") == 0 ||
-         g_strcmp0 (name, "emacs24") == 0 ||
-         g_strcmp0 (name, "emacs24-lucid") == 0 ||
-         g_strcmp0 (name, "freeciv") == 0 ||
-         g_strcmp0 (name, "freeciv-gtk2") == 0 ||
-         g_strcmp0 (name, "glade") == 0 ||
-         g_strcmp0 (name, "acroread") == 0 ||
-         g_strcmp0 (name, "gwyddion") == 0;
+  guint n;
+  guint i;
+
+  n = sizeof (BLACKLIST) / sizeof (const char *);
+
+  for (i = 0; i < n; i++)
+    {
+      if (g_strcmp0 (name, BLACKLIST[i]) == 0)
+        return !is_listed (name, WHITELIST_KEY);
+    }
+
+  return is_listed (name, BLACKLIST_KEY);
 }
 
 static gboolean