I suppose gnome-bg nor the gnome-desktop package is not going away for 3.0 right? Or does anybody have different plans?

I'm willing to volunteer for the port, though I fear we're in API freeze for 2.32 already.

Yeah, gnome-desktop will still be there in 3.0.
You can probably ask for an API freeze break if e.g. nautilus requires it (not sure there are other clients of the GnomeBG API in the desktop set).

gnome-control-center/appearance-applet should be ported to at least get consistent desktop user experience, i.e. be able to change the background and allow nautilus to detect the change and use the new settings.

Same thing for gnome-settings-daemon for picking the right background after session start. There's no other usage of GConf in gnome-settings-daemon, no tracker bug exists.

Created attachment 168410 [details] [review]
proposed patch, first version

This is a first shot, port to GSettings with several API changes:
 - removed GnomeBGColorType and GnomeBGPlacement enums in favor of GDesktopBackgroundStyle and GDesktopBackgroundShading from gdesktop-enums.h

 - removed GNOME_BG_KEY_DIR, it's a GConf path

 - removed GConfClient* argument from gnome_bg_load_from_preferences() / gnome_bg_save_to_preferences(), the GnomeBG class now own its instance of GSettings object. This has its advantages and disadvantages, overall it seems to me like a good approach since other components don't need to be aware of GSettings at all. Please discuss.

 - added setter and getter for the 'draw-background' key. Applications should be aware of this value and act accordingly.


Concerns:
 - the 'draw-background' key and 'picture-options' = "none" looks duplicate to me. Not sure what was the historic reason for introducing this option, however clients are handling this differently, definitely not in any consistent way. We should write down some rules. That brings me to my second concern:

 - missing API docs + describe usage

 - there's no monitoring support for GSettings keys, clients handle this its own way at the moment. We do have "changed" signal however which is emitted when source file changes (a background image on disk) and also on every setter use (seems kinda weird to me). 


Patches for nautilus and gnome-control-center will follow, I still need to port gnome-settings-daemon (but got distracted by some other work now). Leaving as 'needs-work' for the moment until we make all components working.

Review of attachment 168410 [details] [review]:

::: libgnome-desktop/gnome-bg.c
@@ +213,3 @@
 	gdk_color_parse (string, colorp);
+	
+	/* FIXME: is this necessary? */

Note to myself: according to nautilus commit http://git.gnome.org/browse/nautilus/commit/?id=6c691075c09ec23620484da00ffd43d3dfe0b75b this can be safely removed.

nautilus patch posted: bug 626018
gnome-control-center patch posted: bug 625899

gnome-settings-daemon patch posted: bug 628624

Review of attachment 168410 [details] [review]:

You seem to have many lines changed just for some space/tabs things. Maybe remove those changes, unless they are needed to properly align some stuff.

::: libgnome-desktop/gnome-bg.c
@@ +326,3 @@
 			g_free (filename);
 			filename = NULL;
+			/* FIXME: do we want to load default background here as defined in gsettings schemas? */

Yes we want :-)

@@ +481,3 @@
 
+	if (bg->color_type != type ||
+	    (primary && !gdk_color_equal (&bg->primary, primary)) ||

Looks to me like this change and the one below should be part of another  commit. And most importantly, I probably wouldn't do it this way: it's likely that we always want a primary, and so a g_return_if_fail() would be more appropriate. I could be wrong, though :-)

@@ +869,2 @@
 		draw_color_each_monitor (bg, dest, screen);
+		if (bg->placement != G_DESKTOP_BACKGROUND_STYLE_NONE)

In this case, this means we only have a color or gradient for backround (I guess). Will it still be drawn?

@@ +1102,3 @@
 	gboolean result;
 
+        if (filename && gdk_pixbuf_get_file_info (filename, orig_width, orig_height))

Should be in another commit, or needs some explanation.

@@ +1189,3 @@
 	draw_color (bg, result, screen);
 	
+	if (bg->placement != G_DESKTOP_BACKGROUND_STYLE_NONE) {

Can't we have a thumbnail for just a color/gradient background? Or is there a bigger issue in such a case?

@@ +1883,3 @@
 		      int                           frame_num)
 {
+	if (bg->filename && strlen (bg->filename) > 0) {

I prefer bg->filename[0] != '\0' to the strlen check.

@@ +2758,3 @@
 	mtime = get_mtime (filename);
 	
+	if (mtime == (time_t)-1 || ! filename || strlen (filename) == 0)

Same comment here. Is there any reason to have all those paranoid checks?

@@ +2898,3 @@
 	draw_color (bg, result, screen);
 
+	if (bg->placement != G_DESKTOP_BACKGROUND_STYLE_NONE) {

I guess that I have the same question than earlier, about color/gradient backgrounds.

::: libgnome-desktop/libgnomeui/gnome-bg.h
@@ +62,3 @@
 						 GdkColor              *secondary);
+void             gnome_bg_set_draw_background   (GnomeBG               *bg,
+						 gboolean               draw_background);

Why do we need this now?

Created attachment 170676 [details] [review]
proposed patch

(In reply to comment #8)
> You seem to have many lines changed just for some space/tabs things. Maybe
> remove those changes, unless they are needed to properly align some stuff.
I've removed some useless formatting lines, the rest are good to have, aligning function parameters.

> ::: libgnome-desktop/gnome-bg.c
> @@ +326,3 @@
>              g_free (filename);
>              filename = NULL;
> +            /* FIXME: do we want to load default background here as defined in
> gsettings schemas? */
> 
> Yes we want :-)
I mean, when the image file is not available, do we want to load default one instead of drawing just a solid color (gradient respectively)?

I've changed this, but the default filename needs to be hardcoded due to missing GSettings API to retrieve schema default value. This also brings issues to distributors who will need to patch two places. I don't like this approach.

> @@ +481,3 @@
> 
> +    if (bg->color_type != type ||
> +        (primary && !gdk_color_equal (&bg->primary, primary)) ||
> 
> Looks to me like this change and the one below should be part of another 
> commit. And most importantly, I probably wouldn't do it this way: it's likely
> that we always want a primary, and so a g_return_if_fail() would be more
> appropriate. I could be wrong, though :-)
Agree, this is silly. It was done for gnome-control-center sending NULL colors, but I've fixed that there instead.

> @@ +869,2 @@
>          draw_color_each_monitor (bg, dest, screen);
> +        if (bg->placement != G_DESKTOP_BACKGROUND_STYLE_NONE)
> 
> In this case, this means we only have a color or gradient for backround (I
> guess). Will it still be drawn?
Yes, drawing color is done by draw_color_each_monitor(), while only draw_each_monitor() drawing a pixmap is conditional (to prevent asserts).

Please see my 'draw-background' / 'picture-options' note in comment 4.

> 
> @@ +1102,3 @@
>      gboolean result;
> 
> +        if (filename && gdk_pixbuf_get_file_info (filename, orig_width,
> orig_height))
> 
> Should be in another commit, or needs some explanation.
Removed, couldn't trigger the problem anymore, this would need to be fixed in apps anyway.

> 
> @@ +1189,3 @@
>      draw_color (bg, result, screen);
> 
> +    if (bg->placement != G_DESKTOP_BACKGROUND_STYLE_NONE) {
> 
> Can't we have a thumbnail for just a color/gradient background? Or is there a
> bigger issue in such a case?
No issue, it works the way you described. Again here, G_DESKTOP_BACKGROUND_STYLE_NONE is a new member of the enum and was not handled previously. Thus, skip image drawing when not desired and draw only solid color or gradient.

> @@ +1883,3 @@
>                int                           frame_num)
>  {
> +    if (bg->filename && strlen (bg->filename) > 0) {
> 
> I prefer bg->filename[0] != '\0' to the strlen check.
Removed, same reason as above.

> @@ +2758,3 @@
>      mtime = get_mtime (filename);
> 
> +    if (mtime == (time_t)-1 || ! filename || strlen (filename) == 0)
> 
> Same comment here. Is there any reason to have all those paranoid checks?
Changed to a separate test to catch more problems (invalid or NULL filename). There are cases when filename is not NULL but zero-length.
+       if (uri == NULL)
+               return NULL;

> 
> @@ +2898,3 @@
>      draw_color (bg, result, screen);
> 
> +    if (bg->placement != G_DESKTOP_BACKGROUND_STYLE_NONE) {
> 
> I guess that I have the same question than earlier, about color/gradient
> backgrounds.
Yes, same case.

> 
> ::: libgnome-desktop/libgnomeui/gnome-bg.h
> @@ +62,3 @@
>                           GdkColor              *secondary);
> +void             gnome_bg_set_draw_background   (GnomeBG               *bg,
> +                         gboolean               draw_background);
> 
> Why do we need this now?
To prevent drawing anything, i.e. don't draw color/gradient nor the image. This flag is mostly for indication that we shouldn't bother rendering background. Introduced to match the 'draw-background' schema key, have setter and getter for this property and allow batch loading/saving by gnome-bg itself. Also see my note in comment 4.

Review of attachment 170676 [details] [review]:

This looks mostly good; I left some comments inlined below.
I must say I don't really like the GDesktop* namespace for gsettings-desktop-schemas (I'd expect modules not to pollute the G* namespace with things from outside GLib), but that has nothing to do with your patch :)

::: libgnome-desktop/gnome-bg.c
@@ +303,3 @@
+
+	if (bg->settings == NULL)
+		bg->settings = g_settings_new (BG_SETTINGS_SCHEMA);

AFAICS GnomeBG's coding style is similar to nautilus in using curly braces for statements in if() blocks, even if it's just a single line.

@@ +323,3 @@
 			g_free (filename);
+			/* FIXME: There's no way to get default value from schema with GSettings.
+			          Hardcoding is not the best way here, find a better solution. */

Maybe one workaround-ish way to get the default for the schema could be

- call g_settings_delay () on the GSettings object
- call g_settings_reset () on the filename key
- get the default with g_settings_get_string ()
- call g_settings_revert () to discard the _reset () call and g_settings_apply () to exit delay-mode.

Thinking about it a bit more, we would end up in this chunk only when we have a key pointing to a file that doesn't exist anymore on the file system, so would it be really this harmful just using the color/gradient in this case and ignore the image setting at all (if the previous workaround doesn't work correctly)?

@@ +360,3 @@
+
+	if (bg->settings == NULL)
+		bg->settings = g_settings_new (BG_SETTINGS_SCHEMA);

Coding style: missing curly braces.

@@ +369,3 @@
+	}
+	else {
+		filename = "(none)";

Coding style: the else block should be on the same line of the closing brace.

Also, why do we save "(none)" here? Can't we just set the string to NULL or to an empty one if we don't have a filename to save?

::: libgnome-desktop/libgnomeui/gnome-bg.h
@@ +32,2 @@
 #include <gdk/gdk.h>
+#include <gio/gio.h>

This include is not really needed here AFAICS <nitpick/>

(In reply to comment #10)

> Maybe one workaround-ish way to get the default for the schema could be
> 
> - call g_settings_delay () on the GSettings object
> - call g_settings_reset () on the filename key
> - get the default with g_settings_get_string ()
> - call g_settings_revert () to discard the _reset () call and g_settings_apply
> () to exit delay-mode.

After talking with Ryan on IRC, he pointed me to g_settings_get_mapped() (yeah, the function name is misleading!), which would solve this case if called with a mapping function that checks if the file at the path specified by the key actually exists.
In case every possible fallback value fails too, I think it's safe to just set a NULL filename.

Created attachment 172717 [details] [review]
proposed patch

Thanks for the review, code style glitches fixed.

(In reply to comment #10)
> I must say I don't really like the GDesktop* namespace for
> gsettings-desktop-schemas (I'd expect modules not to pollute the G* namespace
> with things from outside GLib), but that has nothing to do with your patch :)
Feel free to open separate bug report while we still can break the API.

> @@ +369,3 @@
> +    }
> +    else {
> +        filename = "(none)";
> 
> Coding style: the else block should be on the same line of the closing brace.
> 
> Also, why do we save "(none)" here? Can't we just set the string to NULL or to
> an empty one if we don't have a filename to save?
Umm, historical reasons, I just reformatted the old code. Thinking about it, I'm not aware of any application requiring this. The gnome-bg code can handle NULL filenames just well. Removed.

> ::: libgnome-desktop/libgnomeui/gnome-bg.h
> @@ +32,2 @@
>  #include <gdk/gdk.h>
> +#include <gio/gio.h>
> 
> This include is not really needed here AFAICS <nitpick/>
We use GIO for monitoring background file changes.

(In reply to comment #11)
> After talking with Ryan on IRC, he pointed me to g_settings_get_mapped() (yeah,
> the function name is misleading!), which would solve this case if called with a
> mapping function that checks if the file at the path specified by the key
> actually exists.
> In case every possible fallback value fails too, I think it's safe to just set
> a NULL filename.
Good shot, this will nicely handle all fallback cases. I've implemented this in the patch, though we would still fall back to default GNOME or distribution background.

Vincent, can you please elaborate a little what should be the desired behaviour here? For new users the key value is inherited from defaults until it's set for the first time. Wouldn't it be confusing for users to fall back to a different background than the user set last time?

Note that NULL value would show only solid color (or gradient if set), no other issues.

gnome-screensaver patch posted: bug 632566

Review of attachment 172717 [details] [review]:

::: libgnome-desktop/gnome-bg.c
@@ +292,3 @@
+background_image_mapping_fn (GVariant *value,
+                             gpointer *result,
+                             gpointer user_data)

You seem to do the g_file_test() twice per-call, which is not necessary (probably the old code did the same).
Also, I think all the UTF-8 checks should go away and the setting itself (in gsettings-desktop-schemas) should become an array of bytes instead of a string, as it stores a filename (see http://mail.gnome.org/archives/desktop-devel-list/2010-July/msg00047.html for previous discussions).
That way, this function would just become a little more than a simple g_file_test() as far as I understand it.

::: libgnome-desktop/libgnomeui/gnome-bg.h
@@ +32,2 @@
 #include <gdk/gdk.h>
+#include <gio/gio.h>

I meant this could be included in the .c file and not in the header.

Seems to lose the ability to load background from the system defaults?  https://bugzilla.gnome.org/show_bug.cgi?id=632566#c1

Created attachment 173821 [details] [review]
proposed patch, fourth version

(In reply to comment #14)
> You seem to do the g_file_test() twice per-call, which is not necessary
> (probably the old code did the same).
> Also, I think all the UTF-8 checks should go away and the setting itself (in
> gsettings-desktop-schemas) should become an array of bytes instead of a string,
> as it stores a filename (see
> http://mail.gnome.org/archives/desktop-devel-list/2010-July/msg00047.html for
> previous discussions).
> That way, this function would just become a little more than a simple
> g_file_test() as far as I understand it.

Right, encoding checks are obsolete, I've removed them. Filed bug 633983 for the "ay" type.

Also after quick talk on #fedora-desktop, I've changed the fallback case - we found that falling back to default background is confusing, hence display just solid color (or gradient if set). The mapping code is available in patch from comment #12, we can add it back later when deserved.
 
(In reply to comment #15)
> Seems to lose the ability to load background from the system defaults? 
> https://bugzilla.gnome.org/show_bug.cgi?id=632566#c1
Correct, I originally removed that parameter as useless but apparently gnome-screensaver is using it to display system background. I've changed API of gnome_bg_load_from_preferences() and gnome_bg_save_to_preferences() functions and will post updated patches for required components. 

This is the last API break and I think we can push it to git master now.

> ::: libgnome-desktop/libgnomeui/gnome-bg.h
> @@ +32,2 @@
>  #include <gdk/gdk.h>
> +#include <gio/gio.h>
> 
> I meant this could be included in the .c file and not in the header.
So now we need need it after all :-)

(In reply to comment #16)
> Also after quick talk on #fedora-desktop, I've changed the fallback case - we
> found that falling back to default background is confusing, hence display just
> solid color (or gradient if set). The mapping code is available in patch from
> comment #12, we can add it back later when deserved.

What's the rationale here (ie, why is it confusing)? I find it equally confusing to fallback to some ugly solid color, and, well, it's ugly :-)

(btw, please discuss this kind of stuff in an upstream channel if possible, like #gnome-hackers)

Review of attachment 173821 [details] [review]:

Just a minor detail in the patch...

::: libgnome-desktop/gnome-desktop-3.0.pc.in
@@ +6,3 @@
 Name: gnome-desktop-3.0
 Description: Utility library for loading .desktop files
+Requires: gtk+-3.0 gconf-2.0 gsettings-desktop-schemas @STARTUP_NOTIFICATION_PACKAGE@

gconf-2.0 should probably be moved to Requires.Private, I guess.

(In reply to comment #17)
> (In reply to comment #16)
> > Also after quick talk on #fedora-desktop, I've changed the fallback case - we
> > found that falling back to default background is confusing, hence display just
> > solid color (or gradient if set). The mapping code is available in patch from
> > comment #12, we can add it back later when deserved.
> 
> What's the rationale here (ie, why is it confusing)? I find it equally
> confusing to fallback to some ugly solid color, and, well, it's ugly :-)

You'd probably start wondering why you get the wrong background image. The empty desktop makes it easier to guess why it happened. This is the behaviour on other OSes, afaik.

> (btw, please discuss this kind of stuff in an upstream channel if possible,
> like #gnome-hackers)

(btw, go ahead and commit if you need this for 2.91.2)

(In reply to comment #18)
> Review of attachment 173821 [details] [review]:
> 
> Just a minor detail in the patch...
> 
> ::: libgnome-desktop/gnome-desktop-3.0.pc.in
> @@ +6,3 @@
>  Name: gnome-desktop-3.0
>  Description: Utility library for loading .desktop files
> +Requires: gtk+-3.0 gconf-2.0 gsettings-desktop-schemas
> @STARTUP_NOTIFICATION_PACKAGE@
> 
> gconf-2.0 should probably be moved to Requires.Private, I guess.

That's not my change, let's deal with it separately.

(In reply to comment #20)
> (btw, go ahead and commit if you need this for 2.91.2)
I've committed the last patch:


commit a53c18b3ad9c995ddc79b1f1d05db305f499f42f
Author: Tomas Bzatek <tbzatek@redhat.com>
Date:   Tue Nov 9 18:44:14 2010 +0100

    gnome-bg: Port to GSettings
    
    This also introduces API break for loading/saving settings.
    
    See bug 626021 for details.