Review of attachment 196571 [details] [review]:

Hi, thanks for the patch, I inlined some comments.

Anyway, it looks like that directory is used to store user-added dictionary sources, so should some kind of migration be used here?

::: gnome-dictionary/src/gdict-app.c
@@ +231,3 @@
   gtk_init (argc, argv);
 
+  g_set_prgname ("gnome-dictionary");

This should already have been done automatically by gtk_init().

::: gnome-dictionary/src/gdict-common.c
@@ +46,3 @@
 
+  retval = g_build_filename (g_get_user_data_dir (),
+			     g_get_prgname (),

Maybe you can just use "gnome-dictionary" here instead of g_get_prgname() anyway.

g_get_prgname used in order to keep things DRY. probably can be dropped or replaced by macro.

Yes, migration is something we should provide with this change.
Do You have ideas how to implement it right way? 

I see 2 options here:

1) make it part of gnome-dictionary binary (implement in c)
2) make shell script wrapper that will perform migration if needed

Created attachment 204562 [details] [review]
Use XDG layout for configs

i've updated the patch. Migration of old data implemented this time.

Review of attachment 204562 [details] [review]:

there is *a lot* of whitespace fixes in this patch - it makes reading it and commenting harder. please, resist the urge to fix whitespace - feel free to attach another patch that does just that.

::: src/gdict-common.c
@@ +86,3 @@
+
+  /* move configs from pre-XDG directory to right place */
+  if (g_file_test (old_data_dir_name, G_FILE_TEST_IS_DIR))

don't use g_file_test() to protect g_rename(): it's inherently racy.

just g_rename() the old test directory to the new one, and catch the ENOEXIST error.

see also: http://blogs.gnome.org/ebassi/2006/07/24/ride-on-a-white-horse/

@@ +126,3 @@
+  gdict_migrate_configs ();
+
+  if (!g_file_test (config_dir_name, G_FILE_TEST_IS_DIR))

same as above: just use g_mkdir_with_parents() and check against EEXIST.

Hello, Emmanuele.

On whitespace. You can just use git diff -w

- cut -
  -w, --ignore-all-space
     Ignore whitespace when comparing lines. This ignores differences even if one line has whitespace where the other line has none.
- cut -

On g_file_test. I can change the logic as you suggested in your blog post if it is totally necessary condition to accept the patch. Otherwise i don't see the point to make defensive coding where it's not really necessary. I cannot imagine where someone creates directory after we checked it with g_file_test... in any case result is the same - error message and call to exit().

we only can save one call to stat(). as for me it's false economy koz it doesent make code more readable. kinda premature optimization.

(In reply to comment #5)

> On whitespace. You can just use git diff -w

I know that - but the review system on Bugzilla doesn't. and I generally don't like intermixing completely unrelated whitespace fixes in a commit.

> On g_file_test. I can change the logic as you suggested in your blog post if it
> is totally necessary condition to accept the patch. Otherwise i don't see the
> point to make defensive coding where it's not really necessary. I cannot
> imagine where someone creates directory after we checked it with g_file_test...
> in any case result is the same - error message and call to exit().
> 
> we only can save one call to stat(). as for me it's false economy koz it
> doesent make code more readable. kinda premature optimization.

avoiding inherent races is not "premature optimization", especially since the code already does the right thing elsewhere. and yes: I consider this a prerequisite for the patch to be accepted.

I appreciate the patch, obviously, but I'd also appreciate if I didn't have to do further work to fix it after committing it. :-)

Created attachment 204656 [details] [review]
Second attempt

No whitespace changes and no g_file_test

bump

Review of attachment 204656 [details] [review]:

looks way better, aside from a couple of points.

::: src/gdict-common.c
@@ +77,3 @@
+
+gboolean
+  retval = g_build_filename (g_get_user_config_dir (),

I doubt "vodi" is a valid C type.

@@ +104,3 @@
+    }
+
+

the "success" and "error" labels are pretty much redundant.

I'd use a boolean variable, and unify the code paths.

Created attachment 205308 [details] [review]
Updated patch

Removed labels
vodi -> void in header

Review of attachment 205308 [details] [review]:

::: src/gdict-common.c
@@ +100,3 @@
+          g_free (old_data_dir_name);
+
+  old_data_dir_name = gdict_get_old_data_dir ();

not exactly what I meant; you could declare a "gboolean res = TRUE;" variable, use something like this:

  res = FALSE;

in the error path, and below you could unconditionally free the strings and return the boolean value.

anyway, looks good.

@@ +131,3 @@
+
+            g_free (config_dir_name);
+          g_free (config_dir_name);

same as above, really - though, again, it looks good.

::: src/gdict-source-dialog.c
@@ +346,3 @@
     }
       
   name = g_strdup_printf ("%s.desktop", gdict_source_get_name (source));

this one could be changed to:

  name = g_strconcat (gdict_source_get_name (source), ".desktop", NULL);

as a "drive by" change.

@@ +348,2 @@
   name = g_strdup_printf ("%s.desktop", gdict_source_get_name (source));
+  filename = g_build_filename (gdict_get_config_dir(),

get_config_dir() returns a newly allocated string: you should use a separate variable to store it, and then call g_free() - otherwise, you're leaking a string.

alternatively, gdict_get_config_dir() could store the string statically, and return a const char*. it would be a one-off allocation, and it would avoid hitting the allocator every time we ask for the configuration directory.

@@ +437,3 @@
     }
       
   name = g_strdup_printf ("%s.desktop", gdict_source_get_name (source));

same as above, g_strconcat() would be better.

@@ +439,3 @@
   name = g_strdup_printf ("%s.desktop", gdict_source_get_name (source));
+
+  filename = g_build_filename (gdict_get_config_dir(),

same as above, you're leaking the string returned by gdict_get_config_dir().

Created attachment 205363 [details] [review]
XDG Directories, updated patch

Review of attachment 205363 [details] [review]:

looks good to me

Any chance to see this in git master?

Its marked acn, so you should feel encouraged to commit it.

What do i need to get commit right to this repository?

Comment on attachment 205363 [details] [review]
XDG Directories, updated patch

Committed: http://git.gnome.org/browse/gnome-dictionary/commit/?id=63c5af12646a00cedbce0fe35e0f936822593325

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