From faf7a2bdfdb0685eaeac3b6a8bf8ce84c87f7aa2 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 11:01:03 +1100 Subject: [PATCH 01/12] Add some unit tests for Geary.ImapDb.Account folder management --- src/engine/imap-db/imap-db-account.vala | 36 +-- .../imap-engine-generic-account.vala | 2 +- test/engine/imap-db/imap-db-account-test.vala | 305 ++++++++++++++++++ test/meson.build | 1 + test/test-case.vala | 23 ++ test/test-engine.vala | 1 + 6 files changed, 344 insertions(+), 24 deletions(-) create mode 100644 test/engine/imap-db/imap-db-account-test.vala diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index 6b1db5e6..0353fd2d 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -326,18 +326,7 @@ private class Geary.ImapDB.Account : BaseObject { throw err; } - - Geary.Account account; - try { - account = Geary.Engine.instance.get_account_instance(account_information); - } catch (Error e) { - // If they're opening an account, the engine should already be - // open, and there should be no reason for this to fail. Thus, if - // we get here, it's a programmer error. - - error("Error finding account from its information: %s", e.message); - } - + background_cancellable = new Cancellable(); // Kick off a background update of the search table, but since the database is getting @@ -419,21 +408,23 @@ private class Geary.ImapDB.Account : BaseObject { return yield fetch_folder_async(path, cancellable); } - public async void delete_folder_async(Geary.Folder folder, Cancellable? cancellable) - throws Error { + public async void delete_folder_async(Geary.FolderPath path, + GLib.Cancellable? cancellable) + throws GLib.Error { check_open(); - - Geary.FolderPath path = folder.path; - yield db.exec_transaction_async(Db.TransactionType.RW, (cx) => { int64 folder_id; do_fetch_folder_id(cx, path, false, out folder_id, cancellable); - if (folder_id == Db.INVALID_ROWID) - return Db.TransactionOutcome.ROLLBACK; - + if (folder_id == Db.INVALID_ROWID) { + throw new EngineError.NOT_FOUND( + "Folder not found: %s", path.to_string() + ); + } + if (do_has_children(cx, folder_id, cancellable)) { - debug("Can't delete folder %s because it has children", folder.to_string()); - return Db.TransactionOutcome.ROLLBACK; + throw new ImapError.NOT_SUPPORTED( + "Folder has children: %s", path.to_string() + ); } do_delete_folder(cx, folder_id, cancellable); @@ -441,7 +432,6 @@ private class Geary.ImapDB.Account : BaseObject { return Db.TransactionOutcome.COMMIT; }, cancellable); - } private void initialize_contacts(Cancellable? cancellable = null) throws Error { diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 08cc810a..7276ca9f 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -1268,7 +1268,7 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { foreach (Geary.Folder folder in removed) { try { debug("Locally deleting removed folder %s", folder.to_string()); - yield local.delete_folder_async(folder, cancellable); + yield local.delete_folder_async(folder.path, cancellable); } catch (Error e) { debug("Unable to locally delete removed folder %s: %s", folder.to_string(), e.message); } diff --git a/test/engine/imap-db/imap-db-account-test.vala b/test/engine/imap-db/imap-db-account-test.vala new file mode 100644 index 00000000..146d7127 --- /dev/null +++ b/test/engine/imap-db/imap-db-account-test.vala @@ -0,0 +1,305 @@ +/* + * Copyright 2019 Michael Gratton + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + + +class Geary.ImapDB.AccountTest : TestCase { + + + private GLib.File? tmp_dir = null; + private Geary.AccountInformation? config = null; + private Account? account = null; + + + public AccountTest() { + base("Geary.ImapDB.AccountTest"); + add_test("create_base_folder", create_base_folder); + add_test("create_child_folder", create_child_folder); + add_test("list_folders", list_folders); + add_test("delete_folder", delete_folder); + add_test("delete_folder_with_child", delete_folder_with_child); + add_test("delete_nonexistent_folder", delete_nonexistent_folder); + add_test("fetch_base_folder", fetch_base_folder); + add_test("fetch_child_folder", fetch_child_folder); + add_test("fetch_nonexistent_folder", fetch_nonexistent_folder); + } + + public override void set_up() throws GLib.Error { + this.tmp_dir = GLib.File.new_for_path( + GLib.DirUtils.make_tmp("geary-db-database-test-XXXXXX") + ); + + this.config = new Geary.AccountInformation( + "test", + ServiceProvider.OTHER, + new MockCredentialsMediator(), + new Geary.RFC822.MailboxAddress(null, "test@example.com") + ); + + this.account = new Account(config); + this.account.open_async.begin( + this.tmp_dir, + GLib.File.new_for_path(_SOURCE_ROOT_DIR).get_child("sql"), + null, + (obj, ret) => { async_complete(ret); } + ); + this.account.open_async.end(async_result()); + } + + public override void tear_down() throws GLib.Error { + this.account.close_async.begin( + null, + (obj, ret) => { async_complete(ret); } + ); + this.account.close_async.end(async_result()); + + delete_file(this.tmp_dir); + this.tmp_dir = null; + } + + public void create_base_folder() throws GLib.Error { + Imap.Folder folder = new Imap.Folder( + new Imap.FolderRoot("test"), + new Imap.FolderProperties.selectable( + new Imap.MailboxAttributes( + Gee.Collection.empty() + ), + new Imap.StatusData( + new Imap.MailboxSpecifier("test"), + 10, // total + 9, // recent + new Imap.UID(8), + new Imap.UIDValidity(7), + 6 //unseen + ), + new Imap.Capabilities(1) + ) + ); + + this.account.clone_folder_async.begin( + folder, + null, + (obj, ret) => { async_complete(ret); } + ); + this.account.clone_folder_async.end(async_result()); + + Geary.Db.Result result = this.account.db.query( + "SELECT * FROM FolderTable;" + ); + assert_false(result.finished, "Folder not created"); + assert_string("test", result.string_for("name"), "Folder name"); + assert_true(result.is_null_for("parent_id"), "Folder parent"); + assert_false(result.next(), "Multiple rows inserted"); + } + + public void create_child_folder() throws GLib.Error { + this.account.db.exec( + "INSERT INTO FolderTable (id, name) VALUES (1, 'test');" + ); + + Imap.Folder folder = new Imap.Folder( + new Imap.FolderRoot("test").get_child("child"), + new Imap.FolderProperties.selectable( + new Imap.MailboxAttributes( + Gee.Collection.empty() + ), + new Imap.StatusData( + new Imap.MailboxSpecifier("test>child"), + 10, // total + 9, // recent + new Imap.UID(8), + new Imap.UIDValidity(7), + 6 //unseen + ), + new Imap.Capabilities(1) + ) + ); + + this.account.clone_folder_async.begin( + folder, + null, + (obj, ret) => { async_complete(ret); } + ); + this.account.clone_folder_async.end(async_result()); + + Geary.Db.Result result = this.account.db.query( + "SELECT * FROM FolderTable WHERE id != 1;" + ); + assert_false(result.finished, "Folder not created"); + assert_string("child", result.string_for("name"), "Folder name"); + assert_int(1, result.int_for("parent_id"), "Folder parent"); + assert_false(result.next(), "Multiple rows inserted"); + } + + public void list_folders() throws GLib.Error { + this.account.db.exec(""" + INSERT INTO FolderTable (id, name, parent_id) + VALUES + (1, 'test1', null), + (2, 'test2', 1), + (3, 'test3', 2); + """); + + this.account.list_folders_async.begin( + null, + null, + (obj, ret) => { async_complete(ret); } + ); + Gee.Collection result = + this.account.list_folders_async.end(async_result()); + + Folder test1 = traverse(result).first(); + assert_int(1, result.size, "Base folder not listed"); + assert_string("test1", test1.get_path().basename, "Base folder name"); + + this.account.list_folders_async.begin( + test1.get_path(), + null, + (obj, ret) => { async_complete(ret); } + ); + result = this.account.list_folders_async.end(async_result()); + + Folder test2 = traverse(result).first(); + assert_int(1, result.size, "Child folder not listed"); + assert_string("test2", test2.get_path().basename, "Child folder name"); + + this.account.list_folders_async.begin( + test2.get_path(), + null, + (obj, ret) => { async_complete(ret); } + ); + result = this.account.list_folders_async.end(async_result()); + + Folder test3 = traverse(result).first(); + assert_int(1, result.size, "Grandchild folder not listed"); + assert_string("test3", test3.get_path().basename, "Grandchild folder name"); + } + + public void delete_folder() throws GLib.Error { + this.account.db.exec(""" + INSERT INTO FolderTable (id, name, parent_id) + VALUES + (1, 'test1', null), + (2, 'test2', 1); + """); + + this.account.delete_folder_async.begin( + new Imap.FolderRoot("test1").get_child("test2"), + null, + (obj, ret) => { async_complete(ret); } + ); + this.account.delete_folder_async.end(async_result()); + + this.account.delete_folder_async.begin( + new Imap.FolderRoot("test1"), + null, + (obj, ret) => { async_complete(ret); } + ); + this.account.delete_folder_async.end(async_result()); + } + + public void delete_folder_with_child() throws GLib.Error { + this.account.db.exec(""" + INSERT INTO FolderTable (id, name, parent_id) + VALUES + (1, 'test1', null), + (2, 'test2', 1); + """); + + this.account.delete_folder_async.begin( + new Imap.FolderRoot("test1"), + null, + (obj, ret) => { async_complete(ret); } + ); + try { + this.account.delete_folder_async.end(async_result()); + assert_not_reached(); + } catch (GLib.Error err) { + assert_error(new ImapError.NOT_SUPPORTED(""), err); + } + } + + public void delete_nonexistent_folder() throws GLib.Error { + this.account.db.exec(""" + INSERT INTO FolderTable (id, name, parent_id) + VALUES + (1, 'test1', null), + (2, 'test2', 1); + """); + + this.account.delete_folder_async.begin( + new Imap.FolderRoot("test3"), + null, + (obj, ret) => { async_complete(ret); } + ); + try { + this.account.delete_folder_async.end(async_result()); + assert_not_reached(); + } catch (GLib.Error err) { + assert_error(new EngineError.NOT_FOUND(""), err); + } + } + + public void fetch_base_folder() throws GLib.Error { + this.account.db.exec(""" + INSERT INTO FolderTable (id, name, parent_id) + VALUES + (1, 'test1', null), + (2, 'test2', 1); + """); + + this.account.fetch_folder_async.begin( + new Imap.FolderRoot("test1"), + null, + (obj, ret) => { async_complete(ret); } + ); + + Folder? result = this.account.fetch_folder_async.end(async_result()); + assert_non_null(result); + assert_string("test1", result.get_path().basename); + } + + public void fetch_child_folder() throws GLib.Error { + this.account.db.exec(""" + INSERT INTO FolderTable (id, name, parent_id) + VALUES + (1, 'test1', null), + (2, 'test2', 1); + """); + + this.account.fetch_folder_async.begin( + new Imap.FolderRoot("test1").get_child("test2"), + null, + (obj, ret) => { async_complete(ret); } + ); + + Folder? result = this.account.fetch_folder_async.end(async_result()); + assert_non_null(result); + assert_string("test2", result.get_path().basename); + } + + public void fetch_nonexistent_folder() throws GLib.Error { + this.account.db.exec(""" + INSERT INTO FolderTable (id, name, parent_id) + VALUES + (1, 'test1', null), + (2, 'test2', 1); + """); + + this.account.fetch_folder_async.begin( + new Imap.FolderRoot("test3"), + null, + (obj, ret) => { async_complete(ret); } + ); + try { + this.account.fetch_folder_async.end(async_result()); + assert_not_reached(); + } catch (GLib.Error err) { + assert_error(new EngineError.NOT_FOUND(""), err); + } + } + +} diff --git a/test/meson.build b/test/meson.build index 26e588ba..c8f45530 100644 --- a/test/meson.build +++ b/test/meson.build @@ -36,6 +36,7 @@ geary_test_engine_sources = [ 'engine/imap/parameter/imap-list-parameter-test.vala', 'engine/imap/response/imap-namespace-response-test.vala', 'engine/imap/transport/imap-deserializer-test.vala', + 'engine/imap-db/imap-db-account-test.vala', 'engine/imap-db/imap-db-attachment-test.vala', 'engine/imap-db/imap-db-database-test.vala', 'engine/imap-engine/account-processor-test.vala', diff --git a/test/test-case.vala b/test/test-case.vala index 166bf324..b90de697 100644 --- a/test/test-case.vala +++ b/test/test-case.vala @@ -152,6 +152,27 @@ private inline void print_assert(string message, string? context) { GLib.stderr.putc('\n'); } +public void delete_file(File parent) throws GLib.Error { + FileInfo info = parent.query_info( + "standard::*", + FileQueryInfoFlags.NOFOLLOW_SYMLINKS + ); + + if (info.get_file_type () == FileType.DIRECTORY) { + FileEnumerator enumerator = parent.enumerate_children( + "standard::*", + FileQueryInfoFlags.NOFOLLOW_SYMLINKS + ); + + info = null; + while (((info = enumerator.next_file()) != null)) { + delete_file(parent.get_child(info.get_name())); + } + } + + parent.delete(); +} + public abstract class TestCase : Object { @@ -304,5 +325,7 @@ public abstract class TestCase : Object { assert_no_error(err); } } + } + } diff --git a/test/test-engine.vala b/test/test-engine.vala index d5f1e546..3c2309d8 100644 --- a/test/test-engine.vala +++ b/test/test-engine.vala @@ -45,6 +45,7 @@ int main(string[] args) { engine.add_suite(new Geary.Imap.ListParameterTest().get_suite()); engine.add_suite(new Geary.Imap.MailboxSpecifierTest().get_suite()); engine.add_suite(new Geary.Imap.NamespaceResponseTest().get_suite()); + engine.add_suite(new Geary.ImapDB.AccountTest().get_suite()); engine.add_suite(new Geary.ImapDB.AttachmentTest().get_suite()); engine.add_suite(new Geary.ImapDB.AttachmentIoTest().get_suite()); engine.add_suite(new Geary.ImapDB.DatabaseTest().get_suite()); From 4fefda9695299dcfc69382d90cf6d04c6ccec1b6 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 11:05:54 +1100 Subject: [PATCH 02/12] Remove unwanted debuging cruft --- src/client/accounts/accounts-manager.vala | 7 ------- .../imap-engine/imap-engine-account-synchronizer.vala | 1 - 2 files changed, 8 deletions(-) diff --git a/src/client/accounts/accounts-manager.vala b/src/client/accounts/accounts-manager.vala index e906128a..96167748 100644 --- a/src/client/accounts/accounts-manager.vala +++ b/src/client/accounts/accounts-manager.vala @@ -599,9 +599,6 @@ public class Accounts.Manager : GLib.Object { try { services.load(config, account, account.incoming); services.load(config, account, account.outgoing); - - debug("IMAP host name: %s", account.incoming.host); - } catch (GLib.KeyFileError err) { throw new ConfigError.SYNTAX(err.message); } @@ -1456,8 +1453,6 @@ public class Accounts.ServiceConfigLegacy : ServiceConfig, GLib.Object { Geary.ConfigFile.Group service_config = config.get_group(AccountConfigLegacy.GROUP); - debug("Loading..."); - string prefix = service.protocol == Geary.Protocol.IMAP ? "imap_" : "smtp_"; @@ -1479,8 +1474,6 @@ public class Accounts.ServiceConfigLegacy : ServiceConfig, GLib.Object { prefix + PORT, service.port ); - debug("Host name: %s", service.host); - bool use_tls = service_config.get_bool( prefix + SSL, service.protocol == Geary.Protocol.IMAP ); diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index abe1fafe..ed0f10ef 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -42,7 +42,6 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject { // we do require that for syncing at the moment anyway, // but keep the tests in for that one glorious day where // we can just use a generic folder. - debug("Is folder \"%s\" openable: %s", folder.path.to_string(), folder.properties.is_openable.to_string()); MinimalFolder? imap_folder = folder as MinimalFolder; if (imap_folder != null && folder.properties.is_openable.is_possible() && From b0f85b3af8daf0521fcd2303b66b963c667a1ad8 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 11:17:19 +1100 Subject: [PATCH 03/12] Ensure accounts don't accidentially create multiple inbox-type folders --- .../gmail/imap-engine-gmail-account.vala | 33 +++++++++++-------- .../other/imap-engine-other-account.vala | 14 +++++--- .../outlook/imap-engine-outlook-account.vala | 31 +++++++++-------- 3 files changed, 48 insertions(+), 30 deletions(-) diff --git a/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala b/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala index 0857352c..52127abc 100644 --- a/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala +++ b/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala @@ -1,7 +1,9 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ private class Geary.ImapEngine.GmailAccount : Geary.ImapEngine.GenericAccount { @@ -44,26 +46,31 @@ private class Geary.ImapEngine.GmailAccount : Geary.ImapEngine.GenericAccount { } protected override MinimalFolder new_folder(ImapDB.Folder local_folder) { - Geary.FolderPath path = local_folder.get_path(); - SpecialFolderType special_folder_type; - if (Imap.MailboxSpecifier.folder_path_is_inbox(path)) - special_folder_type = SpecialFolderType.INBOX; - else - special_folder_type = local_folder.get_properties().attrs.get_special_folder_type(); + FolderPath path = local_folder.get_path(); + SpecialFolderType type; + if (Imap.MailboxSpecifier.folder_path_is_inbox(path)) { + type = SpecialFolderType.INBOX; + } else { + type = local_folder.get_properties().attrs.get_special_folder_type(); + // There can be only one Inbox + if (type == SpecialFolderType.INBOX) { + type = SpecialFolderType.NONE; + } + } - switch (special_folder_type) { + switch (type) { case SpecialFolderType.ALL_MAIL: - return new GmailAllMailFolder(this, local_folder, special_folder_type); + return new GmailAllMailFolder(this, local_folder, type); case SpecialFolderType.DRAFTS: - return new GmailDraftsFolder(this, local_folder, special_folder_type); + return new GmailDraftsFolder(this, local_folder, type); case SpecialFolderType.SPAM: case SpecialFolderType.TRASH: - return new GmailSpamTrashFolder(this, local_folder, special_folder_type); + return new GmailSpamTrashFolder(this, local_folder, type); default: - return new GmailFolder(this, local_folder, special_folder_type); + return new GmailFolder(this, local_folder, type); } } diff --git a/src/engine/imap-engine/other/imap-engine-other-account.vala b/src/engine/imap-engine/other/imap-engine-other-account.vala index e3eb94a6..7e5d496f 100644 --- a/src/engine/imap-engine/other/imap-engine-other-account.vala +++ b/src/engine/imap-engine/other/imap-engine-other-account.vala @@ -1,8 +1,9 @@ /* * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ private class Geary.ImapEngine.OtherAccount : Geary.ImapEngine.GenericAccount { @@ -15,12 +16,17 @@ private class Geary.ImapEngine.OtherAccount : Geary.ImapEngine.GenericAccount { } protected override MinimalFolder new_folder(ImapDB.Folder local_folder) { - Geary.FolderPath path = local_folder.get_path(); + FolderPath path = local_folder.get_path(); SpecialFolderType type; - if (Imap.MailboxSpecifier.folder_path_is_inbox(path)) + if (Imap.MailboxSpecifier.folder_path_is_inbox(path)) { type = SpecialFolderType.INBOX; - else + } else { type = local_folder.get_properties().attrs.get_special_folder_type(); + // There can be only one Inbox + if (type == SpecialFolderType.INBOX) { + type = SpecialFolderType.NONE; + } + } return new OtherFolder(this, local_folder, type); } diff --git a/src/engine/imap-engine/outlook/imap-engine-outlook-account.vala b/src/engine/imap-engine/outlook/imap-engine-outlook-account.vala index 2c09c3e5..8d7e162d 100644 --- a/src/engine/imap-engine/outlook/imap-engine-outlook-account.vala +++ b/src/engine/imap-engine/outlook/imap-engine-outlook-account.vala @@ -1,7 +1,9 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ private class Geary.ImapEngine.OutlookAccount : Geary.ImapEngine.GenericAccount { @@ -32,19 +34,22 @@ private class Geary.ImapEngine.OutlookAccount : Geary.ImapEngine.GenericAccount } protected override MinimalFolder new_folder(ImapDB.Folder local_folder) { - // use the Folder's attributes to determine if it's a special folder type, unless it's - // INBOX; that's determined by name - Geary.FolderPath path = local_folder.get_path(); - SpecialFolderType special_folder_type; - if (Imap.MailboxSpecifier.folder_path_is_inbox(path)) - special_folder_type = SpecialFolderType.INBOX; - else - special_folder_type = local_folder.get_properties().attrs.get_special_folder_type(); + FolderPath path = local_folder.get_path(); + SpecialFolderType type; + if (Imap.MailboxSpecifier.folder_path_is_inbox(path)) { + type = SpecialFolderType.INBOX; + } else { + type = local_folder.get_properties().attrs.get_special_folder_type(); + // There can be only one Inbox + if (type == SpecialFolderType.INBOX) { + type = SpecialFolderType.NONE; + } + } - if (special_folder_type == Geary.SpecialFolderType.DRAFTS) - return new OutlookDraftsFolder(this, local_folder, special_folder_type); + if (type == Geary.SpecialFolderType.DRAFTS) + return new OutlookDraftsFolder(this, local_folder, type); - return new OutlookFolder(this, local_folder, special_folder_type); + return new OutlookFolder(this, local_folder, type); } } From 46bb4d1b6cf90f120cd77ff1fcdae3596b4659fc Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 11:29:27 +1100 Subject: [PATCH 04/12] Ensure MinimalFolder remote open forces closed on hard errors Minimal folder should force a folder closed when an error occurrs that it isn't going to be able to recover from. --- .../imap-engine-minimal-folder.vala | 18 +++-- .../imap-engine/imap-engine-replay-queue.vala | 8 +- src/engine/imap-engine/imap-engine.vala | 79 +++++++++++-------- 3 files changed, 61 insertions(+), 44 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala b/src/engine/imap-engine/imap-engine-minimal-folder.vala index ed3d9f09..e9204bba 100644 --- a/src/engine/imap-engine/imap-engine-minimal-folder.vala +++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala @@ -960,12 +960,18 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport ); return; } catch (Error err) { - debug("Other error: %s", err.message); - // Notify that there was a connection error, but don't - // force the folder closed, since it might come good again - // if the user fixes an auth problem or the network comes - // back or whatever. - notify_open_failed(Folder.OpenFailed.REMOTE_ERROR, err); + ErrorContext context = new ErrorContext(err); + if (is_unrecoverable_failure(err)) { + debug("Unrecoverable failure opening remote, forcing closed: %s", + context.format_full_error()); + yield force_close( + CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_ERROR + ); + } else { + debug("Recoverable error opening remote: %s", + context.format_full_error()); + notify_open_failed(Folder.OpenFailed.REMOTE_ERROR, err); + } return; } diff --git a/src/engine/imap-engine/imap-engine-replay-queue.vala b/src/engine/imap-engine/imap-engine-replay-queue.vala index 7238ffa0..8d5e2e61 100644 --- a/src/engine/imap-engine/imap-engine-replay-queue.vala +++ b/src/engine/imap-engine/imap-engine-replay-queue.vala @@ -519,11 +519,11 @@ private class Geary.ImapEngine.ReplayQueue : Geary.BaseObject { } catch (Error replay_err) { debug("Replay remote error for %s on %s: %s (%s)", op.to_string(), to_string(), replay_err.message, op.on_remote_error.to_string()); - - // If a hard failure and operation allows remote replay and not closing, - // re-schedule now + + // If a recoverable failure and operation allows + // remote replay and not closing, re-schedule now if ((op.on_remote_error == ReplayOperation.OnError.RETRY) - && is_hard_failure(replay_err) + && !is_unrecoverable_failure(replay_err) && state == State.OPEN) { debug("Schedule op retry %s on %s", op.to_string(), to_string()); diff --git a/src/engine/imap-engine/imap-engine.vala b/src/engine/imap-engine/imap-engine.vala index 5bad3088..998bb1b8 100644 --- a/src/engine/imap-engine/imap-engine.vala +++ b/src/engine/imap-engine/imap-engine.vala @@ -6,40 +6,51 @@ namespace Geary.ImapEngine { -/** - * A hard failure is defined as one due to hardware or connectivity issues, where a soft failure - * is due to software reasons, like credential failure or protocol violation. - */ -private static bool is_hard_failure(Error err) { - // CANCELLED is not a hard error - if (err is IOError.CANCELLED) - return false; + /** + * Determines if retrying an operation might succeed or not. + * + * A recoverable failure is defined as one that may not occur + * again if the operation that caused it is retried, without + * needing to make some change in the mean time. For example, + * recoverable failures may occur due to transient network + * connectivity issues or server rate limiting. On the other hand, + * an unrecoverable failure is due to some problem that will not + * succeed if tried again unless some action is taken, such as + * authentication failures, protocol parsing errors, and so on. + */ + private static bool is_unrecoverable_failure(GLib.Error err) { + return !( + err is EngineError.SERVER_UNAVAILABLE || + err is IOError.BROKEN_PIPE || + err is IOError.BUSY || + err is IOError.CONNECTION_CLOSED || + err is IOError.NOT_CONNECTED || + err is IOError.TIMED_OUT || + err is ImapError.NOT_CONNECTED || + err is ImapError.TIMED_OUT || + err is ImapError.UNAVAILABLE + ); + } - // Treat other errors -- most likely IOErrors -- as hard failures - if (!(err is ImapError) && !(err is EngineError)) - return true; - - return err is ImapError.NOT_CONNECTED - || err is ImapError.TIMED_OUT - || err is ImapError.SERVER_ERROR - || err is EngineError.SERVER_UNAVAILABLE; -} - -/** - * Determines if this IOError related to a remote host or not. - */ -private static bool is_remote_error(GLib.Error err) { - return err is ImapError - || err is IOError.CONNECTION_CLOSED - || err is IOError.CONNECTION_REFUSED - || err is IOError.HOST_UNREACHABLE - || err is IOError.MESSAGE_TOO_LARGE - || err is IOError.NETWORK_UNREACHABLE - || err is IOError.NOT_CONNECTED - || err is IOError.PROXY_AUTH_FAILED - || err is IOError.PROXY_FAILED - || err is IOError.PROXY_NEED_AUTH - || err is IOError.PROXY_NOT_ALLOWED; -} + /** + * Determines if an error was caused by the remote host or not. + */ + private static bool is_remote_error(GLib.Error err) { + return ( + err is EngineError.NOT_FOUND || + err is EngineError.SERVER_UNAVAILABLE || + err is IOError.CONNECTION_CLOSED || + err is IOError.CONNECTION_REFUSED || + err is IOError.HOST_UNREACHABLE || + err is IOError.MESSAGE_TOO_LARGE || + err is IOError.NETWORK_UNREACHABLE || + err is IOError.NOT_CONNECTED || + err is IOError.PROXY_AUTH_FAILED || + err is IOError.PROXY_FAILED || + err is IOError.PROXY_NEED_AUTH || + err is IOError.PROXY_NOT_ALLOWED || + err is ImapError + ); + } } From 41bb79da9426907551bc7086be10eb27162f4b00 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 12:05:29 +1100 Subject: [PATCH 05/12] Stop duplicate inboxes being created, not being listed Since duplicates are being cleand up on startup, this ensures dups never exist in the DB. --- src/engine/imap-db/imap-db-account.vala | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index 0353fd2d..f6f77f31 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -369,10 +369,19 @@ private class Geary.ImapDB.Account : BaseObject { // XXX this should really be a db table constraint Geary.ImapDB.Folder? folder = get_local_folder(path); - if (folder != null) + if (folder != null) { throw new EngineError.ALREADY_EXISTS( "Folder with path already exists: %s", path.to_string() ); + } + + if (Imap.MailboxSpecifier.folder_path_is_inbox(path) && + !Imap.MailboxSpecifier.is_canonical_inbox_name(path.basename)) { + // Don't add faux inboxes + throw new ImapError.NOT_SUPPORTED( + "Inbox has : %s", path.to_string() + ); + } yield db.exec_transaction_async(Db.TransactionType.RW, (cx) => { // get the parent of this folder, creating parents if necessary ... ok if this fails, @@ -507,15 +516,6 @@ private class Geary.ImapDB.Account : BaseObject { while (!result.finished) { string basename = result.string_for("name"); - // ignore anything that's not canonical Inbox - if (parent == null - && Imap.MailboxSpecifier.is_inbox_name(basename) - && !Imap.MailboxSpecifier.is_canonical_inbox_name(basename)) { - result.next(cancellable); - - continue; - } - Geary.FolderPath path = (parent != null) ? parent.get_child(basename) : new Imap.FolderRoot(basename); From 5a22e8e4a21420351b34109e898e25301f26b849 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 12:10:48 +1100 Subject: [PATCH 06/12] Convert Geary.FolderRoot to be an actual root, not just a top-level Instead of each top-level IMAP folder being a FolderRoot object, then children of that being FolderPath objects, this makes FolderRoot an "empty" FolderPath, so that both top-level and descendant folders are plain FolderPath objects. Aside from being more technically correct, this means that empty namespace roots can now be used interchangably with non-empty namespace roots (addressing issue #181), and custom folder implementations no longer need to provide their own trivial, custom FolderRoot. To support this, a notion of an IMAP root and a local root have been added from which all remote and local folder paths are now derived, existing places that assume top-level == root have been fixed, and unit tests have been added. --- po/POTFILES.in | 2 - .../folder-list-account-branch.vala | 2 +- src/engine/api/geary-account-information.vala | 7 +- src/engine/api/geary-folder-path.vala | 111 ++++++----- src/engine/imap-db/imap-db-account.vala | 180 +++++++++++------- .../search/imap-db-search-folder-root.vala | 14 -- .../imap-db/search/imap-db-search-folder.vala | 25 ++- .../gmail/imap-engine-gmail-account.vala | 3 +- .../imap-engine-gmail-search-folder.vala | 4 +- .../imap-engine-generic-account.vala | 51 +++-- .../yahoo/imap-engine-yahoo-account.vala | 27 ++- src/engine/imap/api/imap-account-session.vala | 53 ++++-- src/engine/imap/api/imap-folder-root.vala | 71 ++++--- .../imap/message/imap-mailbox-specifier.vala | 97 ++++++---- .../response/imap-mailbox-information.vala | 13 +- .../imap/transport/imap-client-session.vala | 9 +- src/engine/meson.build | 2 - src/engine/outbox/outbox-folder-root.vala | 18 -- src/engine/outbox/outbox-folder.vala | 22 ++- test/engine/api/geary-folder-path-mock.vala | 14 -- test/engine/api/geary-folder-path-test.vala | 41 ++++ .../app/app-conversation-monitor-test.vala | 14 +- .../engine/app/app-conversation-set-test.vala | 20 +- test/engine/app/app-conversation-test.vala | 21 +- test/engine/imap-db/imap-db-account-test.vala | 24 ++- .../message/imap-mailbox-specifier-test.vala | 85 ++++++--- test/meson.build | 2 +- test/test-engine.vala | 1 + 28 files changed, 589 insertions(+), 344 deletions(-) delete mode 100644 src/engine/imap-db/search/imap-db-search-folder-root.vala delete mode 100644 src/engine/outbox/outbox-folder-root.vala delete mode 100644 test/engine/api/geary-folder-path-mock.vala create mode 100644 test/engine/api/geary-folder-path-test.vala diff --git a/po/POTFILES.in b/po/POTFILES.in index 5a5b0e98..ca8fcc94 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -221,7 +221,6 @@ src/engine/imap-db/imap-db-message-addresses.vala src/engine/imap-db/imap-db-message-row.vala src/engine/imap-db/search/imap-db-search-email-identifier.vala src/engine/imap-db/search/imap-db-search-folder-properties.vala -src/engine/imap-db/search/imap-db-search-folder-root.vala src/engine/imap-db/search/imap-db-search-folder.vala src/engine/imap-db/search/imap-db-search-query.vala src/engine/imap-db/search/imap-db-search-term.vala @@ -346,7 +345,6 @@ src/engine/nonblocking/nonblocking-variants.vala src/engine/outbox/outbox-email-identifier.vala src/engine/outbox/outbox-email-properties.vala src/engine/outbox/outbox-folder-properties.vala -src/engine/outbox/outbox-folder-root.vala src/engine/outbox/outbox-folder.vala src/engine/rfc822/rfc822-error.vala src/engine/rfc822/rfc822-gmime-filter-blockquotes.vala diff --git a/src/client/folder-list/folder-list-account-branch.vala b/src/client/folder-list/folder-list-account-branch.vala index ac6a5457..6a87da02 100644 --- a/src/client/folder-list/folder-list-account-branch.vala +++ b/src/client/folder-list/folder-list-account-branch.vala @@ -90,7 +90,7 @@ public class FolderList.AccountBranch : Sidebar.Branch { // Special folders go in the root of the account. graft_point = get_root(); - } else if (folder.path.get_parent() == null) { + } else if (folder.path.is_top_level) { // Top-level folders get put in our special user folders group. graft_point = user_folder_group; diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala index 7e9d443c..edc867a1 100644 --- a/src/engine/api/geary-account-information.vala +++ b/src/engine/api/geary-account-information.vala @@ -29,9 +29,10 @@ public class Geary.AccountInformation : BaseObject { if (parts == null || parts.size == 0) return null; - Geary.FolderPath path = new Imap.FolderRoot(parts[0]); - for (int i = 1; i < parts.size; i++) - path = path.get_child(parts.get(i)); + Geary.FolderPath path = new Imap.FolderRoot(); + foreach (string basename in parts) { + path = path.get_child(basename); + } return path; } diff --git a/src/engine/api/geary-folder-path.vala b/src/engine/api/geary-folder-path.vala index 5193a638..4d398d99 100644 --- a/src/engine/api/geary-folder-path.vala +++ b/src/engine/api/geary-folder-path.vala @@ -15,11 +15,13 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, Gee.Comparable { + + /** * The name of this folder (without any child or parent names or delimiters). */ public string basename { get; private set; } - + /** * Whether this path is lexiographically case-sensitive. * @@ -27,16 +29,34 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, */ public bool case_sensitive { get; private set; } + /** + * Determines if this path is a root folder path. + */ + public virtual bool is_root { + get { return this.path == null || this.path.size == 0; } + } + + /** + * Determines if this path is a child of the root folder. + */ + public bool is_top_level { + get { + FolderPath? parent = get_parent(); + return parent != null && parent.is_root; + } + } + + private Gee.List? path = null; private uint stored_hash = uint.MAX; - - protected FolderPath(string basename, bool case_sensitive) { - assert(this is FolderRoot); - - this.basename = basename; - this.case_sensitive = case_sensitive; + + + /** Constructor only for use by {@link FolderRoot}. */ + internal FolderPath() { + this.basename = ""; + this.case_sensitive = false; } - + private FolderPath.child(Gee.List path, string basename, bool case_sensitive) { assert(path[0] is FolderRoot); @@ -44,19 +64,7 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, this.basename = basename; this.case_sensitive = case_sensitive; } - - /** - * Returns true if this {@link FolderPath} is a root folder. - * - * This means that the FolderPath ''should'' be castable into {@link FolderRoot}, which is - * enforced through the constructor and accessor styles of this class. However, this test - * merely checks if this FolderPath has any children. A GObject "is" operation is the - * reliable way to cast to FolderRoot. - */ - public bool is_root() { - return (path == null || path.size == 0); - } - + /** * Returns the {@link FolderRoot} of this path. */ @@ -127,24 +135,31 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, return list; } - + /** - * Creates a {@link FolderPath} object that is a child of this folder. + * Creates a path that is a child of this folder. * - * {@link Trillian.TRUE} and {@link Trillian.FALSE} force case-sensitivity. - * {@link Trillian.UNKNOWN} indicates to use {@link FolderRoot.default_case_sensitivity}. + * Specifying {@link Trillian.TRUE} or {@link Trillian.FALSE} for + * `is_case_sensitive` forces case-sensitivity either way. If + * {@link Trillian.UNKNOWN}, then {@link + * FolderRoot.default_case_sensitivity} is used. */ - public Geary.FolderPath get_child(string basename, Trillian child_case_sensitive = Trillian.UNKNOWN) { + public virtual Geary.FolderPath + get_child(string basename, + Trillian is_case_sensitive = Trillian.UNKNOWN) { // Build the child's path, which is this node's path plus this node Gee.List child_path = new Gee.ArrayList(); if (path != null) child_path.add_all(path); child_path.add(this); - - return new FolderPath.child(child_path, basename, - child_case_sensitive.to_boolean(get_root().default_case_sensitivity)); + + return new FolderPath.child( + child_path, + basename, + is_case_sensitive.to_boolean(get_root().default_case_sensitivity) + ); } - + /** * Returns true if the other {@link FolderPath} has the same parent as this one. * @@ -312,28 +327,36 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, } /** - * The root of a folder heirarchy. + * The root of a folder hierarchy. * - * A {@link FolderPath} can only be created by starting with a FolderRoot and adding children - * via {@link FolderPath.get_child}. Because all FolderPaths hold references to their parents, - * this element can be retrieved with {@link FolderPath.get_root}. - * - * Since each email system may have different requirements for its paths, this is an abstract - * class. + * A {@link FolderPath} can only be created by starting with a + * FolderRoot and adding children via {@link FolderPath.get_child}. + * Because all FolderPaths hold references to their parents, this + * element can be retrieved with {@link FolderPath.get_root}. */ -public abstract class Geary.FolderRoot : Geary.FolderPath { +public class Geary.FolderRoot : Geary.FolderPath { + + + /** {@inheritDoc} */ + public override bool is_root { + get { return true; } + } + /** - * The default case sensitivity of each element in the {@link FolderPath}. + * The default case sensitivity of descendant folders. * * @see FolderRoot.case_sensitive * @see FolderPath.get_child */ public bool default_case_sensitivity { get; private set; } - - protected FolderRoot(string basename, bool case_sensitive, bool default_case_sensitivity) { - base (basename, case_sensitive); - + + + /** + * Constructs a new folder root with given default sensitivity. + */ + public FolderRoot(bool default_case_sensitivity) { + base(); this.default_case_sensitivity = default_case_sensitivity; } -} +} diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index f6f77f31..2b67edd7 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -1,7 +1,9 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton . * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ private class Geary.ImapDB.Account : BaseObject { @@ -74,9 +76,22 @@ private class Geary.ImapDB.Account : BaseObject { public signal void contacts_loaded(); + /** + * The root path for all remote IMAP folders. + * + * No folder exists for this path locally or on the remote server, + * it merely exists to provide a common root for the paths of all + * IMAP folders. + * + * @see list_folders_async + */ + public Imap.FolderRoot imap_folder_root { + get; private set; default = new Imap.FolderRoot(); + } + // Only available when the Account is opened public ImapEngine.ContactStore contact_store { get; private set; } - public IntervalProgressMonitor search_index_monitor { get; private set; + public IntervalProgressMonitor search_index_monitor { get; private set; default = new IntervalProgressMonitor(ProgressType.SEARCH_INDEX, 0, 0); } public SimpleProgressMonitor upgrade_monitor { get; private set; default = new SimpleProgressMonitor( ProgressType.DB_UPGRADE); } @@ -476,11 +491,19 @@ private class Geary.ImapDB.Account : BaseObject { contacts_loaded(); } } - - public async Gee.Collection list_folders_async(Geary.FolderPath? parent, - Cancellable? cancellable = null) throws Error { + + /** + * Lists all children of a given folder. + * + * To list all top-level folders, pass in {@link imap_folder_root} + * as the parent. + */ + public async Gee.Collection + list_folders_async(Geary.FolderPath parent, + GLib.Cancellable? cancellable) + throws GLib.Error { check_open(); - + // TODO: A better solution here would be to only pull the FolderProperties if the Folder // object itself doesn't already exist Gee.HashMap id_map = new Gee.HashMap< @@ -489,17 +512,14 @@ private class Geary.ImapDB.Account : BaseObject { Geary.FolderPath, Geary.Imap.FolderProperties>(); yield db.exec_transaction_async(Db.TransactionType.RO, (cx) => { int64 parent_id = Db.INVALID_ROWID; - if (parent != null) { - if (!do_fetch_folder_id(cx, parent, false, out parent_id, cancellable)) { - debug("Unable to find folder ID for %s to list folders", parent.to_string()); - - return Db.TransactionOutcome.ROLLBACK; - } - - if (parent_id == Db.INVALID_ROWID) - throw new EngineError.NOT_FOUND("Folder %s not found", parent.to_string()); + if (!parent.is_root && + !do_fetch_folder_id( + cx, parent, false, out parent_id, cancellable + )) { + debug("Unable to find folder ID for \"%s\" to list folders", parent.to_string()); + return Db.TransactionOutcome.ROLLBACK; } - + Db.Statement stmt; if (parent_id != Db.INVALID_ROWID) { stmt = cx.prepare( @@ -511,15 +531,11 @@ private class Geary.ImapDB.Account : BaseObject { "SELECT id, name, last_seen_total, unread_count, last_seen_status_total, " + "uid_validity, uid_next, attributes FROM FolderTable WHERE parent_id IS NULL"); } - + Db.Result result = stmt.exec(cancellable); while (!result.finished) { string basename = result.string_for("name"); - - Geary.FolderPath path = (parent != null) - ? parent.get_child(basename) - : new Imap.FolderRoot(basename); - + Geary.FolderPath path = parent.get_child(basename); Geary.Imap.FolderProperties properties = new Geary.Imap.FolderProperties.from_imapdb( Geary.Imap.MailboxAttributes.deserialize(result.string_for("attributes")), result.int_for("last_seen_total"), @@ -544,12 +560,13 @@ private class Geary.ImapDB.Account : BaseObject { }, cancellable); assert(id_map.size == prop_map.size); - + if (id_map.size == 0) { - throw new EngineError.NOT_FOUND("No local folders in %s", - (parent != null) ? parent.to_string() : "root"); + throw new EngineError.NOT_FOUND( + "No local folders under \"%s\"", parent.to_string() + ); } - + Gee.Collection folders = new Gee.ArrayList(); foreach (Geary.FolderPath path in id_map.keys) { Geary.ImapDB.Folder? folder = get_local_folder(path); @@ -1555,23 +1572,32 @@ private class Geary.ImapDB.Account : BaseObject { folder_stmt.exec(cancellable); } - - // If the FolderPath has no parent, returns true and folder_id will be set to Db.INVALID_ROWID. - // If cannot create path or there is a logical problem traversing it, returns false with folder_id - // set to Db.INVALID_ROWID. - internal bool do_fetch_folder_id(Db.Connection cx, Geary.FolderPath path, bool create, out int64 folder_id, - Cancellable? cancellable) throws Error { - int length = path.get_path_length(); - if (length < 0) - throw new EngineError.BAD_PARAMETERS("Invalid path %s", path.to_string()); - + + // If the FolderPath has no parent, returns true and folder_id + // will be set to Db.INVALID_ROWID. If cannot create path or + // there is a logical problem traversing it, returns false with + // folder_id set to Db.INVALID_ROWID. + internal bool do_fetch_folder_id(Db.Connection cx, + Geary.FolderPath path, + bool create, + out int64 folder_id, + GLib.Cancellable? cancellable) + throws GLib.Error { + if (path.is_root) { + throw new EngineError.BAD_PARAMETERS( + "Cannot fetch folder for root path" + ); + } + + // Don't include the root since top-level folders are stored + // with no parent. + Gee.List parts = path.as_list(); + parts.remove_at(0); + folder_id = Db.INVALID_ROWID; int64 parent_id = Db.INVALID_ROWID; - - // walk the folder tree to the final node (which is at length - 1 - 1) - for (int ctr = 0; ctr < length; ctr++) { - string basename = path.get_folder_at(ctr).basename; - + + foreach (string basename in parts) { Db.Statement stmt; if (parent_id != Db.INVALID_ROWID) { stmt = cx.prepare("SELECT id FROM FolderTable WHERE parent_id=? AND name=?"); @@ -1616,19 +1642,28 @@ private class Geary.ImapDB.Account : BaseObject { return true; } - - // See do_fetch_folder_id() for return semantics. - internal bool do_fetch_parent_id(Db.Connection cx, Geary.FolderPath path, bool create, out int64 parent_id, - Cancellable? cancellable = null) throws Error { - if (path.is_root()) { + + internal bool do_fetch_parent_id(Db.Connection cx, + FolderPath path, + bool create, + out int64 parent_id, + GLib.Cancellable? cancellable = null) + throws GLib.Error { + // See do_fetch_folder_id() for return semantics + bool ret = true; + + // No folder for the root is saved in the database, so + // top-levels should not have a parent. + if (path.is_top_level) { parent_id = Db.INVALID_ROWID; - - return true; + } else { + ret = do_fetch_folder_id( + cx, path.get_parent(), create, out parent_id, cancellable + ); } - - return do_fetch_folder_id(cx, path.get_parent(), create, out parent_id, cancellable); + return ret; } - + private bool do_has_children(Db.Connection cx, int64 folder_id, Cancellable? cancellable) throws Error { Db.Statement stmt = cx.prepare("SELECT 1 FROM FolderTable WHERE parent_id = ?"); stmt.bind_rowid(0, folder_id); @@ -1700,8 +1735,12 @@ private class Geary.ImapDB.Account : BaseObject { // For a message row id, return a set of all folders it's in, or null if // it's not in any folders. - private static Gee.Set? do_find_email_folders(Db.Connection cx, int64 message_id, - bool include_removed, Cancellable? cancellable) throws Error { + private Gee.Set? + do_find_email_folders(Db.Connection cx, + int64 message_id, + bool include_removed, + GLib.Cancellable? cancellable) + throws GLib.Error { string sql = "SELECT folder_id FROM MessageLocationTable WHERE message_id=?"; if (!include_removed) sql += " AND remove_marker=0"; @@ -1724,16 +1763,20 @@ private class Geary.ImapDB.Account : BaseObject { return (folder_paths.size == 0 ? null : folder_paths); } - + // For a folder row id, return the folder path (constructed with default // separator and case sensitivity) of that folder, or null in the event // it's not found. - private static Geary.FolderPath? do_find_folder_path(Db.Connection cx, int64 folder_id, - Cancellable? cancellable) throws Error { - Db.Statement stmt = cx.prepare("SELECT parent_id, name FROM FolderTable WHERE id=?"); + private Geary.FolderPath? do_find_folder_path(Db.Connection cx, + int64 folder_id, + GLib.Cancellable? cancellable) + throws GLib.Error { + Db.Statement stmt = cx.prepare( + "SELECT parent_id, name FROM FolderTable WHERE id=?" + ); stmt.bind_int64(0, folder_id); Db.Result result = stmt.exec(cancellable); - + if (result.finished) return null; @@ -1746,12 +1789,19 @@ private class Geary.ImapDB.Account : BaseObject { folder_id.to_string(), parent_id.to_string()); return null; } - - if (parent_id <= 0) - return new Imap.FolderRoot(name); - - Geary.FolderPath? parent_path = do_find_folder_path(cx, parent_id, cancellable); - return (parent_path == null ? null : parent_path.get_child(name)); + + Geary.FolderPath? path = null; + if (parent_id <= 0) { + path = this.imap_folder_root.get_child(name); + } else { + Geary.FolderPath? parent_path = do_find_folder_path( + cx, parent_id, cancellable + ); + if (parent_path != null) { + path = parent_path.get_child(name); + } + } + return path; } private void on_unread_updated(ImapDB.Folder source, Gee.Map diff --git a/src/engine/imap-db/search/imap-db-search-folder-root.vala b/src/engine/imap-db/search/imap-db-search-folder-root.vala deleted file mode 100644 index 98be1d91..00000000 --- a/src/engine/imap-db/search/imap-db-search-folder-root.vala +++ /dev/null @@ -1,14 +0,0 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. - * - * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. - */ - -private class Geary.ImapDB.SearchFolderRoot : Geary.FolderRoot { - public const string MAGIC_BASENAME = "$GearySearchFolder$"; - - public SearchFolderRoot() { - base(MAGIC_BASENAME, false, false); - } -} - diff --git a/src/engine/imap-db/search/imap-db-search-folder.vala b/src/engine/imap-db/search/imap-db-search-folder.vala index 16b957fb..5f590635 100644 --- a/src/engine/imap-db/search/imap-db-search-folder.vala +++ b/src/engine/imap-db/search/imap-db-search-folder.vala @@ -5,23 +5,34 @@ */ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSupport.Remove { - // Max number of emails that can ever be in the folder. + + + /** Max number of emails that can ever be in the folder. */ public const int MAX_RESULT_EMAILS = 1000; - + + /** The canonical name of the search folder. */ + public const string MAGIC_BASENAME = "$GearySearchFolder$"; + private const Geary.SpecialFolderType[] exclude_types = { Geary.SpecialFolderType.SPAM, Geary.SpecialFolderType.TRASH, Geary.SpecialFolderType.DRAFTS, // Orphan emails (without a folder) are also excluded; see ctor. }; - + + private Gee.HashSet exclude_folders = new Gee.HashSet(); private Gee.TreeSet search_results; private Geary.Nonblocking.Mutex result_mutex = new Geary.Nonblocking.Mutex(); - - public SearchFolder(Geary.Account account) { - base (account, new SearchFolderProperties(0, 0), new SearchFolderRoot()); - + + + public SearchFolder(Geary.Account account, FolderRoot root) { + base( + account, + new SearchFolderProperties(0, 0), + root.get_child(MAGIC_BASENAME, Trillian.TRUE) + ); + account.folders_available_unavailable.connect(on_folders_available_unavailable); account.email_locally_complete.connect(on_email_locally_complete); account.email_removed.connect(on_account_email_removed); diff --git a/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala b/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala index 52127abc..f246e5b6 100644 --- a/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala +++ b/src/engine/imap-engine/gmail/imap-engine-gmail-account.vala @@ -75,6 +75,7 @@ private class Geary.ImapEngine.GmailAccount : Geary.ImapEngine.GenericAccount { } protected override SearchFolder new_search_folder() { - return new GmailSearchFolder(this); + return new GmailSearchFolder(this, this.local_folder_root); } + } diff --git a/src/engine/imap-engine/gmail/imap-engine-gmail-search-folder.vala b/src/engine/imap-engine/gmail/imap-engine-gmail-search-folder.vala index 9dea5b2a..ef47256d 100644 --- a/src/engine/imap-engine/gmail/imap-engine-gmail-search-folder.vala +++ b/src/engine/imap-engine/gmail/imap-engine-gmail-search-folder.vala @@ -12,8 +12,8 @@ private class Geary.ImapEngine.GmailSearchFolder : ImapDB.SearchFolder { private Geary.App.EmailStore email_store; - public GmailSearchFolder(Geary.Account account) { - base (account); + public GmailSearchFolder(Geary.Account account, FolderRoot root) { + base (account, root); this.email_store = new Geary.App.EmailStore(account); } diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 7276ca9f..dd3a08fc 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -1,6 +1,6 @@ /* * Copyright 2016 Software Freedom Conservancy Inc. - * Copyright 2017-2018 Michael Gratton . + * Copyright 2017-2019 Michael Gratton . * * This software is licensed under the GNU Lesser General Public License * (version 2.1 or later). See the COPYING file in this distribution. @@ -34,6 +34,14 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { /** Local database for the account. */ public ImapDB.Account local { get; private set; } + /** + * The root path for all local folders. + * + * No folder exists for this path, it merely exists to provide a + * common root for the paths of all local folders. + */ + protected FolderRoot local_folder_root = new Geary.FolderRoot(true); + private bool open = false; private Cancellable? open_cancellable = null; private Nonblocking.Semaphore? remote_ready_lock = null; @@ -78,7 +86,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { ); this.imap = imap; - smtp.outbox = new Outbox.Folder(this, local); + smtp.outbox = new Outbox.Folder(this, local_folder_root, local); smtp.email_sent.connect(on_email_sent); smtp.report_problem.connect(notify_report_problem); this.smtp = smtp; @@ -139,10 +147,10 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // Create/load local folders - local_only.set(new Outbox.FolderRoot(), this.smtp.outbox); + local_only.set(this.smtp.outbox.path, this.smtp.outbox); this.search_folder = new_search_folder(); - local_only.set(new ImapDB.SearchFolderRoot(), this.search_folder); + local_only.set(this.search_folder.path, this.search_folder); this.open = true; notify_opened(); @@ -300,7 +308,9 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { yield this.remote_ready_lock.wait_async(cancellable); Imap.ClientSession client = yield this.imap.claim_authorized_session_async(cancellable); - return new Imap.AccountSession(this.information.id, client); + return new Imap.AccountSession( + this.information.id, this.local.imap_folder_root, client + ); } /** @@ -350,7 +360,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { Imap.ClientSession? client = yield this.imap.claim_authorized_session_async(cancellable); Imap.AccountSession account = new Imap.AccountSession( - this.information.id, client + this.information.id, this.local.imap_folder_root, client ); Imap.Folder? folder = null; @@ -688,11 +698,15 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { Cancellable? cancellable) throws Error { Geary.FolderPath? path = information.get_special_folder_path(special); - if (path != null) { - debug("Previously used %s for special folder %s", path.to_string(), special.to_string()); - } else { - // This is the first time we're turning a non-special folder into a special one. - // After we do this, we'll record which one we picked in the account info. + if (!remote.is_folder_path_valid(path)) { + debug("Ignoring bad special folder path '%s' for type %s", + path.to_string(), + special.to_string()); + path = null; + } + if (path == null) { + debug("Guessing path for special folder type: %s", + special.to_string()); Geary.FolderPath root = yield remote.get_default_personal_namespace(cancellable); Gee.List search_names = special_search_names.get(special); @@ -779,7 +793,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { * override this to return the correct subclass. */ protected virtual SearchFolder new_search_folder() { - return new ImapDB.SearchFolder(this); + return new ImapDB.SearchFolder(this, this.local_folder_root); } /** {@inheritDoc} */ @@ -1028,7 +1042,9 @@ internal class Geary.ImapEngine.LoadFolders : AccountOperation { GenericAccount generic = (GenericAccount) this.account; Gee.List folders = new Gee.LinkedList(); - yield enumerate_local_folders_async(folders, null, cancellable); + yield enumerate_local_folders_async( + folders, generic.local.imap_folder_root, cancellable + ); generic.add_folders(folders, true); if (!folders.is_empty) { // If we have some folders to load, then this isn't the @@ -1039,7 +1055,7 @@ internal class Geary.ImapEngine.LoadFolders : AccountOperation { } private async void enumerate_local_folders_async(Gee.List folders, - Geary.FolderPath? parent, + Geary.FolderPath parent, Cancellable? cancellable) throws Error { Gee.Collection? children = null; @@ -1074,7 +1090,7 @@ internal class Geary.ImapEngine.LoadFolders : AccountOperation { Geary.Folder target = yield generic.fetch_folder_async(path, cancellable); specials.set(special, target); } catch (Error err) { - debug("%s: Previously used special folder %s does not exist: %s", + debug("%s: Previously used special folder %s not loaded: %s", generic.information.id, special.to_string(), err.message); } } @@ -1137,7 +1153,10 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { ); try { bool is_suspect = yield enumerate_remote_folders_async( - remote, remote_folders, null, cancellable + remote, + remote_folders, + account.local.imap_folder_root, + cancellable ); // pair the local and remote folders and make sure diff --git a/src/engine/imap-engine/yahoo/imap-engine-yahoo-account.vala b/src/engine/imap-engine/yahoo/imap-engine-yahoo-account.vala index b64c5afb..cf0d6bc0 100644 --- a/src/engine/imap-engine/yahoo/imap-engine-yahoo-account.vala +++ b/src/engine/imap-engine/yahoo/imap-engine-yahoo-account.vala @@ -1,7 +1,9 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ private class Geary.ImapEngine.YahooAccount : Geary.ImapEngine.GenericAccount { @@ -36,11 +38,22 @@ private class Geary.ImapEngine.YahooAccount : Geary.ImapEngine.GenericAccount { if (special_map == null) { special_map = new Gee.HashMap(); - special_map.set(Imap.MailboxSpecifier.inbox.to_folder_path(null, null), Geary.SpecialFolderType.INBOX); - special_map.set(new Imap.FolderRoot("Sent"), Geary.SpecialFolderType.SENT); - special_map.set(new Imap.FolderRoot("Draft"), Geary.SpecialFolderType.DRAFTS); - special_map.set(new Imap.FolderRoot("Bulk Mail"), Geary.SpecialFolderType.SPAM); - special_map.set(new Imap.FolderRoot("Trash"), Geary.SpecialFolderType.TRASH); + FolderRoot root = this.local.imap_folder_root; + special_map.set( + this.local.imap_folder_root.inbox, Geary.SpecialFolderType.INBOX + ); + special_map.set( + root.get_child("Sent"), Geary.SpecialFolderType.SENT + ); + special_map.set( + root.get_child("Draft"), Geary.SpecialFolderType.DRAFTS + ); + special_map.set( + root.get_child("Bulk Mail"), Geary.SpecialFolderType.SPAM + ); + special_map.set( + root.get_child("Trash"), Geary.SpecialFolderType.TRASH + ); } } diff --git a/src/engine/imap/api/imap-account-session.vala b/src/engine/imap/api/imap-account-session.vala index 99823f6c..45358e3f 100644 --- a/src/engine/imap/api/imap-account-session.vala +++ b/src/engine/imap/api/imap-account-session.vala @@ -23,6 +23,7 @@ */ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { + private FolderRoot root; private Gee.HashMap folders = new Gee.HashMap(); @@ -32,8 +33,10 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { internal AccountSession(string account_id, + FolderRoot root, ClientSession session) { base("%s:account".printf(account_id), session); + this.root = root; session.list.connect(on_list_data); session.status.connect(on_status_data); @@ -56,7 +59,26 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { prefix = prefix.substring(0, prefix.length - delim.length); } - return new FolderRoot(prefix); + return Geary.String.is_empty(prefix) + ? this.root + : this.root.get_child(prefix); + } + + /** + * Determines if the given folder path appears to a valid mailbox. + */ + public bool is_folder_path_valid(FolderPath? path) throws GLib.Error { + bool is_valid = false; + if (path != null) { + ClientSession session = claim_session(); + try { + session.get_mailbox_for_path(path); + is_valid = true; + } catch (GLib.Error err) { + // still not valid + } + } + return is_valid; } /** @@ -139,17 +161,18 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { /** * Returns a list of children of the given folder. * - * If the parent folder is `null`, then the root of the server - * will be listed. - * * This method will perform a pipe-lined IMAP SELECT for all * folders found, and hence should be used with care. */ - public async Gee.List fetch_child_folders_async(FolderPath? parent, Cancellable? cancellable) - throws Error { + public async Gee.List + fetch_child_folders_async(FolderPath parent, + GLib.Cancellable? cancellable) + throws GLib.Error { ClientSession session = claim_session(); Gee.List children = new Gee.ArrayList(); - Gee.List mailboxes = yield send_list_async(session, parent, true, cancellable); + Gee.List mailboxes = yield send_list_async( + session, parent, true, cancellable + ); if (mailboxes.size == 0) { return children; } @@ -172,7 +195,9 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { // Mailbox is unselectable, so doesn't need a STATUS, // so we can create it now if it does not already // exist - FolderPath path = session.get_path_for_mailbox(mailbox_info.mailbox); + FolderPath path = session.get_path_for_mailbox( + this.root, mailbox_info.mailbox + ); Folder? child = this.folders.get(path); if (child == null) { child = new Imap.Folder( @@ -223,7 +248,9 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { } status_results.remove(status); - FolderPath child_path = session.get_path_for_mailbox(mailbox_info.mailbox); + FolderPath child_path = session.get_path_for_mailbox( + this.root, mailbox_info.mailbox + ); Imap.Folder? child = this.folders.get(child_path); if (child != null) { @@ -269,7 +296,7 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { // Performs a LIST against the server, returning the results private async Gee.List send_list_async(ClientSession session, - FolderPath? folder, + FolderPath folder, bool list_children, Cancellable? cancellable) throws Error { @@ -283,7 +310,7 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { } ListCommand cmd; - if (folder == null) { + if (folder.is_root) { // List the server root cmd = new ListCommand.wildcarded( "", new MailboxSpecifier("%"), can_xlist, return_param @@ -314,7 +341,9 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject { if (folder != null && list_children) { Gee.Iterator iter = list_results.iterator(); while (iter.next()) { - FolderPath list_path = session.get_path_for_mailbox(iter.get().mailbox); + FolderPath list_path = session.get_path_for_mailbox( + this.root, iter.get().mailbox + ); if (list_path.equal_to(folder)) { debug("Removing parent from LIST results: %s", list_path.to_string()); iter.remove(); diff --git a/src/engine/imap/api/imap-folder-root.vala b/src/engine/imap/api/imap-folder-root.vala index e19a4a1f..0f8a39ee 100644 --- a/src/engine/imap/api/imap-folder-root.vala +++ b/src/engine/imap/api/imap-folder-root.vala @@ -1,40 +1,55 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ /** * The root of all IMAP mailbox paths. * - * Because IMAP has peculiar requirements about its mailbox paths (in particular, Inbox is - * guaranteed at the root and is named case-insensitive, and that delimiters are particular to - * each path), this class ensure certain requirements are held throughout the library. + * Because IMAP has peculiar requirements about its mailbox paths (in + * particular, Inbox is guaranteed at the root and is named + * case-insensitive, and that delimiters are particular to each path), + * this class ensure certain requirements are held throughout the + * library. */ +public class Geary.Imap.FolderRoot : Geary.FolderRoot { -private class Geary.Imap.FolderRoot : Geary.FolderRoot { - public bool is_inbox { get; private set; } - - public FolderRoot(string basename) { - bool init_is_inbox; - string normalized_basename = init(basename, out init_is_inbox); - - base (normalized_basename, !init_is_inbox, true); - - is_inbox = init_is_inbox; + + /** + * The canonical path for the IMAP inbox. + * + * This specific path object will always be returned when a child + * with some case-insensitive version of the IMAP inbox mailbox is + * obtained via {@link get_child} from this root folder. However + * since multiple folder roots may be constructed, in general + * {@link FolderPath.equal_to} or {@link FolderPath.compare_to} + * should still be used for testing equality with this path. + */ + public FolderPath inbox { get; private set; } + + + public FolderRoot() { + base(false); + this.inbox = base.get_child( + MailboxSpecifier.CANONICAL_INBOX_NAME, + Trillian.FALSE + ); } - - // This is the magic that ensures the canonical IMAP Inbox name is used throughout the engine - private static string init(string basename, out bool is_inbox) { - if (MailboxSpecifier.is_inbox_name(basename)) { - is_inbox = true; - - return MailboxSpecifier.CANONICAL_INBOX_NAME; - } - - is_inbox = false; - - return basename; + + /** + * Creates a path that is a child of this folder. + * + * If the given basename is that of the IMAP inbox, then {@link + * inbox} will be returned. + */ + public override + FolderPath get_child(string basename, + Trillian is_case_sensitive = Trillian.UNKNOWN) { + return (MailboxSpecifier.is_inbox_name(basename)) + ? this.inbox + : base.get_child(basename, is_case_sensitive); } + } - diff --git a/src/engine/imap/message/imap-mailbox-specifier.vala b/src/engine/imap/message/imap-mailbox-specifier.vala index 1984338b..fb325676 100644 --- a/src/engine/imap/message/imap-mailbox-specifier.vala +++ b/src/engine/imap/message/imap-mailbox-specifier.vala @@ -84,9 +84,9 @@ public class Geary.Imap.MailboxSpecifier : BaseObject, Gee.Hashable parts = path.as_list(); - if (parts.size > 1 && delim == null) { - // XXX not quite right - throw new ImapError.INVALID("Path has more than one part but no delimiter given"); + public MailboxSpecifier.from_folder_path(FolderPath path, + MailboxSpecifier inbox, + string? delim) + throws ImapError { + if (path.is_root) { + throw new ImapError.INVALID( + "Cannot convert root path into a mailbox" + ); } - // Don't include the root if it is an empty string so that - // mailboxes do not begin with the delim. - if (parts.size > 1 && parts[0] == "") { - parts.remove_at(0); + Gee.List parts = path.as_list(); + // Don't include the root so that mailboxes do not begin with + // the delim. + parts.remove_at(0); + + if (parts.size > 1 && delim == null) { + throw new ImapError.INVALID( + "Path has more than one part but no delimiter given" + ); + } + + if (String.is_empty_or_whitespace(parts[0])) { + throw new ImapError.INVALID( + "Path contains empty base part: '%s'", path.to_string() + ); } StringBuilder builder = new StringBuilder( - is_inbox_name(parts[0]) ? inbox.name : parts[0]); + is_inbox_name(parts[0]) ? inbox.name : parts[0] + ); for (int i = 1; i < parts.size; i++) { + string basename = parts[i]; + if (String.is_empty_or_whitespace(basename)) { + throw new ImapError.INVALID( + "Path contains empty part: '%s'", path.to_string() + ); + } builder.append(delim); - builder.append(parts[i]); + builder.append(basename); } init(builder.str); @@ -156,7 +176,7 @@ public class Geary.Imap.MailboxSpecifier : BaseObject, Gee.Hashable to_list(string? delim) { - Gee.List path = new Gee.ArrayList(); + Gee.List path = new Gee.LinkedList(); if (!String.is_empty(delim)) { string[] split = name.split(delim); @@ -171,33 +191,36 @@ public class Geary.Imap.MailboxSpecifier : BaseObject, Gee.Hashable list = to_list(delim); - - // if root element is same as supplied inbox specifier, use canonical inbox name, otherwise - // keep - FolderPath path; - if (inbox_specifier != null && list[0] == inbox_specifier.name) - path = new Imap.FolderRoot(CANONICAL_INBOX_NAME); - else - path = new Imap.FolderRoot(list[0]); - - // walk down rest of elements adding as we go - for (int ctr = 1; ctr < list.size; ctr++) - path = path.get_child(list[ctr]); - + + // If the first element is same as supplied inbox specifier, + // use canonical inbox name, otherwise keep + FolderPath? path = ( + (inbox_specifier != null && list[0] == inbox_specifier.name) + ? root.get_child(CANONICAL_INBOX_NAME) + : root.get_child(list[0]) + ); + list.remove_at(0); + + foreach (string name in list) { + path = path.get_child(name); + } return path; } - + /** * The mailbox's name without parent folders. * diff --git a/src/engine/imap/response/imap-mailbox-information.vala b/src/engine/imap/response/imap-mailbox-information.vala index 6d8c3a0c..0d00324f 100644 --- a/src/engine/imap/response/imap-mailbox-information.vala +++ b/src/engine/imap/response/imap-mailbox-information.vala @@ -86,19 +86,8 @@ public class Geary.Imap.MailboxInformation : BaseObject { ); } - /** - * The {@link Geary.FolderPath} for the {@link mailbox}. - * - * This is constructed from the supplied {@link mailbox} and {@link delim} returned from the - * server. If the mailbox is the same as the supplied inbox_specifier, a canonical name for - * the Inbox is returned. - */ - public Geary.FolderPath get_path(MailboxSpecifier? inbox_specifier) { - return mailbox.to_folder_path(delim, inbox_specifier); - } - public string to_string() { return "%s/%s".printf(mailbox.to_string(), attrs.to_string()); } -} +} diff --git a/src/engine/imap/transport/imap-client-session.vala b/src/engine/imap/transport/imap-client-session.vala index 4613a0d6..0e7910ba 100644 --- a/src/engine/imap/transport/imap-client-session.vala +++ b/src/engine/imap/transport/imap-client-session.vala @@ -509,7 +509,7 @@ public class Geary.Imap.ClientSession : BaseObject { * Determines the SELECT-able mailbox name for a specific folder path. */ public MailboxSpecifier get_mailbox_for_path(FolderPath path) - throws ImapError { + throws ImapError { string? delim = get_delimiter_for_path(path); return new MailboxSpecifier.from_folder_path(path, this.inbox.mailbox, delim); } @@ -517,10 +517,11 @@ public class Geary.Imap.ClientSession : BaseObject { /** * Determines the folder path for a mailbox name. */ - public FolderPath get_path_for_mailbox(MailboxSpecifier mailbox) - throws ImapError { + public FolderPath get_path_for_mailbox(FolderRoot root, + MailboxSpecifier mailbox) + throws ImapError { string? delim = get_delimiter_for_mailbox(mailbox); - return mailbox.to_folder_path(delim, this.inbox.mailbox); + return mailbox.to_folder_path(root, delim, this.inbox.mailbox); } /** diff --git a/src/engine/meson.build b/src/engine/meson.build index 85d256a5..067a400d 100644 --- a/src/engine/meson.build +++ b/src/engine/meson.build @@ -181,7 +181,6 @@ geary_engine_vala_sources = files( 'imap-db/search/imap-db-search-email-identifier.vala', 'imap-db/search/imap-db-search-folder.vala', 'imap-db/search/imap-db-search-folder-properties.vala', - 'imap-db/search/imap-db-search-folder-root.vala', 'imap-db/search/imap-db-search-query.vala', 'imap-db/search/imap-db-search-term.vala', @@ -264,7 +263,6 @@ geary_engine_vala_sources = files( 'outbox/outbox-email-properties.vala', 'outbox/outbox-folder.vala', 'outbox/outbox-folder-properties.vala', - 'outbox/outbox-folder-root.vala', 'rfc822/rfc822.vala', 'rfc822/rfc822-error.vala', diff --git a/src/engine/outbox/outbox-folder-root.vala b/src/engine/outbox/outbox-folder-root.vala deleted file mode 100644 index 8799bb43..00000000 --- a/src/engine/outbox/outbox-folder-root.vala +++ /dev/null @@ -1,18 +0,0 @@ -/* - * Copyright 2016 Software Freedom Conservancy Inc. - * - * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. - */ - -private class Geary.Outbox.FolderRoot : Geary.FolderRoot { - - - public const string MAGIC_BASENAME = "$GearyOutbox$"; - - - public FolderRoot() { - base(MAGIC_BASENAME, false, false); - } - -} diff --git a/src/engine/outbox/outbox-folder.vala b/src/engine/outbox/outbox-folder.vala index cf8be4a0..6f72194f 100644 --- a/src/engine/outbox/outbox-folder.vala +++ b/src/engine/outbox/outbox-folder.vala @@ -16,6 +16,10 @@ private class Geary.Outbox.Folder : Geary.FolderSupport.Remove { + /** The canonical name of the outbox folder. */ + public const string MAGIC_BASENAME = "$GearyOutbox$"; + + private class OutboxRow { public int64 id; public int position; @@ -38,19 +42,32 @@ private class Geary.Outbox.Folder : } + /** {@inheritDoc} */ public override Account account { get { return this._account; } } + /** {@inheritDoc} */ public override Geary.FolderProperties properties { get { return _properties; } } - private FolderRoot _path = new FolderRoot(); + /** + * Returns the path to this folder. + * + * This is always the child of the root given to the constructor, + * with the name given by @{link MAGIC_BASENAME}. + */ public override FolderPath path { get { return _path; } } + private FolderPath _path; + /** + * Returns the type of this folder. + * + * This is always {@link Geary.SpecialFolderType.OUTBOX} + */ public override SpecialFolderType special_folder_type { get { return Geary.SpecialFolderType.OUTBOX; @@ -66,8 +83,9 @@ private class Geary.Outbox.Folder : // Requires the Database from the get-go because it runs a background task that access it // whether open or not - public Folder(Account account, ImapDB.Account local) { + public Folder(Account account, FolderRoot root, ImapDB.Account local) { this._account = account; + this._path = root.get_child(MAGIC_BASENAME, Trillian.TRUE); this.local = local; } diff --git a/test/engine/api/geary-folder-path-mock.vala b/test/engine/api/geary-folder-path-mock.vala deleted file mode 100644 index c9048aec..00000000 --- a/test/engine/api/geary-folder-path-mock.vala +++ /dev/null @@ -1,14 +0,0 @@ -/* - * Copyright 2017 Michael Gratton - * - * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. - */ - -public class Geary.MockFolderRoot : FolderRoot { - - public MockFolderRoot(string name) { - base(name, false, false); - } - -} diff --git a/test/engine/api/geary-folder-path-test.vala b/test/engine/api/geary-folder-path-test.vala new file mode 100644 index 00000000..9cce3dbd --- /dev/null +++ b/test/engine/api/geary-folder-path-test.vala @@ -0,0 +1,41 @@ +/* + * Copyright 2019 Michael Gratton + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +public class Geary.FolderPathTest : TestCase { + + public FolderPathTest() { + base("Geary.FolderPathTest"); + add_test("get_child_from_root", get_child_from_root); + add_test("get_child_from_child", get_child_from_child); + add_test("root_is_root", root_is_root); + add_test("child_is_not_root", root_is_root); + } + + public void get_child_from_root() throws GLib.Error { + assert_string( + ">test", + new Geary.FolderRoot(false).get_child("test").to_string() + ); + } + + public void get_child_from_child() throws GLib.Error { + assert_string( + ">test1>test2", + new Geary.FolderRoot(false) + .get_child("test1").get_child("test2").to_string() + ); + } + + public void root_is_root() throws GLib.Error { + assert_true(new Geary.FolderRoot(false).is_root); + } + + public void child_root_is_not_root() throws GLib.Error { + assert_false(new Geary.FolderRoot(false).get_child("test").is_root); + } + +} diff --git a/test/engine/app/app-conversation-monitor-test.vala b/test/engine/app/app-conversation-monitor-test.vala index 8f3c2a7e..f65720ec 100644 --- a/test/engine/app/app-conversation-monitor-test.vala +++ b/test/engine/app/app-conversation-monitor-test.vala @@ -11,6 +11,7 @@ class Geary.App.ConversationMonitorTest : TestCase { AccountInformation? account_info = null; MockAccount? account = null; + FolderRoot? folder_root = null; MockFolder? base_folder = null; MockFolder? other_folder = null; @@ -35,22 +36,31 @@ class Geary.App.ConversationMonitorTest : TestCase { new RFC822.MailboxAddress(null, "test1@example.com") ); this.account = new MockAccount(this.account_info); + this.folder_root = new FolderRoot(false); this.base_folder = new MockFolder( this.account, null, - new MockFolderRoot("base"), + this.folder_root.get_child("base"), SpecialFolderType.NONE, null ); this.other_folder = new MockFolder( this.account, null, - new MockFolderRoot("other"), + this.folder_root.get_child("other"), SpecialFolderType.NONE, null ); } + public override void tear_down() { + this.other_folder = null; + this.base_folder = null; + this.folder_root = null; + this.account_info = null; + this.account = null; + } + public void start_stop_monitoring() throws Error { ConversationMonitor monitor = new ConversationMonitor( this.base_folder, Folder.OpenFlags.NONE, Email.Field.NONE, 10 diff --git a/test/engine/app/app-conversation-set-test.vala b/test/engine/app/app-conversation-set-test.vala index 1bc34210..a662e9e3 100644 --- a/test/engine/app/app-conversation-set-test.vala +++ b/test/engine/app/app-conversation-set-test.vala @@ -9,6 +9,7 @@ class Geary.App.ConversationSetTest : TestCase { ConversationSet? test = null; + FolderRoot? folder_root = null; Folder? base_folder = null; public ConversationSetTest() { @@ -26,14 +27,21 @@ class Geary.App.ConversationSetTest : TestCase { } public override void set_up() { - this.test = new ConversationSet(); + this.folder_root = new FolderRoot(false); this.base_folder = new MockFolder( null, null, - new MockFolderRoot("test"), + this.folder_root.get_child("test"), SpecialFolderType.NONE, null ); + this.test = new ConversationSet(); + } + + public override void tear_down() { + this.test = null; + this.folder_root = null; + this.base_folder = null; } public void add_all_basic() throws Error { @@ -144,7 +152,7 @@ class Geary.App.ConversationSetTest : TestCase { Gee.MultiMap email_paths = new Gee.HashMultiMap(); email_paths.set(e1.id, this.base_folder.path); - email_paths.set(e2.id, new MockFolderRoot("other")); + email_paths.set(e2.id, this.folder_root.get_child("other")); Gee.Collection? added = null; Gee.MultiMap? appended = null; @@ -310,7 +318,7 @@ class Geary.App.ConversationSetTest : TestCase { public void add_all_multi_path() throws Error { Email e1 = setup_email(1); - MockFolderRoot other_path = new MockFolderRoot("other"); + FolderPath other_path = this.folder_root.get_child("other"); Gee.LinkedList emails = new Gee.LinkedList(); emails.add(e1); @@ -340,7 +348,7 @@ class Geary.App.ConversationSetTest : TestCase { Email e1 = setup_email(1); add_email_to_test_set(e1); - MockFolderRoot other_path = new MockFolderRoot("other"); + FolderPath other_path = this.folder_root.get_child("other"); Gee.LinkedList emails = new Gee.LinkedList(); emails.add(e1); @@ -426,7 +434,7 @@ class Geary.App.ConversationSetTest : TestCase { } public void remove_all_remove_path() throws Error { - MockFolderRoot other_path = new MockFolderRoot("other"); + FolderPath other_path = this.folder_root.get_child("other"); Email e1 = setup_email(1); add_email_to_test_set(e1, other_path); diff --git a/test/engine/app/app-conversation-test.vala b/test/engine/app/app-conversation-test.vala index d3f3d429..709d88ca 100644 --- a/test/engine/app/app-conversation-test.vala +++ b/test/engine/app/app-conversation-test.vala @@ -10,6 +10,8 @@ class Geary.App.ConversationTest : TestCase { Conversation? test = null; Folder? base_folder = null; + FolderRoot? folder_root = null; + public ConversationTest() { base("Geary.App.ConversationTest"); @@ -24,16 +26,23 @@ class Geary.App.ConversationTest : TestCase { } public override void set_up() { + this.folder_root = new FolderRoot(false); this.base_folder = new MockFolder( null, null, - new MockFolderRoot("test"), + this.folder_root.get_child("test"), SpecialFolderType.NONE, null ); this.test = new Conversation(this.base_folder); } + public override void tear_down() { + this.test = null; + this.folder_root = null; + this.base_folder = null; + } + public void add_basic() throws Error { Geary.Email e1 = setup_email(1); Geary.Email e2 = setup_email(2); @@ -78,8 +87,8 @@ class Geary.App.ConversationTest : TestCase { Geary.Email e2 = setup_email(2); this.test.add(e2, singleton(this.base_folder.path)); - FolderRoot other_path = new MockFolderRoot("other"); - Gee.LinkedList other_paths = new Gee.LinkedList(); + FolderPath other_path = this.folder_root.get_child("other"); + Gee.LinkedList other_paths = new Gee.LinkedList(); other_paths.add(other_path); assert(this.test.add(e1, other_paths) == false); @@ -145,7 +154,7 @@ class Geary.App.ConversationTest : TestCase { Geary.Email e1 = setup_email(1); this.test.add(e1, singleton(this.base_folder.path)); - FolderRoot other_path = new MockFolderRoot("other"); + FolderPath other_path = this.folder_root.get_child("other"); Geary.Email e2 = setup_email(2); this.test.add(e2, singleton(other_path)); @@ -158,7 +167,7 @@ class Geary.App.ConversationTest : TestCase { Geary.Email e1 = setup_email(1); this.test.add(e1, singleton(this.base_folder.path)); - FolderRoot other_path = new MockFolderRoot("other"); + FolderPath other_path = this.folder_root.get_child("other"); Geary.Email e2 = setup_email(2); this.test.add(e2, singleton(other_path)); @@ -193,7 +202,7 @@ class Geary.App.ConversationTest : TestCase { Geary.Email e1 = setup_email(1); this.test.add(e1, singleton(this.base_folder.path)); - FolderRoot other_path = new MockFolderRoot("other"); + FolderPath other_path = this.folder_root.get_child("other"); Geary.Email e2 = setup_email(2); this.test.add(e2, singleton(other_path)); diff --git a/test/engine/imap-db/imap-db-account-test.vala b/test/engine/imap-db/imap-db-account-test.vala index 146d7127..27c33de8 100644 --- a/test/engine/imap-db/imap-db-account-test.vala +++ b/test/engine/imap-db/imap-db-account-test.vala @@ -12,6 +12,7 @@ class Geary.ImapDB.AccountTest : TestCase { private GLib.File? tmp_dir = null; private Geary.AccountInformation? config = null; private Account? account = null; + private FolderRoot? root = null; public AccountTest() { @@ -47,9 +48,12 @@ class Geary.ImapDB.AccountTest : TestCase { (obj, ret) => { async_complete(ret); } ); this.account.open_async.end(async_result()); + + this.root = new FolderRoot(false); } public override void tear_down() throws GLib.Error { + this.root = null; this.account.close_async.begin( null, (obj, ret) => { async_complete(ret); } @@ -62,7 +66,7 @@ class Geary.ImapDB.AccountTest : TestCase { public void create_base_folder() throws GLib.Error { Imap.Folder folder = new Imap.Folder( - new Imap.FolderRoot("test"), + this.root.get_child("test"), new Imap.FolderProperties.selectable( new Imap.MailboxAttributes( Gee.Collection.empty() @@ -101,7 +105,7 @@ class Geary.ImapDB.AccountTest : TestCase { ); Imap.Folder folder = new Imap.Folder( - new Imap.FolderRoot("test").get_child("child"), + this.root.get_child("test").get_child("child"), new Imap.FolderProperties.selectable( new Imap.MailboxAttributes( Gee.Collection.empty() @@ -144,7 +148,7 @@ class Geary.ImapDB.AccountTest : TestCase { """); this.account.list_folders_async.begin( - null, + this.account.imap_folder_root, null, (obj, ret) => { async_complete(ret); } ); @@ -187,14 +191,14 @@ class Geary.ImapDB.AccountTest : TestCase { """); this.account.delete_folder_async.begin( - new Imap.FolderRoot("test1").get_child("test2"), + this.root.get_child("test1").get_child("test2"), null, (obj, ret) => { async_complete(ret); } ); this.account.delete_folder_async.end(async_result()); this.account.delete_folder_async.begin( - new Imap.FolderRoot("test1"), + this.root.get_child("test1"), null, (obj, ret) => { async_complete(ret); } ); @@ -210,7 +214,7 @@ class Geary.ImapDB.AccountTest : TestCase { """); this.account.delete_folder_async.begin( - new Imap.FolderRoot("test1"), + this.root.get_child("test1"), null, (obj, ret) => { async_complete(ret); } ); @@ -231,7 +235,7 @@ class Geary.ImapDB.AccountTest : TestCase { """); this.account.delete_folder_async.begin( - new Imap.FolderRoot("test3"), + this.root.get_child("test3"), null, (obj, ret) => { async_complete(ret); } ); @@ -252,7 +256,7 @@ class Geary.ImapDB.AccountTest : TestCase { """); this.account.fetch_folder_async.begin( - new Imap.FolderRoot("test1"), + this.root.get_child("test1"), null, (obj, ret) => { async_complete(ret); } ); @@ -271,7 +275,7 @@ class Geary.ImapDB.AccountTest : TestCase { """); this.account.fetch_folder_async.begin( - new Imap.FolderRoot("test1").get_child("test2"), + this.root.get_child("test1").get_child("test2"), null, (obj, ret) => { async_complete(ret); } ); @@ -290,7 +294,7 @@ class Geary.ImapDB.AccountTest : TestCase { """); this.account.fetch_folder_async.begin( - new Imap.FolderRoot("test3"), + this.root.get_child("test3"), null, (obj, ret) => { async_complete(ret); } ); diff --git a/test/engine/imap/message/imap-mailbox-specifier-test.vala b/test/engine/imap/message/imap-mailbox-specifier-test.vala index 741f279e..6488e5e9 100644 --- a/test/engine/imap/message/imap-mailbox-specifier-test.vala +++ b/test/engine/imap/message/imap-mailbox-specifier-test.vala @@ -13,6 +13,7 @@ class Geary.Imap.MailboxSpecifierTest : TestCase { add_test("to_parameter", to_parameter); add_test("from_parameter", from_parameter); add_test("from_folder_path", from_folder_path); + add_test("folder_path_is_inbox", folder_path_is_inbox); } public void to_parameter() throws Error { @@ -59,54 +60,82 @@ class Geary.Imap.MailboxSpecifierTest : TestCase { } public void from_folder_path() throws Error { - MockFolderRoot empty_root = new MockFolderRoot(""); - MailboxSpecifier empty_inbox = new MailboxSpecifier("Inbox"); + FolderRoot root = new FolderRoot(); + MailboxSpecifier inbox = new MailboxSpecifier("Inbox"); assert_string( "Foo", new MailboxSpecifier.from_folder_path( - empty_root.get_child("Foo"), empty_inbox, "$" + root.get_child("Foo"), inbox, "$" ).name ); assert_string( "Foo$Bar", new MailboxSpecifier.from_folder_path( - empty_root.get_child("Foo").get_child("Bar"), empty_inbox, "$" + root.get_child("Foo").get_child("Bar"), inbox, "$" ).name ); assert_string( "Inbox", new MailboxSpecifier.from_folder_path( - empty_root.get_child(MailboxSpecifier.CANONICAL_INBOX_NAME), - empty_inbox, + root.get_child(MailboxSpecifier.CANONICAL_INBOX_NAME), + inbox, "$" ).name ); - MockFolderRoot non_empty_root = new MockFolderRoot("Root"); - MailboxSpecifier non_empty_inbox = new MailboxSpecifier("Inbox"); - assert_string( - "Root$Foo", + try { new MailboxSpecifier.from_folder_path( - non_empty_root.get_child("Foo"), - non_empty_inbox, - "$" - ).name + root.get_child(""), inbox, "$" + ); + assert_not_reached(); + } catch (GLib.Error err) { + // all good + } + + try { + new MailboxSpecifier.from_folder_path( + root.get_child("test").get_child(""), inbox, "$" + ); + assert_not_reached(); + } catch (GLib.Error err) { + // all good + } + + try { + new MailboxSpecifier.from_folder_path(root, inbox, "$"); + assert_not_reached(); + } catch (GLib.Error err) { + // all good + } + } + + public void folder_path_is_inbox() throws GLib.Error { + FolderRoot root = new FolderRoot(); + assert_true( + MailboxSpecifier.folder_path_is_inbox(root.get_child("Inbox")) ); - assert_string( - "Root$Foo$Bar", - new MailboxSpecifier.from_folder_path( - non_empty_root.get_child("Foo").get_child("Bar"), - non_empty_inbox, - "$" - ).name + assert_true( + MailboxSpecifier.folder_path_is_inbox(root.get_child("inbox")) ); - assert_string( - "Root$INBOX", - new MailboxSpecifier.from_folder_path( - non_empty_root.get_child(MailboxSpecifier.CANONICAL_INBOX_NAME), - non_empty_inbox, - "$" - ).name + assert_true( + MailboxSpecifier.folder_path_is_inbox(root.get_child("INBOX")) + ); + + assert_false( + MailboxSpecifier.folder_path_is_inbox(root) + ); + assert_false( + MailboxSpecifier.folder_path_is_inbox(root.get_child("blah")) + ); + assert_false( + MailboxSpecifier.folder_path_is_inbox( + root.get_child("blah").get_child("Inbox") + ) + ); + assert_false( + MailboxSpecifier.folder_path_is_inbox( + root.get_child("Inbox").get_child("Inbox") + ) ); } diff --git a/test/meson.build b/test/meson.build index c8f45530..26eed436 100644 --- a/test/meson.build +++ b/test/meson.build @@ -18,11 +18,11 @@ geary_test_engine_sources = [ 'engine/api/geary-email-identifier-mock.vala', 'engine/api/geary-email-properties-mock.vala', 'engine/api/geary-folder-mock.vala', - 'engine/api/geary-folder-path-mock.vala', 'engine/api/geary-account-information-test.vala', 'engine/api/geary-attachment-test.vala', 'engine/api/geary-engine-test.vala', + 'engine/api/geary-folder-path-test.vala', 'engine/api/geary-service-information-test.vala', 'engine/app/app-conversation-test.vala', 'engine/app/app-conversation-monitor-test.vala', diff --git a/test/test-engine.vala b/test/test-engine.vala index 3c2309d8..a520c379 100644 --- a/test/test-engine.vala +++ b/test/test-engine.vala @@ -25,6 +25,7 @@ int main(string[] args) { engine.add_suite(new Geary.AccountInformationTest().get_suite()); engine.add_suite(new Geary.AttachmentTest().get_suite()); engine.add_suite(new Geary.EngineTest().get_suite()); + engine.add_suite(new Geary.FolderPathTest().get_suite()); engine.add_suite(new Geary.IdleManagerTest().get_suite()); engine.add_suite(new Geary.TimeoutManagerTest().get_suite()); engine.add_suite(new Geary.TlsNegotiationMethodTest().get_suite()); From 423a1dcbf095215ee5a9e8d210126d694849f4e7 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 13:17:06 +1100 Subject: [PATCH 07/12] Add additional FolderPath unit tests --- test/engine/api/geary-folder-path-test.vala | 175 +++++++++++++++++++- 1 file changed, 170 insertions(+), 5 deletions(-) diff --git a/test/engine/api/geary-folder-path-test.vala b/test/engine/api/geary-folder-path-test.vala index 9cce3dbd..5d3ef675 100644 --- a/test/engine/api/geary-folder-path-test.vala +++ b/test/engine/api/geary-folder-path-test.vala @@ -7,35 +7,200 @@ public class Geary.FolderPathTest : TestCase { + + private FolderRoot? root = null; + + public FolderPathTest() { base("Geary.FolderPathTest"); add_test("get_child_from_root", get_child_from_root); add_test("get_child_from_child", get_child_from_child); add_test("root_is_root", root_is_root); add_test("child_is_not_root", root_is_root); + add_test("as_list", as_list); + add_test("is_top_level", is_top_level); + add_test("path_parent", path_parent); + add_test("path_equal", path_equal); + add_test("path_compare", path_compare); + add_test("path_compare_normalised", path_compare_normalised); + } + + public override void set_up() { + this.root = new FolderRoot(false); + } + + public override void tear_down() { + this.root = null; } public void get_child_from_root() throws GLib.Error { assert_string( ">test", - new Geary.FolderRoot(false).get_child("test").to_string() + this.root.get_child("test").to_string() ); } public void get_child_from_child() throws GLib.Error { assert_string( ">test1>test2", - new Geary.FolderRoot(false) - .get_child("test1").get_child("test2").to_string() + this.root.get_child("test1").get_child("test2").to_string() ); } public void root_is_root() throws GLib.Error { - assert_true(new Geary.FolderRoot(false).is_root); + assert_true(this.root.is_root); } public void child_root_is_not_root() throws GLib.Error { - assert_false(new Geary.FolderRoot(false).get_child("test").is_root); + assert_false(this.root.get_child("test").is_root); + } + + public void as_list() throws GLib.Error { + assert_true(this.root.as_list().size == 1, "Root list"); + assert_int( + 2, + this.root.get_child("test").as_list().size, + "Child list size" + ); + assert_string( + "test", + this.root.get_child("test").as_list()[1], + "Child list contents" + ); + } + + public void is_top_level() throws GLib.Error { + assert_false(this.root.is_top_level, "Root is top_level"); + assert_true( + this.root.get_child("test").is_top_level, + "Top level is top_level" + ); + assert_false( + this.root.get_child("test").get_child("test").is_top_level, + "Descendent is top_level" + ); + } + + public void path_parent() throws GLib.Error { + assert_null(this.root.get_parent(), "Root parent"); + assert_string( + "", + this.root.get_child("test").get_parent().basename, + "Root child parent"); + assert_string( + "test1", + this.root.get_child("test1").get_child("test2").get_parent().basename, + "Child parent"); + } + + public void path_equal() throws GLib.Error { + assert_true(this.root.equal_to(this.root), "Root equality"); + assert_true( + this.root.get_child("test").equal_to(this.root.get_child("test")), + "Child equality" + ); + assert_false( + this.root.get_child("test1").equal_to(this.root.get_child("test2")), + "Child names" + ); + assert_false( + this.root.get_child("test1").get_child("test") + .equal_to(this.root.get_child("test2").get_child("test")), + "Disjoint parents" + ); + } + + public void path_compare() throws GLib.Error { + assert_int(0, this.root.compare_to(this.root), "Root equality"); + assert_int(0, + this.root.get_child("test").compare_to(this.root.get_child("test")), + "Equal child comparison" + ); + + assert_int( + -1, + this.root.get_child("test1").compare_to(this.root.get_child("test2")), + "Greater than child comparison" + ); + assert_int( + 1, + this.root.get_child("test2").compare_to(this.root.get_child("test1")), + "Less than child comparison" + ); + + assert_int( + -1, + this.root.get_child("test1").get_child("test") + .compare_to(this.root.get_child("test2").get_child("test")), + "Greater than disjoint parents" + ); + assert_int( + 1, + this.root.get_child("test2").get_child("test") + .compare_to(this.root.get_child("test1").get_child("test")), + "Less than disjoint parents" + ); + + assert_int( + 1, + this.root.get_child("test1").get_child("test") + .compare_to(this.root.get_child("test1")), + "Greater than descendant" + ); + assert_int( + -1, + this.root.get_child("test1") + .compare_to(this.root.get_child("test1").get_child("test")), + "Less than descendant" + ); + } + + public void path_compare_normalised() throws GLib.Error { + assert_int(0, this.root.compare_normalized_ci(this.root), "Root equality"); + assert_int(0, + this.root.get_child("test") + .compare_normalized_ci(this.root.get_child("test")), + "Equal child comparison" + ); + + assert_int( + -1, + this.root.get_child("test1") + .compare_normalized_ci(this.root.get_child("test2")), + "Greater than child comparison" + ); + assert_int( + 1, + this.root.get_child("test2") + .compare_normalized_ci(this.root.get_child("test1")), + "Less than child comparison" + ); + + assert_int( + -1, + this.root.get_child("test1").get_child("test") + .compare_normalized_ci(this.root.get_child("test2").get_child("test")), + "Greater than disjoint parents" + ); + assert_int( + 1, + this.root.get_child("test2").get_child("test") + .compare_normalized_ci(this.root.get_child("test1").get_child("test")), + "Less than disjoint parents" + ); + + assert_int( + 1, + this.root.get_child("test1").get_child("test") + .compare_normalized_ci(this.root.get_child("test1")), + "Greater than descendant" + ); + assert_int( + -1, + this.root.get_child("test1") + .compare_normalized_ci(this.root.get_child("test1").get_child("test")), + "Less than descendant" + ); } } From ddbe6e0b09c93de1b4dbf81fca934fc19b11d9af Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 17:26:18 +1100 Subject: [PATCH 08/12] Revamp Geary.FolderPath implementation Convert getters that look like properties into actual properties, remove unused and redundant public and internal API, convert implementation to use a tree that aucyally maintains references between steps, rather than each creating a new list of path steps and manipulating that. --- src/client/accounts/accounts-manager.vala | 46 +- .../folder-list-account-branch.vala | 4 +- src/engine/api/geary-folder-path.vala | 412 +++++++++--------- src/engine/api/geary-folder.vala | 6 +- src/engine/imap-db/imap-db-account.vala | 14 +- .../imap-engine-generic-account.vala | 6 +- .../imap/message/imap-mailbox-specifier.vala | 25 +- .../imap/transport/imap-client-session.vala | 26 +- test/engine/api/geary-folder-path-test.vala | 67 ++- test/engine/imap-db/imap-db-account-test.vala | 10 +- 10 files changed, 323 insertions(+), 293 deletions(-) diff --git a/src/client/accounts/accounts-manager.vala b/src/client/accounts/accounts-manager.vala index 96167748..426cb28a 100644 --- a/src/client/accounts/accounts-manager.vala +++ b/src/client/accounts/accounts-manager.vala @@ -1152,7 +1152,9 @@ public class Accounts.AccountConfigV1 : AccountConfig, GLib.Object { string key, Geary.FolderPath? path) { if (path != null) { - config.set_string_list(key, path.as_list()); + config.set_string_list( + key, new Gee.ArrayList.wrap(path.as_array()) + ); } } @@ -1311,17 +1313,37 @@ public class Accounts.AccountConfigLegacy : AccountConfig, GLib.Object { ); } - Gee.LinkedList empty = new Gee.LinkedList(); - config.set_string_list(DRAFTS_FOLDER_KEY, (info.drafts_folder_path != null - ? info.drafts_folder_path.as_list() : empty)); - config.set_string_list(SENT_MAIL_FOLDER_KEY, (info.sent_folder_path != null - ? info.sent_folder_path.as_list() : empty)); - config.set_string_list(SPAM_FOLDER_KEY, (info.spam_folder_path != null - ? info.spam_folder_path.as_list() : empty)); - config.set_string_list(TRASH_FOLDER_KEY, (info.trash_folder_path != null - ? info.trash_folder_path.as_list() : empty)); - config.set_string_list(ARCHIVE_FOLDER_KEY, (info.archive_folder_path != null - ? info.archive_folder_path.as_list() : empty)); + Gee.ArrayList empty = new Gee.ArrayList(); + config.set_string_list( + DRAFTS_FOLDER_KEY, + (info.drafts_folder_path != null + ? new Gee.ArrayList.wrap(info.drafts_folder_path.as_array()) + : empty) + ); + config.set_string_list( + SENT_MAIL_FOLDER_KEY, + (info.sent_folder_path != null + ? new Gee.ArrayList.wrap(info.sent_folder_path.as_array()) + : empty) + ); + config.set_string_list( + SPAM_FOLDER_KEY, + (info.spam_folder_path != null + ? new Gee.ArrayList.wrap(info.spam_folder_path.as_array()) + : empty) + ); + config.set_string_list( + TRASH_FOLDER_KEY, + (info.trash_folder_path != null + ? new Gee.ArrayList.wrap(info.trash_folder_path.as_array()) + : empty) + ); + config.set_string_list( + ARCHIVE_FOLDER_KEY, + (info.archive_folder_path != null + ? new Gee.ArrayList.wrap(info.archive_folder_path.as_array()) + : empty) + ); config.set_bool(SAVE_DRAFTS_KEY, info.save_drafts); } diff --git a/src/client/folder-list/folder-list-account-branch.vala b/src/client/folder-list/folder-list-account-branch.vala index 6a87da02..70587d9e 100644 --- a/src/client/folder-list/folder-list-account-branch.vala +++ b/src/client/folder-list/folder-list-account-branch.vala @@ -98,11 +98,11 @@ public class FolderList.AccountBranch : Sidebar.Branch { graft(get_root(), user_folder_group); } } else { - Sidebar.Entry? entry = folder_entries.get(folder.path.get_parent()); + Sidebar.Entry? entry = folder_entries.get(folder.path.parent); if (entry != null) graft_point = entry; } - + // Due to how we enumerate folders on the server, it's unfortunately // possible now to have two folders that we'd put in the same place in // our tree. In that case, we just ignore the second folder for now. diff --git a/src/engine/api/geary-folder-path.vala b/src/engine/api/geary-folder-path.vala index 4d398d99..4f01d168 100644 --- a/src/engine/api/geary-folder-path.vala +++ b/src/engine/api/geary-folder-path.vala @@ -13,14 +13,28 @@ * @see FolderRoot */ -public class Geary.FolderPath : BaseObject, Gee.Hashable, - Gee.Comparable { +public class Geary.FolderPath : + BaseObject, Gee.Hashable, Gee.Comparable { - /** - * The name of this folder (without any child or parent names or delimiters). - */ - public string basename { get; private set; } + // Workaround for Vala issue #659. See children below. + private class FolderPathWeakRef { + + GLib.WeakRef weak_ref; + + public FolderPathWeakRef(FolderPath path) { + this.weak_ref = GLib.WeakRef(path); + } + + public FolderPath? get() { + return this.weak_ref.get() as FolderPath; + } + + } + + + /** The base name of this folder, excluding parents. */ + public string name { get; private set; } /** * Whether this path is lexiographically case-sensitive. @@ -29,111 +43,68 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, */ public bool case_sensitive { get; private set; } - /** - * Determines if this path is a root folder path. - */ - public virtual bool is_root { - get { return this.path == null || this.path.size == 0; } + /** Determines if this path is a root folder path. */ + public bool is_root { + get { return this.parent == null; } } - /** - * Determines if this path is a child of the root folder. - */ + /** Determines if this path is a child of the root folder. */ public bool is_top_level { get { - FolderPath? parent = get_parent(); + FolderPath? parent = parent; return parent != null && parent.is_root; } } + /** Returns the parent of this path. */ + public FolderPath? parent { get; private set; } - private Gee.List? path = null; - private uint stored_hash = uint.MAX; + private string[] path; + + // Would use a `weak FolderPath` value type for this map instead of + // the custom class, but we can't currently reassign built-in + // weak refs back to a strong ref at the moment, nor use a + // GLib.WeakRef as a generics param. See Vala issue #659. + private Gee.Map children = + new Gee.HashMap(); + + private uint? stored_hash = null; /** Constructor only for use by {@link FolderRoot}. */ internal FolderPath() { - this.basename = ""; + this.name = ""; + this.parent = null; this.case_sensitive = false; + this.path = new string[0]; } - private FolderPath.child(Gee.List path, string basename, bool case_sensitive) { - assert(path[0] is FolderRoot); - - this.path = path; - this.basename = basename; + private FolderPath.child(FolderPath parent, + string name, + bool case_sensitive) { + this.parent = parent; + this.name = name; this.case_sensitive = case_sensitive; + this.path = parent.path.copy(); + this.path += name; } /** * Returns the {@link FolderRoot} of this path. */ public Geary.FolderRoot get_root() { - return (FolderRoot) ((path != null && path.size > 0) ? path[0] : this); - } - - /** - * Returns the parent {@link FolderPath} of this folder or null if this is the root. - * - * @see is_root - */ - public Geary.FolderPath? get_parent() { - return (path != null && path.size > 0) ? path.last() : null; - } - - /** - * Returns the number of folders in this path, not including any children of this object. - */ - public int get_path_length() { - // include self, which is not stored in the path list - return (path != null) ? path.size + 1 : 1; - } - - /** - * Returns the {@link FolderPath} object at the index, with this FolderPath object being - * the farthest child. - * - * Root is at index 0 (zero). - * - * Returns null if index is out of bounds. There is always at least one element in the path, - * namely this one, meaning zero is always acceptable and that index[length - 1] will always - * return this object. - * - * @see get_path_length - */ - public Geary.FolderPath? get_folder_at(int index) { - // include self, which is not stored in the path list ... essentially, this logic makes it - // look like "this" is stored at the end of the path list - if (path == null) - return (index == 0) ? this : null; - - int length = path.size; - if (index < length) - return path[index]; - - if (index == length) - return this; - - return null; - } - - /** - * Returns the {@link FolderPath} as a List of {@link basename} strings, this FolderPath's - * being the last in the list. - * - * Thus, the list should have at least one element. - */ - public Gee.List as_list() { - Gee.List list = new Gee.ArrayList(); - - if (path != null) { - foreach (Geary.FolderPath folder in path) - list.add(folder.basename); + FolderPath? path = this; + while (path.parent != null) { + path = path.parent; } - - list.add(basename); - - return list; + return (FolderRoot) path; + } + + /** + * Returns an array of the names of non-root elements in the path. + */ + public string[] as_array() { + return this.path; } /** @@ -144,39 +115,25 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, * {@link Trillian.UNKNOWN}, then {@link * FolderRoot.default_case_sensitivity} is used. */ - public virtual Geary.FolderPath - get_child(string basename, + public virtual FolderPath + get_child(string name, Trillian is_case_sensitive = Trillian.UNKNOWN) { - // Build the child's path, which is this node's path plus this node - Gee.List child_path = new Gee.ArrayList(); - if (path != null) - child_path.add_all(path); - child_path.add(this); - - return new FolderPath.child( - child_path, - basename, - is_case_sensitive.to_boolean(get_root().default_case_sensitivity) - ); - } - - /** - * Returns true if the other {@link FolderPath} has the same parent as this one. - * - * Like {@link equal_to} and {@link compare_to}, this comparison the comparison is - * lexiographic, not by reference. - */ - public bool has_same_parent(FolderPath other) { - FolderPath? parent = get_parent(); - FolderPath? other_parent = other.get_parent(); - - if (parent == other_parent) - return true; - - if (parent != null && other_parent != null) - return parent.equal_to(other_parent); - - return false; + FolderPath? child = null; + FolderPathWeakRef? child_ref = this.children.get(name); + if (child_ref != null) { + child = child_ref.get(); + } + if (child == null) { + child = new FolderPath.child( + this, + name, + is_case_sensitive.to_boolean( + get_root().default_case_sensitivity + ) + ); + this.children.set(name, new FolderPathWeakRef(child)); + } + return child; } /** @@ -184,124 +141,96 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, */ public bool is_descendant(FolderPath target) { bool is_descendent = false; - Geary.FolderPath? path = target.get_parent(); + FolderPath? path = target.parent; while (path != null) { if (path.equal_to(this)) { is_descendent = true; break; } - path = path.get_parent(); + path = path.parent; } return is_descendent; } - private uint get_basename_hash() { - return case_sensitive ? str_hash(basename) : str_hash(basename.down()); - } - - private int compare_internal(Geary.FolderPath other, bool allow_case_sensitive, bool normalize) { - if (this == other) - return 0; - - // walk elements using as_list() as that includes the basename (whereas path does not), - // avoids the null problem, and makes comparisons straightforward - Gee.List this_list = as_list(); - Gee.List other_list = other.as_list(); - - // if paths exist, do comparison of each parent in order - int min = int.min(this_list.size, other_list.size); - for (int ctr = 0; ctr < min; ctr++) { - string this_element = this_list[ctr]; - string other_element = other_list[ctr]; - - if (normalize) { - this_element = this_element.normalize(); - other_element = other_element.normalize(); - } - if (!allow_case_sensitive - // if either case-sensitive, then comparison is CS - || (!get_folder_at(ctr).case_sensitive && !other.get_folder_at(ctr).case_sensitive)) { - this_element = this_element.casefold(); - other_element = other_element.casefold(); - } - - int result = this_element.collate(other_element); - if (result != 0) - return result; - } - - // paths up to the min element count are equal, shortest path is less-than, otherwise - // equal paths - return this_list.size - other_list.size; - } - /** - * Does a Unicode-normalized, case insensitive match. Useful for getting a rough idea if - * a folder matches a name, but shouldn't be used to determine strict equality. + * Does a Unicode-normalized, case insensitive match. Useful for + * getting a rough idea if a folder matches a name, but shouldn't + * be used to determine strict equality. */ - public int compare_normalized_ci(Geary.FolderPath other) { + public int compare_normalized_ci(FolderPath other) { return compare_internal(other, false, true); } - + /** * {@inheritDoc} * - * Comparisons for Geary.FolderPath is defined as (a) empty paths are less-than non-empty paths - * and (b) each element is compared to the corresponding path element of the other FolderPath - * following collation rules for casefolded (case-insensitive) compared, and (c) shorter paths - * are less-than longer paths, assuming the path elements are equal up to the shorter path's + * Comparisons for FolderPath is defined as (a) empty paths + * are less-than non-empty paths and (b) each element is compared + * to the corresponding path element of the other FolderPath + * following collation rules for casefolded (case-insensitive) + * compared, and (c) shorter paths are less-than longer paths, + * assuming the path elements are equal up to the shorter path's * length. * * Note that {@link FolderPath.case_sensitive} affects comparisons. * - * Returns -1 if this path is lexiographically before the other, 1 if its after, and 0 if they - * are equal. + * Returns -1 if this path is lexiographically before the other, 1 + * if its after, and 0 if they are equal. */ - public int compare_to(Geary.FolderPath other) { + public int compare_to(FolderPath other) { return compare_internal(other, true, false); } - + /** * {@inheritDoc} * * Note that {@link FolderPath.case_sensitive} affects comparisons. */ public uint hash() { - if (stored_hash != uint.MAX) - return stored_hash; - - // always one element in path - stored_hash = get_folder_at(0).get_basename_hash(); - - int path_length = get_path_length(); - for (int ctr = 1; ctr < path_length; ctr++) - stored_hash ^= get_folder_at(ctr).get_basename_hash(); - - return stored_hash; - } - - private bool is_basename_equal(string cmp, bool other_cs) { - // case-sensitive comparison if either is sensitive - return (other_cs || case_sensitive) ? (basename == cmp) : (basename.down() == cmp.down()); - } - - /** - * {@inheritDoc} - */ - public bool equal_to(Geary.FolderPath other) { - int path_length = get_path_length(); - if (other.get_path_length() != path_length) - return false; - - for (int ctr = 0; ctr < path_length; ctr++) { - // this should never return null as length is already checked - FolderPath? other_folder = other.get_folder_at(ctr); - assert(other_folder != null); - - if (!get_folder_at(ctr).is_basename_equal(other_folder.basename, other_folder.case_sensitive)) - return false; + if (this.stored_hash == null) { + this.stored_hash = 0; + FolderPath? path = this; + while (path != null) { + this.stored_hash ^= (case_sensitive) + ? str_hash(path.name) : str_hash(path.name.down()); + path = path.parent; + } } - + return this.stored_hash; + } + + /** {@inheritDoc} */ + public bool equal_to(FolderPath other) { + if (this == other) { + return true; + } + + FolderPath? a = this; + FolderPath? b = other; + while (a != null && b != null) { + if (a == b) { + return true; + } + + if ((a != null && b == null) || + (a == null && b != null)) { + return false; + } + + if (a.case_sensitive || b.case_sensitive) { + if (a.name != b.name) { + return false; + } + } else { + if (a.name.down() != b.name.down()) { + return false; + } + } + + a = a.parent; + b = b.parent; + } + return true; } @@ -314,16 +243,68 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, * instead. This method is useful for debugging and logging only. */ public string to_string() { + const char SEP = '>'; StringBuilder builder = new StringBuilder(); - if (this.path != null) { - foreach (Geary.FolderPath folder in this.path) { - builder.append(folder.basename); - builder.append_c('>'); + if (this.is_root) { + builder.append_c(SEP); + } else { + foreach (string name in this.path) { + builder.append_c(SEP); + builder.append(name); } } - builder.append(basename); return builder.str; } + + private int compare_internal(FolderPath other, + bool allow_case_sensitive, + bool normalize) { + if (this == other) + return 0; + + FolderPath a = this; + FolderPath b = other; + + // Get the common-length prefix of both + while (a.path.length != b.path.length) { + if (a.path.length > b.path.length) { + a = a.parent; + } else if (b.path.length > a.path.length) { + b = b.parent; + } + } + + // Compare the common-length prefixes of both + while (a != null && b != null) { + string a_name = a.name; + string b_name = b.name; + + if (normalize) { + a_name = a_name.normalize(); + b_name = b_name.normalize(); + } + + if (!allow_case_sensitive + // if either case-sensitive, then comparison is CS + || (!a.case_sensitive && !b.case_sensitive)) { + a_name = a_name.casefold(); + b_name = b_name.casefold(); + } + + int result = a_name.collate(b_name); + if (result != 0) { + return result; + } + + a = a.parent; + b = b.parent; + } + + // paths up to the min element count are equal, shortest path + // is less-than, otherwise equal paths + return this.path.length - other.path.length; + } + } /** @@ -334,14 +315,9 @@ public class Geary.FolderPath : BaseObject, Gee.Hashable, * Because all FolderPaths hold references to their parents, this * element can be retrieved with {@link FolderPath.get_root}. */ -public class Geary.FolderRoot : Geary.FolderPath { +public class Geary.FolderRoot : FolderPath { - /** {@inheritDoc} */ - public override bool is_root { - get { return true; } - } - /** * The default case sensitivity of descendant folders. * diff --git a/src/engine/api/geary-folder.vala b/src/engine/api/geary-folder.vala index afdd1ee7..227379f7 100644 --- a/src/engine/api/geary-folder.vala +++ b/src/engine/api/geary-folder.vala @@ -451,16 +451,16 @@ public abstract class Geary.Folder : BaseObject { protected virtual void notify_display_name_changed() { display_name_changed(); } - + /** * Returns a name suitable for displaying to the user. * - * Default is to display the basename of the Folder's path, unless it's a special folder, + * Default is to display the name of the Folder's path, unless it's a special folder, * in which case {@link SpecialFolderType.get_display_name} is returned. */ public virtual string get_display_name() { return (special_folder_type == Geary.SpecialFolderType.NONE) - ? path.basename : special_folder_type.get_display_name(); + ? path.name : special_folder_type.get_display_name(); } /** Determines if a folder has been opened, and if so in which way. */ diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index 2b67edd7..d221af52 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -391,7 +391,7 @@ private class Geary.ImapDB.Account : BaseObject { } if (Imap.MailboxSpecifier.folder_path_is_inbox(path) && - !Imap.MailboxSpecifier.is_canonical_inbox_name(path.basename)) { + !Imap.MailboxSpecifier.is_canonical_inbox_name(path.name)) { // Don't add faux inboxes throw new ImapError.NOT_SUPPORTED( "Inbox has : %s", path.to_string() @@ -412,7 +412,7 @@ private class Geary.ImapDB.Account : BaseObject { Db.Statement stmt = cx.prepare( "INSERT INTO FolderTable (name, parent_id, last_seen_total, last_seen_status_total, " + "uid_validity, uid_next, attributes, unread_count) VALUES (?, ?, ?, ?, ?, ?, ?, ?)"); - stmt.bind_string(0, path.basename); + stmt.bind_string(0, path.name); stmt.bind_rowid(1, parent_id); stmt.bind_int(2, Numeric.int_floor(properties.select_examine_messages, 0)); stmt.bind_int(3, Numeric.int_floor(properties.status_messages, 0)); @@ -1589,13 +1589,9 @@ private class Geary.ImapDB.Account : BaseObject { ); } - // Don't include the root since top-level folders are stored - // with no parent. - Gee.List parts = path.as_list(); - parts.remove_at(0); - - folder_id = Db.INVALID_ROWID; + string[] parts = path.as_array(); int64 parent_id = Db.INVALID_ROWID; + folder_id = Db.INVALID_ROWID; foreach (string basename in parts) { Db.Statement stmt; @@ -1658,7 +1654,7 @@ private class Geary.ImapDB.Account : BaseObject { parent_id = Db.INVALID_ROWID; } else { ret = do_fetch_folder_id( - cx, path.get_parent(), create, out parent_id, cancellable + cx, path.parent, create, out parent_id, cancellable ); } return ret; diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index dd3a08fc..f24a6027 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -421,7 +421,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { return Geary.traverse(folder_map.keys) .filter(p => { - FolderPath? path_parent = p.get_parent(); + FolderPath? path_parent = p.parent; return ((parent == null && path_parent == null) || (parent != null && path_parent != null && path_parent.equal_to(parent))); }) @@ -1283,7 +1283,9 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { this.generic_account.remove_folders(to_remove); // Sort by path length descending, so we always remove children first. - removed.sort((a, b) => b.path.get_path_length() - a.path.get_path_length()); + removed.sort( + (a, b) => b.path.as_array().length - a.path.as_array().length + ); foreach (Geary.Folder folder in removed) { try { debug("Locally deleting removed folder %s", folder.to_string()); diff --git a/src/engine/imap/message/imap-mailbox-specifier.vala b/src/engine/imap/message/imap-mailbox-specifier.vala index fb325676..6d7c9160 100644 --- a/src/engine/imap/message/imap-mailbox-specifier.vala +++ b/src/engine/imap/message/imap-mailbox-specifier.vala @@ -84,14 +84,14 @@ public class Geary.Imap.MailboxSpecifier : BaseObject, Gee.Hashable parts = path.as_list(); - // Don't include the root so that mailboxes do not begin with - // the delim. - parts.remove_at(0); - - if (parts.size > 1 && delim == null) { - throw new ImapError.INVALID( + string[] parts = path.as_array(); + if (parts.length > 1 && delim == null) { + throw new ImapError.NOT_SUPPORTED( "Path has more than one part but no delimiter given" ); } if (String.is_empty_or_whitespace(parts[0])) { - throw new ImapError.INVALID( + throw new ImapError.NOT_SUPPORTED( "Path contains empty base part: '%s'", path.to_string() ); } @@ -150,15 +146,14 @@ public class Geary.Imap.MailboxSpecifier : BaseObject, Gee.Hashabletest", - this.root.get_child("test").to_string() + "test", + this.root.get_child("test").name ); } public void get_child_from_child() throws GLib.Error { assert_string( - ">test1>test2", - this.root.get_child("test1").get_child("test2").to_string() + "test2", + this.root.get_child("test1").get_child("test2").name ); } @@ -55,17 +57,32 @@ public class Geary.FolderPathTest : TestCase { assert_false(this.root.get_child("test").is_root); } - public void as_list() throws GLib.Error { - assert_true(this.root.as_list().size == 1, "Root list"); + public void as_array() throws GLib.Error { + assert_true(this.root.as_array().length == 0, "Root list"); assert_int( - 2, - this.root.get_child("test").as_list().size, - "Child list size" + 1, + this.root.get_child("test").as_array().length, + "Child array length" ); assert_string( "test", - this.root.get_child("test").as_list()[1], - "Child list contents" + this.root.get_child("test").as_array()[0], + "Child array contents" + ); + assert_int( + 2, + this.root.get_child("test1").get_child("test2").as_array().length, + "Descendent array length" + ); + assert_string( + "test1", + this.root.get_child("test1").get_child("test2").as_array()[0], + "Descendent first child" + ); + assert_string( + "test2", + this.root.get_child("test1").get_child("test2").as_array()[1], + "Descendent second child" ); } @@ -81,15 +98,24 @@ public class Geary.FolderPathTest : TestCase { ); } + public void path_to_string() throws GLib.Error { + assert_string(">", this.root.to_string()); + assert_string(">test", this.root.get_child("test").to_string()); + assert_string( + ">test1>test2", + this.root.get_child("test1").get_child("test2").to_string() + ); + } + public void path_parent() throws GLib.Error { - assert_null(this.root.get_parent(), "Root parent"); + assert_null(this.root.parent, "Root parent"); assert_string( "", - this.root.get_child("test").get_parent().basename, + this.root.get_child("test").parent.name, "Root child parent"); assert_string( "test1", - this.root.get_child("test1").get_child("test2").get_parent().basename, + this.root.get_child("test1").get_child("test2").parent.name, "Child parent"); } @@ -110,6 +136,17 @@ public class Geary.FolderPathTest : TestCase { ); } + public void path_hash() throws GLib.Error { + assert_true( + this.root.hash() != + this.root.get_child("test").hash() + ); + assert_true( + this.root.get_child("test1").hash() != + this.root.get_child("test2").hash() + ); + } + public void path_compare() throws GLib.Error { assert_int(0, this.root.compare_to(this.root), "Root equality"); assert_int(0, diff --git a/test/engine/imap-db/imap-db-account-test.vala b/test/engine/imap-db/imap-db-account-test.vala index 27c33de8..d70e44e5 100644 --- a/test/engine/imap-db/imap-db-account-test.vala +++ b/test/engine/imap-db/imap-db-account-test.vala @@ -157,7 +157,7 @@ class Geary.ImapDB.AccountTest : TestCase { Folder test1 = traverse(result).first(); assert_int(1, result.size, "Base folder not listed"); - assert_string("test1", test1.get_path().basename, "Base folder name"); + assert_string("test1", test1.get_path().name, "Base folder name"); this.account.list_folders_async.begin( test1.get_path(), @@ -168,7 +168,7 @@ class Geary.ImapDB.AccountTest : TestCase { Folder test2 = traverse(result).first(); assert_int(1, result.size, "Child folder not listed"); - assert_string("test2", test2.get_path().basename, "Child folder name"); + assert_string("test2", test2.get_path().name, "Child folder name"); this.account.list_folders_async.begin( test2.get_path(), @@ -179,7 +179,7 @@ class Geary.ImapDB.AccountTest : TestCase { Folder test3 = traverse(result).first(); assert_int(1, result.size, "Grandchild folder not listed"); - assert_string("test3", test3.get_path().basename, "Grandchild folder name"); + assert_string("test3", test3.get_path().name, "Grandchild folder name"); } public void delete_folder() throws GLib.Error { @@ -263,7 +263,7 @@ class Geary.ImapDB.AccountTest : TestCase { Folder? result = this.account.fetch_folder_async.end(async_result()); assert_non_null(result); - assert_string("test1", result.get_path().basename); + assert_string("test1", result.get_path().name); } public void fetch_child_folder() throws GLib.Error { @@ -282,7 +282,7 @@ class Geary.ImapDB.AccountTest : TestCase { Folder? result = this.account.fetch_folder_async.end(async_result()); assert_non_null(result); - assert_string("test2", result.get_path().basename); + assert_string("test2", result.get_path().name); } public void fetch_nonexistent_folder() throws GLib.Error { From e1fd9daa83817eb3b6fca57fcec18f8572e10acb Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 18:03:47 +1100 Subject: [PATCH 09/12] Fix pathological FolderPath.is_equal() case, add unit test --- src/engine/api/geary-folder-path.vala | 2 +- test/engine/api/geary-folder-path-test.vala | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/engine/api/geary-folder-path.vala b/src/engine/api/geary-folder-path.vala index 4f01d168..a33ade74 100644 --- a/src/engine/api/geary-folder-path.vala +++ b/src/engine/api/geary-folder-path.vala @@ -207,7 +207,7 @@ public class Geary.FolderPath : FolderPath? a = this; FolderPath? b = other; - while (a != null && b != null) { + while (a != null || b != null) { if (a == b) { return true; } diff --git a/test/engine/api/geary-folder-path-test.vala b/test/engine/api/geary-folder-path-test.vala index c1c16643..18db7308 100644 --- a/test/engine/api/geary-folder-path-test.vala +++ b/test/engine/api/geary-folder-path-test.vala @@ -134,6 +134,12 @@ public class Geary.FolderPathTest : TestCase { .equal_to(this.root.get_child("test2").get_child("test")), "Disjoint parents" ); + + assert_false( + this.root.get_child("test").equal_to( + this.root.get_child("").get_child("test")), + "Pathological case" + ); } public void path_hash() throws GLib.Error { From 1ed0f5acbfdf2552335cb741787ebd41d0c1cfb5 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 18:05:20 +1100 Subject: [PATCH 10/12] Fix ambiguous test in AccountInformation.set_special_folder --- src/engine/api/geary-account-information.vala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala index edc867a1..35b4a5fd 100644 --- a/src/engine/api/geary-account-information.vala +++ b/src/engine/api/geary-account-information.vala @@ -437,8 +437,9 @@ public class Geary.AccountInformation : BaseObject { break; } - if (old_path == null && new_path != null || - old_path != null && !old_path.equal_to(new_path)) { + if ((old_path == null && new_path != null) || + (old_path != null && new_path == null) || + (old_path != null && !old_path.equal_to(new_path))) { changed(); } } From b1461d84b5ebaab0deef979d9c4f51fa59da0c43 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 18:08:31 +1100 Subject: [PATCH 11/12] Add some useful debugging to GenericAcount --- .../imap-engine/imap-engine-generic-account.vala | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index f24a6027..708b6750 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -698,15 +698,13 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { Cancellable? cancellable) throws Error { Geary.FolderPath? path = information.get_special_folder_path(special); - if (!remote.is_folder_path_valid(path)) { + if (path != null && !remote.is_folder_path_valid(path)) { debug("Ignoring bad special folder path '%s' for type %s", path.to_string(), special.to_string()); path = null; } if (path == null) { - debug("Guessing path for special folder type: %s", - special.to_string()); Geary.FolderPath root = yield remote.get_default_personal_namespace(cancellable); Gee.List search_names = special_search_names.get(special); @@ -725,6 +723,9 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { if (path == null) path = root.get_child(search_names[0]); + debug("Guessed folder \'%s\' for special_path %s", + path.to_string(), special.to_string() + ); information.set_special_folder_path(special, path); } @@ -1159,6 +1160,15 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { cancellable ); + debug("Existing folders:"); + foreach (FolderPath path in existing_folders.keys) { + debug(" - %s (%u)", path.to_string(), path.hash()); + } + debug("Remote folders:"); + foreach (FolderPath path in remote_folders.keys) { + debug(" - %s (%u)", path.to_string(), path.hash()); + } + // pair the local and remote folders and make sure // everything is up-to-date yield update_folders_async( From bb0060b571c70948c29deb14e449868643812248 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 23:22:57 +1100 Subject: [PATCH 12/12] Rework GenericAccount::ensure_special_folder_asyc yet again This time, it was overriding SPECIAL-USE attrs based on the config file, causing bad config folders to be used as GMail specials. Now, it checks the folder is already set as the special type before going looking for it by name. --- .../imap-engine-generic-account.vala | 193 ++++++++++-------- 1 file changed, 111 insertions(+), 82 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 708b6750..ba045ff8 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -638,6 +638,8 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { foreach (Geary.SpecialFolderType special in specials.keys) { MinimalFolder? minimal = specials.get(special) as MinimalFolder; if (minimal.special_folder_type != special) { + debug("%s: Promoting %s to %s", + to_string(), minimal.to_string(), special.to_string()); minimal.set_special_folder_type(special); changed.add(minimal); @@ -693,85 +695,94 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { /** * Locates a special folder, creating it if needed. */ - internal async Geary.Folder ensure_special_folder_async(Imap.AccountSession remote, - Geary.SpecialFolderType special, - Cancellable? cancellable) - throws Error { - Geary.FolderPath? path = information.get_special_folder_path(special); - if (path != null && !remote.is_folder_path_valid(path)) { - debug("Ignoring bad special folder path '%s' for type %s", - path.to_string(), - special.to_string()); - path = null; - } - if (path == null) { - Geary.FolderPath root = - yield remote.get_default_personal_namespace(cancellable); - Gee.List search_names = special_search_names.get(special); - foreach (string search_name in search_names) { - Geary.FolderPath search_path = root.get_child(search_name); - foreach (Geary.FolderPath test_path in folder_map.keys) { - if (test_path.compare_normalized_ci(search_path) == 0) { - path = search_path; + internal async Folder + ensure_special_folder_async(Imap.AccountSession remote, + SpecialFolderType type, + GLib.Cancellable? cancellable) + throws GLib.Error { + Folder? special = get_special_folder(type); + if (special == null) { + FolderPath? path = information.get_special_folder_path(type); + if (path != null && !remote.is_folder_path_valid(path)) { + debug("%s: Ignoring bad special folder path '%s' for type %s", + to_string(), + path.to_string(), + type.to_string()); + path = null; + } + if (path == null) { + FolderPath root = + yield remote.get_default_personal_namespace(cancellable); + Gee.List search_names = special_search_names.get(type); + foreach (string search_name in search_names) { + FolderPath search_path = root.get_child(search_name); + foreach (FolderPath test_path in folder_map.keys) { + if (test_path.compare_normalized_ci(search_path) == 0) { + path = search_path; + break; + } + } + if (path != null) break; + } + + if (path == null) { + path = root.get_child(search_names[0]); + } + + debug("%s: Guessed folder \'%s\' for special_path %s", + to_string(), path.to_string(), type.to_string() + ); + information.set_special_folder_path(type, path); + } + + if (!this.folder_map.has_key(path)) { + debug("%s: Creating \"%s\" to use as special folder %s", + to_string(), path.to_string(), type.to_string()); + + GLib.Error? created_err = null; + try { + yield remote.create_folder_async(path, type, cancellable); + } catch (GLib.Error err) { + // Hang on to the error since the folder might exist + // on the remote, so try fetching it anyway. + created_err = err; + } + + Imap.Folder? remote_folder = null; + try { + remote_folder = yield remote.fetch_folder_async( + path, cancellable + ); + } catch (GLib.Error err) { + // If we couldn't fetch it after also failing to + // create it, it's probably due to the problem + // creating it, so throw that error instead. + if (created_err != null) { + throw created_err; + } else { + throw err; } } - if (path != null) - break; - } - if (path == null) - path = root.get_child(search_names[0]); - - debug("Guessed folder \'%s\' for special_path %s", - path.to_string(), special.to_string() - ); - information.set_special_folder_path(special, path); - } - - if (!this.folder_map.has_key(path)) { - debug("Creating \"%s\" to use as special folder %s", - path.to_string(), special.to_string()); - - GLib.Error? created_err = null; - try { - yield remote.create_folder_async(path, special, cancellable); - } catch (GLib.Error err) { - // Hang on to the error since the folder might exist - // on the remote, so try fetching it anyway. - created_err = err; - } - - Imap.Folder? remote_folder = null; - try { - remote_folder = yield remote.fetch_folder_async( - path, cancellable + ImapDB.Folder local_folder = + yield this.local.clone_folder_async( + remote_folder, cancellable + ); + add_folders( + Collection.single(local_folder), created_err != null ); - } catch (GLib.Error err) { - // If we couldn't fetch it after also failing to - // create it, it's probably due to the problem - // creating it, so throw that error instead. - if (created_err != null) { - throw created_err; - } else { - throw err; - } } - ImapDB.Folder local_folder = yield this.local.clone_folder_async( - remote_folder, cancellable + special= this.folder_map.get(path); + promote_folders( + Collection.single_map( + type, special + ) ); - add_folders(Collection.single(local_folder), created_err != null); } - Geary.Folder special_folder = this.folder_map.get(path); - promote_folders( - Collection.single_map( - special, special_folder - ) - ); - - return special_folder; + return special; } /** @@ -1079,25 +1090,43 @@ internal class Geary.ImapEngine.LoadFolders : AccountOperation { } } - private async void check_special_folders(Cancellable cancellable) - throws Error { + private async void check_special_folders(GLib.Cancellable cancellable) + throws GLib.Error { + // Local folders loaded that have the SPECIAL-USE flags set + // will have been promoted already via derived account type's + // new_child overrides or some other means. However for those + // that do not have the flag, check here against the local + // config and promote ASAP. + // + // Can't just use ensure_special_folder_async however since + // that will attempt to create the folders if missing, which + // is bad if offline. GenericAccount generic = (GenericAccount) this.account; - Gee.Map specials = + Gee.Map added_specials = new Gee.HashMap(); - foreach (Geary.SpecialFolderType special in this.specials) { - Geary.FolderPath? path = generic.information.get_special_folder_path(special); - if (path != null) { - try { - Geary.Folder target = yield generic.fetch_folder_async(path, cancellable); - specials.set(special, target); - } catch (Error err) { - debug("%s: Previously used special folder %s not loaded: %s", - generic.information.id, special.to_string(), err.message); + foreach (Geary.SpecialFolderType type in this.specials) { + if (generic.get_special_folder(type) == null) { + Geary.FolderPath? path = + generic.information.get_special_folder_path(type); + if (path != null) { + try { + Geary.Folder target = yield generic.fetch_folder_async( + path, cancellable + ); + added_specials.set(type, target); + } catch (Error err) { + debug( + "%s: Previously used special folder %s not loaded: %s", + generic.information.id, + type.to_string(), + err.message + ); + } } } } - generic.promote_folders(specials); + generic.promote_folders(added_specials); } }