Merge lp:~desrt/notify-osd/GNOME_BACKGROUND_REPRESENTATIVE_COLORS into lp:~canonical-dx-team/notify-osd/precise

[Allison Karlitskaya]

Get background colour using _GNOME_BACKGROUND_REPRESENTATIVE_COLORS

...instead of pulling it out of Unity's GSettings.

[Allison Karlitskaya (desrt)]

This is being added to libgnome-desktop (which is how the desktop gets set) here: https://bugzilla.gnome.org/show_bug.cgi?id=671750

[David Barth (dbarth)]

Looks good, but can you add some tests?

What happens if the property does not exist? Is there a corresponding default mechanism to provide a sane value?

IMO, the test should verify that changing the background color results in the right color being received in the bubble.c function. Then, that border line values don't result in a crash and trigger a reasonable fallback value.

[Mirco Müller (macslow)]

The way a bubble is initialized and defaults_refresh_bg_color_property() goes about updating the "bubble-bg-color", it'll fall back to the typical dark grey color, should the X-property "_GNOME_BACKGROUND_REPRESENTATIVE_COLORS" not be available. I tested that by defaults_refresh_bg_color_property() returning early not touching "bubble-bg-color" at all.

[Mirco Müller (macslow)]

After discussion on IRC, the effort/benefit relation forbids an automated test for this. We need a proper framework for testing such system-wide changes correctly. Until we have that in place a manual-test will have to do.

review: Needs Fixing

[Mirco Müller (macslow)]

Since I can't commit to desrt's branch I'll approve this and add the manual-test with my merge to trunk. So... approved.

review: Approve

=== modified file 'src/bubble.c'
--- src/bubble.c	2012-02-08 09:18:02 +0000
+++ src/bubble.c	2012-03-09 21:34:18 +0000
@@ -668,10 +668,10 @@
 	cairo_paint (cr);
 	cairo_set_operator (cr, CAIRO_OPERATOR_OVER);
 
-	GdkColor color;
+	GdkRGBA color;
 	gchar* color_string = NULL;
     color_string = defaults_get_bubble_bg_color (d);
-	gdk_color_parse (color_string, &color);
+	gdk_rgba_parse (&color, color_string);
     g_free (color_string);
 	
 	if (priv->composited)
@@ -695,16 +695,16 @@
 		cairo_fill (cr);
 		cairo_set_operator (cr, CAIRO_OPERATOR_OVER);
 		cairo_set_source_rgba (cr,
-				       0.75 * ((double) color.red / 65535.0),
-				       0.75 * ((double) color.green / 65535.0),
-				       0.75 * ((double) color.blue / 65535.0),
+				       0.75 * color.red,
+				       0.75 * color.green,
+				       0.75 * color.blue,
 				       BUBBLE_BG_COLOR_A);
 	}
 	else
 		cairo_set_source_rgb (cr,
-				       0.75 * ((double) color.red / 65535.0),
-				       0.75 * ((double) color.green / 65535.0),
-				       0.75 * ((double) color.blue / 65535.0));
+				       0.75 * color.red,
+				       0.75 * color.green,
+				       0.75 * color.blue);
 
 	draw_round_rect (
 		cr,

=== modified file 'src/defaults.c'
--- src/defaults.c	2012-02-08 09:18:02 +0000
+++ src/defaults.c	2012-03-09 21:34:18 +0000
@@ -289,59 +289,50 @@
 	g_signal_emit (defaults, g_defaults_signals[GRAVITY_CHANGED], 0);
 }
 
