Re: [Nautilus-list] [PATCH] Use GConf for background settings

[Darin Adler <darin bentspoon com>]

On 1/25/02 3:40 PM, "Håvard Wigtil" <havardw stud ntnu no> wrote:

> The included patch reads desktop settings from GConf, and updates the
> desktop when these settings change. This is (an attempt) at a straight
> port, so it doesn't do any more error checking than the original code
> did. It will crash on bad settings for colors or file name.

Please fix that, regardless of the status of the original code. I don't
think you need "complete" error checking, just enough to avoid a crash if
you get an unexpected value from gconf.

> It doesn't remove methods that are left unused, I'll send a new patch
> for that if this patch is accepted.

OK.

> Also note that this patch uses the GConf keys described in
> libgnome/schemas/desktop_gnome_background.schemas, while the current
> background capplet doesn't, so to test changing desktop settings you
> have to use gconftool.

Who is going to update the background capplet to use the same tools?

How have you tested these changes?

> + void nautilus_desktop_color_changed_callback (GConfClient* client,
> +                                                guint cnxn_id,
> +                                                GConfEntry *entry,
> +                                                gpointer user_data);
> + 
> + void nautilus_desktop_picture_options_changed_callback (GConfClient* client,
> +                                                         guint cnxn_id,
> +                                                         GConfEntry *entry,
> +                                                         gpointer user_data);
> + 
> + void nautilus_desktop_picture_filename_changed_callback (GConfClient*
client,
> +                                                          guint cnxn_id,
> +                                                          GConfEntry *entry,
> +                                                          gpointer
user_data);
> + 
> + gboolean nautilus_desktop_parse_image_placement (const char *spec,
> +                                                  EelBackgroundImagePlacement
*placement);

Since these functions are local to this file, they need the "static"
qualifier.

> + void
> + nautilus_desktop_color_changed_callback (GConfClient* client,
> +                                          guint cnxn_id,
> +                                          GConfEntry *entry,
> +                                          gpointer user_data) {

Function start braces are at the left side.

>+         g_message ("Values: %s, %s, %s\n", primary_color, secondary_color,
shading_type);

These kinds of debugging statements should be removed before committing.

> +         image_type = gconf_value_get_string (entry->value);
> +         image_name = eel_background_get_image_uri (background);
> + 
> +         if (strcmp (image_type, "none") == 0) {

Can gconf_value_get_string ever return NULL? If so, then you have to change
all these calls to strcmp to use something else, like maybe eel_strcmp, that
won't cause a seg. fault if it encounters NULL.

>+         image_name = gconf_value_get_string(entry->value);
>+         eel_background_set_image_uri_sync (background, image_name);

Need to free image_name here.

> +         g_object_set_data (G_OBJECT (background), "theme_source", (gpointer)
desktop_theme_source);

Do you need this (gpointer) cast?

Overall, looks OK.

    -- Darin