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

Proposed by Jeremy Wootten on 2017-01-11
Status: Merged
Approved by: Daniel Fore on 2017-06-07
Approved revision: 2374
Merged at revision: 2574
Proposed branch: lp:~jeremywootten/pantheon-files/fix-hang-on-large-copy
Merge into: lp:~elementary-apps/pantheon-files/trunk
Diff against target: 383 lines (+110/-63)
4 files modified
src/View/AbstractDirectoryView.vala (+62/-31)
src/View/AbstractTreeView.vala (+26/-18)
src/View/IconView.vala (+21/-13)
src/View/Slot.vala (+1/-1)
To merge this branch: bzr merge lp:~jeremywootten/pantheon-files/fix-hang-on-large-copy
Reviewer Review Type Date Requested Status
Adam Bieńkowski testing Approve on 2017-06-04
Dieter Debast (community) ux Approve on 2017-06-03
Zisu Andrei 2017-01-11 Pending
Review via email: mp+314546@code.launchpad.net

This proposal supersedes a proposal from 2016-11-04.

Commit message

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

Description of the 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).

To post a comment you must log in.
Zisu Andrei (matzipan) wrote : Posted in a previous version of this proposal

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

review: Approve (functionality)
Zisu Andrei (matzipan) wrote : Posted in a previous version of this proposal

 Code also LGTM.

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

review: Approve
Zisu Andrei (matzipan) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

Testing instructions corrected and extended.

Dieter Debast (ddieter) wrote :

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

review: Approve (ux)
Adam Bieńkowski (donadigo) wrote :

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) wrote :

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

text conflict in src/View/AbstractDirectoryView.vala

Jeremy Wootten (jeremywootten) wrote :

Conflict with trunk r2572 resolved.

RabbitBot (rabbitbot-a) wrote :

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

text conflict in src/View/AbstractDirectoryView.vala

2374. By Jeremy Wootten on 2017-06-06

Merge trunk to r2573 and resolve conflict in AbstractDirectoryView

Jeremy Wootten (jeremywootten) wrote :

Conflict with trunk r2573 resolved.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/View/AbstractDirectoryView.vala'
2--- src/View/AbstractDirectoryView.vala 2017-06-05 20:25:37 +0000
3+++ src/View/AbstractDirectoryView.vala 2017-06-06 09:01:03 +0000
4@@ -239,12 +239,20 @@
5 action_set_enabled (common_actions, "paste_into", false);
6 action_set_enabled (window.win_actions, "select_all", false);
7
8+ /* Fix problems when navigating away from directory with large number
9+ * of selected files (e.g. OverlayBar critical errors)
10+ */
11+ disconnect_tree_signals ();
12+
13 size_allocate.disconnect (on_size_allocate);
14 clipboard.changed.disconnect (on_clipboard_changed);
15 view.enter_notify_event.disconnect (on_enter_notify_event);
16 view.key_press_event.disconnect (on_view_key_press_event);
17 } else if (!value && _is_frozen) {
18- update_menu_actions ();
19+ /* Ensure selected files and menu actions are up to date */
20+ connect_tree_signals ();
21+ on_view_selection_changed ();
22+
23 size_allocate.connect (on_size_allocate);
24 clipboard.changed.connect (on_clipboard_changed);
25 view.enter_notify_event.connect (on_enter_notify_event);
26@@ -427,37 +435,45 @@
27 });
28 }
29 }
30- public void select_glib_files (GLib.List<GLib.File> location_list, GLib.File? focus_location) {
31- unselect_all ();
32+
33+ public void select_glib_files_when_thawed (GLib.List<GLib.File> location_list, GLib.File? focus_location) {
34 GLib.List<GOF.File>? file_list = null;
35
36 location_list.@foreach ((loc) => {
37 file_list.prepend (GOF.File.@get (loc));
38 });
39
40+ GLib.File? focus = focus_location != null ? focus_location.dup () : null;
41+
42 /* Because the Icon View disconnects the model while loading, we need to wait until
43 * the tree is thawed and the model reconnected before selecting the files */
44 Idle.add_full (GLib.Priority.LOW, () => {
45 if (!tree_frozen) {
46- file_list.@foreach ((file) => {
47- Gtk.TreeIter iter;
48- if (model.get_first_iter_for_file (file, out iter)) {
49- Gtk.TreePath path = model.get_path (iter);
50- if (path != null) {
51- select_path (path);
52- if (focus_location != null && focus_location.equal (file.location)) {
53- /* set cursor and scroll to focus location*/
54- set_cursor (path, false, false, true);
55- }
56- }
57- }
58- });
59+ select_file_paths (file_list, focus);
60 return false;
61- } else
62+ } else {
63 return true;
64+ }
65 });
66 }
67
68+ private void select_file_paths (GLib.List<GOF.File> files, GLib.File? focus) {
69+
70+ Gtk.TreeIter iter;
71+ disconnect_tree_signals (); /* Avoid unnecessary signal processing */
72+ unselect_all ();
73+
74+ foreach (GOF.File f in files) {
75+ if (model.get_first_iter_for_file (f, out iter)) {
76+ var path = model.get_path (iter);
77+ select_path (path, focus != null && focus.equal (f.location)); /* Cursor follows if matches focus location*/
78+ }
79+ }
80+
81+ connect_tree_signals ();
82+ on_view_selection_changed (); /* Update selected files and menu actions */
83+ }
84+
85 public unowned GLib.List<GLib.AppInfo> get_open_with_apps () {
86 return open_with_apps;
87 }
88@@ -587,7 +603,7 @@
89 return; /* file not in model */
90
91 var path = model.get_path (iter);
92- select_path (path);
93+ select_path (path); /* Cursor does not follow */
94 }
95
96 /** Directory signal handlers. */
97@@ -1203,6 +1219,7 @@
98 clipboard.copy_files (get_selected_files_for_transfer (get_files_for_action ()));
99 }
100
101+
102 public static void after_pasting_files (GLib.HashTable? uris, void* pointer) {
103 if (pointer == null)
104 return;
105@@ -1225,7 +1242,7 @@
106 pasted_files_list.prepend (k as File);
107 });
108
109- view.select_glib_files (pasted_files_list, pasted_files_list.first ().data);
110+ view.select_glib_files_when_thawed (pasted_files_list, pasted_files_list.first ().data);
111 return false;
112 });
113 }
114@@ -2562,15 +2579,25 @@
115 activate_selected_items (Marlin.OpenFlag.DEFAULT);
116 }
117
118+ uint update_selected_timeout_id = 0;
119 protected virtual void on_view_selection_changed () {
120+ /* updating selecting file list is expensive for large selections so throttle */
121+ if (update_selected_timeout_id == 0) {
122+ after_selected_files_changed (); /* Make sure first update happens immediately */
123+ update_selected_timeout_id = Timeout.add_full (GLib.Priority.LOW, 100, () => {
124+ update_selected_timeout_id = 0;
125+ after_selected_files_changed ();
126+ return false;
127+ });
128+ }
129+ }
130+
131+ private void after_selected_files_changed () {
132 update_selected_files ();
133 update_menu_actions ();
134- if (is_frozen)
135- return;
136-
137 selection_changed (get_selected_files ());
138 }
139-
140+
141 /** Keyboard event handling **/
142
143 /** Returns true if the code parameter matches the keycode of the keyval parameter for
144@@ -2793,7 +2820,7 @@
145 linear_select_path (path);
146 } else if (no_mods) {
147 unselect_path (old_path);
148- select_path (path);
149+ select_path (path, true); /* Cursor follows */
150 }
151 }
152 return true;
153@@ -3153,12 +3180,13 @@
154 }
155
156 if (!path_selected && click_zone != ClickZone.HELPER) {
157- if (no_mods)
158+ if (no_mods) {
159 unselect_all ();
160-
161+ }
162 /* If modifier pressed then default handler determines selection */
163- if (no_mods && !on_blank)
164- select_path (path);
165+ if (no_mods && !on_blank) {
166+ select_path (path, true); /* Cursor follows */
167+ }
168 }
169
170 bool result = true;
171@@ -3216,7 +3244,7 @@
172 unselect_path (path);
173 } else {
174 should_deselect = false;
175- select_path (path);
176+ select_path (path); /* Cursor follows */
177 }
178 }
179
180@@ -3245,7 +3273,7 @@
181
182 case Gdk.BUTTON_SECONDARY:
183 if (click_zone == ClickZone.NAME ||
184- (click_zone == ClickZone.BLANK_PATH)) {
185+ click_zone == ClickZone.BLANK_PATH) {
186
187 select_path (path);
188 } else if (click_zone == ClickZone.INVALID) {
189@@ -3458,6 +3486,7 @@
190 cancel_drag_timer ();
191 cancel_timeout (ref drag_scroll_timer_id);
192 cancel_timeout (ref add_remove_file_timeout_id);
193+ cancel_timeout (ref update_selected_timeout_id);
194 /* List View will take care of unloading subdirectories */
195 }
196
197@@ -3498,7 +3527,7 @@
198 public abstract Gtk.TreePath? get_path_at_cursor ();
199 public abstract void select_all ();
200 public abstract void unselect_all ();
201- public abstract void select_path (Gtk.TreePath? path);
202+ public abstract void select_path (Gtk.TreePath? path, bool cursor_follows = false);
203 public abstract void unselect_path (Gtk.TreePath? path);
204 public abstract bool path_is_selected (Gtk.TreePath? path);
205 public abstract bool get_visible_range (out Gtk.TreePath? start_path, out Gtk.TreePath? end_path);
206@@ -3524,6 +3553,8 @@
207 protected abstract void thaw_tree ();
208 protected new abstract void freeze_child_notify ();
209 protected new abstract void thaw_child_notify ();
210+ protected abstract void connect_tree_signals ();
211+ protected abstract void disconnect_tree_signals ();
212 protected abstract bool is_on_icon (int x, int y, int orig_x, int orig_y, ref bool on_helper);
213
214 /** Unimplemented methods
215
216=== modified file 'src/View/AbstractTreeView.vala'
217--- src/View/AbstractTreeView.vala 2017-03-01 17:43:40 +0000
218+++ src/View/AbstractTreeView.vala 2017-06-06 09:01:03 +0000
219@@ -64,6 +64,10 @@
220
221 protected void set_up_view () {
222 connect_tree_signals ();
223+ tree.realize.connect ((w) => {
224+ tree.grab_focus ();
225+ tree.columns_autosize ();
226+ });
227 }
228
229 protected override void set_up_name_renderer () {
230@@ -76,13 +80,11 @@
231 name_renderer.yalign = 0.5f;
232 }
233
234- protected void connect_tree_signals () {
235+ protected override void connect_tree_signals () {
236 tree.get_selection ().changed.connect (on_view_selection_changed);
237-
238- tree.realize.connect ((w) => {
239- tree.grab_focus ();
240- tree.columns_autosize ();
241- });
242+ }
243+ protected override void disconnect_tree_signals () {
244+ tree.get_selection ().changed.disconnect (on_view_selection_changed);
245 }
246
247 protected override Gtk.Widget? create_view () {
248@@ -131,21 +133,27 @@
249 tree.get_selection ().unselect_all ();
250 }
251
252- public override void select_path (Gtk.TreePath? path) {
253+ /* Avoid using this function with "cursor_follows = true" to select large numbers of files one by one
254+ * It would take an exponentially long time. Use "select_files" function in parent class.
255+ */
256+ public override void select_path (Gtk.TreePath? path, bool cursor_follows = false) {
257 if (path != null) {
258 var selection = tree.get_selection ();
259- /* Unlike for IconView, set_cursor unselects previously selected paths (Gtk bug?),
260- * so we have to remember them and reselect afterwards */
261- GLib.List<Gtk.TreePath> selected_paths = null;
262- selection.selected_foreach ((m, p, i) => {
263- selected_paths.prepend (p);
264- });
265- /* Ensure cursor follows last selection */
266- tree.set_cursor (path, null, false); /* This selects path but unselects rest! */
267 selection.select_path (path);
268- selected_paths.@foreach ((p) => {
269- selection.select_path (p);
270- });
271+ if (cursor_follows) {
272+ /* Unlike for IconView, set_cursor unselects previously selected paths (Gtk bug?),
273+ * so we have to remember them and reselect afterwards */
274+ GLib.List<Gtk.TreePath> selected_paths = null;
275+ selection.selected_foreach ((m, p, i) => {
276+ selected_paths.prepend (p);
277+ });
278+ /* Ensure cursor follows last selection */
279+ tree.set_cursor (path, null, false); /* This selects path but unselects rest! */
280+
281+ selected_paths.@foreach ((p) => {
282+ selection.select_path (p);
283+ });
284+ }
285 }
286 }
287 public override void unselect_path (Gtk.TreePath? path) {
288
289=== modified file 'src/View/IconView.vala'
290--- src/View/IconView.vala 2017-03-01 17:43:40 +0000
291+++ src/View/IconView.vala 2017-06-06 09:01:03 +0000
292@@ -49,6 +49,9 @@
293 (tree as Gtk.CellLayout).add_attribute (icon_renderer, "file", FM.ListModel.ColumnID.FILE_COLUMN);
294
295 connect_tree_signals ();
296+ tree.realize.connect ((w) => {
297+ tree.grab_focus ();
298+ });
299 }
300
301 protected override void set_up_name_renderer () {
302@@ -63,12 +66,11 @@
303 }
304
305
306- private void connect_tree_signals () {
307+ protected override void connect_tree_signals () {
308 tree.selection_changed.connect (on_view_selection_changed);
309-
310- tree.realize.connect ((w) => {
311- tree.grab_focus ();
312- });
313+ }
314+ protected override void disconnect_tree_signals () {
315+ tree.selection_changed.disconnect (on_view_selection_changed);
316 }
317
318 protected override Gtk.Widget? create_view () {
319@@ -134,14 +136,19 @@
320 }
321
322 public override void unselect_all () {
323- tree.unselect_all ();
324+ tree.unselect_all ();
325 }
326
327- public override void select_path (Gtk.TreePath? path) {
328+ /* Avoid using this function with "cursor_follows = true" to select large numbers of files one by one
329+ * It would take an exponentially long time. Use "select_files" function in parent class.
330+ */
331+ public override void select_path (Gtk.TreePath? path, bool cursor_follows = false) {
332 if (path != null) {
333- /* Ensure cursor follows last selection */
334- tree.set_cursor (path, null, false); /* This selects path but does not unselect the rest (unlike TreeView) */
335- tree.select_path (path);
336+ tree.select_path (path); /* This selects path but does not unselect the rest (unlike TreeView) */
337+
338+ if (cursor_follows) {
339+ tree.set_cursor (path, null, false);
340+ }
341 }
342 }
343
344@@ -362,22 +369,23 @@
345 end_path = direction_change ? last_selected : first_selected;
346 }
347
348+ /* Cursor follows when selecting path */
349 if (before_first) {
350 do {
351 p2 = p.copy ();
352- select_path (p);
353+ select_path (p, true);
354 p.next ();
355 } while (p.compare (p2) != 0 && p.compare (end_path) <= 0);
356 } else if (after_last) {
357 do {
358- select_path (p);
359+ select_path (p, true);
360 p2 = p.copy ();
361 p.prev ();
362 } while (p.compare (p2) != 0 && p.compare (end_path) >= 0);
363 } else {/* between first and last */
364 do {
365 p2 = p.copy ();
366- select_path (p);
367+ select_path (p, true);
368 p.prev ();
369 } while (p.compare (p2) != 0 && p.compare (first_selected) >= 0);
370
371
372=== modified file 'src/View/Slot.vala'
373--- src/View/Slot.vala 2017-02-04 10:57:49 +0000
374+++ src/View/Slot.vala 2017-06-06 09:01:03 +0000
375@@ -350,7 +350,7 @@
376
377 public override void select_glib_files (GLib.List<GLib.File> files, GLib.File? focus_location) {
378 if (dir_view != null)
379- dir_view.select_glib_files (files, focus_location);
380+ dir_view.select_glib_files_when_thawed (files, focus_location);
381 }
382
383 public void select_gof_file (GOF.File gof) {

Subscribers

People subscribed via source and target branches

to all changes: