Merge lp:~jeremywootten/pantheon-files/fix-hang-on-large-copy into lp:~elementary-apps/pantheon-files/trunk

[Jeremy Wootten]

Fix hang on selecting many files after paste or view change.

This fixes the exponentially increasing time take to select the debuting files in the view after pasting. With a large number of files (e.g. 1000+) the interface appears to hang when using List View.

TO TEST:

A.
IN ICON VIEW
1) Create two test folders and open each in a tab
2) In one folder create a large number (~1000) of small files (~1K)
3) Select all the files (Ctrl-A)
4) Copy the files to the clipboard
5) Switch to empty folder and paste the files.

In trunk the interface appears to hang for a long time after the pasted files appear (A core is maxed out).

B. IN LIST VIEW
1) Create many files as described under A.
2) Select them all.
3) Switch to icon view.

In trunk the interface appears to hang for a long time while the selected files are re-selected (one at a time) in the new view (A core is maxed out).

KILL THE PANTHEON-FILES PROCESS BEFORE RUNNING THIS BRANCH.
In this branch the pasted files or reselected files are all selected soon after they appear. The interface does not hang.

The branch additionally fixes pasting into a List or Column View - all the pasted files are selected (trunk only selects the one file, unlike Icon View)

The same code is now used whether pasting is done via keyboard or drag and drop (simplification).

[Zisu Andrei (matzipan)]

I agree that this branch seems to alleviate the issue, but it does not make it disappear entirely.

review: Approve (functionality)

[Zisu Andrei (matzipan)]

 Code also LGTM.

Not merging this yet. Waiting on merging okay from Cody.

review: Approve

[Zisu Andrei (matzipan)]

Scratch that. I'm seeing a very nasty bug. I thought it was in trunk too but it isn't.

My folder structure is like this:

folder 1
2000 text files

If I am viewing this folder and I press ctrl+a, I then deselect "folder 1", I then click "folder 1" to enter it. This results in opening all 2000 files in Scratch.

review: Needs Fixing

[Jeremy Wootten (jeremywootten)]

Thanks for spotting that bad regression. The latest version fixes that and also simplifies the code a little. Both pasted and dropped files are selected but it was being done in different ways for some reason. Now pasted files are selected in the same way as dropped files.

[Zisu Andrei (matzipan)]

I can't seem to be noticing any significant improvement (maybe I was mistaken earlier). I made sure to kill all the processes, double checked the versions I have installed, etc.

I tried both over an SSD and an HDD.

review: Needs Information

[Jeremy Wootten (jeremywootten)]

The problem does not occur if you use List or Column view (at least not with the operations mentioned in TESTING). Only if both views are Icon View. This is because only one of the pasted files is selected in List View, but they are all selected in Icon View (which is a bug in itself - the behaviour should be consistent).

You should be able to reproduce the bug using only one tab using the second set of instructions now in the description.

[Jeremy Wootten (jeremywootten)]

Testing instructions corrected and extended.

[Dieter Debast (ddieter)]

I can confirm that it works much smoother on this branch.

review: Approve (ux)

[Adam Bieńkowski (donadigo)]

This works properly here however I cannot verify if the changes in the code make sense because of Files code complexity, anyway I'm giving this an approve.

review: Approve (testing)

[RabbitBot (rabbitbot-a)]

Attempt to merge into lp:pantheon-files failed due to conflicts:

text conflict in src/View/AbstractDirectoryView.vala

[Jeremy Wootten (jeremywootten)]

Conflict with trunk r2572 resolved.

[RabbitBot (rabbitbot-a)]

Attempt to merge into lp:pantheon-files failed due to conflicts:

text conflict in src/View/AbstractDirectoryView.vala

[lp:~jeremywootten/pantheon-files/fix-hang-on-large-copy]

2374. By Jeremy Wootten on 2017-06-06

Merge trunk to r2573 and resolve conflict in AbstractDirectoryView

[Jeremy Wootten (jeremywootten)]

Conflict with trunk r2573 resolved.

=== modified file 'src/View/AbstractDirectoryView.vala'
--- src/View/AbstractDirectoryView.vala	2017-06-05 20:25:37 +0000
+++ src/View/AbstractDirectoryView.vala	2017-06-06 09:01:03 +0000
@@ -239,12 +239,20 @@
                     action_set_enabled (common_actions, "paste_into", false);
                     action_set_enabled (window.win_actions, "select_all", false);
 
+                    /* Fix problems when navigating away from directory with large number
+                     * of selected files (e.g. OverlayBar critical errors)
+                     */  
+                    disconnect_tree_signals ();
+
                     size_allocate.disconnect (on_size_allocate);
                     clipboard.changed.disconnect (on_clipboard_changed);
                     view.enter_notify_event.disconnect (on_enter_notify_event);
                     view.key_press_event.disconnect (on_view_key_press_event);
                 } else if (!value && _is_frozen) {
-                    update_menu_actions ();
+                    /* Ensure selected files and menu actions are up to date */
+                    connect_tree_signals ();
+                    on_view_selection_changed ();
+
                     size_allocate.connect (on_size_allocate);
                     clipboard.changed.connect (on_clipboard_changed);
                     view.enter_notify_event.connect (on_enter_notify_event);
@@ -427,37 +435,45 @@
                 });
             }
         }
-        public void select_glib_files (GLib.List<GLib.File> location_list, GLib.File? focus_location) {
-            unselect_all ();
+
+        public void select_glib_files_when_thawed (GLib.List<GLib.File> location_list, GLib.File? focus_location) {
             GLib.List<GOF.File>? file_list = null;
 
             location_list.@foreach ((loc) => {
                 file_list.prepend (GOF.File.@get (loc));
             });
 
+            GLib.File? focus = focus_location != null ? focus_location.dup () : null;
+
             /* Because the Icon View disconnects the model while loading, we need to wait until
              * the tree is thawed and the model reconnected before selecting the files */
             Idle.add_full (GLib.Priority.LOW, () => {
                 if (!tree_frozen) {
-                    file_list.@foreach ((file) => {
-                        Gtk.TreeIter iter;
-                        if (model.get_first_iter_for_file (file, out iter)) {
-                            Gtk.TreePath path = model.get_path (iter);
-                            if (path != null) {
-                                select_path (path);
-                                if (focus_location != null && focus_location.equal (file.location)) {
-                                    /* set cursor and scroll to focus location*/
-                                    set_cursor (path, false, false, true);
-                                }
-                            }
-                        }
-                    });
+                    select_file_paths (file_list, focus);
                     return false;
-                } else
+                } else {
                     return true;
+                }
             });
         }
 
+        private void select_file_paths (GLib.List<GOF.File> files, GLib.File? focus) {
+
+            Gtk.TreeIter iter;
+            disconnect_tree_signals (); /* Avoid unnecessary signal processing */
+            unselect_all ();
+
+            foreach (GOF.File f in files) {
+               if (model.get_first_iter_for_file (f, out iter)) {
+                    var path = model.get_path (iter);
+                    select_path (path, focus != null && focus.equal (f.location));  /* Cursor follows if matches focus location*/
+                }
+            }
+
+            connect_tree_signals ();
+            on_view_selection_changed (); /* Update selected files and menu actions */
+        }
+
         public unowned GLib.List<GLib.AppInfo> get_open_with_apps () {
             return open_with_apps;
         }
@@ -587,7 +603,7 @@
                 return; /* file not in model */
 
             var path = model.get_path (iter);
-            select_path (path);
+            select_path (path); /* Cursor does not follow */
         }
 
     /** Directory signal handlers. */
@@ -1203,6 +1219,7 @@
             clipboard.copy_files (get_selected_files_for_transfer (get_files_for_action ()));
         }
 
+
         public static void after_pasting_files (GLib.HashTable? uris, void* pointer) {
             if (pointer == null)
                 return;
@@ -1225,7 +1242,7 @@
                         pasted_files_list.prepend (k as File);
                 });
 
-                view.select_glib_files (pasted_files_list, pasted_files_list.first ().data);
+                view.select_glib_files_when_thawed (pasted_files_list, pasted_files_list.first ().data);
                 return false;
             });
         }
@@ -2562,15 +2579,25 @@
             activate_selected_items (Marlin.OpenFlag.DEFAULT);
         }
 
+        uint update_selected_timeout_id = 0;
         protected virtual void on_view_selection_changed () {
+            /* updating selecting file list is expensive for large selections so throttle */
+            if (update_selected_timeout_id == 0) {
+                after_selected_files_changed (); /* Make sure first update happens immediately */
+                update_selected_timeout_id = Timeout.add_full (GLib.Priority.LOW, 100, () => {
+                    update_selected_timeout_id = 0;
+                    after_selected_files_changed ();
+                    return false;
+                });
+            }
+        }
+
+        private void after_selected_files_changed () {
             update_selected_files ();
             update_menu_actions ();
-            if (is_frozen)
-                return;
-
             selection_changed (get_selected_files ());
         }
-
+ 
 /** Keyboard event handling **/
 
         /** Returns true if the code parameter matches the keycode of the keyval parameter for
@@ -2793,7 +2820,7 @@
                                     linear_select_path (path);
                                 } else if (no_mods) {
                                     unselect_path (old_path);
-                                    select_path (path);
+                                    select_path (path, true);  /* Cursor follows */
                                 }
                             }
                             return true;
@@ -3153,12 +3180,13 @@
             }
 
             if (!path_selected && click_zone != ClickZone.HELPER) {
-                if (no_mods)
+                if (no_mods) {
                     unselect_all ();
-
+                }
                 /* If modifier pressed then default handler determines selection */
-                if (no_mods && !on_blank)
-                    select_path (path);
+                if (no_mods && !on_blank) {
+                    select_path (path, true);  /* Cursor follows */
+                }
             }
 
             bool result = true;
@@ -3216,7 +3244,7 @@
                                     unselect_path (path);
                                 } else {
                                     should_deselect = false;
-                                    select_path (path);
+                                    select_path (path);  /* Cursor follows */
                                 }
                             }
 
@@ -3245,7 +3273,7 @@
 
                 case Gdk.BUTTON_SECONDARY:
                     if (click_zone == ClickZone.NAME ||
-                        (click_zone == ClickZone.BLANK_PATH)) {
+                        click_zone == ClickZone.BLANK_PATH) {
 
                         select_path (path);
                     } else if (click_zone == ClickZone.INVALID) {
@@ -3458,6 +3486,7 @@
             cancel_drag_timer ();
             cancel_timeout (ref drag_scroll_timer_id);
             cancel_timeout (ref add_remove_file_timeout_id);
+            cancel_timeout (ref update_selected_timeout_id);
             /* List View will take care of unloading subdirectories */
         }
 
@@ -3498,7 +3527,7 @@
         public abstract Gtk.TreePath? get_path_at_cursor ();
         public abstract void select_all ();
         public abstract void unselect_all ();
-        public abstract void select_path (Gtk.TreePath? path);
+        public abstract void select_path (Gtk.TreePath? path, bool cursor_follows = false);
         public abstract void unselect_path (Gtk.TreePath? path);
         public abstract bool path_is_selected (Gtk.TreePath? path);
         public abstract bool get_visible_range (out Gtk.TreePath? start_path, out Gtk.TreePath? end_path);
@@ -3524,6 +3553,8 @@
         protected abstract void thaw_tree ();
         protected new abstract void freeze_child_notify ();
         protected new abstract void thaw_child_notify ();
+        protected abstract void connect_tree_signals ();
+        protected abstract void disconnect_tree_signals ();
         protected abstract bool is_on_icon (int x, int y, int orig_x, int orig_y, ref bool on_helper);
 
 /** Unimplemented methods
 
=== modified file 'src/View/AbstractTreeView.vala'
--- src/View/AbstractTreeView.vala	2017-03-01 17:43:40 +0000
+++ src/View/AbstractTreeView.vala	2017-06-06 09:01:03 +0000
@@ -64,6 +64,10 @@
 
         protected void set_up_view () {
             connect_tree_signals ();
+            tree.realize.connect ((w) => {
+                tree.grab_focus ();
+                tree.columns_autosize ();
+            });
         }
 
         protected override void set_up_name_renderer () {
@@ -76,13 +80,11 @@
             name_renderer.yalign = 0.5f;
         }
 
-        protected void connect_tree_signals () {
+        protected override void connect_tree_signals () {
             tree.get_selection ().changed.connect (on_view_selection_changed);
-
-            tree.realize.connect ((w) => {
-                tree.grab_focus ();
-                tree.columns_autosize ();
-            });
+        }
+        protected override void disconnect_tree_signals () {
+            tree.get_selection ().changed.disconnect (on_view_selection_changed);
         }
 
         protected override Gtk.Widget? create_view () {
@@ -131,21 +133,27 @@
             tree.get_selection ().unselect_all ();
         }
 
-        public override void select_path (Gtk.TreePath? path) {
+        /* Avoid using this function with "cursor_follows = true" to select large numbers of files one by one
+         * It would take an exponentially long time. Use "select_files" function in parent class.
+         */  
+        public override void select_path (Gtk.TreePath? path, bool cursor_follows = false) {
             if (path != null) {
                 var selection = tree.get_selection ();
-                /* Unlike for IconView, set_cursor unselects previously selected paths (Gtk bug?),
-                 * so we have to remember them and reselect afterwards */ 
-                GLib.List<Gtk.TreePath> selected_paths = null;
-                selection.selected_foreach ((m, p, i) => {
-                    selected_paths.prepend (p);
-                });
-                /* Ensure cursor follows last selection */
-                tree.set_cursor (path, null, false);  /* This selects path but unselects rest! */
                 selection.select_path (path);
-                selected_paths.@foreach ((p) => {
-                    selection.select_path (p);
-                });
+                if (cursor_follows) {
+                    /* Unlike for IconView, set_cursor unselects previously selected paths (Gtk bug?),
+                     * so we have to remember them and reselect afterwards */ 
+                    GLib.List<Gtk.TreePath> selected_paths = null;
+                    selection.selected_foreach ((m, p, i) => {
+                        selected_paths.prepend (p);
+                    });
+                    /* Ensure cursor follows last selection */
+                    tree.set_cursor (path, null, false);  /* This selects path but unselects rest! */
+
+                    selected_paths.@foreach ((p) => {
+                       selection.select_path (p);
+                    });
+                }
             }
         }
         public override void unselect_path (Gtk.TreePath? path) {
 
=== modified file 'src/View/IconView.vala'
--- src/View/IconView.vala	2017-03-01 17:43:40 +0000
+++ src/View/IconView.vala	2017-06-06 09:01:03 +0000
@@ -49,6 +49,9 @@
             (tree as Gtk.CellLayout).add_attribute (icon_renderer, "file", FM.ListModel.ColumnID.FILE_COLUMN);
 
             connect_tree_signals ();
+            tree.realize.connect ((w) => {
+                tree.grab_focus ();
+            });
         }
 
         protected override void set_up_name_renderer () {
@@ -63,12 +66,11 @@
         }
 
 
