[geary/wip/765516-gtk-widget-conversation-viewer: 143/187] Replace the conversation list store's model on folder change.



commit 51206daa8e45d8c0d1f2b850049694805fb4361b
Author: Michael James Gratton <mike vee net>
Date:   Fri Aug 19 12:03:40 2016 +1000

    Replace the conversation list store's model on folder change.
    
    Rather than keeping a single conversation list model instance around -
    attaching and adding conversations then clearing and detaching
    conversation viewers, simply remove the old model instance and add a new
    one.
    
    This also allows the ConversationListStore::is_clearing property to be
    removed, fixing a related critical warning, and simply unhooking the
    selection-changed signal handler instead when changing the model.
    
    * src/client/components/main-window.vala (MainWindow): Remove props that
      are now dynamically managed. When the conversation monitor changes,
      replace the ConversationListView's model, update progress listeners.
    
    * src/client/conversation-list/conversation-list-view.vala
      (ConversationListView): Remove list store property since the parent
      class already maintains it. Override ::get_model() and ::set_model() to
      use ConversationListStore instances, manage signals on model instances
      when they are set/unset.
    
    * src/client/conversation-list/conversation-list-store.vala
      (ConversationListStore): Replae destructor with an explict destroy
      method that ensures the object is cleaned up enough to actualy be
      finalised. Remove use of is_clearing in ::on_selection_changed() to fix
      critical warning.

 src/client/application/geary-controller.vala       |    2 +-
 src/client/components/main-window.vala             |   40 ++++---
 .../conversation-list/conversation-list-store.vala |  127 +++++++++----------
 .../conversation-list/conversation-list-view.vala  |  125 ++++++++++++--------
 4 files changed, 156 insertions(+), 138 deletions(-)
---
diff --git a/src/client/application/geary-controller.vala b/src/client/application/geary-controller.vala
index 515db3f..a12dacc 100644
--- a/src/client/application/geary-controller.vala
+++ b/src/client/application/geary-controller.vala
@@ -1304,7 +1304,7 @@ public class GearyController : Geary.BaseObject {
         // If the folder is being unset, clear the message list and exit here.
         if (folder == null) {
             current_folder = null;
-            main_window.conversation_list_store.clear();
+            main_window.conversation_list_view.set_model(null);
             main_window.main_toolbar.folder = null;
             folder_selected(null);
 
diff --git a/src/client/components/main-window.vala b/src/client/components/main-window.vala
index ed006dc..65bdac3 100644
--- a/src/client/components/main-window.vala
+++ b/src/client/components/main-window.vala
@@ -15,10 +15,9 @@ public class MainWindow : Gtk.ApplicationWindow {
     public signal void on_shift_key(bool pressed);
     
     public FolderList.Tree folder_list { get; private set; default = new FolderList.Tree(); }
-    public ConversationListStore conversation_list_store { get; private set; default = new 
ConversationListStore(); }
     public MainToolbar main_toolbar { get; private set; }
     public SearchBar search_bar { get; private set; default = new SearchBar(); }
-    public ConversationListView conversation_list_view  { get; private set; }
+    public ConversationListView conversation_list_view  { get; private set; default = new 
ConversationListView(); }
     public ConversationViewer conversation_viewer { get; private set; default = new ConversationViewer(); }
     public StatusBar status_bar { get; private set; default = new StatusBar(); }
     public Geary.Folder? current_folder { get; private set; default = null; }
@@ -35,15 +34,12 @@ public class MainWindow : Gtk.ApplicationWindow {
     private Gtk.Box folder_box;
     private Gtk.Box conversation_box;
     private Geary.AggregateProgressMonitor progress_monitor = new Geary.AggregateProgressMonitor();
-    private Geary.ProgressMonitor? conversation_monitor_progress = null;
     private Geary.ProgressMonitor? folder_progress = null;
     
     public MainWindow(GearyApplication application) {
         Object(application: application);
         set_show_menubar(false);
         
-        conversation_list_view = new ConversationListView(conversation_list_store);
-        
         add_events(Gdk.EventMask.KEY_PRESS_MASK | Gdk.EventMask.KEY_RELEASE_MASK
             | Gdk.EventMask.FOCUS_CHANGE_MASK);
         
@@ -77,7 +73,6 @@ public class MainWindow : Gtk.ApplicationWindow {
         add_accel_group(GearyApplication.instance.ui_manager.get_accel_group());
         
         spinner.set_progress_monitor(progress_monitor);
-        progress_monitor.add(conversation_list_store.preview_monitor);
 
         delete_event.connect(on_delete_event);
         key_press_event.connect(on_key_press_event);
@@ -249,24 +244,31 @@ public class MainWindow : Gtk.ApplicationWindow {
         on_shift_key(false);
         return false;
     }
-    
+
     private void on_conversation_monitor_changed() {
-        Geary.App.ConversationMonitor? conversation_monitor =
+        ConversationListStore? old_model = this.conversation_list_view.get_model();
+        if (old_model != null) {
+            this.progress_monitor.remove(old_model.preview_monitor);
+            this.progress_monitor.remove(old_model.conversations.progress_monitor);
+        }
+
+        Geary.App.ConversationMonitor? conversations =
             GearyApplication.instance.controller.current_conversations;
-        
-        // Remove existing progress monitor.
-        if (conversation_monitor_progress != null) {
-            progress_monitor.remove(conversation_monitor_progress);
-            conversation_monitor_progress = null;
+
+        if (conversations != null) {
+            ConversationListStore new_model =
+                new ConversationListStore(conversations);
+            this.progress_monitor.add(new_model.preview_monitor);
+            this.progress_monitor.add(conversations.progress_monitor);
+            this.conversation_list_view.set_model(new_model);
         }
-        
-        // Add new one.
-        if (conversation_monitor != null) {
-            conversation_monitor_progress = conversation_monitor.progress_monitor;
-            progress_monitor.add(conversation_monitor_progress);
+
+        if (old_model != null) {
+            // Must be destroyed, but only after it has been replaced.
+            old_model.destroy();
         }
     }
-    
+
     private void on_folder_selected(Geary.Folder? folder) {
         if (folder_progress != null) {
             progress_monitor.remove(folder_progress);
diff --git a/src/client/conversation-list/conversation-list-store.vala 
b/src/client/conversation-list/conversation-list-store.vala
index 7e3d8c2..e869591 100644
--- a/src/client/conversation-list/conversation-list-store.vala
+++ b/src/client/conversation-list/conversation-list-store.vala
@@ -67,12 +67,11 @@ public class ConversationListStore : Gtk.ListStore {
             return row.get_model().get_iter(out iter, get_path());
         }
     }
-    
-    public Geary.ProgressMonitor preview_monitor { get; private set; default = 
+
+    public Geary.App.ConversationMonitor conversations { get; set; }
+    public Geary.ProgressMonitor preview_monitor { get; private set; default =
         new Geary.SimpleProgressMonitor(Geary.ProgressType.ACTIVITY); }
-    public bool is_clearing { get; private set; default = false; }
-    
-    private Geary.App.ConversationMonitor conversation_monitor;
+
     private Gee.HashMap<Geary.App.Conversation, RowWrapper> row_map = new Gee.HashMap<
         Geary.App.Conversation, RowWrapper>();
     private Geary.App.EmailStore? email_store = null;
@@ -84,65 +83,50 @@ public class ConversationListStore : Gtk.ListStore {
     public signal void conversations_added_began();
     
     public signal void conversations_added_finished();
-    
-    public ConversationListStore() {
+
+    public ConversationListStore(Geary.App.ConversationMonitor conversations) {
         set_column_types(Column.get_types());
         set_default_sort_func(sort_by_date);
         set_sort_column_id(Gtk.SortColumn.DEFAULT, Gtk.SortType.DESCENDING);
-        
+
+        this.conversations = conversations;
+        this.update_id = Timeout.add_seconds_full(
+            Priority.LOW, 60, update_date_strings
+        );
+        this.email_store = new Geary.App.EmailStore(
+            conversations.folder.account
+        );
         GearyApplication.instance.config.settings.changed[Configuration.DISPLAY_PREVIEW_KEY].connect(
             on_display_preview_changed);
-        update_id = Timeout.add_seconds_full(Priority.LOW, 60, update_date_strings);
-        
-        GearyApplication.instance.controller.notify[GearyController.PROP_CURRENT_CONVERSATION].
-            connect(on_conversation_monitor_changed);
-    }
-    
-    ~ConversationListStore() {
-        if (update_id != 0)
-            Source.remove(update_id);
-        
-        GearyApplication.instance.controller.notify[GearyController.PROP_CURRENT_CONVERSATION].
-            disconnect(on_conversation_monitor_changed);
+
+        conversations.scan_completed.connect(on_scan_completed);
+        conversations.conversations_added.connect(on_conversations_added);
+        conversations.conversation_removed.connect(on_conversation_removed);
+        conversations.conversation_appended.connect(on_conversation_appended);
+        conversations.conversation_trimmed.connect(on_conversation_trimmed);
+        conversations.email_flags_changed.connect(on_email_flags_changed);
+
+        // add all existing conversations
+        on_conversations_added(conversations.get_conversations());
     }
-    
-    private void on_conversation_monitor_changed() {
-        is_clearing = true;
-        
-        if (conversation_monitor != null) {
-            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.email_flags_changed.disconnect(on_email_flags_changed);
-        }
-        
-        cancellable.cancel();
-        cancellable = new Cancellable();
-        
+
+    public void destroy() {
+        this.cancellable.cancel();
         clear();
-        row_map.clear();
-        conversation_monitor = GearyApplication.instance.controller.current_conversations;
-        email_store = null;
-        
-        if (conversation_monitor != null) {
-            email_store = new Geary.App.EmailStore(conversation_monitor.folder.account);
-            
-            // add all existing conversations
-            on_conversations_added(conversation_monitor.get_conversations());
-            
-            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.email_flags_changed.connect(on_email_flags_changed);
+
+        // Release circular refs.
+        this.row_map.clear();
+        if (this.update_id != 0) {
+            Source.remove(this.update_id);
+            this.update_id = 0;
         }
-        
-        is_clearing = false;
+        // We need to clear the sort func to release its ref to this
+        // object so it can be finalised, but the API doesn't let a
+        // null sort fun to be set, hence set it to a dummy static
+        // func.
+        set_default_sort_func(ConversationListStore.finaliser_sort_by_date);
     }
-    
+
     public Geary.App.Conversation? get_conversation_at_path(Gtk.TreePath path) {
         Gtk.TreeIter iter;
         if (!get_iter(out iter, path))
@@ -228,7 +212,7 @@ public class ConversationListStore : Gtk.ListStore {
         // the user experience
         Gee.TreeSet<Geary.App.Conversation> sorted_conversations = new Gee.TreeSet<Geary.App.Conversation>(
             compare_conversation_descending);
-        sorted_conversations.add_all(conversation_monitor.get_conversations());
+        sorted_conversations.add_all(this.conversations.get_conversations());
         foreach (Geary.App.Conversation conversation in sorted_conversations) {
             // find oldest unread message for the preview
             Geary.Email? need_preview = null;
@@ -282,12 +266,15 @@ public class ConversationListStore : Gtk.ListStore {
         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,
-            conversation_monitor.folder.account.information.get_all_mailboxes());
-        
+        FormattedConversationData conversation_data = new FormattedConversationData(
+            conversation,
+            preview,
+            this.conversations.folder,
+            this.conversations.folder.account.information.get_all_mailboxes()
+        );
+
         Gtk.TreePath? path = get_path(iter);
         assert(path != null);
         RowWrapper wrapper = new RowWrapper(this, conversation, path);
@@ -452,7 +439,7 @@ public class ConversationListStore : Gtk.ListStore {
     }
     
     private void on_display_preview_changed() {
-        refresh_previews_async.begin(conversation_monitor);
+        refresh_previews_async.begin(this.conversations);
     }
     
     private void on_email_flags_changed(Geary.App.Conversation conversation) {
@@ -461,7 +448,7 @@ public class ConversationListStore : Gtk.ListStore {
         // refresh previews because the oldest unread message is displayed as the preview, and if
         // that's changed, need to change the preview
         // TODO: need support code to load preview for single conversation, not scan all
-        refresh_previews_async.begin(conversation_monitor);
+        refresh_previews_async.begin(this.conversations);
     }
     
     private int sort_by_date(Gtk.TreeModel model, Gtk.TreeIter aiter, Gtk.TreeIter biter) {
@@ -472,13 +459,12 @@ public class ConversationListStore : Gtk.ListStore {
         
         return compare_conversation_ascending(a, b);
     }
-    
+
     private bool update_date_strings() {
         this.foreach(update_date_string);
-        // Keep calling this SourceFunc
-        return true;
+        return Source.CONTINUE;
     }
-    
+
     private bool update_date_string(Gtk.TreeModel model, Gtk.TreePath path, Gtk.TreeIter iter) {
         FormattedConversationData? message_data;
         model.get(iter, Column.CONVERSATION_DATA, out message_data);
@@ -489,5 +475,12 @@ public class ConversationListStore : Gtk.ListStore {
         // Continue iterating, don't stop
         return false;
     }
+
+    private static int finaliser_sort_by_date(Gtk.TreeModel m,
+                                              Gtk.TreeIter a,
+                                              Gtk.TreeIter b) {
+        return 0;
+    }
+
 }
 
diff --git a/src/client/conversation-list/conversation-list-view.vala 
b/src/client/conversation-list/conversation-list-view.vala
index 587b07f..2a14eba 100644
--- a/src/client/conversation-list/conversation-list-view.vala
+++ b/src/client/conversation-list/conversation-list-view.vala
@@ -13,14 +13,13 @@ public class ConversationListView : Gtk.TreeView {
     // scroll adjustment seen at the call to load_more().
     private double last_upper = -1.0;
     private bool reset_adjustment = false;
-    private Gee.Set<Geary.App.Conversation> selected = new Gee.HashSet<Geary.App.Conversation>();
-    private ConversationListStore conversation_list_store;
     private Geary.App.ConversationMonitor? conversation_monitor;
     private Gee.Set<Geary.App.Conversation>? current_visible_conversations = null;
     private Geary.Scheduler.Scheduled? scheduled_update_visible_conversations = null;
     private Gtk.Menu? context_menu = null;
+    private Gee.Set<Geary.App.Conversation> selected = new Gee.HashSet<Geary.App.Conversation>();
     private uint selection_changed_id = 0;
-    
+
     public signal void conversations_selected(Gee.Set<Geary.App.Conversation> selected);
     
     // Signal for when a conversation has been double-clicked, or selected and enter is pressed.
@@ -34,33 +33,21 @@ public class ConversationListView : Gtk.TreeView {
         Geary.EmailFlags? flags_to_add, Geary.EmailFlags? flags_to_remove, bool only_mark_preview);
     
     public signal void visible_conversations_changed(Gee.Set<Geary.App.Conversation> visible);
-    
-    public ConversationListView(ConversationListStore conversation_list_store) {
-        this.conversation_list_store = conversation_list_store;
-        set_model(conversation_list_store);
-        
+
+    public ConversationListView() {
         set_show_expanders(false);
         set_headers_visible(false);
-        
+
         append_column(create_column(ConversationListStore.Column.CONVERSATION_DATA,
             new ConversationListCellRenderer(), ConversationListStore.Column.CONVERSATION_DATA.to_string(),
             0));
-        
+
         Gtk.TreeSelection selection = get_selection();
-        selection.changed.connect(on_selection_changed);
         selection.set_mode(Gtk.SelectionMode.MULTIPLE);
         style_set.connect(on_style_changed);
         show.connect(on_show);
         row_activated.connect(on_row_activated);
-        
-        get_model().row_inserted.connect(on_rows_changed);
-        get_model().rows_reordered.connect(on_rows_changed);
-        get_model().row_changed.connect(on_rows_changed);
-        get_model().row_deleted.connect(on_rows_changed);
-        get_model().row_deleted.connect(on_row_deleted);
-        
-        conversation_list_store.conversations_added_began.connect(on_conversations_added_began);
-        conversation_list_store.conversations_added_finished.connect(on_conversations_added_finished);
+
         button_press_event.connect(on_button_press);
 
         // Set up drag and drop.
@@ -82,7 +69,50 @@ public class ConversationListView : Gtk.TreeView {
         assert(binding_set != null);
         Gtk.BindingEntry.remove(binding_set, Gdk.Key.N, Gdk.ModifierType.CONTROL_MASK);
     }
-    
+
+    public new ConversationListStore? get_model() {
+        return (this as Gtk.TreeView).get_model() as ConversationListStore;
+    }
+
+    public new void set_model(ConversationListStore? new_store) {
+        ConversationListStore? old_store = get_model();
+        if (old_store != null) {
+            old_store.conversations_added_finished.disconnect(
+                on_conversations_added_finished
+            );
+            old_store.conversations_added_began.disconnect(
+                on_conversations_added_began
+            );
+            old_store.row_inserted.disconnect(on_rows_changed);
+            old_store.rows_reordered.disconnect(on_rows_changed);
+            old_store.row_changed.disconnect(on_rows_changed);
+            old_store.row_deleted.disconnect(on_rows_changed);
+            old_store.row_deleted.disconnect(on_row_deleted);
+        }
+
+        if (new_store != null) {
+            new_store.row_inserted.connect(on_rows_changed);
+            new_store.rows_reordered.connect(on_rows_changed);
+            new_store.row_changed.connect(on_rows_changed);
+            new_store.row_deleted.connect(on_rows_changed);
+            new_store.row_deleted.connect(on_row_deleted);
+            new_store.conversations_added_began.connect(
+                on_conversations_added_began
+            );
+            new_store.conversations_added_finished.connect(
+                on_conversations_added_finished
+            );
+        }
+
+        // Disconnect the selection handler since we don't want to
+        // fire selection signals while changing the model.
+        Gtk.TreeSelection selection = get_selection();
+        selection.changed.disconnect(on_selection_changed);
+        (this as Gtk.TreeView).set_model(new_store);
+        this.selected.clear();
+        selection.changed.connect(on_selection_changed);
+    }
+
     private void on_conversation_monitor_changed() {
         if (conversation_monitor != null) {
             conversation_monitor.scan_started.disconnect(on_scan_started);
@@ -178,7 +208,7 @@ public class ConversationListView : Gtk.TreeView {
             
             // Get the current conversation.  If it's selected, we'll apply the mark operation to
             // all selected conversations; otherwise, it just applies to this one.
-            Geary.App.Conversation conversation = conversation_list_store.get_conversation_at_path(path);
+            Geary.App.Conversation conversation = get_model().get_conversation_at_path(path);
             Gee.Collection<Geary.App.Conversation> to_mark;
             if (GearyApplication.instance.controller.get_selected_conversations().contains(conversation))
                 to_mark = GearyApplication.instance.controller.get_selected_conversations();
@@ -215,7 +245,7 @@ public class ConversationListView : Gtk.TreeView {
             return true;
         
         if (event.button == 3 && event.type == Gdk.EventType.BUTTON_PRESS) {
-            Geary.App.Conversation conversation = conversation_list_store.get_conversation_at_path(path);
+            Geary.App.Conversation conversation = get_model().get_conversation_at_path(path);
             
             string?[] action_names = {};
             action_names += GearyController.ACTION_DELETE_MESSAGE;
@@ -316,29 +346,22 @@ public class ConversationListView : Gtk.TreeView {
         if (this.selection_changed_id != 0)
             Source.remove(this.selection_changed_id);
 
-        if (this.conversation_list_store.is_clearing) {
-            // The list store is clearing, so the folder has changed
-            // and we don't want to notify about the selection
-            // changing, so just clear it.
-            this.selected.clear();
-        } else {
-            // Schedule processing selection changes at low idle for
-            // two reasons: (a) if a lot of changes come in
-            // back-to-back, this allows for all that activity to
-            // settle before updating state and firing signals (which
-            // results in a lot of I/O), and (b) it means the
-            // ConversationMonitor's signals may be processed in any
-            // order by this class and the ConversationListView and
-            // not result in a lot of screen flashing and (again)
-            // unnecessary I/O as both classes update selection state.
-            this.selection_changed_id = Idle.add(() => {
-                    // De-schedule the callback
-                    this.selection_changed_id = 0;
+        // Schedule processing selection changes at low idle for
+        // two reasons: (a) if a lot of changes come in
+        // back-to-back, this allows for all that activity to
+        // settle before updating state and firing signals (which
+        // results in a lot of I/O), and (b) it means the
+        // ConversationMonitor's signals may be processed in any
+        // order by this class and the ConversationListView and
+        // not result in a lot of screen flashing and (again)
+        // unnecessary I/O as both classes update selection state.
+        this.selection_changed_id = Idle.add(() => {
+                // De-schedule the callback
+                this.selection_changed_id = 0;
 
-                    do_selection_changed();
-                    return false;
-                }, Priority.LOW);
-        }
+                do_selection_changed();
+                return Source.REMOVE;
+            }, Priority.LOW);
     }
 
     // Gtk.TreeSelection can fire its "changed" signal even when
@@ -353,7 +376,7 @@ public class ConversationListView : Gtk.TreeView {
             // signal if different
             foreach (Gtk.TreePath path in paths) {
                 Geary.App.Conversation? conversation =
-                this.conversation_list_store.get_conversation_at_path(path);
+                get_model().get_conversation_at_path(path);
                 if (conversation != null)
                     new_selection.add(conversation);
             }
@@ -376,7 +399,7 @@ public class ConversationListView : Gtk.TreeView {
             return visible_conversations;
         
         while (start_path.compare(end_path) <= 0) {
-            Geary.App.Conversation? conversation = 
conversation_list_store.get_conversation_at_path(start_path);
+            Geary.App.Conversation? conversation = get_model().get_conversation_at_path(start_path);
             if (conversation != null)
                 visible_conversations.add(conversation);
             
@@ -390,7 +413,7 @@ public class ConversationListView : Gtk.TreeView {
         Gee.HashSet<Geary.App.Conversation> selected_conversations = new 
Gee.HashSet<Geary.App.Conversation>();
         
         foreach (Gtk.TreePath path in get_all_selected_paths()) {
-            Geary.App.Conversation? conversation = conversation_list_store.get_conversation_at_path(path);
+            Geary.App.Conversation? conversation = get_model().get_conversation_at_path(path);
             if (path != null)
                 selected_conversations.add(conversation);
         }
@@ -427,7 +450,7 @@ public class ConversationListView : Gtk.TreeView {
     }
 
     public void select_conversation(Geary.App.Conversation conversation) {
-        Gtk.TreePath path = conversation_list_store.get_path_for_conversation(conversation);
+        Gtk.TreePath path = get_model().get_path_for_conversation(conversation);
         if (path != null)
             set_cursor(path, null, false);
     }
@@ -435,7 +458,7 @@ public class ConversationListView : Gtk.TreeView {
     public void select_conversations(Gee.Set<Geary.App.Conversation> conversations) {
         Gtk.TreeSelection selection = get_selection();
         foreach (Geary.App.Conversation conversation in conversations) {
-            Gtk.TreePath path = conversation_list_store.get_path_for_conversation(conversation);
+            Gtk.TreePath path = get_model().get_path_for_conversation(conversation);
             if (path != null)
                 selection.select_path(path);
         }
@@ -465,7 +488,7 @@ public class ConversationListView : Gtk.TreeView {
     }
     
     private void on_row_activated(Gtk.TreePath path) {
-        Geary.App.Conversation? c = conversation_list_store.get_conversation_at_path(path);
+        Geary.App.Conversation? c = get_model().get_conversation_at_path(path);
         if (c != null)
             conversation_activated(c);
     }


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]