diff --git a/src/client/application/application-client.vala b/src/client/application/application-client.vala index bb64b290..a96732a2 100644 --- a/src/client/application/application-client.vala +++ b/src/client/application/application-client.vala @@ -801,14 +801,13 @@ public class Application.Client : Gtk.Application { window.focus_in_event.connect(on_main_window_focus_in); if (select_first_inbox) { try { - var config = this.controller.get_first_account(); - if (config != null) { - var first = this.engine.get_account_instance(config); - if (first != null) { - Geary.Folder? inbox = first.get_special_folder(INBOX); - if (inbox != null) { - window.select_folder.begin(inbox, true); - } + Geary.Account first = Geary.Collection.get_first( + this.engine.get_accounts() + ); + if (first != null) { + Geary.Folder? inbox = first.get_special_folder(INBOX); + if (inbox != null) { + window.select_folder.begin(inbox, true); } } } catch (GLib.Error error) { @@ -970,12 +969,9 @@ public class Application.Client : Gtk.Application { private Geary.Folder? get_folder_from_action_target(GLib.Variant target) { Geary.Folder? folder = null; - string account_id = (string) target.get_child_value(0); + string id = (string) target.get_child_value(0); try { - Geary.AccountInformation? account_config = - this.engine.get_account(account_id); - Geary.Account? account = - this.engine.get_account_instance(account_config); + Geary.Account account = this.engine.get_account_for_id(id); Geary.FolderPath? path = account.to_folder_path( target.get_child_value(1).get_variant() diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index d3c37985..2fc3fae2 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -1695,9 +1695,14 @@ internal class Application.Controller : Geary.BaseObject { private void on_account_available(Geary.AccountInformation info) { Geary.Account? account = null; try { - account = Geary.Engine.instance.get_account_instance(info); - } catch (Error e) { - error("Error creating account instance: %s", e.message); + account = this.application.engine.get_account(info); + } catch (GLib.Error error) { + report_problem(new Geary.ProblemReport(error)); + warning( + "Error creating account %s instance: %s", + info.id, + error.message + ); } if (account != null) { @@ -1720,7 +1725,7 @@ internal class Application.Controller : Geary.BaseObject { Accounts.Manager.Status status) { switch (status) { case Accounts.Manager.Status.ENABLED: - if (!this.application.engine.has_account(changed.id)) { + if (!this.application.engine.has_account(changed)) { try { this.application.engine.add_account(changed); } catch (GLib.Error err) { @@ -1731,7 +1736,7 @@ internal class Application.Controller : Geary.BaseObject { case Accounts.Manager.Status.UNAVAILABLE: case Accounts.Manager.Status.DISABLED: - if (this.application.engine.has_account(changed.id)) { + if (this.application.engine.has_account(changed)) { this.close_account.begin( changed, false, diff --git a/src/client/composer/composer-widget.vala b/src/client/composer/composer-widget.vala index 7274abae..d47af36c 100644 --- a/src/client/composer/composer-widget.vala +++ b/src/client/composer/composer-widget.vala @@ -413,7 +413,7 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface { } } - private Gee.Map accounts; + private Gee.Collection accounts; private string body_html = ""; @@ -478,7 +478,7 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface { try { this.accounts = this.application.engine.get_accounts(); - } catch (Error e) { + } catch (GLib.Error e) { warning("Could not fetch account info: %s", e.message); } @@ -2379,7 +2379,7 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface { // show nothing. if (this.accounts.size < 1 || (this.accounts.size == 1 && - !Geary.traverse(this.accounts.values).first().has_sender_aliases)) { + !Geary.traverse(this.accounts).first().information.has_sender_aliases)) { return false; } @@ -2397,13 +2397,11 @@ public class Composer.Widget : Gtk.EventBox, Geary.BaseInterface { // is set to true if the current message's from address has // been set in the ComboBox. bool set_active = add_account_emails_to_from_list(this.account); - foreach (Geary.AccountInformation info in this.accounts.values) { - try { - Geary.Account a = this.application.engine.get_account_instance(info); - if (a != this.account) - set_active = add_account_emails_to_from_list(a, set_active); - } catch (Error e) { - debug("Error getting account in composer: %s", e.message); + foreach (var account in this.accounts) { + if (account != this.account) { + set_active = add_account_emails_to_from_list( + account, set_active + ); } } diff --git a/src/engine/api/geary-engine.vala b/src/engine/api/geary-engine.vala index 71243002..67298b92 100644 --- a/src/engine/api/geary-engine.vala +++ b/src/engine/api/geary-engine.vala @@ -49,16 +49,20 @@ public class Geary.Engine : BaseObject { /** Determines if any accounts have been added to this instance. */ public bool has_accounts { - get { return this.accounts != null && !this.accounts.is_empty; } + get { return this.is_open && !this.accounts.is_empty; } + } + + /** Determines the number of accounts added to this instance. */ + public uint accounts_count { + get { return this.accounts.size; } } /** Location of the directory containing shared resource files. */ public File? resource_dir { get; private set; default = null; } - private Gee.HashMap? accounts = null; - private Gee.HashMap? account_instances = null; private bool is_initialized = false; private bool is_open = false; + private Gee.List accounts = new Gee.ArrayList(); // Would use a `weak Endpoint` value type for this map instead of // the custom class, but we can't currently reassign built-in @@ -127,19 +131,16 @@ public class Geary.Engine : BaseObject { public async void open_async(GLib.File resource_dir, GLib.Cancellable? cancellable = null) throws GLib.Error { - // initialize *before* opening the Engine ... all initialize code should assume the Engine - // is closed + // initialize *before* opening the Engine ... all initialize + // code should assume the Engine is closed initialize_library(); - if (is_open) - throw new EngineError.ALREADY_OPEN("Geary.Engine instance already open"); + if (is_open) { + throw new EngineError.ALREADY_OPEN("Already open"); + } this.resource_dir = resource_dir; - - accounts = new Gee.HashMap(); - account_instances = new Gee.HashMap(); - - is_open = true; + this.is_open = true; opened(); } @@ -147,58 +148,159 @@ public class Geary.Engine : BaseObject { /** * Uninitializes the engine, and removes all known accounts. */ - public async void close_async(Cancellable? cancellable = null) throws Error { - if (!is_open) - return; + public async void close_async(Cancellable? cancellable = null) + throws GLib.Error { + if (is_open) { + foreach (var account in this.accounts) { + account_unavailable(account.information); + } + this.accounts.clear(); + this.resource_dir = null; + this.is_open = false; - Gee.Collection unavailable_accounts = accounts.values; - accounts.clear(); - - foreach(AccountInformation account in unavailable_accounts) - account_unavailable(account); - - resource_dir = null; - accounts = null; - account_instances = null; - - is_open = false; - closed(); + closed(); + } } /** - * Determines if an account with a specific id has added. + * Determines if an account with a specific configuration has been added. */ - public bool has_account(string id) { - return (this.accounts != null && this.accounts.has_key(id)); + public bool has_account(AccountInformation config) { + return this.accounts.any_match(account => account.information == config); } /** - * Returns a current account given its id. + * Returns the account for the given configuration, if present. * * Throws an error if the engine has not been opened or if the * requested account does not exist. */ - public AccountInformation get_account(string id) throws Error { + public Account get_account(AccountInformation config) throws GLib.Error { check_opened(); - AccountInformation? info = accounts.get(id); - if (info == null) { - throw new EngineError.NOT_FOUND("No such account: %s", id); + Account? account = this.accounts.first_match( + account => account.information == config + ); + if (account == null) { + throw new EngineError.NOT_FOUND("No such account"); } - return info; + return account; } /** - * Returns the current accounts list as a map keyed by account id. + * Returns the account for the given configuration id, if present. + * + * Throws an error if the engine has not been opened or if the + * requested account does not exist. + */ + public Account get_account_for_id(string id) throws GLib.Error { + check_opened(); + + Account? account = this.accounts.first_match( + account => account.information.id == id + ); + if (account == null) { + throw new EngineError.NOT_FOUND("No such account"); + } + return account; + } + + /** + * Returns a read-only collection of current accounts. + * + * The collection is guaranteed to be ordered by {@link + * AccountInformation.compare_ascending}. * * Throws an error if the engine has not been opened. */ - public Gee.Map get_accounts() throws Error { + public Gee.Collection get_accounts() throws GLib.Error { check_opened(); return accounts.read_only_view; } + /** + * Adds the account to the engine. + * + * The account will not be automatically opened, this must be done + * once added. + */ + public void add_account(AccountInformation config) throws GLib.Error { + check_opened(); + + if (has_account(config)) { + throw new EngineError.ALREADY_EXISTS("Account already exists"); + } + + ImapDB.Account local = new ImapDB.Account( + config, + config.data_dir, + this.resource_dir.get_child("sql") + ); + Endpoint incoming_remote = get_shared_endpoint( + config.service_provider, config.incoming + ); + Endpoint outgoing_remote = get_shared_endpoint( + config.service_provider, config.outgoing + ); + + Geary.Account account; + switch (config.service_provider) { + case ServiceProvider.GMAIL: + account = new ImapEngine.GmailAccount( + config, local, incoming_remote, outgoing_remote + ); + break; + + case ServiceProvider.YAHOO: + account = new ImapEngine.YahooAccount( + config, local, incoming_remote, outgoing_remote + ); + break; + + case ServiceProvider.OUTLOOK: + account = new ImapEngine.OutlookAccount( + config, local, incoming_remote, outgoing_remote + ); + break; + + case ServiceProvider.OTHER: + account = new ImapEngine.OtherAccount( + config, local, incoming_remote, outgoing_remote + ); + break; + + default: + assert_not_reached(); + } + + config.notify["ordinal"].connect(on_account_ordinal_changed); + this.accounts.add(account); + sort_accounts(); + account_available(config); + } + + /** + * Removes an account from the engine. + * + * The account must be closed before removing it. + */ + public void remove_account(AccountInformation config) throws GLib.Error { + check_opened(); + + Account account = get_account(config); + if (account.is_open()) { + throw new EngineError.CLOSE_REQUIRED( + "Account must be closed before removal" + ); + } + + config.notify["ordinal"].disconnect(on_account_ordinal_changed); + this.accounts.remove(account); + + account_unavailable(config); + } + /** * Determines if an account's IMAP service can be connected to. */ @@ -300,105 +402,6 @@ public class Geary.Engine : BaseObject { } } - /** - * Creates a Geary.Account from a Geary.AccountInformation (which is what - * other methods in this interface deal in). - */ - public Geary.Account get_account_instance(AccountInformation config) - throws Error { - check_opened(); - - if (account_instances.has_key(config.id)) - return account_instances.get(config.id); - - ImapDB.Account local = new ImapDB.Account( - config, - config.data_dir, - this.resource_dir.get_child("sql") - ); - Endpoint incoming_remote = get_shared_endpoint( - config.service_provider, config.incoming - ); - Endpoint outgoing_remote = get_shared_endpoint( - config.service_provider, config.outgoing - ); - - Geary.Account account; - switch (config.service_provider) { - case ServiceProvider.GMAIL: - account = new ImapEngine.GmailAccount( - config, local, incoming_remote, outgoing_remote - ); - break; - - case ServiceProvider.YAHOO: - account = new ImapEngine.YahooAccount( - config, local, incoming_remote, outgoing_remote - ); - break; - - case ServiceProvider.OUTLOOK: - account = new ImapEngine.OutlookAccount( - config, local, incoming_remote, outgoing_remote - ); - break; - - case ServiceProvider.OTHER: - account = new ImapEngine.OtherAccount( - config, local, incoming_remote, outgoing_remote - ); - break; - - default: - assert_not_reached(); - } - - account_instances.set(config.id, account); - return account; - } - - /** - * Adds the account to be tracked by the engine. - */ - public void add_account(AccountInformation account) throws Error { - check_opened(); - - if (accounts.has_key(account.id)) { - throw new EngineError.ALREADY_EXISTS( - "Account id '%s' already exists", account.id - ); - } - - accounts.set(account.id, account); - account_available(account); - } - - /** - * Removes an account from the engine. - */ - public void remove_account(AccountInformation account) - throws GLib.Error { - check_opened(); - - // Ensure account is closed. - if (this.account_instances.has_key(account.id) && - this.account_instances.get(account.id).is_open()) { - throw new EngineError.CLOSE_REQUIRED( - "Account %s must be closed before removal", account.id - ); - } - - if (this.accounts.has_key(account.id)) { - // Send the account-unavailable signal, account will be - // removed client side. - account_unavailable(account); - - // Then remove the account data from the engine. - this.accounts.unset(account.id); - this.account_instances.unset(account.id); - } - } - /** * Changes the service configuration for an account. * @@ -409,33 +412,28 @@ public class Geary.Engine : BaseObject { * will also be updated so that the new configuration will start * taking effect immediately. */ - public async void update_account_service(AccountInformation account, + public async void update_account_service(AccountInformation config, ServiceInformation updated, GLib.Cancellable? cancellable) throws GLib.Error { - Account? impl = this.account_instances.get(account.id); - if (impl == null) { - throw new EngineError.BAD_PARAMETERS( - "Account has not been added to the engine: %s", account.id - ); - } + Account account = get_account(config); ClientService? service = null; switch (updated.protocol) { case Protocol.IMAP: - account.incoming = updated; - service = impl.incoming; + config.incoming = updated; + service = account.incoming; break; case Protocol.SMTP: - account.outgoing = updated; - service = impl.outgoing; + config.outgoing = updated; + service = account.outgoing; break; } - Endpoint remote = get_shared_endpoint(account.service_provider, updated); + Endpoint remote = get_shared_endpoint(config.service_provider, updated); yield service.update_configuration(updated, remote, cancellable); - account.changed(); + config.changed(); } private Geary.Endpoint get_shared_endpoint(ServiceProvider provider, @@ -489,4 +487,16 @@ public class Geary.Engine : BaseObject { ); } + private void sort_accounts() { + this.accounts.sort((a, b) => { + return AccountInformation.compare_ascending( + a.information, b.information + ); + }); + } + + private void on_account_ordinal_changed() { + sort_accounts(); + } + } diff --git a/test/engine/api/geary-engine-test.vala b/test/engine/api/geary-engine-test.vala index 87adde59..aa98a970 100644 --- a/test/engine/api/geary-engine-test.vala +++ b/test/engine/api/geary-engine-test.vala @@ -58,6 +58,7 @@ class Geary.EngineTest : TestCase { new MockCredentialsMediator(), new RFC822.MailboxAddress(null, "test1@example.com") ); + this.account.set_account_directories(this.tmp, this.tmp); } public override void tear_down () { @@ -72,10 +73,10 @@ class Geary.EngineTest : TestCase { } public void add_account() throws GLib.Error { - assert_false(this.engine.has_account(this.account.id)); + assert_false(this.engine.has_account(this.account)); this.engine.add_account(this.account); - assert_true(this.engine.has_account(this.account.id), "Account not added"); + assert_true(this.engine.has_account(this.account), "Account not added"); try { this.engine.add_account(this.account); @@ -87,23 +88,27 @@ class Geary.EngineTest : TestCase { public void remove_account() throws GLib.Error { this.engine.add_account(this.account); - assert_true(this.engine.has_account(this.account.id)); + assert_true(this.engine.has_account(this.account)); this.engine.remove_account(this.account); - assert_false(this.engine.has_account(this.account.id), "Account not rmoeved"); + assert_false(this.engine.has_account(this.account), "Account not removed"); - // Should not throw an error - this.engine.remove_account(this.account); + try { + this.engine.remove_account(this.account); + assert_not_reached(); + } catch (GLib.Error err) { + // expected + } } public void re_add_account() throws GLib.Error { - assert_false(this.engine.has_account(this.account.id)); + assert_false(this.engine.has_account(this.account)); this.engine.add_account(this.account); this.engine.remove_account(this.account); this.engine.add_account(this.account); - assert_true(this.engine.has_account(this.account.id)); + assert_true(this.engine.has_account(this.account)); } private void delete(File parent) throws Error {