Re: Nautilus background patch

[Alex Larsson <alexl redhat com>]

On 22 Feb 2002, Richard Hestilow wrote:

> I fixed nautilus-directory-background to load and save the background
> settings via libbackground. May I commit?

diff -u -r1.416 configure.in
--- configure.in	2002/02/22 21:30:17	1.416
+++ configure.in	2002/02/23 05:05:45
@@ -62,7 +62,6 @@
 	libgnomeui-2.0 >= $GNOME_UI_REQUIRED \
 	librsvg-2.0 >= $RSVG_REQUIRED \
 	libxml-2.0 >= $XML_REQUIRED)
-
 dnl ==========================================================================
 
 ALL_LINGUAS="az ca cs da de el en_GB es fi fr ga gl hu it ja ko lt lv nl nn no pl pt pt_BR ro ru sk sl sv ta tr uk zh_CN zh_TW"


Please don't make unncessary whitespace changes.


-static gboolean
-eel_gnome_config_string_match_no_case_with_default (const char *path, const char *test_value, gboolean *was_default)
-{
-	char *value;
-	gboolean result;
-	value = gnome_config_get_string_with_default (path, was_default);
-	result = !eel_strcasecmp (value, test_value);
-	g_free (value);
-	return result;
-}
-
 /* This enum is from gnome-source/control-center/capplets/background-properties/render-background.h */
 enum {
 	WALLPAPER_TILED,
 	WALLPAPER_CENTERED,
-	WALLPAPER_SCALED,
 	WALLPAPER_SCALED_KEEP,
+	WALLPAPER_SCALED,
 	WALLPAPER_EMBOSSED
 };
 

The comment above this enum is no longer true, and should be removed. But 
i don't understand why you use this enum at all. Why don't you use 
wallpaper_type_t? Having to sync this enum with wallpaper_type_t seems 
fragile and ugly.


@@ -231,27 +221,24 @@
                                                 char **image,
                                                 EelBackgroundImagePlacement *placement)
 {
-	int	 image_alignment;
-	char*	 image_local_path;
+	int      image_alignment;
+	const char* image_local_path;
 	char*	 default_image_uri;
-	gboolean no_alignment_set;
-	gboolean no_image_set;
 	EelBackgroundImagePlacement default_placement;
 	

+	char    *cur_theme_name;
 	char	*end_color;
 	char	*start_color;
 	char	*default_color;
 	gboolean use_gradient;
 	gboolean is_horizontal;
-	gboolean no_start_color_set;
-	gboolean no_end_color_set;
-	gboolean no_gradient_set;
-	gboolean no_gradient_orientation_set;
+	gboolean switch_to_cur_theme_default;
 
 	char *theme_name;
-	char *cur_theme_name;
 	gboolean no_theme_name_set;
-	gboolean switch_to_cur_theme_default;
+	BGPreferences *prefs = BG_PREFERENCES (bg_preferences_new ());

We never initialize local variables in their declarations. See the Nautilus style guide.

+
+	bg_preferences_load (prefs);
 
 	theme_name = gnome_config_get_string_with_default ("/Background/Default/nautilus_theme", &no_theme_name_set);
 
@@ -263,9 +250,9 @@
 			(theme_name, desktop_theme_source, &default_color, &default_image_uri, &default_placement);
 	}
 
-	image_local_path = gnome_config_get_string_with_default ("/Background/Default/wallpaper", &no_image_set);
+	image_local_path = prefs->wallpaper_filename; 
 
