[geary/wip/730682-refine-convo-list: 26/37] Ensure conversations are actually removed from the list when removed.
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/730682-refine-convo-list: 26/37] Ensure conversations are actually removed from the list when removed.
- Date: Mon, 11 Dec 2017 21:14:46 +0000 (UTC)
commit 193c3712d8917a0e815f6d0364d823d2f6ee279a
Author: Michael James Gratton <mike vee net>
Date: Mon Dec 11 13:51:49 2017 +1100
Ensure conversations are actually removed from the list when removed.
* src/client/conversation-list/conversation-list-model.vala
(ConversationListModel): Convert to using a Glib.Sequence for the
backing store so we can use pre-existing sorted insert and lookup
methods. Correctly update and remove conversations by identity-only
lookup though since the sort order depends on the emails in the
conversation, so as soon as a conversation is appended, trimmed or
removed its position in the backing store will no longer be sorted. Add
unit tests to ensure sorted adding and removal and signals fired are
correct.
* src/engine/app/app-conversation.vala (Conversation): Make some methods
public so they can be used in the unit test.
.../conversation-list/conversation-list-model.vala | 157 +++++++-----
src/engine/app/app-conversation.vala | 9 +-
test/CMakeLists.txt | 6 +
.../conversation-list-model-test.vala | 261 ++++++++++++++++++++
test/test-client.vala | 1 +
5 files changed, 363 insertions(+), 71 deletions(-)
---
diff --git a/src/client/conversation-list/conversation-list-model.vala
b/src/client/conversation-list/conversation-list-model.vala
index c164958..65393e7 100644
--- a/src/client/conversation-list/conversation-list-model.vala
+++ b/src/client/conversation-list/conversation-list-model.vala
@@ -21,102 +21,127 @@ public class ConversationListModel : Geary.BaseObject, GLib.ListModel {
/** The source of conversations for this model. */
public Geary.App.ConversationMonitor monitor { get; private set; }
- // The model's native sort order
- private static int model_sort(Geary.App.Conversation a, Geary.App.Conversation b) {
- return compare_conversation_descending(a, b);
- }
-
+ // Backing store for this model
+ private Sequence<Geary.App.Conversation> conversations =
+ new Sequence<Geary.App.Conversation>();
- // Backing store for this model. We can't just derive from this
- // directly since GLib.ListStore is a compact class.
- private ListStore conversations = new ListStore(typeof(Geary.App.Conversation));
+ /** The model's native sort order. */
+ private CompareDataFunc model_sort =
+ (CompareDataFunc) compare_conversation_descending;
+ /**
+ * Constructs a new model for the conversation list.
+ */
public ConversationListModel(Geary.App.ConversationMonitor monitor) {
this.monitor = monitor;
+ // XXX Should only start loading when scan is completed
//monitor.scan_completed.connect(on_scan_completed);
- monitor.conversations_added.connect(on_conversations_added);
- monitor.conversations_removed.connect(on_conversations_removed);
- // XXX
- //monitor.email_flags_changed.connect((convo) => { update(convo); });
+ monitor.conversations_added.connect(add);
+ monitor.conversations_removed.connect(on_removed);
+ monitor.conversation_appended.connect(on_updated);
+ monitor.conversation_trimmed.connect(on_updated);
// add all existing monitor
- on_conversations_added(monitor.get_conversations());
+ add(monitor.get_conversations());
+ }
- this.conversations.items_changed.connect((position, removed, added) => {
- this.items_changed(position, removed, added);
- });
+ public void add(Gee.Collection<Geary.App.Conversation> to_add) {
+ foreach (Geary.App.Conversation convo in to_add) {
+ SequenceIter<Geary.App.Conversation>? existing =
+ this.conversations.lookup(convo, this.model_sort);
+ if (existing == null) {
+ add_internal(convo);
+ }
+ }
+ }
+
+ public void remove(Gee.Collection<Geary.App.Conversation> to_remove) {
+ foreach (Geary.App.Conversation convo in to_remove) {
+ SequenceIter<Geary.App.Conversation>? existing =
+ this.conversations.lookup(convo, this.model_sort);
+ if (existing != null) {
+ remove_internal(existing);
+ }
+ }
+ }
+
+ public Geary.App.Conversation get_conversation(uint position) {
+ SequenceIter<Geary.App.Conversation>? existing =
+ this.conversations.get_iter_at_pos((int) position);
+ // XXX handle null here by throwing an error
+ return (existing != null) ? existing.get() : null;
}
public Object? get_item(uint position) {
- return this.conversations.get_item(position);
+ SequenceIter<Geary.App.Conversation>? existing =
+ this.conversations.get_iter_at_pos((int) position);
+ return (existing != null) ? existing.get() : null;
}
public uint get_n_items() {
- return this.monitor.get_conversation_count();
+ return (uint) this.conversations.get_length();
}
public Type get_item_type() {
- return this.conversations.get_item_type();
+ return typeof(Geary.App.Conversation);
}
- public Geary.App.Conversation get_conversation(uint position) {
- // XXX need to handle null here by throwing an error
- return this.conversations.get_item(position) as Geary.App.Conversation;
- }
-
- // XXX Something like this should be enabled so that if flags like
- // received date changes, the ordering will change to reflect
- // that.
- // private void update(Geary.App.Conversation target) {
- // // XXX this is horribly inefficient
- // this.conversations.sort((a, b) => {
- // return model_sort(a as Geary.App.Conversation,
- // b as Geary.App.Conversation);
- // });
- // }
-
- private uint get_index(Geary.App.Conversation target)
- throws Error {
- // Yet Another Binary Search Implementation :<
- int lower = 0;
- int upper = ((int) get_n_items()) - 1;
- while (lower < upper) {
- int mid = (int) Math.floor((upper + lower) / 2);
- int cmp = model_sort(get_conversation(mid), target);
- if (cmp < 0) {
- lower = mid + 1;
- } else if (cmp > 0) {
- upper = mid - 1;
- } else {
- return mid;
+ public SequenceIter<Geary.App.Conversation>?
+ get_by_identity(Geary.App.Conversation target) {
+ SequenceIter<Geary.App.Conversation> existing =
+ this.conversations.get_begin_iter();
+ while (!existing.is_end()) {
+ if (existing.get() == target) {
+ return existing;
}
+ existing = existing.next();
}
- // XXX UGH
- throw new IOError.NOT_FOUND("Not found");
+ return null;
}
- private void on_conversations_added(Gee.Collection<Geary.App.Conversation> conversations) {
- foreach (Geary.App.Conversation convo in conversations) {
- this.conversations.insert_sorted(
- convo,
- (a, b) => {
- return model_sort(a as Geary.App.Conversation,
- b as Geary.App.Conversation);
- }
+ private void on_removed(Gee.Collection<Geary.App.Conversation> removed) {
+ // We can't just use the conversation's sorted positions since
+ // it would have changed as its emails were removed from it,
+ // so we need to find it by identity instead.
+ foreach (Geary.App.Conversation target in removed) {
+ SequenceIter<Geary.App.Conversation>? existing = get_by_identity(
+ target
);
+ if (existing != null) {
+ debug("Removed conversation: %s", target.to_string());
+ remove_internal(existing);
+ }
}
}
- private void on_conversations_removed(Gee.Collection<Geary.App.Conversation> conversations) {
- foreach (Geary.App.Conversation convo in conversations) {
- try {
- this.conversations.remove(get_index(convo));
- } catch (Error err) {
- debug("Failed to remove conversation: %s", err.message);
- }
+ private void on_updated(Geary.App.Conversation updated) {
+ debug("Conversation updated: %s", updated.to_string());
+ // Need to remove and re-add the conversation to take into
+ // account its new position. We can't just use its sorted
+ // position however since it may have changed, so we need to
+ // find it by identity instead.
+ SequenceIter<Geary.App.Conversation>? existing = get_by_identity(
+ updated
+ );
+ if (existing != null) {
+ debug("Updating conversation: %s", updated.to_string());
+ remove_internal(existing);
+ add_internal(updated);
}
}
+ private void add_internal(Geary.App.Conversation convo) {
+ SequenceIter<Geary.App.Conversation> pos =
+ this.conversations.insert_sorted(convo, this.model_sort);
+ this.items_changed(pos.get_position(), 0, 1);
+ }
+
+ private void remove_internal(SequenceIter<Geary.App.Conversation> existing) {
+ int pos = existing.get_position();
+ existing.remove();
+ this.items_changed(pos, 1, 0);
+ }
+
}
diff --git a/src/engine/app/app-conversation.vala b/src/engine/app/app-conversation.vala
index 6442710..466a75c 100644
--- a/src/engine/app/app-conversation.vala
+++ b/src/engine/app/app-conversation.vala
@@ -86,7 +86,7 @@ public class Geary.App.Conversation : BaseObject {
/**
* Constructs a conversation relative to the given base folder.
*/
- internal Conversation(Geary.Folder base_folder) {
+ public Conversation(Geary.Folder base_folder) {
this.convnum = Conversation.next_convnum++;
this.base_folder = base_folder;
}
@@ -295,7 +295,7 @@ public class Geary.App.Conversation : BaseObject {
* Returns if the email was added, else false if already present
* and only `known_paths` were merged.
*/
- internal bool add(Email email, Gee.Collection<Geary.FolderPath> known_paths) {
+ public bool add(Email email, Gee.Collection<Geary.FolderPath> known_paths) {
// Add the known paths to the path map regardless of whether
// the email is already in the conversation or not, so that it
// remains complete
@@ -326,7 +326,7 @@ public class Geary.App.Conversation : BaseObject {
* Returns all Message-IDs that should be removed as result of
* removing this message, or `null` if none were removed.
*/
- internal Gee.Set<RFC822.MessageID>? remove(Email email) {
+ public Gee.Set<RFC822.MessageID>? remove(Email email) {
Gee.Set<RFC822.MessageID>? removed_ids = null;
if (emails.unset(email.id)) {
@@ -353,7 +353,6 @@ public class Geary.App.Conversation : BaseObject {
}
}
-
trimmed(email);
}
@@ -363,7 +362,7 @@ public class Geary.App.Conversation : BaseObject {
/**
* Removes the target path from the known set for the given id.
*/
- internal void remove_path(Geary.EmailIdentifier id, FolderPath path) {
+ public void remove_path(Geary.EmailIdentifier id, FolderPath path) {
this.path_map.remove(id, path);
}
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 0d8d33a..f396fca 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -32,10 +32,16 @@ set(TEST_CLIENT_SRC
test-client.vala
testcase.vala # Based on same file in libgee, courtesy Julien Peeters
+ # XXX
+ engine/api/geary-email-identifier-test.vala
+ engine/api/geary-folder-test.vala
+ engine/api/geary-folder-path-test.vala
+
client/application/geary-configuration-test.vala
client/components/client-web-view-test.vala
client/components/client-web-view-test-case.vala
client/composer/composer-web-view-test.vala
+ client/conversation-list/conversation-list-model-test.vala
js/client-page-state-test.vala
js/composer-page-state-test.vala
diff --git a/test/client/conversation-list/conversation-list-model-test.vala
b/test/client/conversation-list/conversation-list-model-test.vala
new file mode 100644
index 0000000..fd41840
--- /dev/null
+++ b/test/client/conversation-list/conversation-list-model-test.vala
@@ -0,0 +1,261 @@
+/*
+ * Copyright 2017 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.
+ */
+
+class ConversationListModelTest : Gee.TestCase {
+
+
+ private ConversationListModel? test = null;
+ private Geary.App.ConversationMonitor? monitor = null;
+ private Geary.Folder? base_folder = null;
+
+
+ public ConversationListModelTest() {
+ base("ConversationListModel");
+ add_test("add_ascending", add_ascending);
+ add_test("add_descending", add_descending);
+ add_test("add_out_of_order", add_out_of_order);
+ add_test("add_duplicate", add_duplicate);
+ add_test("remove_first", remove_first);
+ add_test("remove_middle", remove_middle);
+ add_test("remove_last", remove_last);
+ add_test("remove_nonexistent", remove_nonexistent);
+ }
+
+ public override void set_up() {
+ this.base_folder = new Geary.MockFolder(
+ null,
+ null,
+ new Geary.MockFolderRoot("test"),
+ Geary.SpecialFolderType.NONE,
+ null
+ );
+ this.monitor = new Geary.App.ConversationMonitor(
+ this.base_folder,
+ Geary.Folder.OpenFlags.NONE,
+ Geary.Email.Field.NONE,
+ 0
+ );
+ this.test = new ConversationListModel(this.monitor);
+ }
+
+ public void add_ascending() {
+ int pos = -1;
+ int removed = -1;
+ int added = -1;
+ this.test.items_changed.connect(
+ (item_pos, item_removed, item_added) => {
+ pos = (int) item_pos;
+ removed = (int) item_removed;
+ added = (int) item_added;
+ }
+ );
+
+ Geary.App.Conversation c1 = setup_conversation(1);
+ assert(pos == 0);
+ assert(removed == 0);
+ assert(added == 1);
+
+ Geary.App.Conversation c2 = setup_conversation(2);
+ assert(pos == 0);
+ assert(removed == 0);
+ assert(added == 1);
+
+ Geary.App.Conversation c3 = setup_conversation(3);
+ assert(pos == 0);
+ assert(removed == 0);
+ assert(added == 1);
+
+ assert(this.test.get_n_items() == 3);
+ assert(this.test.get_item(0) == c3);
+ assert(this.test.get_item(1) == c2);
+ assert(this.test.get_item(2) == c1);
+ }
+
+ public void add_descending() {
+ int pos = -1;
+ int removed = -1;
+ int added = -1;
+ this.test.items_changed.connect(
+ (item_pos, item_removed, item_added) => {
+ pos = (int) item_pos;
+ removed = (int) item_removed;
+ added = (int) item_added;
+ }
+ );
+
+ Geary.App.Conversation c3 = setup_conversation(3);
+ assert(pos == 0);
+ assert(removed == 0);
+ assert(added == 1);
+
+ Geary.App.Conversation c2 = setup_conversation(2);
+ assert(pos == 1);
+ assert(removed == 0);
+ assert(added == 1);
+
+ Geary.App.Conversation c1 = setup_conversation(1);
+ assert(pos == 2);
+ assert(removed == 0);
+ assert(added == 1);
+
+ assert(this.test.get_n_items() == 3);
+ assert(this.test.get_item(0) == c3);
+ assert(this.test.get_item(1) == c2);
+ assert(this.test.get_item(2) == c1);
+ }
+
+ public void add_out_of_order() {
+ int pos = -1;
+ int removed = -1;
+ int added = -1;
+ this.test.items_changed.connect(
+ (item_pos, item_removed, item_added) => {
+ pos = (int) item_pos;
+ removed = (int) item_removed;
+ added = (int) item_added;
+ }
+ );
+
+ Geary.App.Conversation c2 = setup_conversation(2);
+ assert(pos == 0);
+ assert(removed == 0);
+ assert(added == 1);
+
+ Geary.App.Conversation c3 = setup_conversation(3);
+ assert(pos == 0);
+ assert(removed == 0);
+ assert(added == 1);
+
+ Geary.App.Conversation c1 = setup_conversation(1);
+ assert(pos == 2);
+ assert(removed == 0);
+ assert(added == 1);
+
+ assert(this.test.get_n_items() == 3);
+ assert(this.test.get_item(0) == c3);
+ assert(this.test.get_item(1) == c2);
+ assert(this.test.get_item(2) == c1);
+ }
+
+ public void add_duplicate() {
+ Geary.App.Conversation c1 = setup_conversation(1);
+
+ this.test.add(singleton(c1));
+ this.test.add(singleton(c1));
+
+ assert(this.test.get_n_items() == 1);
+ }
+
+ public void remove_first() {
+ int pos = -1;
+ int removed = -1;
+ int added = -1;
+ this.test.items_changed.connect(
+ (item_pos, item_removed, item_added) => {
+ pos = (int) item_pos;
+ removed = (int) item_removed;
+ added = (int) item_added;
+ }
+ );
+
+ Geary.App.Conversation c1 = setup_conversation(1);
+ Geary.App.Conversation c2 = setup_conversation(2);
+ Geary.App.Conversation c3 = setup_conversation(3);
+
+ this.test.remove(singleton(c1));
+ assert(pos == 2);
+ assert(removed == 1);
+ assert(added == 0);
+
+ assert(this.test.get_n_items() == 2);
+ assert(this.test.get_item(0) == c3);
+ assert(this.test.get_item(1) == c2);
+ }
+
+ public void remove_middle() {
+ int pos = -1;
+ int removed = -1;
+ int added = -1;
+ this.test.items_changed.connect(
+ (item_pos, item_removed, item_added) => {
+ pos = (int) item_pos;
+ removed = (int) item_removed;
+ added = (int) item_added;
+ }
+ );
+
+ Geary.App.Conversation c1 = setup_conversation(1, true);
+ Geary.App.Conversation c2 = setup_conversation(2);
+ Geary.App.Conversation c3 = setup_conversation(3);
+
+ this.test.remove(singleton(c2));
+ assert(pos == 1);
+ assert(removed == 1);
+ assert(added == 0);
+
+ assert(this.test.get_n_items() == 2);
+ assert(this.test.get_item(0) == c3);
+ assert(this.test.get_item(1) == c1);
+ }
+
+ public void remove_last() {
+ int pos = -1;
+ int removed = -1;
+ int added = -1;
+ this.test.items_changed.connect(
+ (item_pos, item_removed, item_added) => {
+ pos = (int) item_pos;
+ removed = (int) item_removed;
+ added = (int) item_added;
+ }
+ );
+
+ Geary.App.Conversation c1 = setup_conversation(1);
+ Geary.App.Conversation c2 = setup_conversation(2);
+ Geary.App.Conversation c3 = setup_conversation(3);
+
+ this.test.remove(singleton(c3));
+ assert(pos == 0);
+ assert(removed == 1);
+ assert(added == 0);
+
+ assert(this.test.get_n_items() == 2);
+ assert(this.test.get_item(0) == c2);
+ assert(this.test.get_item(1) == c1);
+ }
+
+ public void remove_nonexistent() {
+ Geary.App.Conversation c1 = setup_conversation(1);
+ Geary.App.Conversation c2 = setup_conversation(2);
+
+ this.test.remove(singleton(c2));
+
+ assert(this.test.get_n_items() == 1);
+ assert(this.test.get_item(0) == c1);
+ }
+
+ private Geary.App.Conversation setup_conversation(int id, bool do_add = true) {
+ Geary.App.Conversation conversation = new Geary.App.Conversation(
+ this.base_folder
+ );
+ conversation.add(
+ new Geary.Email(new Geary.MockEmailIdentifer(id)),
+ singleton(this.base_folder.path)
+ );
+ if (do_add) {
+ this.test.add(singleton(conversation));
+ }
+ return conversation;
+ }
+
+ private Gee.Collection<E> singleton<E>(E element) {
+ Gee.LinkedList<E> collection = new Gee.LinkedList<E>();
+ collection.add(element);
+ return collection;
+ }
+
+}
diff --git a/test/test-client.vala b/test/test-client.vala
index 06352e8..5311b2f 100644
--- a/test/test-client.vala
+++ b/test/test-client.vala
@@ -42,6 +42,7 @@ int main(string[] args) {
client.add_suite(new ClientWebViewTest().get_suite());
client.add_suite(new ComposerWebViewTest().get_suite());
client.add_suite(new ConfigurationTest().get_suite());
+ client.add_suite(new ConversationListModelTest().get_suite());
TestSuite js = new TestSuite("js");
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
