[geary/wip/713150-conversations] Improved ConversationListStore row handling

[Jim Nelson <jnelson src gnome org>]

commit 5b1ab0d2df03687c1791e4616ad399f6e2a183ce
Author: Jim Nelson <jim yorba org>
Date:   Fri Mar 20 15:31:08 2015 -0700

    Improved ConversationListStore row handling
    
    Old code would re-sort an updated row (cell) in the list by removing
    the old one and inserting a new one with the new conversation data.
    This caused unsightly flicker and selection problems.  This new code
    will update the existing row and force a re-sort of the list.
    
    Preview handling is still frightful.

 .../conversation-list/conversation-list-store.vala |   61 +++++++++-----------
 .../formatted-conversation-data.vala               |   53 +++++++++++-------
 src/engine/app/app-conversation-monitor.vala       |   20 ++++---
 3 files changed, 71 insertions(+), 63 deletions(-)
---
diff --git a/src/client/conversation-list/conversation-list-store.vala 
b/src/client/conversation-list/conversation-list-store.vala
index 552463a..9594ef8 100644
--- a/src/client/conversation-list/conversation-list-store.vala
+++ b/src/client/conversation-list/conversation-list-store.vala
@@ -113,8 +113,8 @@ public class ConversationListStore : Gtk.ListStore {
             conversation_monitor.scan_completed.disconnect(on_scan_completed);
             conversation_monitor.conversations_added.disconnect(on_conversations_added);
             conversation_monitor.conversation_removed.disconnect(on_conversation_removed);
-            conversation_monitor.conversation_appended.disconnect(on_conversation_appended);
-            conversation_monitor.conversation_trimmed.disconnect(on_conversation_trimmed);
+            conversation_monitor.conversation_appended.disconnect(on_refresh_conversation);
+            conversation_monitor.conversation_trimmed.disconnect(on_refresh_conversation);
             conversation_monitor.email_flags_changed.disconnect(on_email_flags_changed);
             conversation_monitor.email_paths_changed.disconnect(on_email_paths_changed);
         }
@@ -136,8 +136,8 @@ public class ConversationListStore : Gtk.ListStore {
             conversation_monitor.scan_completed.connect(on_scan_completed);
             conversation_monitor.conversations_added.connect(on_conversations_added);
             conversation_monitor.conversation_removed.connect(on_conversation_removed);
-            conversation_monitor.conversation_appended.connect(on_conversation_appended);
-            conversation_monitor.conversation_trimmed.connect(on_conversation_trimmed);
+            conversation_monitor.conversation_appended.connect(on_refresh_conversation);
+            conversation_monitor.conversation_trimmed.connect(on_refresh_conversation);
             conversation_monitor.email_flags_changed.connect(on_email_flags_changed);
             conversation_monitor.email_paths_changed.connect(on_email_paths_changed);
         }
@@ -197,7 +197,7 @@ public class ConversationListStore : Gtk.ListStore {
         foreach (Geary.Email email in emails) {
             Geary.App.Conversation? conversation = conversation_monitor.get_conversation_for_email(email.id);
             if (conversation != null)
-                set_preview_for_conversation(conversation, email);
+                refresh_conversation(conversation, email);
             else
                 debug("Couldn't find conversation for %s", email.id.to_string());
         }
@@ -277,14 +277,6 @@ public class ConversationListStore : Gtk.ListStore {
         return message_data == null ? null : message_data.preview;
     }
     
-    private void set_preview_for_conversation(Geary.App.Conversation conversation, Geary.Email preview) {
-        Gtk.TreeIter iter;
-        if (get_iter_for_conversation(conversation, out iter))
-            set_row(iter, conversation, preview);
-        else
-            debug("Unable to find preview for conversation");
-    }
-    
     private void set_row(Gtk.TreeIter iter, Geary.App.Conversation conversation, Geary.Email preview) {
         FormattedConversationData conversation_data = new FormattedConversationData(conversation,
             preview, conversation_monitor.folder,
@@ -303,29 +295,42 @@ public class ConversationListStore : Gtk.ListStore {
         row_map.set(conversation, wrapper);
     }
     
-    private void refresh_conversation(Geary.App.Conversation conversation) {
+    private void on_refresh_conversation(Geary.App.Conversation conversation) {
+        refresh_conversation(conversation, null);
+    }
+    
+    private void refresh_conversation(Geary.App.Conversation conversation, Geary.Email? preview) {
         Gtk.TreeIter iter;
         if (!get_iter_for_conversation(conversation, out iter)) {
-            // Unknown conversation, attempt to append it.
+            debug("Can't refresh conversation: iter not found, adding...");
+            
             add_conversation(conversation);
+            
             return;
         }
         
-        Geary.Email? last_email = 
conversation.get_latest_recv_email(Geary.App.Conversation.Location.ANYWHERE);
-        if (last_email == null) {
-            debug("Cannot refresh conversation: last email is null");
+        FormattedConversationData? cell = get_message_data_at_iter(iter);
+        if (cell == null) {
+            debug("Can't refresh conversation: no FormattedConversationData, adding...");
+            
+            add_conversation(conversation);
             
-            remove(iter);
             return;
         }
         
-        set_row(iter, conversation, last_email);
+        assert(cell.conversation == conversation);
         
+        cell.update_conversation(preview);
+        
+        // mark row as changed (which queues a redraw)
         Gtk.TreePath? path = get_path(iter);
         if (path != null)
             row_changed(path, iter);
         else
-            debug("Cannot refresh conversation: no path for iterator");
+            debug("Error refreshing conversation: no path for iterator, can't report row changed");
+        
+        // force a re-sort of the list
+        set_default_sort_func(sort_by_date);
     }
     
     private void refresh_flags(Geary.App.Conversation conversation) {
@@ -441,18 +446,6 @@ public class ConversationListStore : Gtk.ListStore {
         remove_conversation(conversation);
     }
     
-    private void on_conversation_appended(Geary.App.Conversation conversation) {
-        if (has_conversation(conversation)) {
-            refresh_conversation(conversation);
-        } else {
-            debug("Unable to append conversation; conversation not present in list store");
-        }
-    }
-    
-    private void on_conversation_trimmed(Geary.App.Conversation conversation) {
-        refresh_conversation(conversation);
-    }
-    
     private void on_display_preview_changed() {
         refresh_previews_async.begin(conversation_monitor);
     }
@@ -469,7 +462,7 @@ public class ConversationListStore : Gtk.ListStore {
     private void on_email_paths_changed(Geary.App.Conversation conversation, 
Gee.Collection<Geary.EmailIdentifier> ids) {
         // refresh the conversation because the change in paths can change the sort order (sorting
         // depends on dates of email in this folder)
-        refresh_conversation(conversation);
+        refresh_conversation(conversation, null);
     }
     
     private int sort_by_date(Gtk.TreeModel model, Gtk.TreeIter aiter, Gtk.TreeIter biter) {
diff --git a/src/client/conversation-list/formatted-conversation-data.vala 
b/src/client/conversation-list/formatted-conversation-data.vala
index 07e01ae..21b30d4 100644
--- a/src/client/conversation-list/formatted-conversation-data.vala
+++ b/src/client/conversation-list/formatted-conversation-data.vala
@@ -72,6 +72,8 @@ public class FormattedConversationData : Geary.BaseObject {
         }
     }
     
+    public Geary.App.Conversation? conversation { get; private set; }
+    
     public bool is_unread { get; set; }
     public bool is_flagged { get; set; }
     public string date { get; private set; }
@@ -80,17 +82,14 @@ public class FormattedConversationData : Geary.BaseObject {
     public int num_emails { get; set; }
     public Geary.Email? preview { get; private set; default = null; }
     
-    private Geary.App.Conversation? conversation = null;
     private Gee.List<Geary.RFC822.MailboxAddress>? account_owner_emails = null;
     private bool use_to = true;
     private CountBadge count_badge = new CountBadge(2);
     private ConversationListCellDimensions cell_dimensions;
     
     // Creates a formatted message data from an e-mail.
-    public FormattedConversationData(Geary.App.Conversation conversation, Geary.Email preview,
+    public FormattedConversationData(Geary.App.Conversation conversation, Geary.Email? preview,
         Geary.Folder folder, Gee.List<Geary.RFC822.MailboxAddress> account_owner_emails) {
-        assert(preview.fields.fulfills(ConversationListStore.REQUIRED_FIELDS));
-        
         this.conversation = conversation;
         this.account_owner_emails = account_owner_emails;
         use_to = (folder != null) && folder.special_folder_type.is_outgoing();
@@ -99,26 +98,18 @@ public class FormattedConversationData : Geary.BaseObject {
         // the GearyController
         cell_dimensions = GearyApplication.instance.controller.cell_dimensions;
         
-        // Load preview-related data.
-        update_date_string();
-        this.subject = EmailUtil.strip_subject_prefixes(preview);
-        this.body = Geary.String.reduce_whitespace(preview.get_preview_as_string());
-        this.preview = preview;
-        
-        // Load conversation-related data.
-        this.is_unread = conversation.is_unread();
-        this.is_flagged = conversation.is_flagged();
-        this.num_emails = conversation.get_count();
+        update_conversation(preview);
     }
     
     // Creates an example message (used interally for styling calculations.)
     public FormattedConversationData.create_example() {
-        this.is_unread = false;
-        this.is_flagged = false;
-        this.date = STYLE_EXAMPLE;
-        this.subject = STYLE_EXAMPLE;
-        this.body = STYLE_EXAMPLE + "\n" + STYLE_EXAMPLE;
-        this.num_emails = 1;
+        conversation = null;
+        is_unread = false;
+        is_flagged = false;
+        date = STYLE_EXAMPLE;
+        subject = STYLE_EXAMPLE;
+        body = STYLE_EXAMPLE + "\n" + STYLE_EXAMPLE;
+        num_emails = 1;
         
         // take a reference to the global ConversationListCellDimensions object, which is held by
         // the GearyController
@@ -143,6 +134,28 @@ public class FormattedConversationData : Geary.BaseObject {
         return true;
     }
     
+    public void update_conversation(Geary.Email? supplied_preview) {
+        update_date_string();
+        
+        // caller can override which preview is shown, otherwise use the latest email in conversation
+        if (supplied_preview != null)
+            preview = supplied_preview;
+        else if (preview == null)
+            preview = conversation.get_latest_recv_email(Geary.App.Conversation.Location.ANYWHERE);
+        
+        if (preview != null) {
+            if (preview.fields.fulfills(Geary.Email.Field.SUBJECT))
+                subject = EmailUtil.strip_subject_prefixes(preview);
+            
+            if (preview.fields.fulfills(Geary.Email.Field.PREVIEW))
+                body = Geary.String.reduce_whitespace(preview.get_preview_as_string());
+        }
+        
+        is_unread = conversation.is_unread();
+        is_flagged = conversation.is_flagged();
+        num_emails = conversation.get_count();
+    }
+    
     private uint8 gdk_to_rgb(double gdk) {
         return (uint8) (gdk.clamp(0.0, 1.0) * 255.0);
     }
diff --git a/src/engine/app/app-conversation-monitor.vala b/src/engine/app/app-conversation-monitor.vala
index c4a2547..448d315 100644
--- a/src/engine/app/app-conversation-monitor.vala
+++ b/src/engine/app/app-conversation-monitor.vala
@@ -821,8 +821,9 @@ public class Geary.App.ConversationMonitor : BaseObject {
             // if conversation is empty or has no messages in primary folder path, remove it
             // entirely
             if (conversation.get_count() == 0 || !conversation.any_in_folder_path(folder.path)) {
-                // could have been trimmed earlier in the loop
+                // could have been trimmed/updated earlier in the loop
                 trimmed_conversations.remove_all(conversation);
+                paths_changed.remove_all(conversation);
                 
                 // strip Conversation from local storage and lookup tables
                 conversations.remove(conversation);
@@ -836,6 +837,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
                 // since the email was fully removed from conversation, report as trimmed
                 trimmed_conversations.set(conversation, email);
             } else if (!fully_removed && email != null) {
+                // the path was removed but the email remains
                 paths_changed.set(conversation, email.id);
             }
         }
@@ -849,6 +851,14 @@ public class Geary.App.ConversationMonitor : BaseObject {
         foreach (Conversation conversation in trimmed_conversations.get_keys())
             notify_conversation_trimmed(conversation, trimmed_conversations.get(conversation));
         
+        if (paths_changed.size > 0) {
+            debug("[%s] Paths changed in %d conversations of %d emails from %s", to_string(),
+                paths_changed.get_keys().size, paths_changed.get_values().size, path.to_string());
+        }
+        
+        foreach (Conversation conversation in paths_changed.get_keys())
+            notify_email_paths_changed(conversation, paths_changed.get(conversation));
+        
         if (removed_conversations.size > 0) {
             debug("[%s] Removed %d conversations from %s", to_string(), removed_conversations.size,
             path.to_string());
@@ -858,14 +868,6 @@ public class Geary.App.ConversationMonitor : BaseObject {
             notify_conversation_removed(conversation);
             conversation.clear_owner();
         }
-        
-        if (paths_changed.size > 0) {
-            debug("[%s] Paths changed in %d conversations of %d emails from %s", to_string(),
-                paths_changed.get_keys().size, paths_changed.get_values().size, path.to_string());
-        }
-        
-        foreach (Conversation conversation in paths_changed.get_keys())
-            notify_email_paths_changed(conversation, paths_changed.get(conversation));
     }
     
     private void on_account_email_flags_changed(Geary.Folder folder,