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

[Darin Adler <darin bentspoon com>]

On 2/9/02 3:58 PM, "Håvard Wigtil" <havardw stud ntnu no> wrote:

> Here is an improved patch that survives "gconftool --break-directory"
> and handles corrupted values from GConf at startup fairly well. (At
> least I haven't been able to break it, but I'm sure someone else will ;)

Great.

> I don't think that the "Reset Desktop Background" option in the desktop
> menu makes much sense for Gnome2, as MetaTheme will handle desktop
> themes. I think it should be removed (and I'll do it together with the
> code removal patch I promised if you agree), but it could also be made
> to reset according to the current metatheme. But that sould be a job for
> the metatheme capplet IMHO.

I don't agree. No matter what part of the system does the theming, I think
that a choice in the right click menu to restore the desktop to the theme
default is a nice thing to have. I do not really care how we make it work,
but I think it should continue to work.

I think that we shouldn't remove features just because we are changing the
internal structure of the Gnome code. This isn't a "Nautilus" menu. It's the
desktop right-click menu.

>>> +         g_object_set_data (G_OBJECT (background), "theme_source",
>>> (gpointer) desktop_theme_source);
>> 
>> Do you need this (gpointer) cast?
> 
> I get a warning if I don't, probably because desktop_theme_source is
> defined as static.

I don't think that "static" is the issue, but perhaps "const" is.

This code is crying out for a version of gconf_value_get_string that handles
the NULL and the type-checking and returns NULL if something is wrong.
Because otherwise we have the same sequence of if statements in every
function. Cut and paste code bad -- shared functions good.

> +        primary_color = gconf_value_get_string (entry->value);

So primary_color can't be NULL here? Is it guaranteed that the string is
non-NULL if the type is GCONF_VALUE_STRING.

> +        /* Don't need to free this sting, it's a copy from entry->value */

No need for the comment. I was confused, but that doesn't mean later people
will be.

I would have liked to see a bit more shared code. It does bother me that
nautilus_desktop_set_background has code that duplicates decisions made in
each of the four callbacks; that code should be shared and not copied and
pasted.

The patch otherwise looks good.

    -- Darin