-        private void connect_tree_signals () {
+        protected override void connect_tree_signals () {
             tree.selection_changed.connect (on_view_selection_changed);
-
-            tree.realize.connect ((w) => {
-                tree.grab_focus ();
-            });
+        }
+        protected override void disconnect_tree_signals () {
+            tree.selection_changed.disconnect (on_view_selection_changed);
         }
 
         protected override Gtk.Widget? create_view () {
@@ -134,14 +136,19 @@
         }
 
         public override void unselect_all () {
-                tree.unselect_all ();
+            tree.unselect_all ();
         }
 
-        public override void select_path (Gtk.TreePath? path) {
+        /* Avoid using this function with "cursor_follows = true" to select large numbers of files one by one
+         * It would take an exponentially long time. Use "select_files" function in parent class.
+         */ 
+        public override void select_path (Gtk.TreePath? path, bool cursor_follows = false) {
             if (path != null) {
-                /* Ensure cursor follows last selection */
-                tree.set_cursor (path, null, false); /* This selects path but does not unselect the rest (unlike TreeView) */
-                tree.select_path (path);
+                tree.select_path (path);  /* This selects path but does not unselect the rest (unlike TreeView) */
+
+                if (cursor_follows) {
+                    tree.set_cursor (path, null, false);
+                }
             }
         }
 
@@ -362,22 +369,23 @@
                 end_path = direction_change ? last_selected : first_selected;
             }
 
+            /* Cursor follows when selecting path */
             if (before_first) {
                 do {
                     p2 = p.copy ();
-                    select_path (p);
+                    select_path (p, true);
                     p.next ();
                 } while (p.compare (p2) != 0 && p.compare (end_path) <= 0);
             } else if (after_last) {
                 do {
-                    select_path (p);
+                    select_path (p, true);
                     p2 = p.copy ();
                     p.prev ();
                 } while (p.compare (p2) != 0 && p.compare (end_path) >= 0);
             } else {/* between first and last */
                 do {
                     p2 = p.copy ();
-                    select_path (p);
+                    select_path (p, true);
                     p.prev ();
                 } while (p.compare (p2) != 0 && p.compare (first_selected) >= 0);
 
 
=== modified file 'src/View/Slot.vala'
--- src/View/Slot.vala	2017-02-04 10:57:49 +0000
+++ src/View/Slot.vala	2017-06-06 09:01:03 +0000
@@ -350,7 +350,7 @@
 
         public override void select_glib_files (GLib.List<GLib.File> files, GLib.File? focus_location) {
             if (dir_view != null)
-                dir_view.select_glib_files (files, focus_location);
+                dir_view.select_glib_files_when_thawed (files, focus_location);
         }
 
         public void select_gof_file (GOF.File gof) {