GNOME Bugzilla – Bug 671750
set "representative color" property on the root window
Last modified: 2012-03-10 16:12:26 UTC
Some various parts of chrome in Unity (and notify-osd) want to change their colour according to the predominant/average background image colour. They currently have some bad hacks to deal with that (including writing the current average colour to GSettings from the window manager). Far better would be to go straight to the source: set an X property from the same place as we set the other X properties: GnomeBG.
Created attachment 209354 [details] [review] gnome-bg: have pixbuf_average_value return GdkRGBA Now that we have a proper type for RGBA values, we should use it (instead of packing the four components into a uint32).
Created attachment 209355 [details] [review] gnome-bg: publish the 'representative color' Publish a list of 'representative colours' as a property on the root window _GNOME_BACKGROUND_REPRESENTATIVE_COLORS. This is alongside the normal properties pointing at the pixmap of the background image. Right now, this is just implemented as an average using the existing pixbuf_average_value() function. Strictly speaking, the property is a list of colours and in the future we may set more than one.
Review of attachment 209354 [details] [review]: reasonable cleanup ::: libgnome-desktop/gnome-bg.c @@ +138,2 @@ /* Pixbuf utils */ +static GdkRGBA pixbuf_average_value (GdkPixbuf *pixbuf); Can you make this an out arg? returning structs makes me irrationally twitchy
Review of attachment 209354 [details] [review]: Also, any chance you want to clean up the other things that could be GdkRGBA (the GdkColor stuff i mean) ?
Review of attachment 209355 [details] [review]: ::: libgnome-desktop/gnome-bg.c @@ +128,3 @@ }; +static cairo_user_data_key_t average_color_key; should be const and maybe called representative_colors_key ? @@ +1126,3 @@ + cairo_surface_set_user_data (surface, &average_color_key, + gdk_rgba_copy (&average), + (cairo_destroy_func_t) gdk_rgba_free); Oh I see you aren't bothering with the list internally yet. That's probably fine, but if you did do it now, would mean less typing a more clear roadmap for the future later on. @@ +1496,3 @@ + XA_STRING, 8, PropModeReplace, + (guchar *) string, strlen (string) + 1); + g_free (string); strlen (string) not strlen (string) + 1 (according to xprop.c in xorg-x11-utils anyway) @@ +1499,3 @@ + } else { + XDeleteProperty (display, RootWindow (display, screen_num), + gdk_x11_get_xatom_by_name ("_GNOME_BACKGROUND_REPRESENTATIVE_COLORS")); When will this code path hit?
yay dupes
Review of attachment 209355 [details] [review]: ::: libgnome-desktop/gnome-bg.c @@ +128,3 @@ }; +static cairo_user_data_key_t average_color_key; will make this const. @@ +1126,3 @@ + cairo_surface_set_user_data (surface, &average_color_key, + gdk_rgba_copy (&average), + (cairo_destroy_func_t) gdk_rgba_free); I'd rather not complicate the code on the basis of a feature we may or may not add later. It's internal API so there is no risk associated with not doing it now. @@ +1496,3 @@ + XA_STRING, 8, PropModeReplace, + (guchar *) string, strlen (string) + 1); + g_free (string); Keep in mind that this is actually a string array. String arrays are encoded with a nul following each item. so ["foo", "bar", "baz"] is encoded like 'f' 'o' 'o' '\0' 'b' 'a' 'r' '\0' 'b' 'a' 'z' '\0' and has a length of 12. Could probably use a comment. I'll add it. @@ +1499,3 @@ + } else { + XDeleteProperty (display, RootWindow (display, screen_num), + gdk_x11_get_xatom_by_name ("_GNOME_BACKGROUND_REPRESENTATIVE_COLORS")); It could happen if someone gives us a cairo surface that we did not create. I don't know if anyone will do that, but it's a theoretical possibility.
Review of attachment 209354 [details] [review]: If we are going to have a general GdkColor -> GdkRGBA, I think there should be a separate bug. ::: libgnome-desktop/gnome-bg.c @@ +138,2 @@ /* Pixbuf utils */ +static GdkRGBA pixbuf_average_value (GdkPixbuf *pixbuf); if it makes you feel better.... :)
Created attachment 209361 [details] [review] gnome-bg: have pixbuf_average_value return GdkRGBA Now that we have a proper type for RGBA values, we should use it (instead of packing the four components into a uint32).
Created attachment 209362 [details] [review] gnome-bg: publish the 'representative color' Publish a list of 'representative colours' as a property on the root window _GNOME_BACKGROUND_REPRESENTATIVE_COLORS. This is alongside the normal properties pointing at the pixmap of the background image. Right now, this is just implemented as an average using the existing pixbuf_average_value() function. Strictly speaking, the property is a list of colours and in the future we may set more than one.
Review of attachment 209361 [details] [review]: ::: libgnome-desktop/gnome-bg.c @@ +2380,3 @@ } + + dd = dividend * 0xFF; I don't really understand the naming scheme used here (even before your changes). I would probably rename dividend to devisor or denominator, and roll dd into it. But up to you.
Review of attachment 209362 [details] [review]: ::: libgnome-desktop/gnome-bg.c @@ +134,3 @@ G_DEFINE_TYPE (GnomeBG, gnome_bg, G_TYPE_OBJECT) + drop this newline @@ +1105,3 @@ if (!bg->filename && bg->color_type == G_DESKTOP_BACKGROUND_SHADING_SOLID) { gdk_cairo_set_source_color (cr, &(bg->primary)); + average.red = bg->primary.red / (double) 0xff; maybe 255.0 instead of (double) 0xff ? doesn't really matter i guess.
oh one other thought, should probably only muck with the property/do the average if create_surface has is_root = true
'dd' is 'dividend, as a double' *shrug* patch pushed with requested changes. Thanks for the fast review. Attachment 209361 [details] pushed as e8afbf1 - gnome-bg: have pixbuf_average_value return GdkRGBA Attachment 209362 [details] pushed as 22d7f10 - gnome-bg: publish the 'representative color'
Ryan, did you mesure how long takes the pixbuf_average_value() computation? When does it happen? I would like to avoid adding extra cpu use to calculate that value at login, ideally that would be stored and read at login and only updated when somebody change backgrounds...
hi Seb, (In reply to comment #16) > Ryan, did you mesure how long takes the pixbuf_average_value() computation? > When does it happen? I would like to avoid adding extra cpu use to calculate > that value at login, ideally that would be stored and read at login and only > updated when somebody change backgrounds... The value can't really be stored anywhere because of how some background setups are slideshow-type backgrounds where the average colour is going to be changing dynamically. We considered the performance when discussing the bug on IRC. The idea was that we would go ahead with the current code unless it became a problem and then we would look for ways to fix it. A rather good low-impact way is quite obvious: instead of sampling every pixel, sample a point grid spaced at 16x16 or so -- this would cut the amount of time by 1/256. In any case, the question is a good one. I'll do some measurements.
-> Bug 671780