Commit 744cde0c authored by Michael Gratton's avatar Michael Gratton 🤞

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
parent 272d95e3
Pipeline #59341 failed with stages
in 8 minutes and 57 seconds
......@@ -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;
}
......
......@@ -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);
......
......@@ -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)
......
......@@ -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);
}
......
......@@ -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 {
......
......@@ -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];
......
......@@ -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)) {
......
......@@ -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;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment