Commit 9619d18e authored by Michael Gratton's avatar Michael Gratton 🤞

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.
parent 67bf1183
Pipeline #59657 passed with stages
in 11 minutes and 52 seconds
......@@ -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();
}
......
......@@ -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;
}
......
......@@ -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;
}
}
......@@ -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(
......
......@@ -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");
......
......@@ -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();
......
......@@ -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);
......
......@@ -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) {
......
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