Commit 6a42d110 authored by Michael Gratton's avatar Michael Gratton 🤞

Merge branch 'wip/all-mail-sticky-conversations' into 'master'

Fix sticky conversations in GMail

See merge request !108
parents 88c82217 9619d18e
Pipeline #59947 failed with stages
in 34 minutes and 51 seconds
......@@ -101,9 +101,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
}
/** The set of all conversations loaded by the monitor. */
internal ConversationSet conversations {
get; private set; default = new ConversationSet();
}
internal ConversationSet conversations { get; private set; }
/** The oldest message from the base folder in the loaded window. */
internal EmailIdentifier? window_lowest {
......@@ -263,6 +261,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
this.open_flags = open_flags;
this.required_fields = required_fields | REQUIRED_FIELDS;
this._min_window_count = min_window_count;
this.conversations = new ConversationSet(base_folder);
}
/**
......@@ -556,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();
}
......@@ -726,7 +695,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
// Add them to the conversation set
if (email_paths != null) {
this.conversations.add_all_emails(
job.emails.values, email_paths, this.base_folder,
job.emails.values, email_paths,
out added, out appended, out removed_due_to_merge
);
}
......
......@@ -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;
}
......
/*
* Copyright 2016 Software Freedom Conservancy Inc.
* Copyright 2017 Michael Gratton <mike@vee.net>
* Copyright 2017-2019 Michael Gratton <mike@vee.net>
*
* This software is licensed under the GNU Lesser General Public License
* (version 2.1 or later). See the COPYING file in this distribution.
......@@ -11,6 +11,10 @@
*/
private class Geary.App.ConversationSet : BaseObject {
/** The base folder for this set of conversations. */
public Folder base_folder { get; private set; }
/** Determines the number of conversations in the set. */
public int size { get { return _conversations.size; } }
......@@ -35,6 +39,16 @@ private class Geary.App.ConversationSet : BaseObject {
= new Gee.HashMap<Geary.RFC822.MessageID, Conversation>();
/**
* Constructs a new conversation set.
*
* The `base_folder` argument is the base folder for the
* conversation monitor that owns this set.
*/
public ConversationSet(Folder base_folder) {
this.base_folder = base_folder;
}
public int get_email_count() {
return email_id_map.size;
}
......@@ -60,8 +74,7 @@ private class Geary.App.ConversationSet : BaseObject {
* needed. The collection `emails` contains the messages to be
* added, and for each email in the collection, there should be an
* entry in `id_to_paths` that indicates the folders each message
* is known to belong to. The folder `base_folder` is the base
* folder for the conversation monitor that owns this set.
* is known to belong to.
*
* The three collections returned include any conversation that
* were created, any that had email appended to them (and the
......@@ -70,7 +83,6 @@ private class Geary.App.ConversationSet : BaseObject {
*/
public void add_all_emails(Gee.Collection<Email> emails,
Gee.MultiMap<EmailIdentifier, FolderPath> id_to_paths,
Folder base_folder,
out Gee.Collection<Conversation> added,
out Gee.MultiMap<Conversation, Email> appended,
out Gee.Collection<Conversation> removed_due_to_merge) {
......@@ -124,8 +136,7 @@ private class Geary.App.ConversationSet : BaseObject {
// Don't add an email with no known paths - it may
// have been removed after being listed for adding.
conversation = add_email(
email, base_folder, known_paths,
out added_conversation
email, known_paths, out added_conversation
);
}
......@@ -157,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);
}
}
}
}
/**
......@@ -235,7 +286,6 @@ private class Geary.App.ConversationSet : BaseObject {
* was created, else it is set to `false`.
*/
private Conversation? add_email(Geary.Email email,
Folder base_folder,
Gee.Collection<FolderPath>? known_paths,
out bool added_conversation) {
added_conversation = false;
......@@ -255,7 +305,7 @@ private class Geary.App.ConversationSet : BaseObject {
if (conversation == null) {
// Not in or related to any existing conversations, so
// create one
conversation = new Conversation(base_folder);
conversation = new Conversation(this.base_folder);
_conversations.add(conversation);
added_conversation = true;
}
......@@ -330,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)) {
......@@ -346,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() {
......@@ -35,7 +36,7 @@ class Geary.App.ConversationSetTest : TestCase {
SpecialFolderType.NONE,
null
);
this.test = new ConversationSet();
this.test = new ConversationSet(this.base_folder);
}
public override void tear_down() {
......@@ -62,7 +63,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
emails, email_paths,
out added, out appended, out removed
);
......@@ -108,8 +109,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.MultiMap<Conversation,Email>? appended = null;
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -127,8 +127,7 @@ class Geary.App.ConversationSetTest : TestCase {
appended = null;
removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -158,8 +157,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.MultiMap<Conversation,Email>? appended = null;
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -188,8 +186,7 @@ class Geary.App.ConversationSetTest : TestCase {
appended = null;
removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -228,8 +225,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.MultiMap<Conversation,Email>? appended = null;
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -278,8 +274,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.MultiMap<Conversation,Email>? appended = null;
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -332,8 +327,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.MultiMap<Conversation,Email>? appended = null;
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -361,8 +355,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.MultiMap<Conversation,Email>? appended = null;
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
assert(this.test.size == 1);
......@@ -387,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);
......@@ -416,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);
......@@ -445,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();
......@@ -499,8 +511,7 @@ class Geary.App.ConversationSetTest : TestCase {
Gee.MultiMap<Conversation,Email>? appended = null;
Gee.Collection<Conversation>? removed = null;
this.test.add_all_emails(
emails, email_paths, this.base_folder,
out added, out appended, out removed
emails, email_paths, out added, out appended, out removed
);
}
......
......@@ -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