From 744cde0c2f9e8464f5e48fcad102490ad3262121 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Fri, 15 Feb 2019 13:40:04 +1100 Subject: [PATCH] Don't update a folder's unread email count during normalisation Updating the unread count after opening a folder and finding email that has an unexpected unread status messes up the count obtained from the server, which has already taken these messages into account. Here, both the main normalisation process and the email flag updater are prevented from adjusting the unread count for a folder when they encounter email that are new and unread, or have an unread status different from what was last seen by the engine. See #213 --- src/engine/api/geary-folder.vala | 8 +- src/engine/imap-db/imap-db-folder.vala | 32 +++++--- .../imap-engine-minimal-folder.vala | 16 +++- .../imap-engine-abstract-list-email.vala | 16 +++- .../replay-ops/imap-engine-create-email.vala | 8 +- .../replay-ops/imap-engine-fetch-email.vala | 4 +- .../replay-ops/imap-engine-replay-append.vala | 4 +- test/engine/imap-db/imap-db-folder-test.vala | 74 ++++++++++++++----- 8 files changed, 116 insertions(+), 46 deletions(-) diff --git a/src/engine/api/geary-folder.vala b/src/engine/api/geary-folder.vala index 227379f7..f8c0cea2 100644 --- a/src/engine/api/geary-folder.vala +++ b/src/engine/api/geary-folder.vala @@ -188,8 +188,12 @@ public abstract class Geary.Folder : BaseObject { /** * Direction of list traversal (if not set, from newest to oldest). */ - OLDEST_TO_NEWEST; - + OLDEST_TO_NEWEST, + /** + * Internal use only, prevents flag changes updating unread count. + */ + NO_UNREAD_UPDATE; + public bool is_any_set(ListFlags flags) { return (this & flags) != 0; } diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 53f8c5fb..8e22b5cf 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -285,8 +285,11 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { // function (see ImapDB.EmailIdentifier.promote_with_message_id). This // means if you've hashed the collection of EmailIdentifiers prior, you may // not be able to find them after this function. Be warned. - public async Gee.Map create_or_merge_email_async(Gee.Collection emails, - Cancellable? cancellable) throws Error { + public async Gee.Map + create_or_merge_email_async(Gee.Collection emails, + bool update_totals, + GLib.Cancellable? cancellable) + throws GLib.Error { Gee.HashMap results = new Gee.HashMap(); Gee.ArrayList list = traverse(emails).to_array_list(); @@ -316,22 +319,27 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { // have all the fields but after the create/merge now does if (post_fields.is_all_set(Geary.Email.Field.ALL) && !pre_fields.is_all_set(Geary.Email.Field.ALL)) complete_ids.add(email.id); - - // Update unread count in DB. - do_add_to_unread_count(cx, unread_change, cancellable); - - total_unread_change += unread_change; + + if (update_totals) { + // Update unread count in DB. + do_add_to_unread_count(cx, unread_change, cancellable); + total_unread_change += unread_change; + } } - + return Db.TransactionOutcome.COMMIT; }, cancellable); if (updated_contacts.size > 0) contact_store.update_contacts(updated_contacts); - - // Update the email_unread properties. - properties.set_status_unseen((properties.email_unread + total_unread_change).clamp(0, int.MAX)); - + + if (update_totals) { + // Update the email_unread properties. + properties.set_status_unseen( + (properties.email_unread + total_unread_change).clamp(0, int.MAX) + ); + } + if (complete_ids.size > 0) email_complete(complete_ids); diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala b/src/engine/imap-engine/imap-engine-minimal-folder.vala index e76c1987..d5827733 100644 --- a/src/engine/imap-engine/imap-engine-minimal-folder.vala +++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala @@ -530,10 +530,15 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport Gee.Set inserted_ids = new Gee.HashSet(); Gee.Set locally_inserted_ids = new Gee.HashSet(); if (to_create.size > 0) { - Gee.Map? created_or_merged = yield local_folder.create_or_merge_email_async( - to_create, cancellable); + // Don't update the unread count here, since it'll get + // updated once normalisation has finished anyway. See + // also Issue #213. + Gee.Map? created_or_merged = + yield local_folder.create_or_merge_email_async( + to_create, false, cancellable + ); assert(created_or_merged != null); - + // it's possible a large number of messages have come in, so process them in the // background yield Nonblocking.Concurrent.global.schedule_async(() => { @@ -1486,7 +1491,10 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport Gee.List? list_remote = yield list_email_by_sparse_id_async( local_map.keys, Email.Field.FLAGS, - Geary.Folder.ListFlags.FORCE_UPDATE, + Folder.ListFlags.FORCE_UPDATE | + // Updating read/unread count here breaks the unread + // count, so don't do it. See issue #213. + Folder.ListFlags.NO_UNREAD_UPDATE, cancellable ); if (list_remote == null || list_remote.is_empty) diff --git a/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala b/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala index 8adf79e2..16ef683d 100644 --- a/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala +++ b/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala @@ -18,6 +18,7 @@ private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.Sen public Imap.MessageSet msg_set; public Geary.Email.Field unfulfilled_fields; public Geary.Email.Field required_fields; + public bool update_unread; // OUT public Gee.Set created_ids = new Gee.HashSet(); @@ -26,12 +27,14 @@ private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.Sen ImapDB.Folder local, Imap.MessageSet msg_set, Geary.Email.Field unfulfilled_fields, - Geary.Email.Field required_fields) { + Geary.Email.Field required_fields, + bool update_unread) { this.remote = remote; this.local = local; this.msg_set = msg_set; this.unfulfilled_fields = unfulfilled_fields; this.required_fields = required_fields; + this.update_unread = update_unread; } public override async Object? execute_async(Cancellable? cancellable) throws Error { @@ -43,8 +46,12 @@ private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.Sen return null; // TODO: create_or_merge_email_async() should only write if something has changed - Gee.Map created_or_merged = yield this.local.create_or_merge_email_async( - list, cancellable); + Gee.Map created_or_merged = + yield this.local.create_or_merge_email_async( + list, + this.update_unread, + cancellable + ); for (int ctr = 0; ctr < list.size; ctr++) { Geary.Email email = list[ctr]; @@ -171,7 +178,8 @@ private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.Sen this.owner.local_folder, msg_set, unfulfilled_fields, - required_fields + required_fields, + !this.flags.is_any_set(NO_UNREAD_UPDATE) ); batch.add(remote_op); } diff --git a/src/engine/imap-engine/replay-ops/imap-engine-create-email.vala b/src/engine/imap-engine/replay-ops/imap-engine-create-email.vala index 9cd5a89a..9062c235 100644 --- a/src/engine/imap-engine/replay-ops/imap-engine-create-email.vala +++ b/src/engine/imap-engine/replay-ops/imap-engine-create-email.vala @@ -56,8 +56,12 @@ private class Geary.ImapEngine.CreateEmail : Geary.ImapEngine.SendReplayOperatio if (created_id != null) { // TODO: need to prevent gaps that may occur here Geary.Email created = new Geary.Email(created_id); - Gee.Map results = yield engine.local_folder.create_or_merge_email_async( - Geary.iterate(created).to_array_list(), cancellable); + Gee.Map results = + yield this.engine.local_folder.create_or_merge_email_async( + Geary.iterate(created).to_array_list(), + true, + this.cancellable + ); if (results.size > 0) { created_id = Collection.get_first(results.keys).id; } else { diff --git a/src/engine/imap-engine/replay-ops/imap-engine-fetch-email.vala b/src/engine/imap-engine/replay-ops/imap-engine-fetch-email.vala index e1a88844..5ecdc200 100644 --- a/src/engine/imap-engine/replay-ops/imap-engine-fetch-email.vala +++ b/src/engine/imap-engine/replay-ops/imap-engine-fetch-email.vala @@ -110,8 +110,8 @@ private class Geary.ImapEngine.FetchEmail : Geary.ImapEngine.SendReplayOperation throw new EngineError.NOT_FOUND("Unable to fetch %s in %s", id.to_string(), engine.to_string()); Gee.Map created_or_merged = - yield engine.local_folder.create_or_merge_email_async( - list, cancellable + yield this.engine.local_folder.create_or_merge_email_async( + list, true, this.cancellable ); Geary.Email email = list[0]; diff --git a/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala b/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala index 9a5f9165..1908c64f 100644 --- a/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala +++ b/src/engine/imap-engine/replay-ops/imap-engine-replay-append.vala @@ -90,7 +90,9 @@ private class Geary.ImapEngine.ReplayAppend : Geary.ImapEngine.ReplayOperation { // need to report both if it was created (not known before) and appended (which // could mean created or simply a known email associated with this folder) Gee.Map created_or_merged = - yield this.owner.local_folder.create_or_merge_email_async(list, this.cancellable); + yield this.owner.local_folder.create_or_merge_email_async( + list, true, this.cancellable + ); foreach (Geary.Email email in created_or_merged.keys) { // true means created if (created_or_merged.get(email)) { diff --git a/test/engine/imap-db/imap-db-folder-test.vala b/test/engine/imap-db/imap-db-folder-test.vala index abb3fefd..ce32de7c 100644 --- a/test/engine/imap-db/imap-db-folder-test.vala +++ b/test/engine/imap-db/imap-db-folder-test.vala @@ -17,7 +17,9 @@ class Geary.ImapDB.FolderTest : TestCase { public FolderTest() { base("Geary.ImapDB.FolderTest"); - add_test("create_email", create_email); + add_test("create_read_email", create_read_email); + add_test("create_unread_email", create_unread_email); + add_test("create_no_unread_update", create_no_unread_update); add_test("merge_email", merge_email); add_test("merge_add_flags", merge_add_flags); add_test("merge_remove_flags", merge_remove_flags); @@ -75,11 +77,12 @@ class Geary.ImapDB.FolderTest : TestCase { this.tmp_dir = null; } - public void create_email() throws GLib.Error { + public void create_read_email() throws GLib.Error { Email mock = new_mock_remote_email(1, "test"); this.folder.create_or_merge_email_async.begin( Collection.single(mock), + true, null, (obj, ret) => { async_complete(ret); } ); @@ -88,6 +91,45 @@ class Geary.ImapDB.FolderTest : TestCase { assert_int(1, results.size); assert(results.get(mock)); + assert_int(0, this.folder.get_properties().email_unread); + } + + public void create_unread_email() throws GLib.Error { + Email mock = new_mock_remote_email( + 1, "test", new EmailFlags.with(EmailFlags.UNREAD) + ); + + this.folder.create_or_merge_email_async.begin( + Collection.single(mock), + true, + null, + (obj, ret) => { async_complete(ret); } + ); + Gee.Map results = + this.folder.create_or_merge_email_async.end(async_result()); + + assert_int(1, results.size); + assert(results.get(mock)); + assert_int(1, this.folder.get_properties().email_unread); + } + + public void create_no_unread_update() throws GLib.Error { + Email mock = new_mock_remote_email( + 1, "test", new EmailFlags.with(EmailFlags.UNREAD) + ); + + this.folder.create_or_merge_email_async.begin( + Collection.single(mock), + false, + null, + (obj, ret) => { async_complete(ret); } + ); + Gee.Map results = + this.folder.create_or_merge_email_async.end(async_result()); + + assert_int(1, results.size); + assert(results.get(mock)); + assert_int(0, this.folder.get_properties().email_unread); } public void merge_email() throws GLib.Error { @@ -107,6 +149,7 @@ class Geary.ImapDB.FolderTest : TestCase { this.folder.create_or_merge_email_async.begin( Collection.single(mock), + true, null, (obj, ret) => { async_complete(ret); } ); @@ -137,10 +180,7 @@ class Geary.ImapDB.FolderTest : TestCase { } public void merge_add_flags() throws GLib.Error { - // Note: Flags in the DB are expected to be Imap.MessageFlags, - // and flags passed in to ImapDB.Folder are expected to be - // Imap.EmailFlags - + // Flags in the DB are expected to be Imap.MessageFlags Email.Field fixture_fields = Email.Field.FLAGS; Imap.MessageFlags fixture_flags = new Imap.MessageFlags(Collection.single(Imap.MessageFlag.SEEN)); @@ -155,13 +195,11 @@ class Geary.ImapDB.FolderTest : TestCase { VALUES (1, 1, 1, 1); """); - Imap.EmailFlags test_flags = Imap.EmailFlags.from_api_email_flags( - new EmailFlags.with(EmailFlags.UNREAD) - ) ; + EmailFlags test_flags = new EmailFlags.with(EmailFlags.UNREAD); Email test = new_mock_remote_email(1, null, test_flags); - this.folder.create_or_merge_email_async.begin( Collection.single(test), + true, null, (obj, ret) => { async_complete(ret); } ); @@ -175,10 +213,7 @@ class Geary.ImapDB.FolderTest : TestCase { } public void merge_remove_flags() throws GLib.Error { - // Note: Flags in the DB are expected to be Imap.MessageFlags, - // and flags passed in to ImapDB.Folder are expected to be - // Imap.EmailFlags - + // Flags in the DB are expected to be Imap.MessageFlags Email.Field fixture_fields = Email.Field.FLAGS; Imap.MessageFlags fixture_flags = new Imap.MessageFlags(Gee.Collection.empty()); @@ -193,12 +228,11 @@ class Geary.ImapDB.FolderTest : TestCase { VALUES (1, 1, 1, 1); """); - Imap.EmailFlags test_flags = Imap.EmailFlags.from_api_email_flags( - new EmailFlags() - ) ; + EmailFlags test_flags = new EmailFlags(); Email test = new_mock_remote_email(1, null, test_flags); this.folder.create_or_merge_email_async.begin( Collection.single(test), + true, null, (obj, ret) => { async_complete(ret); } ); @@ -283,15 +317,17 @@ class Geary.ImapDB.FolderTest : TestCase { private Email new_mock_remote_email(int64 uid, string? subject = null, - EmailFlags? flags = null) { + Geary.EmailFlags? flags = null) { Email mock = new Email( new EmailIdentifier.no_message_id(new Imap.UID(uid)) ); if (subject != null) { mock.set_message_subject(new RFC822.Subject(subject)); } + // Flags passed in to ImapDB.Folder are expected to be + // Imap.EmailFlags if (flags != null) { - mock.set_flags(flags); + mock.set_flags(Imap.EmailFlags.from_api_email_flags(flags)); } return mock; }