(In reply to comment #0)
> If G_USER_DIRECTORY_PICTURES is not defined, the background chooser should
> either display pictures from a fallback location (e.g. '~/') or allow the user
> to select their Pictures directory and set the appropriate XDG variable in
> '~/.config/user-dirs.dirs'.

I think XDG_PICTURES_DIR not being defined is indicative of a broken system. Not sure we need to have any elaborate fallback mechanism. Distributions should just define the variable.

Created attachment 268803 [details] [review]
background: Guard against XDG_PICTURES_DIR not being defined

(In reply to comment #1)
> I think XDG_PICTURES_DIR not being defined is indicative of a broken system.
> Not sure we need to have any elaborate fallback mechanism. Distributions should
> just define the variable.
For shrinkwrapped distros, certianly. If XDG_PICTURES_DIR isn't defined, then the user has clearly broken something and there's little reason to bother holding their hand. For the more cobbled-together distros such as Gentoo and Arch, its debatable whether not this is really a "broken" condition (perhaps xdg-user-dirs should be a dependency of GNOME; perhaps `g_get_user_special_dir` should define a fallback for more than just G_USER_DIRECTORY_DESKTOP if XDG special user directories have not been set up...)

(In reply to comment #2)
> Created an attachment (id=268803) [details] [review]
> background: Guard against XDG_PICTURES_DIR not being defined
This could use some improvement. You really should display a message explaining how the user can resolve an error if at all possible. A flat "No Pictures Found" is entirely un-helpful: I'd used my Arch system for nearly a year before bug 630892 added the "you can add images..." message that wound up cluing me in to the fact that GNOME had no idea /where/ my Pictures directory was.

Perhaps changing the message to "You must define a Pictures folder to use this feature." would be sufficient for this particular case. Its no elaborate fallback, but it does at least provide a clue about what is going wrong.

Review of attachment 268803 [details] [review]:

::: panels/background/bg-pictures-source.c
@@ -720,2 @@
   pictures_path = g_get_user_special_dir (G_USER_DIRECTORY_PICTURES);
-  g_mkdir_with_parents (pictures_path, USER_DIR_MODE);

if (pictures_path == NULL)
  pictures_path = g_get_home_dir();

That's how fallback is defined in the users dirs "spec".

::: panels/background/cc-background-chooser-dialog.c
@@ +389,1 @@
   pictures_dir = g_get_user_special_dir (G_USER_DIRECTORY_PICTURES);

Ditto.

Created attachment 268923 [details] [review]
background: Guard against XDG_PICTURES_DIR not being defined

Review of attachment 268923 [details] [review]:

::: panels/background/cc-background-chooser-dialog.c
@@ +392,3 @@
   gtk_label_set_use_markup (GTK_LABEL (label), TRUE);
   gtk_misc_set_alignment (GTK_MISC (label), 0.0, 0.5);
+

Whitespace change here.

@@ +400,3 @@
+       * directory in the string below when XDG_PICTURES_DIR is
+       * undefined */
+      pictures_dir_basename = g_strdup (_("home"));

"Home" better? That's the string nautilus/GTK+ use.

@@ +405,3 @@
+    pictures_dir_basename = g_path_get_basename (pictures_dir);
+
+  href = g_markup_printf_escaped ("<a href=\"file://%s\">%s</a>", pictures_dir, pictures_dir_basename);

That's definitely not how you construct a URL.
Use g_filename_to_uri()

(I realise it was broken before, please fix it in a subsequent patch).

(In reply to comment #6)
> Review of attachment 268923 [details] [review]:
> 
> ::: panels/background/cc-background-chooser-dialog.c
> @@ +392,3 @@
>    gtk_label_set_use_markup (GTK_LABEL (label), TRUE);
>    gtk_misc_set_alignment (GTK_MISC (label), 0.0, 0.5);
> +
> 
> Whitespace change here.

That was deliberate for the sake of readability.

> @@ +400,3 @@
> +       * directory in the string below when XDG_PICTURES_DIR is
> +       * undefined */
> +      pictures_dir_basename = g_strdup (_("home"));
> 
> "Home" better? That's the string nautilus/GTK+ use.

Ok.

> @@ +405,3 @@
> +    pictures_dir_basename = g_path_get_basename (pictures_dir);
> +
> +  href = g_markup_printf_escaped ("<a href=\"file://%s\">%s</a>",
> pictures_dir, pictures_dir_basename);
> 
> That's definitely not how you construct a URL.
> Use g_filename_to_uri()
> 
> (I realise it was broken before, please fix it in a subsequent patch).

Ok.

Created attachment 268990 [details] [review]
background: Guard against XDG_PICTURES_DIR not being defined

s/home/Home/

Created attachment 268991 [details] [review]
background: Use GLib API to convert absolute filename to URI

Review of attachment 268990 [details] [review]:

Looks good.

Review of attachment 268991 [details] [review]:

Looks good.

Thanks for the reviews!