-	if (no_image_set) {
+	if (!(prefs->wallpaper_filename && prefs->wallpaper_enabled)) {


We always use pointer != NULL istead of just pointer.


 		*image = g_strdup (default_image_uri);
 	} else if (eel_strcasecmp (image_local_path, "none") != 0) {
 		*image = gnome_vfs_get_uri_from_local_path (image_local_path);

The "none" case should never happen anymore, right? Since we never set it to 
none anymore.

@@ -273,48 +260,36 @@
 		*image = NULL;
 	}
 	
-	g_free(image_local_path);
+	image_alignment  = prefs->wallpaper_type;
 
-	image_alignment  = gnome_config_get_int_with_default ("/Background/Default/wallpaperAlign", &no_alignment_set);
-
-	if (no_alignment_set) {
-		*placement = default_placement;
-	} else {
-		 switch (image_alignment) {
-		 	default:
-		 		g_assert_not_reached ();
-			case WALLPAPER_EMBOSSED:
-				/* FIXME bugzilla.gnome.org 42193: we don't support embossing.
-				 * Just treat it as centered - ugh.
-				 */
-			case WALLPAPER_CENTERED:
-				*placement = EEL_BACKGROUND_CENTERED;
-				break;
-			case WALLPAPER_TILED:
-				*placement = EEL_BACKGROUND_TILED;
-				break;
-			case WALLPAPER_SCALED:
-				*placement = EEL_BACKGROUND_SCALED;
-				break;
-			case WALLPAPER_SCALED_KEEP:
-				*placement = EEL_BACKGROUND_SCALED_ASPECT;
-				break;
-		 }
-	}
+	 switch (image_alignment) {
+	 	default:
+	 		g_assert_not_reached ();
+		case WALLPAPER_EMBOSSED:
+			/* FIXME bugzilla.gnome.org 42193: we don't support embossing.
+			 * Just treat it as centered - ugh.
+			 */
+		case WALLPAPER_CENTERED:
+			*placement = EEL_BACKGROUND_CENTERED;
+			break;
+		case WALLPAPER_TILED:
+			*placement = EEL_BACKGROUND_TILED;
+			break;
+		case WALLPAPER_SCALED:
+			*placement = EEL_BACKGROUND_SCALED;
+			break;
+		case WALLPAPER_SCALED_KEEP:
+			*placement = EEL_BACKGROUND_SCALED_ASPECT;
+			break;
+	 }

Here you use the strange enum, just make image_alignment of type 
wallpaper_type_t and use those enum values.

+	
+	end_color     = g_strdup_printf ("#%02x%02x%02x", prefs->color2->red >> 8, prefs->color2->green >> 8, prefs->color2->blue >> 8); 
+	start_color   = g_strdup_printf ("#%02x%02x%02x", prefs->color1->red >> 8, prefs->color1->green >> 8, prefs->color1->blue >> 8); 

Maybe these should use 
eel_gdk_rgb_to_color_spec (eel_gdk_color_to_rgb (prefs->color1)) 
instead. Even better might be to add eel_gdk_color_to_color_spec ().

+	use_gradient  = prefs->gradient_enabled;
+	is_horizontal = (prefs->wallpaper_enabled == ORIENTATION_HORIZ);
 
-	end_color     = gnome_config_get_string_with_default ("/Background/Default/color2", &no_end_color_set);
-	start_color   = gnome_config_get_string_with_default ("/Background/Default/color1", &no_start_color_set);
-	use_gradient  = !eel_gnome_config_string_match_no_case_with_default ("/Background/Default/simple", "solid", &no_gradient_set);
-	is_horizontal = !eel_gnome_config_string_match_no_case_with_default ("/Background/Default/gradient", "vertical", &no_gradient_orientation_set);
-
-	if (no_gradient_set || no_gradient_orientation_set || no_start_color_set) {
-		*color = g_strdup (default_color);
-	} else if (use_gradient) {
-		if (no_end_color_set) {
-			*color = g_strdup (default_color);
-		} else {
-			*color = eel_gradient_new (start_color, end_color , is_horizontal);
-		}
+	if (use_gradient) {
+		*color = eel_gradient_new (start_color, end_color , is_horizontal);
 	} else {
 		*color = g_strdup (start_color);
 	}

The old code spends a lot of time handling what happens if the start color 
or the end color is not set, or if it is set to a bogus value, but you 
deleted that. Does libbackground guarantee that the colors have 
sane values?

@@ -361,6 +336,8 @@
 	g_free (cur_theme_name);	
 	g_free(default_color);
 	g_free(default_image_uri);
+
+	g_object_unref (G_OBJECT (prefs));
 }
 
 static void
@@ -372,38 +349,43 @@
 	char *theme_name;
 
 	int wallpaper_align;
+	BGPreferences *prefs = BG_PREFERENCES (bg_preferences_new ());
 

Again initialization in declaration.

-	int i;
-	int wallpaper_count;
-	char *wallpaper_path_i;
-	char *wallpaper_config_path_i;
-	gboolean found_wallpaper;
+	bg_preferences_load (prefs);
 
 	if (color != NULL) {
 		start_color = eel_gradient_get_start_color_spec (color);
-		gnome_config_set_string ("/Background/Default/color1", start_color);		
+		gdk_color_parse (start_color, prefs->color1);
 		g_free (start_color);
 
 		/* if color is not a gradient, this ends up writing same as start_color */
 		end_color = eel_gradient_get_end_color_spec (color);
-		gnome_config_set_string ("/Background/Default/color2", end_color);		
+		gdk_color_parse (end_color, prefs->color2);
 		g_free (end_color);
 
-		gnome_config_set_string ("/Background/Default/simple", eel_gradient_is_gradient (color) ? "gradient" : "solid");
-		gnome_config_set_string ("/Background/Default/gradient", eel_gradient_is_horizontal (color) ? "horizontal" : "vertical");
+		if (eel_gradient_is_gradient (color)) {
+			prefs->gradient_enabled = TRUE;
+			prefs->orientation = ORIENTATION_SOLID;
+		} else {
+			prefs->gradient_enabled = FALSE;
+			prefs->orientation = eel_gradient_is_horizontal (color) ? ORIENTATION_HORIZ : ORIENTATION_VERT;
+		}

This if is reversed. It sets a solid color if the colorspec is a gradient.



 	} else {
 		/* We set it to white here because that's how backgrounds with a NULL color
 		 * are drawn by Nautilus - due to usage of eel_gdk_color_parse_with_white_default.
 		 */
-		gnome_config_set_string ("/Background/Default/color1", "#FFFFFF");		
-		gnome_config_set_string ("/Background/Default/color2", "#FFFFFF");		
-		gnome_config_set_string ("/Background/Default/simple", "solid");
-		gnome_config_set_string ("/Background/Default/gradient", "vertical");
+		gdk_color_parse ("#FFFFFF", prefs->color1);
+		gdk_color_parse ("#FFFFFF", prefs->color2);
+		prefs->gradient_enabled = FALSE;
+		prefs->orientation = ORIENTATION_SOLID;
 	}
 
+	g_free (prefs->wallpaper_filename);
 	if (image != NULL) {
 		image_local_path = gnome_vfs_get_local_path_from_uri (image);
 		gnome_config_set_string ("/Background/Default/wallpaper", image_local_path);
+		prefs->wallpaper_filename = g_strdup (image_local_path);


This still sets the gnome-config value. Did you mean to do that? You don't 
set the other values, but perhaps we should set them for backwards 
compatibility reasons. Although I guess that would be better done in 
libbackground instead.


 		switch (placement) {
 			case EEL_BACKGROUND_TILED:
 				wallpaper_align = WALLPAPER_TILED;
@@ -422,33 +404,10 @@
 				wallpaper_align = WALLPAPER_TILED;
 				break;	
 		}

This uses the old enum again.



-		
-		gnome_config_set_int ("/Background/Default/wallpaperAlign", wallpaper_align);
-
-		wallpaper_count = gnome_config_get_int ("/Background/Default/wallpapers=0");
-		found_wallpaper = FALSE;
-		for (i = 1; i <= wallpaper_count && !found_wallpaper; ++i) {
-			wallpaper_config_path_i = g_strdup_printf ("/Background/Default/wallpaper%d", i);
-			wallpaper_path_i = gnome_config_get_string (wallpaper_config_path_i);
-			g_free (wallpaper_config_path_i);
-			if (eel_strcmp (wallpaper_path_i, image_local_path) == 0) {
-				found_wallpaper = TRUE;
-			}
-			
-			g_free (wallpaper_path_i);			
-		}
-
-		if (!found_wallpaper) {
-			gnome_config_set_int ("/Background/Default/wallpapers", wallpaper_count + 1);
-			gnome_config_set_string ("/Background/Default/wallpapers_dir", image_local_path);
-			wallpaper_config_path_i = g_strdup_printf ("/Background/Default/wallpaper%d", wallpaper_count + 1);
-			gnome_config_set_string (wallpaper_config_path_i, image_local_path);
-			g_free (wallpaper_config_path_i);
-		}
-		
-		g_free (image_local_path);		
+	
+		prefs->wallpaper_type = wallpaper_align;
 	} else {
-		gnome_config_set_string ("/Background/Default/wallpaper", "none");
+		prefs->wallpaper_filename = NULL;
 	}
 
 	theme_name = nautilus_theme_get_theme ();
@@ -456,6 +415,9 @@
 	g_free (theme_name);
 
 	gnome_config_sync ();
+
+	bg_preferences_save (prefs);
+	g_object_unref (G_OBJECT (prefs));
 }
 
