I've got a branch running in https://github.com/tshikaboom/geary/tree/dirs.

I would like to know if that is the right approach? Perhaps maybe not copy the folder structure named by email address? Or any other suggestions.

Hey, after a really quick look it seems like a good start. Let me know when you think it's ready for a review - preferably by squashing and attaching it here.

We do want a directory per-account under ~/.config, since we may at some point want to store additional account-specific files there. So ~/.config/geary would look much the same as ~/.local/share/geary, except only contain the geary.ini file.

Something else to think about is Bug 714643 - that will require some migration of the config file as well, so it would be good if this migration and the migration outlined there could happen at the same time.

Created attachment 331048 [details] [review]
dd a helper function to copy over configuration and use it

This migrates geary.ini to ~/.config and writes the primary email address to the new config file at the same time.

Created attachment 331049 [details] [review]
Use geary.ini in ~/.config/geary

Created attachment 331070 [details] [review]
Use geary.ini in ~/.config/geary

This shouldn't break the build.

Review of attachment 331070 [details] [review]:

This in general looks good. The config directory should take precedence over the storage directory throughout, however.

::: src/engine/api/geary-account-information.vala
@@ +60,3 @@
      */
+    public File? storage_dir { get; private set; default = null; }
+    public File? config_dir { get; private set; default = null; }

As a matter of style I would place the config dir before storage, since it's the primary directory, and same for the from_file() ctor args and in the method bodies below.

@@ +169,3 @@
     // This constructor is used internally to load accounts from disk.
+    internal AccountInformation.from_file(File storage_directory, File config_directory) {
+        this.email = storage_directory.get_basename();

This should be retrieved from the config dir name.

::: src/engine/api/geary-engine.vala
@@ +52,2 @@
     public File? user_data_dir { get; private set; default = null; }
+    public File? user_config_dir { get; private set; default = null; }

Same comment here about config being primary and hence should appear before the data dir in this file.

Also, it may be worth renaming user_data_dir here to be consistent with "storage" terminology used in geary-account-information.vala, or renaming it there to be consistent with here.

@@ +191,3 @@
                 // TODO: check for geary.ini
+                account_list.add(new AccountInformation.from_file(user_data_dir.get_child(info.get_name()),
+                    user_config_dir.get_child(info.get_name())));

Should be enumerating children of the config dir here, not the data dir.

Review of attachment 331048 [details] [review]:

This also looks pretty good - just some naming, and re-migration and error handling in need of a tidying up.

::: src/client/util/util-xdgdir.vala
@@ +5,3 @@
+ */
+
+namespace XdgDir {

I feel like the namespace for this should probably be "Migrate" or similar - it seems like it would a good place to future migration-related code here as well, rather than just XDG-related code. The function below could then be called something like "xdg_config_dir".

@@ +25,3 @@
+        // Only copy the configuration if $XDG_CONFIG_DIR/geary does not exist.
+        if (user_config_dir.query_exists())
+            return;

This is too strong a test - people may have a ~/.config/geary dir already with a "user-message.css" file in it — see: src/client/conversation-viewer/conversation-web-view.vala

It is annoying that this means Geary will re-check the migration at startup every time, but since most people will only have a small number of accounts, the overhead won't be too bad.

@@ +60,3 @@
+                // ~/.config/geary/<account>/.config_migrated exists.
+                is_migrated = File.new_for_path(user_config_dir.get_path() +
+                    "/" + email + "/" + MIGRATED_FILENAME);

To stop attempts to migrate accounts that were created fresh under ~/.config, this check should first look to see if data_dir/<account>/geary.ini exists and if not, skip migrating that account.

Also, I would be inclined to create and look for the ".config_migrated" file in the data dir instead of the config dir, so the config dir does not even need to get opened before deciding to skip migrating it.

@@ +106,3 @@
+                } catch (Error e) {
+                    debug("Error writing email %s to config file", email);
+                }

The ".config_migrated" file should only be created after this point, so migration is re-attempted if it fails at some point during the process.

@@ +109,3 @@
+            }
+        } catch (Error e) {
+            debug("Error enumerating fileinfo");

A slightly more descriptive string would be useful here - it at least should mention the error occurred during config migration.

However, it would be better to simply declare that the top-level function throws Error, and letting any fatal errors (i.e. those that don't result in a continue around the loop) simply get caught and handled by GearyController. You could probably then remove the try/catch block surrounding the while loop and the first two at the top of the function, as well.

Created attachment 331237 [details] [review]
Add a helper function to copy over configuration and use it

Created attachment 331238 [details] [review]
Use geary.ini in ~/.config/geary

Review of attachment 331237 [details] [review]:

This is almost ready to go. There's a some minor notes below, but also there's two things that I totally missed first time around that are important to address:

 - The new source file also needs to be added to po/POFILES.in
 - For portability, the GLib.File.get_child() interface should be used to construct paths, rather than concatenating strings with "/".

::: src/client/util/util-migrate.vala
@@ +12,3 @@
+    
+    /* Utility function to copy geary.ini to the XDG configuration directory.
+     * It iterates through all the account directories in $XDG_DATA_DIR and copies over

This is a great comment, but would be worth converting to a documentation comment, per Geary's coding conventions.

I.e.:

/**
 * A one line summary.
 *
 * Further explanation...
 */

@@ +34,3 @@
+        FileEnumerator enumerator;
+        enumerator = user_data_dir.enumerate_children ("standard::*",
+        FileQueryInfoFlags.NOFOLLOW_SYMLINKS, null);

Minor indentation problem.

@@ +99,3 @@
+                null);
+            } catch (Error e) {
+                debug("Error writing email %s to config file", email);

Needs to continue in this catch block so .config_migrated isn't created?

Review of attachment 331238 [details] [review]:

Looks good here. Once the other patch has been tidied up I'll take them for a test run.

Review of attachment 331238 [details] [review]:

Whoops, wrongstatus.com

Created attachment 331293 [details] [review]
Add a helper function to copy over configuration and use it

Okay, I just took them for a run and all three patches test out fine. When I applied them `git am` complained about some trailing whitespace errors for the two here however. I'm happy to commit the set as soon as you can fix those errors.

Created attachment 331299 [details] [review]
Add a helper function to copy over configuration and use it

Created attachment 331300 [details] [review]
Use geary.ini in ~/.config/geary

Great, this have been pushed to master as b3b2d58 and d389102. Thanks heaps!

Oh, one thing - I had to push a quick build fix to master for geary-contoller.vala. The compiler was complaining about the call to Geary.Engine.instance.open_async, and the use of account.information.settings_dir.

I didn't catch them because I didn't do a clean rebuild, but I thought these were taken care of previously. Can you have a look at a81cc78 on master and let me know if there's anything missing?

Gah, I almost had these fixed in comment #5 and somehow lost the changes again from the patches!
I wouldn't say there's anything else missing aside from that though.

Sorry for breaking the build!

Okay, cool.

No problem, thanks for getting this done!

Created attachment 331616 [details] [review]
Do not crash if ~/.local/share/geary does not exist

I stumbled upon this one quite randomly, é bit embarrassing (wrt this bug's title) but oh well!

Small ping! The problem is still present on git master if ~/.local/share/geary does not exist.

Oh, thanks for the reminder! I just pushed the fix to master as 46e1764.