GNOME.org
 

[geary/wip/email-flag-refinement-redux] Don't update a folder's unread email count during normalisation

commit 744cde0c2f9e8464f5e48fcad102490ad3262121
Author: Michael Gratton <mike vee net>
Date:   Fri Feb 15 13:40:04 2019 +1100

    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/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<Geary.Email, bool> create_or_merge_email_async(Gee.Collection<Geary.Email> emails,
-        Cancellable? cancellable) throws Error {
+    public async Gee.Map<Email, bool>
+        create_or_merge_email_async(Gee.Collection<Email> emails,
+                                    bool update_totals,
+                                    GLib.Cancellable? cancellable)
+        throws GLib.Error {
         Gee.HashMap<Geary.Email, bool> results = new Gee.HashMap<Geary.Email, bool>();
         
         Gee.ArrayList<Geary.Email> list = traverse<Geary.Email>(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<ImapDB.EmailIdentifier> inserted_ids = new Gee.HashSet<ImapDB.EmailIdentifier>();
         Gee.Set<ImapDB.EmailIdentifier> locally_inserted_ids = new Gee.HashSet<ImapDB.EmailIdentifier>();
         if (to_create.size > 0) {
-            Gee.Map<Email, bool>? 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<Email, bool>? 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<Geary.Email>? 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<Geary.EmailIdentifier> created_ids = new Gee.HashSet<Geary.EmailIdentifier>();
@@ -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<Geary.Email, bool> created_or_merged = yield this.local.create_or_merge_email_async(
-                list, cancellable);
+            Gee.Map<Email, bool> 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<Geary.Email, bool> results = yield engine.local_folder.create_or_merge_email_async(
-                Geary.iterate<Geary.Email>(created).to_array_list(), cancellable);
+            Gee.Map<Geary.Email, bool> results =
+                yield this.engine.local_folder.create_or_merge_email_async(
+                    Geary.iterate<Geary.Email>(created).to_array_list(),
+                    true,
+                    this.cancellable
+                );
             if (results.size > 0) {
                 created_id = Collection.get_first<Geary.Email>(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<Geary.Email, bool> 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<Geary.Email, bool> 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<Email,bool> 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<Email,bool> 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<Geary.Imap.MessageFlag>());
@@ -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;
     }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]