[geary/wip/all-mail-sticky-conversations: 2/2] Ensure conversations that have no email in the base folder are dropped
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/all-mail-sticky-conversations: 2/2] Ensure conversations that have no email in the base folder are dropped
- Date: Sat, 16 Feb 2019 05:30:55 +0000 (UTC)
commit 9619d18eda73839641e8109029e553468c2179d3
Author: Michael Gratton <mike vee net>
Date: Sat Feb 16 16:26:10 2019 +1100
Ensure conversations that have no email in the base folder are dropped
If a least one email in a conversation that is in the base folder but
also in another is removed from the base folder, the conversation may
not be removed from the monitor despite possibly not having any email
in the base folder, since the email may not have been completely removed
from the conversation.
This was particulary being seen with GMail accounts where even single
message conversations were not disappering when trashed because the
converation's email was still in All Mail.
This fix does a few things: Avoids hitting the database when checking
a conversation still has email in the base folder, when it does check
only ensures that email are in the base folder, not *any* folder, and
updates ConversationSet::remove_all_emails_by_identifier to do this
check iternally, clean up its API and implementation and avoids having
to use out args calling it.
src/engine/app/app-conversation-monitor.vala | 30 -----
src/engine/app/app-conversation.vala | 16 +--
.../conversation-monitor/app-conversation-set.vala | 144 ++++++++++-----------
.../conversation-monitor/app-remove-operation.vala | 18 +--
test/engine/app/app-conversation-monitor-test.vala | 3 -
test/engine/app/app-conversation-set-test.vala | 52 +++++---
test/engine/app/app-conversation-test.vala | 16 +++
test/test-case.vala | 8 ++
8 files changed, 138 insertions(+), 149 deletions(-)
---
diff --git a/src/engine/app/app-conversation-monitor.vala b/src/engine/app/app-conversation-monitor.vala
index be6d3e70..83a1a337 100644
--- a/src/engine/app/app-conversation-monitor.vala
+++ b/src/engine/app/app-conversation-monitor.vala
@@ -555,36 +555,6 @@ public class Geary.App.ConversationMonitor : BaseObject {
}
}
- /**
- * Check conversations to see if they still exist in the base folder.
- *
- * Returns the set of emails that were removed due to not being in
- * the base folder.
- */
- internal async Gee.Collection<Conversation>
- check_conversations_in_base_folder(Gee.Collection<Conversation> conversations)
- throws Error {
- Gee.ArrayList<Conversation> evaporated = new Gee.ArrayList<Conversation>();
- foreach (Conversation conversation in conversations) {
- int count = yield conversation.get_count_in_folder_async(
- this.base_folder.account,
- this.base_folder.path,
- this.operation_cancellable
- );
- if (count == 0) {
- Logging.debug(
- Logging.Flag.CONVERSATIONS,
- "Evaporating conversation %s because it has no emails in %s",
- conversation.to_string(), this.base_folder.to_string()
- );
- this.conversations.remove_conversation(conversation);
- evaporated.add(conversation);
- }
- }
-
- return evaporated;
- }
-
protected virtual void notify_scan_started() {
scan_started();
}
diff --git a/src/engine/app/app-conversation.vala b/src/engine/app/app-conversation.vala
index 40d88d89..17c35f82 100644
--- a/src/engine/app/app-conversation.vala
+++ b/src/engine/app/app-conversation.vala
@@ -101,19 +101,13 @@ public class Geary.App.Conversation : BaseObject {
/**
* Returns the number of emails in the conversation in a particular folder.
*/
- public async int get_count_in_folder_async(Geary.Account account, Geary.FolderPath path,
- Cancellable? cancellable) throws Error {
- Gee.MultiMap<Geary.EmailIdentifier, Geary.FolderPath>? folder_map
- = yield account.get_containing_folders_async(emails.keys, cancellable);
-
- int count = 0;
- if (folder_map != null) {
- foreach (Geary.EmailIdentifier id in folder_map.get_keys()) {
- if (path in folder_map.get(id))
- ++count;
+ public uint get_count_in_folder(FolderPath path) {
+ uint count = 0;
+ foreach (Geary.EmailIdentifier id in this.path_map.get_keys()) {
+ if (path in this.path_map.get(id)) {
+ count++;
}
}
-
return count;
}
diff --git a/src/engine/app/conversation-monitor/app-conversation-set.vala
b/src/engine/app/conversation-monitor/app-conversation-set.vala
index 5815c318..ed44dc72 100644
--- a/src/engine/app/conversation-monitor/app-conversation-set.vala
+++ b/src/engine/app/conversation-monitor/app-conversation-set.vala
@@ -168,34 +168,74 @@ private class Geary.App.ConversationSet : BaseObject {
* respectively.
*/
public void remove_all_emails_by_identifier(FolderPath source_path,
- Gee.Collection<Geary.EmailIdentifier> ids,
- out Gee.Collection<Conversation> removed,
- out Gee.MultiMap<Conversation, Geary.Email> trimmed) {
- Gee.HashSet<Conversation> _removed = new Gee.HashSet<Conversation>();
- Gee.HashMultiMap<Conversation, Geary.Email> _trimmed
- = new Gee.HashMultiMap<Conversation, Geary.Email>();
+ Gee.Collection<EmailIdentifier> ids,
+ Gee.Collection<Conversation> removed,
+ Gee.MultiMap<Conversation,Email> trimmed) {
+ Gee.Set<Conversation> remaining = new Gee.HashSet<Conversation>();
foreach (Geary.EmailIdentifier id in ids) {
- Geary.Email email;
- bool removed_conversation;
- Conversation? conversation = remove_email_by_identifier(
- source_path, id, out email, out removed_conversation
- );
-
- if (conversation == null)
- continue;
-
- if (removed_conversation) {
- if (_trimmed.contains(conversation))
- _trimmed.remove_all(conversation);
- _removed.add(conversation);
- } else if (!conversation.contains_email_by_id(id)) {
- _trimmed.set(conversation, email);
+ Conversation? conversation = email_id_map.get(id);
+ // The conversation could be null if the conversation
+ // monitor only goes back a few emails, but something old
+ // gets removed. It's especially likely when changing
+ // search terms in the search folder.
+ if (conversation != null) {
+ // Conditionally remove email from its conversation
+ Geary.Email? email = conversation.get_email_by_id(id);
+ if (email != null) {
+ switch (conversation.get_folder_count(id)) {
+ case 0:
+ warning("Email %s conversation %s not in any folders",
+ id.to_string(), conversation.to_string());
+ break;
+
+ case 1:
+ remove_email_from_conversation(conversation, email);
+ trimmed.set(conversation, email);
+ break;
+
+ default:
+ conversation.remove_path(id, source_path);
+ break;
+ }
+ }
+
+ if (conversation.get_count() == 0) {
+ Logging.debug(
+ Logging.Flag.CONVERSATIONS,
+ "Conversation %s evaporated: No messages remains",
+ conversation.to_string()
+ );
+ removed.add(conversation);
+ trimmed.remove_all(conversation);
+ remove_conversation(conversation);
+ } else {
+ remaining.add(conversation);
+ }
}
}
- removed = _removed;
- trimmed = _trimmed;
+ if (source_path.equal_to(this.base_folder.path)) {
+ // Now that all email have been processed, check reach
+ // remaining conversation to ensure it has at least one
+ // email in the base folder. It might not if remaining
+ // email in the conversation also exists in another
+ // folder, and so is especially likely for servers that
+ // have an All Email folder, since email will likely be in
+ // two different folders.
+ foreach (Conversation conversation in remaining) {
+ if (conversation.get_count_in_folder(source_path) == 0) {
+ Logging.debug(
+ Logging.Flag.CONVERSATIONS,
+ "Conversation %s dropped: No messages in base folder remain",
+ conversation.to_string()
+ );
+ removed.add(conversation);
+ trimmed.remove_all(conversation);
+ remove_conversation(conversation);
+ }
+ }
+ }
}
/**
@@ -340,12 +380,13 @@ private class Geary.App.ConversationSet : BaseObject {
* Unconditionally removes an email from a conversation.
*/
private void remove_email_from_conversation(Conversation conversation, Geary.Email email) {
- // Be very strict about our internal state getting out of whack, since
- // it would indicate a nasty error in our logic that we need to fix.
- if (!email_id_map.unset(email.id))
- error("Email %s already removed from conversation set", email.id.to_string());
+ if (!this.email_id_map.unset(email.id)) {
+ warning("Email %s already removed from conversation set",
+ email.id.to_string());
+ }
Gee.Set<Geary.RFC822.MessageID>? removed_message_ids = conversation.remove(email);
+ debug("Removed %d messages from conversation", removed_message_ids != null ?
removed_message_ids.size : 0);
if (removed_message_ids != null) {
foreach (Geary.RFC822.MessageID removed_message_id in removed_message_ids) {
if (!logical_message_id_map.unset(removed_message_id)) {
@@ -356,53 +397,4 @@ private class Geary.App.ConversationSet : BaseObject {
}
}
- /**
- * Conditionally removes an email from a conversation.
- *
- * The given email will only be removed from the conversation if
- * it is associated with a path, i.e. if it is present in more
- * than one folder. Otherwise `source_path` is removed from the
- * email's set of known paths.
- */
- private Conversation? remove_email_by_identifier(FolderPath source_path,
- EmailIdentifier id,
- out Geary.Email? removed_email,
- out bool removed_conversation) {
- removed_email = null;
- removed_conversation = false;
-
- Conversation? conversation = email_id_map.get(id);
- // This can happen when the conversation monitor only goes back a few
- // emails, but something old gets removed. It's especially likely when
- // changing search terms in the search folder.
- if (conversation == null)
- return null;
-
- Geary.Email? email = conversation.get_email_by_id(id);
- switch (conversation.get_folder_count(id)) {
- case 0:
- error("Email %s conversation %s not in any folders",
- id.to_string(), conversation.to_string());
-
- case 1:
- removed_email = email;
- remove_email_from_conversation(conversation, email);
- break;
-
- default:
- conversation.remove_path(id, source_path);
- break;
- }
-
- // Evaporate conversations with no more messages.
- if (conversation.get_count() == 0) {
- debug("Removing email %s evaporates conversation %s", id.to_string(), conversation.to_string());
- remove_conversation(conversation);
-
- removed_conversation = true;
- }
-
- return conversation;
- }
-
}
diff --git a/src/engine/app/conversation-monitor/app-remove-operation.vala
b/src/engine/app/conversation-monitor/app-remove-operation.vala
index 8f95ecb3..6e31436a 100644
--- a/src/engine/app/conversation-monitor/app-remove-operation.vala
+++ b/src/engine/app/conversation-monitor/app-remove-operation.vala
@@ -23,24 +23,16 @@ private class Geary.App.RemoveOperation : ConversationOperation {
this.removed_ids.size, this.source_folder.to_string()
);
- Gee.Collection<Conversation> removed;
- Gee.MultiMap<Conversation,Email> trimmed;
+ Gee.Set<Conversation> removed = new Gee.HashSet<Conversation>();
+ Gee.MultiMap<Conversation,Email> trimmed =
+ new Gee.HashMultiMap<Conversation, Geary.Email>();
this.monitor.conversations.remove_all_emails_by_identifier(
source_folder.path,
removed_ids,
- out removed,
- out trimmed
+ removed,
+ trimmed
);
- // Check for conversations that have been evaporated as a
- // result, update removed and trimmed collections to reflect
- // any that evaporated
- Gee.Collection<Conversation> evaporated =
- yield this.monitor.check_conversations_in_base_folder(trimmed.get_keys());
- removed.add_all(evaporated);
- foreach (Conversation target in evaporated) {
- trimmed.remove_all(target);
- }
// Fire signals, clean up
this.monitor.removed(
diff --git a/test/engine/app/app-conversation-monitor-test.vala
b/test/engine/app/app-conversation-monitor-test.vala
index 63c05472..6bf85bda 100644
--- a/test/engine/app/app-conversation-monitor-test.vala
+++ b/test/engine/app/app-conversation-monitor-test.vala
@@ -233,13 +233,10 @@ class Geary.App.ConversationMonitorTest : TestCase {
{e3, e2}, paths, {null, e2_related_paths}
);
assert_int(2, monitor.size, "Initial conversation count");
- print("monitor.window_lowest: %s", monitor.window_lowest.to_string());
assert_equal(e2.id, monitor.window_lowest, "Lowest window id");
// Removing a message will trigger another async load
this.base_folder.expect_call("list_email_by_id_async");
- this.account.expect_call("get_containing_folders_async");
- this.base_folder.expect_call("list_email_by_id_async");
this.base_folder.email_removed(new Gee.ArrayList<EmailIdentifier>.wrap({e2.id}));
wait_for_signal(monitor, "conversations-removed");
diff --git a/test/engine/app/app-conversation-set-test.vala b/test/engine/app/app-conversation-set-test.vala
index bae79bc3..7a7fedc2 100644
--- a/test/engine/app/app-conversation-set-test.vala
+++ b/test/engine/app/app-conversation-set-test.vala
@@ -24,6 +24,7 @@ class Geary.App.ConversationSetTest : TestCase {
add_test("remove_all_removed", remove_all_removed);
add_test("remove_all_trimmed", remove_all_trimmed);
add_test("remove_all_remove_path", remove_all_remove_path);
+ add_test("remove_all_base_folder", remove_all_base_folder);
}
public override void set_up() {
@@ -379,10 +380,11 @@ class Geary.App.ConversationSetTest : TestCase {
new Gee.LinkedList<EmailIdentifier>();
ids.add(e1.id);
- Gee.Collection<Conversation>? removed = null;
- Gee.MultiMap<Conversation,Email>? trimmed = null;
+ Gee.Set<Conversation> removed = new Gee.HashSet<Conversation>();
+ Gee.MultiMap<Conversation,Email> trimmed =
+ new Gee.HashMultiMap<Conversation, Geary.Email>();
this.test.remove_all_emails_by_identifier(
- this.base_folder.path, ids, out removed, out trimmed
+ this.base_folder.path, ids, removed, trimmed
);
assert(this.test.size == 0);
@@ -408,18 +410,16 @@ class Geary.App.ConversationSetTest : TestCase {
new Gee.LinkedList<EmailIdentifier>();
ids.add(e1.id);
- Gee.Collection<Conversation>? removed = null;
- Gee.MultiMap<Conversation,Email>? trimmed = null;
+ Gee.Set<Conversation> removed = new Gee.HashSet<Conversation>();
+ Gee.MultiMap<Conversation,Email> trimmed =
+ new Gee.HashMultiMap<Conversation, Geary.Email>();
this.test.remove_all_emails_by_identifier(
- this.base_folder.path, ids, out removed, out trimmed
+ this.base_folder.path, ids, removed, trimmed
);
assert(this.test.size == 1);
assert(this.test.get_email_count() == 1);
- assert(removed != null);
- assert(trimmed != null);
-
assert(removed.is_empty == true);
assert(trimmed.contains(convo) == true);
assert(trimmed.get(convo).contains(e1) == true);
@@ -437,25 +437,45 @@ class Geary.App.ConversationSetTest : TestCase {
new Gee.LinkedList<EmailIdentifier>();
ids.add(e1.id);
- Gee.Collection<Conversation>? removed = null;
- Gee.MultiMap<Conversation,Email>? trimmed = null;
+ Gee.Set<Conversation> removed = new Gee.HashSet<Conversation>();
+ Gee.MultiMap<Conversation,Email> trimmed =
+ new Gee.HashMultiMap<Conversation, Geary.Email>();
this.test.remove_all_emails_by_identifier(
- other_path, ids, out removed, out trimmed
+ other_path, ids, removed, trimmed
);
assert(this.test.size == 1);
assert(this.test.get_email_count() == 1);
- assert(removed != null);
assert(removed.is_empty == true);
-
- assert(trimmed != null);
assert(trimmed.size == 0);
-
assert(convo.is_in_base_folder(e1.id) == true);
assert(convo.get_folder_count(e1.id) == 1);
}
+ public void remove_all_base_folder() throws Error {
+ FolderPath other_path = this.folder_root.get_child("other");
+ Email e1 = setup_email(1);
+ add_email_to_test_set(e1, other_path);
+
+ Gee.LinkedList<EmailIdentifier> ids =
+ new Gee.LinkedList<EmailIdentifier>();
+ ids.add(e1.id);
+
+ Gee.List<Conversation> removed = new Gee.LinkedList<Conversation>();
+ Gee.MultiMap<Conversation,Email> trimmed =
+ new Gee.HashMultiMap<Conversation, Geary.Email>();
+ this.test.remove_all_emails_by_identifier(
+ this.base_folder.path, ids, removed, trimmed
+ );
+
+ assert_int(0, this.test.size, "ConversationSet size");
+ assert_int(0, this.test.get_email_count(), "ConversationSet email size");
+
+ assert_int(1, removed.size, "Removed size");
+ assert_int(0, trimmed.size, "Trimmed size");
+ }
+
private Email setup_email(int id, Email? references = null) {
Email email = new Email(new MockEmailIdentifer(id));
DateTime now = new DateTime.now_local();
diff --git a/test/engine/app/app-conversation-test.vala b/test/engine/app/app-conversation-test.vala
index 59e32d37..1ee1f4a4 100644
--- a/test/engine/app/app-conversation-test.vala
+++ b/test/engine/app/app-conversation-test.vala
@@ -24,6 +24,7 @@ class Geary.App.ConversationTest : TestCase {
add_test("get_emails_by_location", get_emails_by_location);
add_test("get_emails_blacklist", get_emails_blacklist);
add_test("get_emails_marked_for_deletion", get_emails_marked_for_deletion);
+ add_test("count_email_in_folder", count_email_in_folder);
}
public override void set_up() {
@@ -258,6 +259,21 @@ class Geary.App.ConversationTest : TestCase {
);
}
+ public void count_email_in_folder() throws GLib.Error {
+ Geary.Email e1 = setup_email(1);
+ this.test.add(e1, singleton(this.base_folder.path));
+
+ assert_uint(
+ 1, this.test.get_count_in_folder(this.base_folder.path),
+ "In-folder count"
+ );
+ assert_uint(
+ 0,
+ this.test.get_count_in_folder(this.folder_root.get_child("other")),
+ "Out-folder count"
+ );
+ }
+
private Gee.Collection<E> singleton<E>(E element) {
Gee.LinkedList<E> collection = new Gee.LinkedList<E>();
collection.add(element);
diff --git a/test/test-case.vala b/test/test-case.vala
index b90de697..12f8c67e 100644
--- a/test/test-case.vala
+++ b/test/test-case.vala
@@ -82,6 +82,14 @@ public void assert_int(int expected, int actual, string? context = null)
}
}
+public void assert_uint(uint expected, uint actual, string? context = null)
+ throws GLib.Error {
+ if (expected != actual) {
+ print_assert("Expected: %u, was: %u".printf(expected, actual), context);
+ assert_not_reached();
+ }
+}
+
public void assert_true(bool condition, string? context = null)
throws Error {
if (!condition) {
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
