[geary/mjog/engine-api-cleanup: 1/4] Clean up Geary.Engine account API and implementation
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/mjog/engine-api-cleanup: 1/4] Clean up Geary.Engine account API and implementation
- Date: Mon, 18 Nov 2019 22:41:35 +0000 (UTC)
commit ecfd772c07efb8dda725efa5897370f2f89f5c0e
Author: Michael Gratton <mike vee net>
Date: Thu Nov 14 14:44:30 2019 +1100
Clean up Geary.Engine account API and implementation
Keep a single ordered list of accounts around, construct accounts
when their config is first added, and prefer accessing accounts by
config rather than id.
src/client/application/application-client.vala | 22 +-
src/client/application/application-controller.vala | 15 +-
src/client/composer/composer-widget.vala | 18 +-
src/engine/api/geary-engine.vala | 308 +++++++++++----------
test/engine/api/geary-engine-test.vala | 21 +-
5 files changed, 199 insertions(+), 185 deletions(-)
---
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<string, Geary.AccountInformation> accounts;
+ private Gee.Collection<Geary.Account> 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<Geary.AccountInformation>(this.accounts.values).first().has_sender_aliases)) {
+ !Geary.traverse<Geary.Account>(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<string, AccountInformation>? accounts = null;
- private Gee.HashMap<string, Account>? account_instances = null;
private bool is_initialized = false;
private bool is_open = false;
+ private Gee.List<Account> accounts = new Gee.ArrayList<Account>();
// 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<string, AccountInformation>();
- account_instances = new Gee.HashMap<string, Account>();
-
- 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;
-
- Gee.Collection<AccountInformation> unavailable_accounts = accounts.values;
- accounts.clear();
-
- foreach(AccountInformation account in unavailable_accounts)
- account_unavailable(account);
+ 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;
- resource_dir = null;
- accounts = null;
- account_instances = null;
+ closed();
+ }
+ }
- is_open = false;
- closed();
+ /**
+ * Determines if an account with a specific configuration has been added.
+ */
+ public bool has_account(AccountInformation config) {
+ return this.accounts.any_match(account => account.information == config);
}
/**
- * Determines if an account with a specific id has added.
+ * 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 bool has_account(string id) {
- return (this.accounts != null && this.accounts.has_key(id));
+ public Account get_account(AccountInformation config) throws GLib.Error {
+ check_opened();
+
+ Account? account = this.accounts.first_match(
+ account => account.information == config
+ );
+ if (account == null) {
+ throw new EngineError.NOT_FOUND("No such account");
+ }
+ return account;
}
/**
- * Returns a current account given its 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 AccountInformation get_account(string id) throws Error {
+ public Account get_account_for_id(string id) 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.id == id
+ );
+ 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 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<string, AccountInformation> get_accounts() throws Error {
+ public Gee.Collection<Account> 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 {
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
