Created attachment 121902 [details] [review]
the patch

*** Bug 530975 has been marked as a duplicate of this bug. ***

*** Bug 310089 has been marked as a duplicate of this bug. ***

+        if (! g_file_test (font_dir, G_FILE_TEST_EXISTS)) {
+		if (create) {
+			if (g_mkdir_with_parents (font_dir, 0755) != 0) {
+				g_warning ("Cannot create needed directory \"%s\".\n", font_dir);
+				g_free (font_dir);
+				return NULL;
+			}
+		} else {
+			g_free (font_dir);
+			return NULL;
+		}

Why create font_dir and stat it if you only need it when create == TRUE?

+				g_warning ("Cannot create needed directory \"%s\".\n", font_dir);

No trailing \n, please.

+	/* remove the fonts.dir and fonts.scale files that mkfontdir generate. */

s/generate/generates/

+                (void) symlink (cursor_font, newpath);

What's that supposed to mean?

Oh, and broken indentation, of course. :-P

(In reply to comment #4)
> +        if (! g_file_test (font_dir, G_FILE_TEST_EXISTS)) {
> +               if (create) {
> +                       if (g_mkdir_with_parents (font_dir, 0755) != 0) {
> +                               g_warning ("Cannot create needed directory
> \"%s\".\n", font_dir);
> +                               g_free (font_dir);
> +                               return NULL;
> +                       }
> +               } else {
> +                       g_free (font_dir);
> +                       return NULL;
> +               }
> 
> Why create font_dir and stat it if you only need it when create == TRUE?

Humm, where do I create and stat before checking create == TRUE?  Sure, we stat first.  Oh, I see.  If create == TRUE I don't need the g_file_test.  Fixing.

> +                               g_warning ("Cannot create needed directory
> \"%s\".\n", font_dir);
> 
> No trailing \n, please.

Eh, right.  Copied the old message.

> +       /* remove the fonts.dir and fonts.scale files that mkfontdir generate.
> */
> 
> s/generate/generates/

Ack.

> +                (void) symlink (cursor_font, newpath);
> 
> What's that supposed to mean?

The (void) is supposed to silence the gcc warning about unused results for symlink, but it doesn't actually do that.  I'll remove it.

> Oh, and broken indentation, of course. :-P

Haha.

All fixed.

2008-11-03  Behdad Esfahbod  <behdad@gnome.org>

        * plugins/font/gsd-font-manager.c (setup_dir), (empty_check_dir),
        (setup_font_dir), (setup_cursor_dir), (load_font_paths),
        (gsd_font_manager_start): Cleanup font module (bug #559163)

        The old code had several flaws:

        - It tried to create directories in user's home even if we didn't have
          any use for them.

        - It called mkfontdir and XSync even if there was no fonts installed.

        The new code does the following:

        - Only call mkfontdir and XSync if there's actually any fonts in the
          relevant dirs.

        - Remove the ~/.gnome2/share/fonts and/or ~/.gnome2/share/cursor-fonts
          if they are empty and no cursor font is set.