Saving the prefs is gonna give us a notify on the keys, and there is 
nothing that protects us from handling that notify, which can be quite 
costly. We might even get several notifies since bg_preferences_save() 
writes several keys. I'm not sure about the gconf behaviour there.

 static void
@@ -486,64 +448,26 @@
  */
 static int set_root_pixmap_count = 0;

Since you're not using this varaible anymore you should remove it.

 
-static GdkFilterReturn
-nautilus_file_background_event_filter (GdkXEvent *gdk_xevent, GdkEvent *event, gpointer data)
+static void
+desktop_background_destroyed_callback (EelBackground *background, void *georgeWBush)
 {
-	XEvent *xevent;
-	EelBackground *background;
-
-	xevent = (XEvent *) gdk_xevent;
-
-	if (xevent->type == PropertyNotify && xevent->xproperty.atom == gdk_x11_get_xatom_by_name ("ESETROOT_PMAP_ID")) {
-
-		/* If we caused it, ignore it.
-		 */
-		if (set_root_pixmap_count > 0) {
-			--set_root_pixmap_count;
-			return GDK_FILTER_CONTINUE;
-		}
-		
-	    	background = EEL_BACKGROUND (data);
-	    	/* FIXME bugzilla.gnome.org 42220:
-	    	 * We'd like to call saved_settings_changed_callback right here, directly.
-	    	 * However, the current version of the property-background capplet saves
-	    	 * the new setting in gnome_config AFTER setting the root window's property -
-	    	 * i.e. after we get this event. How long afterwards is not knowable - we
-	    	 * guess half a second. Fixing this requires changing the capplet.
-	    	 */
-	    	gtk_timeout_add (500, (GtkFunction) (call_settings_changed), background);
-	}
-
-	return GDK_FILTER_CONTINUE;
+	guint cnxn_id = GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (background), "desktop_gconf_cnxn"));