-static void
-_avg_bg_color_changed (GSettings* settings,
-                       gchar*     key,
-                       gpointer   data)
-{
-	Defaults* defaults     = NULL;
-	gchar*    color_string = NULL;
-
-	if (!data || !settings)
-		return;
-
-	defaults = (Defaults*) data;
-	if (!IS_DEFAULTS (defaults))
-		return;
-
-	color_string = g_settings_get_string (settings, key);
-	g_object_set (defaults, "bubble-bg-color", color_string, NULL);
-	g_free (color_string);
-}
-
-GSettings*
-_get_unity_schema ()
-{
-	// check for availability of unity-schema
-	const gchar* const* schema_list = NULL;
-        int i = 0;
-	gboolean match = FALSE;
-	schema_list = g_settings_list_schemas (); // no need to free/unref list
-        for (i = 0; schema_list[i]; i++)
-		if (g_strcmp0 (UNITY_SCHEMA, schema_list[i]) == 0)
-		{
-			match = TRUE;
-			break;
-		}
-	if (!match)
-		return NULL;
-
-	// be really paranoid and check for "avg. bg-color" key
-	GSettings* settings = g_settings_new (UNITY_SCHEMA);
-	gchar** keys = NULL;
-	keys = g_settings_list_keys (settings);
-	i = 0;
-	match = FALSE;
-	while (keys[i] && !match)
+void
+defaults_refresh_bg_color_property (Defaults *self)
+{
+	Atom         real_type;
+	gint         result;
+	gint         real_format;
+	gulong       items_read;
+	gulong       items_left;
+	gchar*       colors;
+	Atom         representative_colors_atom;
+	Display*     display;
+
+	g_return_if_fail ((self != NULL) && IS_DEFAULTS (self));
+
+	representative_colors_atom = gdk_x11_get_xatom_by_name ("_GNOME_BACKGROUND_REPRESENTATIVE_COLORS");
+	display = gdk_x11_display_get_xdisplay (gdk_display_get_default ());
+
+	gdk_error_trap_push ();
+	result = XGetWindowProperty (display,
+				     GDK_ROOT_WINDOW (),
+				     representative_colors_atom,
+				     0L,
+				     G_MAXLONG,
+				     False,
+				     XA_STRING,
+				     &real_type,
+				     &real_format,
+				     &items_read,
+				     &items_left,
+				     (guchar **) &colors);
+	gdk_flush ();
+	gdk_error_trap_pop_ignored ();
+
+	if (result == Success && items_read)
 	{
-		match = g_strcmp0 (keys[i], GSETTINGS_AVG_BG_COL_KEY) == 0 ? TRUE : FALSE;
-		i++;
+		/* by treating the result as a nul-terminated string, we
+		 * select the first colour in the list.
+		 */
+		g_object_set (self,
+			      "bubble-bg-color",
+			      colors,
+			      NULL);
+		XFree (colors);
 	}
-	g_strfreev (keys);
-	if (!match)
-		return NULL;
-
-	return settings;
 }
 
 void
@@ -425,6 +416,7 @@
 	self = DEFAULTS (gobject);
 
 	defaults_refresh_screen_dimension_properties (self);
+	defaults_refresh_bg_color_property (self);
 
 	/* grab system-wide font-face/size and DPI */
 	_get_font_size_dpi (self);
@@ -462,10 +454,6 @@
 			      NULL);
 	}
 
-	_avg_bg_color_changed (self->unity_settings,
-			       GSETTINGS_AVG_BG_COL_KEY,
-			       self);
-
 	/* FIXME: calling this here causes a segfault */
 	/* chain up to the parent class */
 	/*G_OBJECT_CLASS (defaults_parent_class)->constructed (gobject);*/
@@ -480,7 +468,6 @@
 
 	g_object_unref (defaults->nosd_settings);
 	g_object_unref (defaults->gnome_settings);
-	g_object_unref (defaults->unity_settings);
 
 	if (defaults->bubble_shadow_color)
 	{
@@ -547,7 +534,6 @@
 	/* "connect" to the required GSettings schemas */
 	self->nosd_settings  = g_settings_new (NOTIFY_OSD_SCHEMA);
 	self->gnome_settings = g_settings_new (GNOME_DESKTOP_SCHEMA);
-	self->unity_settings = _get_unity_schema ();
 
 	g_signal_connect (self->gnome_settings,
 					  "changed",
@@ -558,13 +544,6 @@
 					  "changed",
 					  G_CALLBACK (_gravity_changed),
 					  self);
-	if (self->unity_settings)
-	{
-		g_signal_connect (self->unity_settings,
-				  "changed",
-				  G_CALLBACK (_avg_bg_color_changed),
-				  self);
-	}
 
 	// use fixed slot-allocation for async. and sync. bubbles
 	self->slot_allocation = SLOT_ALLOCATION_FIXED;
@@ -1819,6 +1798,8 @@
 	if (!self || !IS_DEFAULTS (self))
 		return NULL;
 
+	defaults_refresh_bg_color_property (self);
+
 	g_object_get (self,
 		      "bubble-bg-color",
 		      &bubble_bg_color,

=== modified file 'src/defaults.h'
--- src/defaults.h	2012-02-08 09:18:02 +0000
+++ src/defaults.h	2012-03-09 21:34:18 +0000
@@ -76,7 +76,6 @@
 	/* private */
 	GSettings*     nosd_settings;
 	GSettings*     gnome_settings;
-	GSettings*     unity_settings;
 	gint           desktop_width;
 	gint           desktop_height;
 	gint           desktop_top;
@@ -253,6 +252,9 @@
 defaults_get_screen_dpi (Defaults* self);
 
 void
+defaults_refresh_bg_property (Defaults *self);
+
+void
 defaults_refresh_screen_dimension_properties (Defaults *self);
 
 void