From 6ce275f61d66450323b51127b69cfb72a742aae1 Mon Sep 17 00:00:00 2001 From: Jim Nelson Date: Tue, 14 May 2013 15:36:33 -0700 Subject: [PATCH] Geary no longer spikes to 100% CPU: Closes #6602 Folders can now be FAST_OPEN'd, which is used by the background account synchronizer to quickly determine what changes have occurred to a folder and get it up-to-date. I've used Geary for a few days with these changes and have seen a dramatic difference in sluggishness. There are still CPU spikes when background work is detected, but not like before and not nearly as sustained. --- src/client/geary-controller.vala | 4 +- src/dbusservice/controller.vala | 2 +- src/dbusservice/dbus-conversations.vala | 4 +- .../abstract/geary-abstract-folder.vala | 3 +- .../api/geary-conversation-monitor.vala | 8 +- src/engine/api/geary-folder.vala | 29 ++++- src/engine/imap-db/imap-db-account.vala | 118 ++++++++++++++---- src/engine/imap-db/imap-db-folder.vala | 2 +- .../imap-db/outbox/smtp-outbox-folder.vala | 6 +- .../imap-engine-account-synchronizer.vala | 2 +- .../imap-engine-generic-account.vala | 2 +- .../imap-engine-generic-folder.vala | 106 +++++++++++----- .../imap/api/imap-folder-properties.vala | 44 ++++++- src/engine/imap/api/imap-folder.vala | 14 +-- 14 files changed, 255 insertions(+), 89 deletions(-) diff --git a/src/client/geary-controller.vala b/src/client/geary-controller.vala index c93a242b..08d6dd0c 100644 --- a/src/client/geary-controller.vala +++ b/src/client/geary-controller.vala @@ -476,7 +476,7 @@ public class GearyController { update_ui(); - current_conversations = new Geary.ConversationMonitor(current_folder, false, + current_conversations = new Geary.ConversationMonitor(current_folder, Geary.Folder.OpenFlags.NONE, ConversationListStore.REQUIRED_FIELDS); if (inboxes.values.contains(current_folder)) { @@ -739,7 +739,7 @@ public class GearyController { main_window.folder_list.select_folder(select_folder); } - folder.open_async.begin(false, inbox_cancellables.get(folder.account)); + folder.open_async.begin(Geary.Folder.OpenFlags.NONE, inbox_cancellables.get(folder.account)); new_messages_monitor.add_folder(folder, inbox_cancellables.get(folder.account)); } diff --git a/src/dbusservice/controller.vala b/src/dbusservice/controller.vala index 102c1e25..2d1742bc 100644 --- a/src/dbusservice/controller.vala +++ b/src/dbusservice/controller.vala @@ -62,7 +62,7 @@ public class Geary.DBus.Controller { warning("No inbox folder found"); return; } - yield folder.open_async(false, null); + yield folder.open_async(Geary.Folder.OpenFlags.NONE, null); conversations = new Geary.DBus.Conversations(folder); diff --git a/src/dbusservice/dbus-conversations.vala b/src/dbusservice/dbus-conversations.vala index 2de5b6f7..c35935c4 100644 --- a/src/dbusservice/dbus-conversations.vala +++ b/src/dbusservice/dbus-conversations.vala @@ -27,8 +27,8 @@ public class Geary.DBus.Conversations : Object { public Conversations(Geary.Folder folder) { this.folder = folder; - conversations = new Geary.ConversationMonitor(folder, false, Geary.Email.Field.ENVELOPE | - Geary.Email.Field.FLAGS); + conversations = new Geary.ConversationMonitor(folder, Geary.Folder.OpenFlags.NONE, + Geary.Email.Field.ENVELOPE | Geary.Email.Field.FLAGS); start_monitoring_async.begin(); } diff --git a/src/engine/abstract/geary-abstract-folder.vala b/src/engine/abstract/geary-abstract-folder.vala index 57195a2d..fdc47be0 100644 --- a/src/engine/abstract/geary-abstract-folder.vala +++ b/src/engine/abstract/geary-abstract-folder.vala @@ -68,7 +68,8 @@ public abstract class Geary.AbstractFolder : BaseObject, Geary.Folder { public abstract Geary.Folder.OpenState get_open_state(); - public abstract async void open_async(bool readonly, Cancellable? cancellable = null) throws Error; + public abstract async bool open_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable = null) + throws Error; public abstract async void wait_for_open_async(Cancellable? cancellable = null) throws Error; diff --git a/src/engine/api/geary-conversation-monitor.vala b/src/engine/api/geary-conversation-monitor.vala index 19e4407b..972dbcfb 100644 --- a/src/engine/api/geary-conversation-monitor.vala +++ b/src/engine/api/geary-conversation-monitor.vala @@ -172,7 +172,7 @@ public class Geary.ConversationMonitor : BaseObject { public bool is_monitoring { get; private set; default = false; } private Geary.Email.Field required_fields; - private bool readonly; + private Geary.Folder.OpenFlags open_flags; private Gee.Set conversations = new Gee.HashSet(); private Gee.HashMap geary_id_map = new Gee.HashMap< Geary.EmailIdentifier, ImplConversation>(); @@ -307,9 +307,9 @@ public class Geary.ConversationMonitor : BaseObject { folder.to_string()); } - public ConversationMonitor(Geary.Folder folder, bool readonly, Geary.Email.Field required_fields) { + public ConversationMonitor(Geary.Folder folder, Geary.Folder.OpenFlags open_flags, Geary.Email.Field required_fields) { this.folder = folder; - this.readonly = readonly; + this.open_flags = open_flags; this.required_fields = required_fields | REQUIRED_FIELDS; folder.account.information.notify["imap-credentials"].connect(on_imap_credentials_notified); @@ -402,7 +402,7 @@ public class Geary.ConversationMonitor : BaseObject { bool reseed_now = (folder.get_open_state() != Geary.Folder.OpenState.CLOSED); try { - yield folder.open_async(readonly, cancellable); + yield folder.open_async(open_flags, cancellable); } catch (Error err) { is_monitoring = false; diff --git a/src/engine/api/geary-folder.vala b/src/engine/api/geary-folder.vala index f3ac6c4a..34c9ac5e 100644 --- a/src/engine/api/geary-folder.vala +++ b/src/engine/api/geary-folder.vala @@ -49,6 +49,28 @@ public interface Geary.Folder : BaseObject { REMOVED } + /** + * Flags describing open modifiers. + * + * * FAST_OPEN: perform the minimal amount of activity possible to open the folder + * and be synchronized with the server. This may mean some attributes of + * the messages (such as their flags or other metadata) may not be up-to-date + * when the folder opens. Not all folders will support this flag. + */ + [Flags] + public enum OpenFlags { + NONE = 0, + FAST_OPEN; + + public bool is_any_set(OpenFlags flags) { + return (this & flags) != 0; + } + + public bool is_all_set(OpenFlags flags) { + return (this & flags) == flags; + } + } + /** * Flags used for retrieving email. * @@ -245,13 +267,16 @@ public interface Geary.Folder : BaseObject { * associated signals will be fired as well. * * If the Folder has been opened previously, an internal open count is incremented and the - * method returns. There are no other side-effects. + * method returns. There are no other side-effects. This means it's possible for the + * open_flags parameter to be ignored. See the returned result for more information. * * A Folder may be reopened after it has been closed. This allows for Folder objects to be * emitted by the Account object cheaply, but the client should only have a few open at a time, * as each may represent an expensive resource (such as a network connection). + * + * Returns false if already opened. */ - public abstract async void open_async(bool readonly, Cancellable? cancellable = null) throws Error; + public abstract async bool open_async(OpenFlags open_flags, Cancellable? cancellable = null) throws Error; /** * Wait for the Folder to become fully open or fails to open due to error. If not opened diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index 3ddcd9d0..f8653e58 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -133,7 +133,11 @@ private class Geary.ImapDB.Account : BaseObject { }, cancellable); } - public async void update_folder_async(Geary.Imap.Folder imap_folder, Cancellable? cancellable = null) + /** + * Only updates folder's STATUS message count, attributes, recent, and unseen; UIDVALIDITY and UIDNEXT + * updated when the folder is SELECT/EXAMINED (see update_folder_select_examine_async()) + */ + public async void update_folder_status_async(Geary.Imap.Folder imap_folder, Cancellable? cancellable) throws Error { check_open(); @@ -151,34 +155,19 @@ private class Geary.ImapDB.Account : BaseObject { Db.Statement stmt; if (parent_id != Db.INVALID_ROWID) { stmt = cx.prepare( - "UPDATE FolderTable SET uid_validity=?, uid_next=?, attributes=? " - + "WHERE parent_id=? AND name=?"); - stmt.bind_int64(0, (properties.uid_validity != null) ? properties.uid_validity.value - : Imap.UIDValidity.INVALID); - stmt.bind_int64(1, (properties.uid_next != null) ? properties.uid_next.value - : Imap.UID.INVALID); - stmt.bind_string(2, properties.attrs.serialize()); - stmt.bind_rowid(3, parent_id); - stmt.bind_string(4, path.basename); + "UPDATE FolderTable SET attributes=? WHERE parent_id=? AND name=?"); + stmt.bind_string(0, properties.attrs.serialize()); + stmt.bind_rowid(1, parent_id); + stmt.bind_string(2, path.basename); } else { stmt = cx.prepare( - "UPDATE FolderTable SET uid_validity=?, uid_next=?, attributes=? " - + "WHERE parent_id IS NULL AND name=?"); - stmt.bind_int64(0, (properties.uid_validity != null) ? properties.uid_validity.value - : Imap.UIDValidity.INVALID); - stmt.bind_int64(1, (properties.uid_next != null) ? properties.uid_next.value - : Imap.UID.INVALID); - stmt.bind_string(2, properties.attrs.serialize()); - stmt.bind_string(3, path.basename); + "UPDATE FolderTable SET attributes=? WHERE parent_id IS NULL AND name=?"); + stmt.bind_string(0, properties.attrs.serialize()); + stmt.bind_string(1, path.basename); } stmt.exec(); - if (properties.select_examine_messages >= 0) { - do_update_last_seen_total(cx, parent_id, path.basename, properties.select_examine_messages, - cancellable); - } - if (properties.status_messages >= 0) { do_update_last_seen_status_total(cx, parent_id, path.basename, properties.status_messages, cancellable); @@ -187,10 +176,83 @@ private class Geary.ImapDB.Account : BaseObject { return Db.TransactionOutcome.COMMIT; }, cancellable); - // update properties in the local folder + // update appropriate properties in the local folder ImapDB.Folder? db_folder = get_local_folder(path); - if (db_folder != null) - db_folder.set_properties(properties); + if (db_folder != null) { + Imap.FolderProperties local_properties = db_folder.get_properties(); + + local_properties.unseen = properties.unseen; + local_properties.recent = properties.recent; + local_properties.attrs = properties.attrs; + + if (properties.status_messages >= 0) + local_properties.set_status_message_count(properties.status_messages, false); + } + } + + /** + * Only updates folder's SELECT/EXAMINE message count, UIDVALIDITY, UIDNEXT, unseen, and recent. + * See also update_folder_status_async(). + */ + public async void update_folder_select_examine_async(Geary.Imap.Folder imap_folder, Cancellable? cancellable) + throws Error { + check_open(); + + Geary.Imap.FolderProperties properties = imap_folder.get_properties(); + Geary.FolderPath path = imap_folder.get_path(); + + yield db.exec_transaction_async(Db.TransactionType.RW, (cx) => { + int64 parent_id; + if (!do_fetch_parent_id(cx, path, true, out parent_id, cancellable)) { + debug("Unable to find parent ID of %s to update properties", path.to_string()); + + return Db.TransactionOutcome.ROLLBACK; + } + + int64 uid_validity = (properties.uid_validity != null) ? properties.uid_validity.value + : Imap.UIDValidity.INVALID; + int64 uid_next = (properties.uid_next != null) ? properties.uid_next.value + : Imap.UID.INVALID; + + Db.Statement stmt; + if (parent_id != Db.INVALID_ROWID) { + stmt = cx.prepare( + "UPDATE FolderTable SET uid_validity=?, uid_next=? WHERE parent_id=? AND name=?"); + stmt.bind_int64(0, uid_validity); + stmt.bind_int64(1, uid_next); + stmt.bind_rowid(2, parent_id); + stmt.bind_string(3, path.basename); + } else { + stmt = cx.prepare( + "UPDATE FolderTable SET uid_validity=?, uid_next=? WHERE parent_id IS NULL AND name=?"); + stmt.bind_int64(0, uid_validity); + stmt.bind_int64(1, uid_next); + stmt.bind_string(2, path.basename); + } + + stmt.exec(); + + if (properties.select_examine_messages >= 0) { + do_update_last_seen_select_examine_total(cx, parent_id, path.basename, + properties.select_examine_messages, cancellable); + } + + return Db.TransactionOutcome.COMMIT; + }, cancellable); + + // update appropriate properties in the local folder + ImapDB.Folder? db_folder = get_local_folder(path); + if (db_folder != null) { + Imap.FolderProperties local_properties = db_folder.get_properties(); + + local_properties.unseen = properties.unseen; + local_properties.recent = properties.recent; + local_properties.uid_validity = properties.uid_validity; + local_properties.uid_next = properties.uid_next; + + if (properties.select_examine_messages >= 0) + local_properties.set_select_examine_message_count(properties.select_examine_messages); + } } private void initialize_contacts(Cancellable? cancellable = null) throws Error { @@ -664,11 +726,13 @@ private class Geary.ImapDB.Account : BaseObject { return (parent_path == null ? null : parent_path.get_child(name)); } - private void do_update_last_seen_total(Db.Connection cx, int64 parent_id, string name, int total, + // For SELECT/EXAMINE responses, not STATUS responses + private void do_update_last_seen_select_examine_total(Db.Connection cx, int64 parent_id, string name, int total, Cancellable? cancellable) throws Error { do_update_total(cx, parent_id, name, "last_seen_total", total, cancellable); } + // For STATUS responses, not SELECT/EXAMINE responses private void do_update_last_seen_status_total(Db.Connection cx, int64 parent_id, string name, int total, Cancellable? cancellable) throws Error { do_update_total(cx, parent_id, name, "last_seen_status_total", total, cancellable); diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 370887a3..733dde14 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -93,7 +93,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { this.properties = properties; } - public async void open_async(bool readonly, Cancellable? cancellable = null) throws Error { + public async void open_async(Cancellable? cancellable = null) throws Error { if (opened) throw new EngineError.ALREADY_OPEN("%s already open", to_string()); diff --git a/src/engine/imap-db/outbox/smtp-outbox-folder.vala b/src/engine/imap-db/outbox/smtp-outbox-folder.vala index bbff4905..0702f6a2 100644 --- a/src/engine/imap-db/outbox/smtp-outbox-folder.vala +++ b/src/engine/imap-db/outbox/smtp-outbox-folder.vala @@ -212,12 +212,14 @@ private class Geary.SmtpOutboxFolder : Geary.AbstractFolder, Geary.FolderSupport throw new EngineError.OPEN_REQUIRED("Outbox not open"); } - public override async void open_async(bool readonly, Cancellable? cancellable = null) + public override async bool open_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable = null) throws Error { if (open_count++ > 0) - return; + return false; notify_opened(Geary.Folder.OpenState.LOCAL, properties.email_total); + + return true; } public override async void close_async(Cancellable? cancellable = null) throws Error { diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 71beab34..604dfa2c 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -237,7 +237,7 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject { } try { - yield folder.open_async(true, bg_cancellable); + yield folder.open_async(Folder.OpenFlags.FAST_OPEN, bg_cancellable); yield folder.wait_for_open_async(bg_cancellable); } catch (Error err) { // don't need to close folder; if either calls throws an error, the folder is not open diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index d369c85d..457774ac 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -337,7 +337,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.AbstractAccount { } try { - yield local.update_folder_async(remote_folder, cancellable); + yield local.update_folder_status_async(remote_folder, cancellable); } catch (Error update_error) { debug("Unable to update local folder %s with remote properties: %s", remote_folder.to_string(), update_error.message); diff --git a/src/engine/imap-engine/imap-engine-generic-folder.vala b/src/engine/imap-engine/imap-engine-generic-folder.vala index 51a6750b..a9a563ea 100644 --- a/src/engine/imap-engine/imap-engine-generic-folder.vala +++ b/src/engine/imap-engine/imap-engine-generic-folder.vala @@ -11,6 +11,8 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde private const Geary.Email.Field NORMALIZATION_FIELDS = Geary.Email.Field.PROPERTIES | Geary.Email.Field.FLAGS | ImapDB.Folder.REQUIRED_FOR_DUPLICATE_DETECTION; + private const Geary.Email.Field FAST_NORMALIZATION_FIELDS = + Geary.Email.Field.PROPERTIES | ImapDB.Folder.REQUIRED_FOR_DUPLICATE_DETECTION; public override Account account { get { return _account; } } internal ImapDB.Folder local_folder { get; protected set; } @@ -102,8 +104,13 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde return (remote_count >= 0) ? remote_count : last_seen_remote_count; } - private async bool normalize_folders(Geary.Imap.Folder remote_folder, Cancellable? cancellable) throws Error { - debug("normalize_folders %s", to_string()); + private async bool normalize_folders(Geary.Imap.Folder remote_folder, Geary.Folder.OpenFlags open_flags, + Cancellable? cancellable) throws Error { + bool is_fast_open = open_flags.is_any_set(Geary.Folder.OpenFlags.FAST_OPEN); + if (is_fast_open) + debug("fast-open normalize_folders %s", to_string()); + else + debug("normalize_folders %s", to_string()); Geary.Imap.FolderProperties local_properties = local_folder.get_properties(); Geary.Imap.FolderProperties remote_properties = remote_folder.get_properties(); @@ -147,17 +154,54 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde return true; } + // if UIDNEXT has changed, that indicates messages have been appended (and possibly removed) + int64 uidnext_diff = remote_properties.uid_next.value - local_properties.uid_next.value; + + // fast-open means no updating of flags, so if UIDNEXT is the same as last time AND the total count + // of email is the same, then nothing has been added or removed (but flags may have changed) + if (is_fast_open && uidnext_diff == 0 && local_properties.email_total == remote_properties.email_total) { + debug("No messages added/removed in fast-open of %s, normalization completed", to_string()); + + return true; + } + Gee.Collection all_appended_ids = new Gee.ArrayList(); Gee.Collection all_locally_appended_ids = new Gee.ArrayList(); Gee.Collection all_removed_ids = new Gee.ArrayList(); Gee.Map all_flags_changed = new Gee.HashMap(); + // to match all flags and find all removed interior to the local store's vector of messages, start from + // local earliest message and work upwards Geary.Imap.EmailIdentifier current_start_id = new Geary.Imap.EmailIdentifier(earliest_uid, local_folder.get_path()); + + // if fast-open and the difference in UIDNEXT values equals the difference in message count, then only + // an append could have happened, so only pull in the new messages ... note that this is not foolproof, + // as UIDs are not guaranteed to increase by 1; however, this is a standard implementation practice, + // so it's worth looking for + // + // (Also, this cannot fail; if this situation exists, then it cannot by definition indicate another + // situation, esp. messages being removed.) + if (is_fast_open) { + if (uidnext_diff == (remote_properties.select_examine_messages - local_properties.select_examine_messages)) { + current_start_id = new Geary.Imap.EmailIdentifier(latest_uid.next(), local_folder.get_path()); + + debug("fast-open %s: Messages only appended (local/remote UIDNEXT=%s/%s total=%d/%d diff=%s), only gathering new mail at %s", + to_string(), local_properties.uid_next.to_string(), remote_properties.uid_next.to_string(), + local_properties.select_examine_messages, remote_properties.select_examine_messages, uidnext_diff.to_string(), + current_start_id.to_string()); + } else { + debug("fast-open %s: Messages appended/removed (local/remote UIDNEXT=%s/%s total=%d/%d diff=%s)", to_string(), + local_properties.uid_next.to_string(), remote_properties.uid_next.to_string(), + local_properties.select_examine_messages, remote_properties.select_examine_messages, uidnext_diff.to_string()); + } + } + for (;;) { // Get the local emails in the range ... use PARTIAL_OK to ensure all emails are normalized Gee.List? old_local = yield local_folder.list_email_by_id_async( - current_start_id, NORMALIZATION_CHUNK_COUNT, NORMALIZATION_FIELDS, + current_start_id, NORMALIZATION_CHUNK_COUNT, + is_fast_open ? FAST_NORMALIZATION_FIELDS : NORMALIZATION_FIELDS, ImapDB.Folder.ListFlags.PARTIAL_OK, cancellable); // verify still open @@ -213,23 +257,26 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde break; if (remote_uid != null && local_uid != null && remote_uid.value == local_uid.value) { - // same, update flags (if changed) and move on - // Because local is PARTIAL_OK, EmailFlags may not be present - Geary.Imap.EmailFlags? local_email_flags = (Geary.Imap.EmailFlags) local_email.email_flags; - Geary.Imap.EmailFlags remote_email_flags = (Geary.Imap.EmailFlags) remote_email.email_flags; - - if ((local_email_flags == null) || !local_email_flags.equal_to(remote_email_flags)) { - // check before writebehind - if (replay_queue.query_local_writebehind_operation(ReplayOperation.WritebehindOperation.UPDATE_FLAGS, - remote_email.id, (Imap.EmailFlags) remote_email.email_flags)) { - to_create_or_merge.add(remote_email); - all_flags_changed.set(remote_email.id, remote_email.email_flags); - - Logging.debug(Logging.Flag.FOLDER_NORMALIZATION, "%s: merging remote ID %s", - to_string(), remote_email.id.to_string()); - } else { - Logging.debug(Logging.Flag.FOLDER_NORMALIZATION, "%s: writebehind cancelled for merge of %s", - to_string(), remote_email.id.to_string()); + // only update flags if not doing a fast-open + if (!is_fast_open) { + // same, update flags (if changed) and move on + // Because local is PARTIAL_OK, EmailFlags may not be present + Geary.Imap.EmailFlags? local_email_flags = (Geary.Imap.EmailFlags) local_email.email_flags; + Geary.Imap.EmailFlags remote_email_flags = (Geary.Imap.EmailFlags) remote_email.email_flags; + + if ((local_email_flags == null) || !local_email_flags.equal_to(remote_email_flags)) { + // check before writebehind + if (replay_queue.query_local_writebehind_operation(ReplayOperation.WritebehindOperation.UPDATE_FLAGS, + remote_email.id, (Imap.EmailFlags) remote_email.email_flags)) { + to_create_or_merge.add(remote_email); + all_flags_changed.set(remote_email.id, remote_email.email_flags); + + Logging.debug(Logging.Flag.FOLDER_NORMALIZATION, "%s: merging remote ID %s", + to_string(), remote_email.id.to_string()); + } else { + Logging.debug(Logging.Flag.FOLDER_NORMALIZATION, "%s: writebehind cancelled for merge of %s", + to_string(), remote_email.id.to_string()); + } } } @@ -408,9 +455,10 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde throw new EngineError.ALREADY_CLOSED("%s failed to open", to_string()); } - public override async void open_async(bool readonly, Cancellable? cancellable = null) throws Error { + public override async bool open_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable = null) + throws Error { if (open_count++ > 0) - return; + return false; remote_semaphore = new Geary.Nonblocking.ReportingSemaphore(false); @@ -418,7 +466,7 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde replay_queue = new ReplayQueue(get_path().to_string(), remote_semaphore); try { - yield local_folder.open_async(readonly, cancellable); + yield local_folder.open_async(cancellable); } catch (Error err) { notify_open_failed(OpenFailed.LOCAL_FAILED, err); @@ -436,22 +484,24 @@ private class Geary.ImapEngine.GenericFolder : Geary.AbstractFolder, Geary.Folde // wait_for_remote_ready_async(), which uses a NonblockingSemaphore to indicate that the remote // is open (or has failed to open). This allows for early calls to list and fetch emails // can work out of the local cache until the remote is ready. - open_remote_async.begin(readonly, cancellable); + open_remote_async.begin(open_flags, cancellable); + + return true; } - private async void open_remote_async(bool readonly, Cancellable? cancellable) { + private async void open_remote_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable) { try { debug("Opening remote %s", to_string()); Imap.Folder folder = (Imap.Folder) yield remote.fetch_folder_async(local_folder.get_path(), cancellable); - yield folder.open_async(readonly, cancellable); + yield folder.open_async(cancellable); // allow subclasses to examine the opened folder and resolve any vital // inconsistencies - if (yield normalize_folders(folder, cancellable)) { + if (yield normalize_folders(folder, open_flags, cancellable)) { // update flags, properties, etc. - yield local.update_folder_async(folder, cancellable); + yield local.update_folder_select_examine_async(folder, cancellable); // signals folder.messages_appended.connect(on_remote_messages_appended); diff --git a/src/engine/imap/api/imap-folder-properties.vala b/src/engine/imap/api/imap-folder-properties.vala index 7f95a141..c666ba7e 100644 --- a/src/engine/imap/api/imap-folder-properties.vala +++ b/src/engine/imap/api/imap-folder-properties.vala @@ -4,6 +4,40 @@ * (version 2.1 or later). See the COPYING file in this distribution. */ +/** + * Geary.Imap.FolderProperties represent the Geary API's notion of FolderProperties but + * also hangs additional useful information available to IMAP-specific code (in the Engine, + * that includes imap, imap-engine, and imap-db). + * + * One important concept here is that there are two IMAP commands that return this information: + * STATUS (which is used by the background folder monitor to watch for specific events) and + * SELECT/EXAMINE (which is used to "enter" or "cd" into a folder and perform operations on mail + * within). + * + * Experience has shown that these commands are *not* guaranteed to return the same information, + * even if no state has changed on the server. This would seem to be a server bug, but one that + * has to be worked around. + * + * In any event, the properties here are updated by the following logic: + * + * When a folder is first "seen" by Geary, it generates an Imap.FolderProperties object with all + * the fields filled in except for status_messages or select_examine_messages, depending on which + * command was used to discover it. (In practice, the folder will be first recognized via STATUS, + * but this isn't guaranteed.) + * + * When new STATUS information comes in, this object's status_messages, unseen, recent, and attrs + * fields are updated. + * + * When a SELECT/EXAMINE occurs on this folder, this object's select_examine_messages, unseen, + * recent, uid_validity, and uid_next are updated. + * + * Over time, this object accumulates information depending on what operation was last + * performed on it. + * + * The base class' email_total is updated when either *_messages is updated; however, SELECT/EXAMINE + * is considered more authoritative than STATUS. + */ + public class Geary.Imap.FolderProperties : Geary.FolderProperties { /** * -1 if the Folder was not opened via SELECT or EXAMINE. @@ -13,11 +47,11 @@ public class Geary.Imap.FolderProperties : Geary.FolderProperties { * -1 if the FolderProperties were not obtained via a STATUS command */ public int status_messages { get; private set; } - public int unseen { get; private set; } - public int recent { get; private set; } - public UIDValidity? uid_validity { get; private set; } - public UID? uid_next { get; private set; } - public MailboxAttributes attrs { get; private set; } + public int unseen { get; internal set; } + public int recent { get; internal set; } + public UIDValidity? uid_validity { get; internal set; } + public UID? uid_next { get; internal set; } + public MailboxAttributes attrs { get; internal set; } // Note that unseen from SELECT/EXAMINE is the *position* of the first unseen message, // not the total unseen count, so it should not be passed in here, but rather the unseen diff --git a/src/engine/imap/api/imap-folder.vala b/src/engine/imap/api/imap-folder.vala index 9a67da71..ff288de0 100644 --- a/src/engine/imap/api/imap-folder.vala +++ b/src/engine/imap/api/imap-folder.vala @@ -10,7 +10,6 @@ private class Geary.Imap.Folder : BaseObject { private ClientSessionManager session_mgr; private MailboxInformation info; private Geary.FolderPath path; - private Trillian readonly; private Imap.FolderProperties properties; private Mailbox? mailbox = null; @@ -26,8 +25,6 @@ private class Geary.Imap.Folder : BaseObject { this.info = info; this.path = path; - readonly = Trillian.UNKNOWN; - properties = new Imap.FolderProperties.status(status, info.attrs); } @@ -37,8 +34,6 @@ private class Geary.Imap.Folder : BaseObject { this.info = info; this.path = path; - readonly = Trillian.UNKNOWN; - properties = new Imap.FolderProperties(0, 0, 0, null, null, info.attrs); } @@ -50,15 +45,11 @@ private class Geary.Imap.Folder : BaseObject { return properties; } - public async void open_async(bool readonly, Cancellable? cancellable = null) throws Error { + public async void open_async(Cancellable? cancellable = null) throws Error { if (mailbox != null) throw new EngineError.ALREADY_OPEN("%s already open", to_string()); - mailbox = yield session_mgr.select_examine_mailbox(path, info.delim, !readonly, - cancellable); - - // update with new information - this.readonly = Trillian.from_boolean(readonly); + mailbox = yield session_mgr.select_mailbox(path, info.delim, cancellable); // connect to signals mailbox.exists_altered.connect(on_exists_altered); @@ -86,7 +77,6 @@ private class Geary.Imap.Folder : BaseObject { mailbox.disconnected.disconnect(on_disconnected); mailbox = null; - readonly = Trillian.UNKNOWN; } private void on_exists_altered(int old_exists, int new_exists) {