From 9e93062d6f3c19b7d827c3f85105a7d64a74b050 Mon Sep 17 00:00:00 2001 From: Michael James Gratton Date: Tue, 19 Dec 2017 10:49:05 +1100 Subject: [PATCH] Create and use a common FolderOperation account operation class. --- .../imap-engine-account-operation.vala | 73 ++++++++++++++++++- .../imap-engine-generic-account.vala | 51 ++++++------- .../imap-engine/account-processor-test.vala | 35 +++++++-- 3 files changed, 120 insertions(+), 39 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-operation.vala b/src/engine/imap-engine/imap-engine-account-operation.vala index 39130212..ffab9cad 100644 --- a/src/engine/imap-engine/imap-engine-account-operation.vala +++ b/src/engine/imap-engine/imap-engine-account-operation.vala @@ -15,12 +15,27 @@ * * Execution of the operation is managed by {@link * AccountProcessor}. Since the processor will not en-queue duplicate - * operations, implementations may override the {@link equal_to} - * method to ensure that the same operation is not queued twice. + * operations, implementations of this class may override the {@link + * equal_to} method to specify which instances are considered to be + * duplicates. */ public abstract class Geary.ImapEngine.AccountOperation : Geary.BaseObject { + /** The account this operation applies to. */ + protected weak Geary.Account account; + + + /** + * Constructs a new account operation. + * + * The passed in `account` will be the account the operation will + * apply to. + */ + protected AccountOperation(Geary.Account account) { + this.account = account; + } + /** * Fired by after processing when the operation has completed. * @@ -84,3 +99,57 @@ public abstract class Geary.ImapEngine.AccountOperation : Geary.BaseObject { } } + + +/** + * An account operation that applies to a specific folder. + * + * By default, instances of this class require that another operation + * applies to the same folder as well as having the same type to be + * considered equal, for the purpose of not en-queuing duplicate + * operations. + */ +public abstract class Geary.ImapEngine.FolderOperation : AccountOperation { + + + /** The folder this operation applies to. */ + protected weak Geary.Folder folder; + + + /** + * Constructs a new folder operation. + * + * The passed in `folder` and `account` will be the objects the + * operation will apply to. + */ + protected FolderOperation(Geary.Account account, Geary.Folder folder) { + base(account); + this.folder = folder; + } + + /** + * Determines if another operation is equal to this. + * + * This method compares both chain's up to {@link + * AccountOperation.equal_to} and if equal, compares the paths of + * both operation's folders to determine if `op` is equal to this + * operation. + */ + public override bool equal_to(AccountOperation op) { + return ( + base.equal_to(op) && + this.folder.path.equal_to(((FolderOperation) op).folder.path) + ); + } + + /** + * Provides a representation of this operation for debugging. + * + * The return value will include its folder's path and the name of + * the class. + */ + public override string to_string() { + return "%s(%s)".printf(base.to_string(), folder.path.to_string()); + } + +} diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index a6a7b71f..ccac9d5d 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -811,7 +811,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { internal class Geary.ImapEngine.LoadFolders : AccountOperation { - private weak GenericAccount account; private weak ImapDB.Account local; private Geary.SpecialFolderType[] specials; @@ -819,16 +818,17 @@ internal class Geary.ImapEngine.LoadFolders : AccountOperation { internal LoadFolders(GenericAccount account, ImapDB.Account local, Geary.SpecialFolderType[] specials) { - this.account = account; + base(account); this.local = local; this.specials = specials; } public override async void execute(Cancellable cancellable) throws Error { + GenericAccount generic = (GenericAccount) this.account; Gee.List folders = new Gee.LinkedList(); yield enumerate_local_folders_async(folders, null, cancellable); debug("%s: found %u folders", to_string(), folders.size); - this.account.add_folders(folders, true); + generic.add_folders(folders, true); if (!folders.is_empty) { // If we have some folders to load, then this isn't the @@ -836,9 +836,12 @@ internal class Geary.ImapEngine.LoadFolders : AccountOperation { // exist foreach (Geary.SpecialFolderType special in this.specials) { try { - yield this.account.ensure_special_folder_async(special, cancellable); + yield generic.ensure_special_folder_async(special, cancellable); } catch (Error e) { - warning("Unable to ensure special folder %s: %s", special.to_string(), e.message); + warning( + "Unable to ensure special folder %s: %s", + special.to_string(), e.message + ); } } } @@ -876,7 +879,7 @@ internal class Geary.ImapEngine.LoadFolders : AccountOperation { internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { - private weak GenericAccount account; + private weak GenericAccount generic_account; private weak Imap.Account remote; private weak ImapDB.Account local; private Gee.Collection local_folders; @@ -888,7 +891,8 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { ImapDB.Account local, Gee.Collection local_folders, Geary.SpecialFolderType[] specials) { - this.account = account; + base(account); + this.generic_account = account; this.remote = remote; this.local = local; this.local_folders = local_folders; @@ -1012,12 +1016,13 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { debug("Unable to fetch local folder after cloning: %s", convert_err.message); } } - this.account.add_folders(to_build, false); + this.generic_account.add_folders(to_build, false); if (remote_folders_suspect) { debug("Skipping removing folders due to prior errors"); } else { - Gee.List removed = this.account.remove_folders(to_remove); + Gee.List removed = + 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()); @@ -1046,13 +1051,15 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { else debug("Unable to report %s altered: no local representation", altered_path.to_string()); } - this.account.update_folders(altered); + this.generic_account.update_folders(altered); } // Ensure each of the important special folders we need already exist foreach (Geary.SpecialFolderType special in this.specials) { try { - yield this.account.ensure_special_folder_async(special, cancellable); + yield this.generic_account.ensure_special_folder_async( + special, cancellable + ); } catch (Error e) { warning("Unable to ensure special folder %s: %s", special.to_string(), e.message); } @@ -1067,11 +1074,9 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { * This performs a IMAP STATUS on the folder, but only if it is not * open - if it is open it is already maintaining its unseen count. */ -internal class Geary.ImapEngine.RefreshFolderUnseen : AccountOperation { +internal class Geary.ImapEngine.RefreshFolderUnseen : FolderOperation { - private weak MinimalFolder folder; - private weak GenericAccount account; private weak Imap.Account remote; private weak ImapDB.Account local; @@ -1080,23 +1085,11 @@ internal class Geary.ImapEngine.RefreshFolderUnseen : AccountOperation { GenericAccount account, Imap.Account remote, ImapDB.Account local) { - this.folder = folder; - this.account = account; + base(account, folder); this.remote = remote; this.local = local; } - public override bool equal_to(AccountOperation op) { - return ( - base.equal_to(op) && - this.folder.path.equal_to(((RefreshFolderUnseen) op).folder.path) - ); - } - - public override string to_string() { - return "%s(%s)".printf(base.to_string(), folder.path.to_string()); - } - public override async void execute(Cancellable cancellable) throws Error { if (this.folder.get_open_state() == Geary.Folder.OpenState.CLOSED) { Imap.Folder remote_folder = yield remote.fetch_folder_cached_async( @@ -1106,13 +1099,13 @@ internal class Geary.ImapEngine.RefreshFolderUnseen : AccountOperation { ); if (remote_folder.properties.have_contents_changed( - this.folder.local_folder.get_properties(), + ((MinimalFolder) this.folder).local_folder.get_properties(), this.folder.to_string())) { yield local.update_folder_status_async( remote_folder, false, true, cancellable ); - this.account.update_folder(this.folder); + ((GenericAccount) this.account).update_folder(this.folder); } } } diff --git a/test/engine/imap-engine/account-processor-test.vala b/test/engine/imap-engine/account-processor-test.vala index 7c142192..15a112fa 100644 --- a/test/engine/imap-engine/account-processor-test.vala +++ b/test/engine/imap-engine/account-processor-test.vala @@ -20,6 +20,10 @@ public class Geary.ImapEngine.AccountProcessorTest : Gee.TestCase { private Nonblocking.Spinlock spinlock = new Nonblocking.Spinlock(); + internal TestOperation(Geary.Account account) { + base(account); + } + public override async void execute(Cancellable cancellable) throws Error { print("Test op/"); @@ -37,10 +41,16 @@ public class Geary.ImapEngine.AccountProcessorTest : Gee.TestCase { public class OtherOperation : TestOperation { + internal OtherOperation(Geary.Account account) { + base(account); + } + } - private AccountProcessor processor; + private AccountProcessor? processor = null; + private Geary.Account? account = null; + private Geary.AccountInformation? info = null; private uint succeeded; private uint failed; private uint completed; @@ -53,17 +63,26 @@ public class Geary.ImapEngine.AccountProcessorTest : Gee.TestCase { add_test("test_duplicate", test_duplicate); add_test("test_stop", test_stop); + // XXX this has to be here instead of in set_up for some + // reason... this.processor = new AccountProcessor("processor"); } public override void set_up() { + this.info = new Geary.AccountInformation( + "test-info", + File.new_for_path("."), + File.new_for_path(".") + ); + this.account = new Geary.MockAccount("test-account", this.info); + this.succeeded = 0; this.failed = 0; this.completed = 0; } public void test_success() { - TestOperation op = setup_operation(new TestOperation()); + TestOperation op = setup_operation(new TestOperation(this.account)); this.processor.enqueue(op); assert(this.processor.waiting == 1); @@ -77,7 +96,7 @@ public class Geary.ImapEngine.AccountProcessorTest : Gee.TestCase { } public void test_failure() { - TestOperation op = setup_operation(new TestOperation()); + TestOperation op = setup_operation(new TestOperation(this.account)); op.throw_error = true; AccountOperation? error_op = null; @@ -98,9 +117,9 @@ public class Geary.ImapEngine.AccountProcessorTest : Gee.TestCase { } public void test_duplicate() { - TestOperation op1 = setup_operation(new TestOperation()); - TestOperation op2 = setup_operation(new TestOperation()); - TestOperation op3 = setup_operation(new OtherOperation()); + TestOperation op1 = setup_operation(new TestOperation(this.account)); + TestOperation op2 = setup_operation(new TestOperation(this.account)); + TestOperation op3 = setup_operation(new OtherOperation(this.account)); this.processor.enqueue(op1); this.processor.enqueue(op2); @@ -111,9 +130,9 @@ public class Geary.ImapEngine.AccountProcessorTest : Gee.TestCase { } public void test_stop() { - TestOperation op1 = setup_operation(new TestOperation()); + TestOperation op1 = setup_operation(new TestOperation(this.account)); op1.wait_for_cancel = true; - TestOperation op2 = setup_operation(new OtherOperation()); + TestOperation op2 = setup_operation(new OtherOperation(this.account)); this.processor.enqueue(op1); this.processor.enqueue(op2);