Don't initialize locals in the declaration.
And please don't use abbrevation like cnxn. Use full words.

+	eel_gconf_notification_remove (cnxn_id);
 }
 
 static void
-desktop_background_destroyed_callback (EelBackground *background, void *georgeWBush)
+desktop_background_gconf_notify_cb (GConfClient *client, guint cnxn_id, GConfEntry *entry, gpointer data)
 {
-	gdk_window_remove_filter (
-                gdk_get_default_root_window (),
-                nautilus_file_background_event_filter, background);
+	call_settings_changed (EEL_BACKGROUND (data));
 }
 
 static void
 nautilus_file_background_receive_root_window_changes (EelBackground *background)
 {
-	XWindowAttributes attribs = { 0 };
+	guint cnxn_id = eel_gconf_notification_add ("/desktop/gnome/background", desktop_background_gconf_notify_cb, background);
 
-	/* set up a filter on the root window to get notified about property changes */
-	gdk_window_add_filter (
-                gdk_get_default_root_window (),
-                nautilus_file_background_event_filter, background);
-
-	gdk_error_trap_push ();
-
-	/* select events, we need to trap the kde status thingies anyway */
-	XGetWindowAttributes (GDK_DISPLAY (), GDK_ROOT_WINDOW (), &attribs);
-	XSelectInput (GDK_DISPLAY (), GDK_ROOT_WINDOW (), attribs.your_event_mask | PropertyChangeMask);
-	
-	gdk_flush ();
-	gdk_error_trap_pop ();
-
+	g_object_set_data (G_OBJECT (background), "desktop_gconf_cnxn", GUINT_TO_POINTER (cnxn_id));
+			
 	g_signal_connect (background,
  			    "destroy",
 			    G_CALLBACK (desktop_background_destroyed_callback),


Since this function has nothing to do with root window changes anymore it 
should be renamed to something that describes what it does.


I also tried this patch with the gnome2-background-capplet, and it didn't 
seem to work very well. Setting the background from nautilus did seem to 
work, but the when doing so the background capplet didn't correctly 
update it's UI to the current setting, and when using the capplet to set 
the background nautuilus didn't update the background (did work when i 
restarted nautilus). It also printed:

nautilus (pid:3034): GConf-CRITICAL **: file gconf-changeset.c: line 313 
(gconf_change_set_set_string): assertion `val != NULL' failed

nautilus (pid:3034): GConf-CRITICAL **: file gconf-changeset.c: line 313 
(gconf_change_set_set_string): assertion `val != NULL' failed

nautilus (pid:3034): GConf-CRITICAL **: file gconf-changeset.c: line 313 
(gconf_change_set_set_string): assertion `val != NULL' failed


I'm not sure about the set_root_pixmap() protocol. Does it still have to 
set ESETROOT_PMAP_ID, since it doesn't write the old gnome-config keys 
anymore? And what about _XROOTPMAP_ID? Perhaps these are needed to support 
other WMs.

/ Alex