From 400850cc44dcac53ab2218fbd023a8f1ceb34eb5 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Wed, 22 Apr 2020 18:56:32 +1000 Subject: [PATCH] Geary.AccountInformation: Rework how special use folder paths are stored Use a simple list of strings to store path steps, since we don't know what the root will be in advance, and this leads to errors if callers try to compare the info's paths to (say) an actual IMAP path. Store path steps in a SpecialUse to path step map property, rather than as individual properties for each special use, to reduce repetition. --- src/client/accounts/accounts-manager.vala | 126 +++++++------ src/engine/api/geary-account-information.vala | 171 +++++++----------- .../imap-engine-generic-account.vala | 26 +-- .../accounts/accounts-manager-test.vala | 21 +-- 4 files changed, 163 insertions(+), 181 deletions(-) diff --git a/src/client/accounts/accounts-manager.vala b/src/client/accounts/accounts-manager.vala index ab2e390e..f6d5749f 100644 --- a/src/client/accounts/accounts-manager.vala +++ b/src/client/accounts/accounts-manager.vala @@ -1108,17 +1108,29 @@ public class Accounts.AccountConfigV1 : AccountConfig, GLib.Object { Geary.ConfigFile.Group folder_config = config.get_group(GROUP_FOLDERS); - account.archive_folder_path = load_folder(folder_config, FOLDER_ARCHIVE); - account.drafts_folder_path = load_folder(folder_config, FOLDER_DRAFTS); - account.sent_folder_path = load_folder(folder_config, FOLDER_SENT); + account.set_folder_steps_for_use( + ARCHIVE, folder_config.get_string_list(FOLDER_ARCHIVE) + ); + account.set_folder_steps_for_use( + DRAFTS, folder_config.get_string_list(FOLDER_DRAFTS) + ); + account.set_folder_steps_for_use( + SENT, folder_config.get_string_list(FOLDER_SENT) + ); // v3.32-3.36 used spam instead of junk if (folder_config.has_key(FOLDER_SPAM)) { - account.junk_folder_path = load_folder(folder_config, FOLDER_SPAM); + account.set_folder_steps_for_use( + JUNK, folder_config.get_string_list(FOLDER_SPAM) + ); } if (folder_config.has_key(FOLDER_JUNK)) { - account.junk_folder_path = load_folder(folder_config, FOLDER_JUNK); + account.set_folder_steps_for_use( + JUNK, folder_config.get_string_list(FOLDER_JUNK) + ); } - account.trash_folder_path = load_folder(folder_config, FOLDER_TRASH); + account.set_folder_steps_for_use( + TRASH, folder_config.get_string_list(FOLDER_TRASH) + ); return account; } @@ -1150,33 +1162,41 @@ public class Accounts.AccountConfigV1 : AccountConfig, GLib.Object { Geary.ConfigFile.Group folder_config = config.get_group(GROUP_FOLDERS); - save_folder(folder_config, FOLDER_ARCHIVE, account.archive_folder_path); - save_folder(folder_config, FOLDER_DRAFTS, account.drafts_folder_path); - save_folder(folder_config, FOLDER_SENT, account.sent_folder_path); - save_folder(folder_config, FOLDER_JUNK, account.junk_folder_path); - save_folder(folder_config, FOLDER_TRASH, account.trash_folder_path); + save_folder( + folder_config, + FOLDER_ARCHIVE, + account.get_folder_steps_for_use(ARCHIVE) + ); + save_folder( + folder_config, + FOLDER_DRAFTS, + account.get_folder_steps_for_use(DRAFTS) + ); + save_folder( + folder_config, + FOLDER_SENT, + account.get_folder_steps_for_use(SENT) + ); + save_folder( + folder_config, + FOLDER_JUNK, + account.get_folder_steps_for_use(JUNK) + ); + save_folder( + folder_config, + FOLDER_TRASH, + account.get_folder_steps_for_use(TRASH) + ); } - private void save_folder(Geary.ConfigFile.Group config, - string key, - Geary.FolderPath? path) { - if (path != null) { - config.set_string_list( - key, new Gee.ArrayList.wrap(path.as_array()) - ); + private inline void save_folder(Geary.ConfigFile.Group config, + string key, + Gee.List? steps) { + if (steps != null) { + config.set_string_list(key, steps); } } - private Geary.FolderPath? load_folder(Geary.ConfigFile.Group config, - string key) { - Geary.FolderPath? path = null; - Gee.List parts = config.get_string_list(key); - if (!parts.is_empty) { - path = Geary.AccountInformation.build_folder_path(parts); - } - return path; - } - } @@ -1274,20 +1294,20 @@ public class Accounts.AccountConfigLegacy : AccountConfig, GLib.Object { EMAIL_SIGNATURE_KEY, info.signature ); - info.drafts_folder_path = Geary.AccountInformation.build_folder_path( - config.get_string_list(DRAFTS_FOLDER_KEY) + info.set_folder_steps_for_use( + DRAFTS, config.get_string_list(DRAFTS_FOLDER_KEY) ); - info.sent_folder_path = Geary.AccountInformation.build_folder_path( - config.get_string_list(SENT_MAIL_FOLDER_KEY) + info.set_folder_steps_for_use( + SENT, config.get_string_list(SENT_MAIL_FOLDER_KEY) ); - info.junk_folder_path = Geary.AccountInformation.build_folder_path( - config.get_string_list(SPAM_FOLDER_KEY) + info.set_folder_steps_for_use( + JUNK, config.get_string_list(SPAM_FOLDER_KEY) ); - info.trash_folder_path = Geary.AccountInformation.build_folder_path( - config.get_string_list(TRASH_FOLDER_KEY) + info.set_folder_steps_for_use( + TRASH, config.get_string_list(TRASH_FOLDER_KEY) ); - info.archive_folder_path = Geary.AccountInformation.build_folder_path( - config.get_string_list(ARCHIVE_FOLDER_KEY) + info.set_folder_steps_for_use( + ARCHIVE, config.get_string_list(ARCHIVE_FOLDER_KEY) ); info.save_drafts = config.get_bool(SAVE_DRAFTS_KEY, true); @@ -1322,36 +1342,34 @@ public class Accounts.AccountConfigLegacy : AccountConfig, GLib.Object { ); } - Gee.ArrayList empty = new Gee.ArrayList(); + Gee.List empty = new Gee.ArrayList(); + Gee.List? steps = null; + + steps = info.get_folder_steps_for_use(DRAFTS); config.set_string_list( DRAFTS_FOLDER_KEY, - (info.drafts_folder_path != null - ? new Gee.ArrayList.wrap(info.drafts_folder_path.as_array()) - : empty) + steps != null ? steps : empty ); + + steps = info.get_folder_steps_for_use(SENT); config.set_string_list( SENT_MAIL_FOLDER_KEY, - (info.sent_folder_path != null - ? new Gee.ArrayList.wrap(info.sent_folder_path.as_array()) - : empty) + steps != null ? steps : empty ); + steps = info.get_folder_steps_for_use(JUNK); config.set_string_list( SPAM_FOLDER_KEY, - (info.junk_folder_path != null - ? new Gee.ArrayList.wrap(info.junk_folder_path.as_array()) - : empty) + steps != null ? steps : empty ); + steps = info.get_folder_steps_for_use(TRASH); config.set_string_list( TRASH_FOLDER_KEY, - (info.trash_folder_path != null - ? new Gee.ArrayList.wrap(info.trash_folder_path.as_array()) - : empty) + steps != null ? steps : empty ); + steps = info.get_folder_steps_for_use(ARCHIVE); config.set_string_list( ARCHIVE_FOLDER_KEY, - (info.archive_folder_path != null - ? new Gee.ArrayList.wrap(info.archive_folder_path.as_array()) - : empty) + steps != null ? steps : empty ); config.set_bool(SAVE_DRAFTS_KEY, info.save_drafts); diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala index 9aa618dc..64a5b188 100644 --- a/src/engine/api/geary-account-information.vala +++ b/src/engine/api/geary-account-information.vala @@ -25,17 +25,6 @@ public class Geary.AccountInformation : BaseObject { return a.display_name.collate(b.display_name); } - public static Geary.FolderPath? build_folder_path(Gee.List? parts) { - if (parts == null || parts.size == 0) - return null; - - Geary.FolderPath path = new Imap.FolderRoot("#geary-config"); - foreach (string basename in parts) { - path = path.get_child(basename); - } - return path; - } - /** A unique (engine-wide), opaque identifier for the account. */ public string id { get; private set; } @@ -174,21 +163,6 @@ public class Geary.AccountInformation : BaseObject { /** Specifies the email sig to be appended to new messages. */ public string signature { get; set; default = ""; } - /** Draft special folder path. */ - public Geary.FolderPath? drafts_folder_path { get; set; default = null; } - - /** Sent special folder path. */ - public Geary.FolderPath? sent_folder_path { get; set; default = null; } - - /** Junk special folder path. */ - public Geary.FolderPath? junk_folder_path { get; set; default = null; } - - /** Trash special folder path. */ - public Geary.FolderPath? trash_folder_path { get; set; default = null; } - - /** Archive special folder path. */ - public Geary.FolderPath? archive_folder_path { get; set; default = null; } - /** * Location of the account's config directory. * @@ -205,6 +179,9 @@ public class Geary.AccountInformation : BaseObject { */ public File? data_dir { get; private set; default = null; } + private Gee.Map> special_use_paths = + new Gee.HashMap>(); + private Gee.List mailboxes { get; private set; default = new Gee.LinkedList(); @@ -287,11 +264,7 @@ public class Geary.AccountInformation : BaseObject { this.incoming = new ServiceInformation.copy(other.incoming); this.outgoing = new ServiceInformation.copy(other.outgoing); - this.drafts_folder_path = other.drafts_folder_path; - this.sent_folder_path = other.sent_folder_path; - this.junk_folder_path = other.junk_folder_path; - this.trash_folder_path = other.trash_folder_path; - this.archive_folder_path = other.archive_folder_path; + this.special_use_paths.set_all(other.special_use_paths); this.config_dir = other.config_dir; this.data_dir = other.data_dir; @@ -368,82 +341,81 @@ public class Geary.AccountInformation : BaseObject { this.mailboxes.set(index, mailbox); } - /** - * Returns the configured path for a special folder use. - * - * This is used when Geary has found or created a special folder - * for this account. The path will be null if Geary has always - * been told about the special folders by the server, and hasn't - * had to go looking for them. Only the ARCHIVE, DRAFTS, SENT, - * JUNK, and TRASH special folder types are valid to pass to this - * function. + /** + * Returns the folder path steps configured for a specific use. */ - public Geary.FolderPath? get_special_folder_path(Folder.SpecialUse special) { - switch (special) { - case DRAFTS: - return this.drafts_folder_path; - - case SENT: - return this.sent_folder_path; - - case JUNK: - return this.junk_folder_path; - - case TRASH: - return this.trash_folder_path; - - case ARCHIVE: - return this.archive_folder_path; + public Gee.List get_folder_steps_for_use(Folder.SpecialUse use) { + var steps = this.special_use_paths.get(use); + if (steps != null) { + steps = steps.read_only_view; + } else { + steps = Gee.List.empty(); } - - return null; + return steps; } /** - * Sets the configured path for a special folder type. - * - * This is only obeyed if the server doesn't tell Geary which - * folders are special. Only the DRAFTS, SENT, JUNK, TRASH and - * ARCHIVE special folder types are valid to pass to this - * function. + * Sets the configured folder path steps for a specific use. */ - public void set_special_folder_path(Folder.SpecialUse special, - FolderPath? new_path) { - Geary.FolderPath? old_path = null; - switch (special) { - case DRAFTS: - old_path = this.drafts_folder_path; - this.drafts_folder_path = new_path; - break; - - case SENT: - old_path = this.sent_folder_path; - this.sent_folder_path = new_path; - break; - - case JUNK: - old_path = this.junk_folder_path; - this.junk_folder_path = new_path; - break; - - case TRASH: - old_path = this.trash_folder_path; - this.trash_folder_path = new_path; - break; - - case ARCHIVE: - old_path = this.archive_folder_path; - this.archive_folder_path = new_path; - break; + public void set_folder_steps_for_use(Folder.SpecialUse special, + Gee.List? new_path) { + var existing = this.special_use_paths.get(special); + if (new_path != null && !new_path.is_empty) { + this.special_use_paths.set(special, new_path); + } else { + this.special_use_paths.unset(special); } - - if ((old_path == null && new_path != null) || - (old_path != null && new_path == null) || - (old_path != null && !old_path.equal_to(new_path))) { + if ((existing == null && new_path != null) || + (existing != null && new_path == null) || + (existing != null && + (existing.size != new_path.size || + existing.contains_all(new_path)))) { changed(); } } + /** + * Returns a folder path based on the configured steps for a use. + */ + public FolderPath? new_folder_path_for_use(FolderRoot root, + Folder.SpecialUse use) { + FolderPath? path = null; + var steps = this.special_use_paths.get(use); + if (steps != null) { + path = root; + foreach (string step in steps) { + path = path.get_child(step); + } + } + return path; + } + + /** + * Returns the configured special folder use for a given path. + */ + public Folder.SpecialUse get_folder_use_for_path(FolderPath path) { + var path_steps = path.as_array(); + var use = Folder.SpecialUse.NONE; + foreach (var entry in this.special_use_paths.entries) { + var use_steps = entry.value; + var found = false; + if (path_steps.length == use_steps.size) { + found = true; + for (int i = path_steps.length - 1; i >= 0; i--) { + if (path_steps[i] != use_steps[i]) { + found = false; + break; + } + } + } + if (found) { + use = entry.key; + break; + } + } + return use; + } + /** * Returns the best credentials to use for the outgoing service. * @@ -539,11 +511,8 @@ public class Geary.AccountInformation : BaseObject { this.signature == other.signature && this.incoming.equal_to(other.incoming) && this.outgoing.equal_to(other.outgoing) && - this.drafts_folder_path == other.drafts_folder_path && - this.sent_folder_path == other.sent_folder_path && - this.junk_folder_path == other.junk_folder_path && - this.trash_folder_path == other.trash_folder_path && - this.archive_folder_path == other.archive_folder_path && + this.special_use_paths.size == other.special_use_paths.size && + this.special_use_paths.has_all(other.special_use_paths) && this.config_dir == other.config_dir && this.data_dir == other.data_dir ) diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 2ca9c39d..d036d564 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -654,18 +654,16 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { throws GLib.Error { Folder? special = get_special_folder(use); if (special == null) { - FolderPath? path = information.get_special_folder_path(use); - if (path != null) { - if (!remote.is_folder_path_valid(path)) { - warning( - "Ignoring bad special folder path '%s' for type %s", - path.to_string(), - use.to_string() - ); - path = null; - } else { - path = this.local.imap_folder_root.copy(path); - } + FolderPath? path = information.new_folder_path_for_use( + this.local.imap_folder_root, use + ); + if (path != null && !remote.is_folder_path_valid(path)) { + warning( + "Ignoring bad special folder path '%s' for type %s", + path.to_string(), + use.to_string() + ); + path = null; } if (path == null) { @@ -691,7 +689,9 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { debug("Guessed folder \'%s\' for special_path %s", path.to_string(), use.to_string() ); - information.set_special_folder_path(use, path); + information.set_folder_steps_for_use( + use, new Gee.ArrayList.wrap(path.as_array()) + ); } if (!this.folder_map.has_key(path)) { diff --git a/test/client/accounts/accounts-manager-test.vala b/test/client/accounts/accounts-manager-test.vala index 0495133f..9f2d57ca 100644 --- a/test/client/accounts/accounts-manager-test.vala +++ b/test/client/accounts/accounts-manager-test.vala @@ -40,15 +40,9 @@ class Accounts.ManagerTest : TestCase { // engine without creating all of these dirs. this.tmp = GLib.File.new_for_path( - GLib.DirUtils.make_tmp("geary-engine-test-XXXXXX") + GLib.DirUtils.make_tmp("accounts-manager-test-XXXXXX") ); - GLib.File config = this.tmp.get_child("config"); - config.make_directory(); - - GLib.File data = this.tmp.get_child("data"); - data.make_directory(); - this.primary_mailbox = new Geary.RFC822.MailboxAddress( null, "test1@example.com" ); @@ -60,7 +54,7 @@ class Accounts.ManagerTest : TestCase { this.mediator, this.primary_mailbox ); - this.test = new Manager(this.mediator, config, data); + this.test = new Manager(this.mediator, this.tmp, this.tmp); } public override void tear_down() throws GLib.Error { @@ -169,12 +163,13 @@ class Accounts.ManagerTest : TestCase { this.account.save_sent = false; this.account.signature = "blarg"; this.account.use_signature = false; - Accounts.AccountConfigV1 config = new Accounts.AccountConfigV1(false); Geary.ConfigFile file = - new Geary.ConfigFile(this.tmp.get_child("config")); + new Geary.ConfigFile(this.tmp.get_child("config.ini")); + Accounts.AccountConfigV1 config = new Accounts.AccountConfigV1(false); config.save(this.account, file); + Geary.AccountInformation copy = config.load( file, TEST_ID, this.mediator, null, null ); @@ -194,7 +189,7 @@ class Accounts.ManagerTest : TestCase { new Accounts.AccountConfigLegacy(); Geary.ConfigFile file = - new Geary.ConfigFile(this.tmp.get_child("config")); + new Geary.ConfigFile(this.tmp.get_child("config.ini")); config.save(this.account, file); Geary.AccountInformation copy = config.load( @@ -221,7 +216,7 @@ class Accounts.ManagerTest : TestCase { Geary.Credentials.Requirement.NONE; Accounts.ServiceConfigV1 config = new Accounts.ServiceConfigV1(); Geary.ConfigFile file = - new Geary.ConfigFile(this.tmp.get_child("config")); + new Geary.ConfigFile(this.tmp.get_child("config.ini")); config.save(this.account, this.account.outgoing, file); config.load(file, copy, copy.outgoing); @@ -246,7 +241,7 @@ class Accounts.ManagerTest : TestCase { Geary.Credentials.Requirement.NONE; Accounts.ServiceConfigLegacy config = new Accounts.ServiceConfigLegacy(); Geary.ConfigFile file = - new Geary.ConfigFile(this.tmp.get_child("config")); + new Geary.ConfigFile(this.tmp.get_child("config.ini")); config.save(this.account, this.account.outgoing, file); config.load(file, copy, copy.outgoing);