Merge lp:~elementary-apps/pantheon-files/find-function-part2 into lp:~elementary-apps/pantheon-files/trunk

Proposed by Daniel Fore on 2017-05-06
Status: Merged
Approved by: Daniel Fore on 2017-06-18
Approved revision: 2426
Merged at revision: 2582
Proposed branch: lp:~elementary-apps/pantheon-files/find-function-part2
Merge into: lp:~elementary-apps/pantheon-files/trunk
Diff against target: 516 lines (+203/-98)
1 file modified
libwidgets/View/SearchResults.vala (+203/-98)
To merge this branch: bzr merge lp:~elementary-apps/pantheon-files/find-function-part2
Reviewer Review Type Date Requested Status
Adam Bieńkowski code Approve on 2017-06-18
Daniel Fore Abstain on 2017-06-08
Review via email: mp+323718@code.launchpad.net

Commit message

* Add multi-level sorting to the search results
* Categories to distinguish files in the current folder, below the current folder, in the bookmarks and in zeitgeist
* Zeitgeist searching fixed
* Each category is counted separately so a large number of hits in one category does not hide all the results in another category

Description of the change

This branch adds multi-level sorting to the search results as well as introducing categories to distinguish files in the current folder, below the current folder, in the bookmarks and in zeitgeist. Within categories the results are sorted with those starting with the search term coming first and then sorting by name.

Zeitgeist searching is fixed so more matches are found.

Each category is counted separately so a large number of hits in one category does not hide all the results in another category.

To post a comment you must log in.
Daniel Fore (danrabbit) wrote :

I don't think I like the use of "current" here instead of "local". To me "current" implies time as in Gdk.CURRENT_TIME but what we really mean here isn't results we have currently but results for the pwd.

review: Needs Fixing
Daniel Fore (danrabbit) wrote :

there's a spot that refers to the headers but it looks like it assigns CURRENT_HEADER for everything. Seems unintentional?

Daniel Fore (danrabbit) :
Jeremy Wootten (jeremywootten) wrote :

For me "LOCAL" sounds like the opposite to "REMOTE", i.e. referring to results on the local, native file system - this is in line with usage in Gtk filechooser, recent, sidebar etc. "CURRENT" in this context means the folder currently being displayed. It could be changed to "PWD_HEADER" etc if you think that is clearer?

Jeremy Wootten (jeremywootten) wrote :

Regarding the header sort keys - yes you are right - good spot! These should be changed to the appropriate Category.

Jeremy Wootten (jeremywootten) wrote :

See inline responses

Daniel Fore (danrabbit) wrote :

If there is already a convention for Files that CURRENT means present working directory, that's probably fine :)

Jeremy Wootten (jeremywootten) wrote :

Made correction to headers.

Daniel Fore (danrabbit) wrote :

While this branch renames things like "local_results" to "current_results", it doesn't rename things like "local_search" so you end up with some inconsistency there. I think I would like to see variable name changes in their own branch

Jeremy Wootten (jeremywootten) wrote :

Removed last variable name change revision

2423. By Jeremy Wootten on 2017-05-20

Revert other variable name changes

Jeremy Wootten (jeremywootten) wrote :

Reverted "current_results" to "local_results" and "zeitgeist_results" to "global_results", as requested.

Adam Bieńkowski (donadigo) wrote :

I've tested this branch, and so far it looks good, although I'm not sure about this "sort_string". Could you explain how this works in detail? I left some comments in the diff.

review: Needs Information (code / testing)
2424. By Jeremy Wootten on 2017-06-04

Improve code style

Daniel Fore (danrabbit) wrote :

I'm changing my review to abstain on this branch so that it doesn't appear that it still needs fixing on my account :)

review: Abstain
Jeremy Wootten (jeremywootten) wrote :

From Slack discussion re sort_string:

I need to have alphabetic sort of the filenames (within categories) so the whole key has to be alphabetic (I think?) How else to combine an integer order and an alphabetic order?

The only slight concern I have is whether it works in other locales.

Adam Bieńkowski (donadigo) wrote :

Couple of additional diff comments. I'm still not sure about the hole Category.to_string (). Those comments are the last thing I want to see addressed in this branch. I won't be pointing anything out afterwards since this is the last branch to migrate Files to GitHub.

review: Needs Fixing (code)
Jeremy Wootten (jeremywootten) wrote :

Addressed code style issues identified and merged latest trunk.

2425. By Jeremy Wootten on 2017-06-17

Code style improvements

2426. By Jeremy Wootten on 2017-06-17

Merge trunk to r2581

Adam Bieńkowski (donadigo) wrote :

Thanks, looks good to me now.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libwidgets/View/SearchResults.vala'
2--- libwidgets/View/SearchResults.vala 2017-05-07 16:16:18 +0000
3+++ libwidgets/View/SearchResults.vala 2017-06-17 09:54:37 +0000
4@@ -20,6 +20,38 @@
5 {
6 public class SearchResults : Gtk.Window, Searchable
7 {
8+ /* The order of these categories governs the order in which matches appear in the search view.
9+ * The category represents a first level sort. Within a category the matches sort alphabetically on name */
10+ private enum Category {
11+ CURRENT_HEADER,
12+ CURRENT_BEGINS,
13+ CURRENT_CONTAINS,
14+ CURRENT_ELLIPSIS,
15+ DEEP_HEADER,
16+ DEEP_BEGINS,
17+ DEEP_CONTAINS,
18+ DEEP_ELLIPSIS,
19+ ZEITGEIST_HEADER,
20+ ZEITGEIST_BEGINS,
21+ ZEITGEIST_CONTAINS,
22+ ZEITGEIST_ELLIPSIS,
23+ BOOKMARK_HEADER,
24+ BOOKMARK_BEGINS,
25+ BOOKMARK_CONTAINS,
26+ BOOKMARK_ELLIPSIS;
27+
28+ /* This function converts a Category enum to a letter which can be prefixed to the match
29+ * name to form a sort key. This ensures that the categories appear in the list in the
30+ * desired order - that is within each class of results (current folder, deep search,
31+ * zeitgeist search and bookmark search), after the header, the matches appear with the
32+ * "begins with" ones first, then the "contains" and finally an "ellipsis" pseudo-match
33+ * appears if MAX_RESULTS is exceeded for that category.
34+ */
35+ public string to_string () {
36+ return CharacterSet.A_2_Z.get_char ((uint)this).to_string ();
37+ }
38+ }
39+
40 class Match : Object
41 {
42 public string name { get; construct; }
43@@ -27,29 +59,34 @@
44 public string path_string { get; construct; }
45 public Icon icon { get; construct; }
46 public File? file { get; construct; }
47+ public string sortkey { get; construct; }
48
49- public Match (FileInfo info, string path_string, File parent) {
50- Object (name: info.get_display_name (),
51+ public Match (FileInfo info, string path_string, File parent, SearchResults.Category category) {
52+ var _name = info.get_display_name ();
53+ Object (name: Markup.escape_text (_name),
54 mime: info.get_content_type (),
55 icon: info.get_icon (),
56 path_string: path_string,
57- file: parent.resolve_relative_path (info.get_name ()));
58+ file: parent.resolve_relative_path (info.get_name ()),
59+ sortkey: category.to_string () + _name);
60 }
61
62- public Match.from_bookmark (Bookmark bookmark) {
63- Object (name: bookmark.label,
64+ public Match.from_bookmark (Bookmark bookmark, SearchResults.Category category) {
65+ Object (name: Markup.escape_text (bookmark.label),
66 mime: "inode/directory",
67 icon: bookmark.get_icon (),
68 path_string: "",
69- file: bookmark.get_location ());
70+ file: bookmark.get_location (),
71+ sortkey: category.to_string () + bookmark.label);
72 }
73
74- public Match.ellipsis () {
75+ public Match.ellipsis (SearchResults.Category category) {
76 Object (name: "...",
77 mime: "",
78 icon: null,
79 path_string: "",
80- file: null);
81+ file: null,
82+ sortkey: category.to_string ());
83 }
84 }
85
86@@ -76,7 +113,8 @@
87 Zeitgeist.Index zg_index;
88 GenericArray<Zeitgeist.Event> templates;
89
90- int display_count;
91+ int current_count;
92+ int deep_count;
93
94 bool local_search_finished = false;
95 bool global_search_finished = false;
96@@ -84,10 +122,11 @@
97 bool is_grabbing = false;
98 Gdk.Device? device = null;
99
100- Gtk.TreeIter local_results;
101- Gtk.TreeIter global_results;
102- Gtk.TreeIter bookmark_results;
103- Gtk.TreeIter no_results_label;
104+ Gtk.TreeIter? local_results = null;
105+ Gtk.TreeIter? deep_results = null;
106+ Gtk.TreeIter? zeitgeist_results = null;
107+ Gtk.TreeIter? bookmark_results = null;
108+
109 Gtk.TreeView view;
110 Gtk.TreeStore list;
111 Gtk.TreeModelFilter filter;
112@@ -154,19 +193,17 @@
113
114 view.append_column (column);
115
116- list = new Gtk.TreeStore (5,
117+ list = new Gtk.TreeStore (6,
118 typeof (string), /*0 file basename or category name */
119 typeof (GLib.Icon), /*1 file icon */
120- typeof (string), /*2 file location */
121- typeof (File), /*3 file object */
122- typeof (bool)); /*4 icon is visible */
123+ typeof (string?), /*2 file location */
124+ typeof (File?), /*3 file object */
125+ typeof (bool), /*4 icon is visible */
126+ typeof (string)); /*5 Sort key */
127
128 filter = new Gtk.TreeModelFilter (list, null);
129
130 filter.set_visible_func ((model, iter) => {
131- if (iter == no_results_label)
132- return n_results < 1;
133-
134 /* hide empty category headers */
135 return list.iter_depth (iter) != 0 || list.iter_has_child (iter);
136 });
137@@ -182,17 +219,27 @@
138 }
139 });
140
141+ list.set_sort_column_id (5, Gtk.SortType.ASCENDING);
142+
143 list.append (out local_results, null);
144 list.@set (local_results,
145- 0, get_category_header (_("In This Folder")));
146+ 0, get_category_header (_("In This Folder")),
147+ 5, Category.CURRENT_HEADER.to_string ());
148+
149+ list.append (out deep_results, null);
150+ list.@set (deep_results,
151+ 0, get_category_header (_("Below This Folder")),
152+ 5, Category.CURRENT_HEADER.to_string ());
153
154 list.append (out bookmark_results, null);
155 list.@set (bookmark_results,
156- 0, get_category_header (_("Bookmarks")));
157+ 0, get_category_header (_("Bookmarks")),
158+ 5, Category.CURRENT_HEADER.to_string ());
159
160- list.append (out global_results, null);
161- list.@set (global_results,
162- 0, get_category_header (_("Everywhere Else")));
163+ list.append (out zeitgeist_results, null);
164+ list.@set (zeitgeist_results,
165+ 0, get_category_header (_("Recently used")),
166+ 5, Category.CURRENT_HEADER.to_string ());
167
168 scroll.add (view);
169 frame.add (scroll);
170@@ -255,8 +302,8 @@
171 }
172
173 var include_hidden = GOF.Preferences.get_default ().show_hidden_files;
174-
175- display_count = 0;
176+ current_count = 0;
177+ deep_count = 0;
178 directory_queue = new Gee.LinkedList<File> ();
179 waiting_results = new Gee.HashMap<Gtk.TreeIter?,Gee.List> ();
180 current_root = folder;
181@@ -290,7 +337,7 @@
182 new Thread<void*> (null, () => {
183 local_search_finished = false;
184 while (!file_search_operation.is_cancelled () && directory_queue.size > 0) {
185- visit (search_term, include_hidden, file_search_operation);
186+ visit (search_term, include_hidden, file_search_operation, folder);
187 }
188
189 local_search_finished = true;
190@@ -302,10 +349,10 @@
191 get_zg_results.begin (search_term);
192
193 var bookmarks_matched = new Gee.LinkedList<Match> ();
194-
195+ var begins_with = false;
196 foreach (var bookmark in BookmarkList.get_instance ().list) {
197- if (term_matches (search_term, bookmark.label)) {
198- bookmarks_matched.add (new Match.from_bookmark (bookmark));
199+ if (term_matches (search_term, bookmark.label, out begins_with)) {
200+ bookmarks_matched.add (new Match.from_bookmark (bookmark, begins_with ? Category.BOOKMARK_BEGINS : Category.BOOKMARK_CONTAINS));
201 }
202 }
203
204@@ -662,10 +709,10 @@
205 }
206
207 foreach (var match in new_results) {
208- Gtk.TreeIter iter;
209+ Gtk.TreeIter? iter = null;
210 File file;
211- /* prevent results from showing in both global and local results */
212- if (parent == global_results) {
213+ /* do not add global result if already in local results */
214+ if (parent == zeitgeist_results) {
215 var already_added = false;
216
217 for (var valid = list.iter_nth_child (out iter, local_results, 0); valid;
218@@ -679,11 +726,25 @@
219 }
220 }
221
222+ if (!already_added) {
223+ for (var valid = list.iter_nth_child (out iter, deep_results, 0); valid;
224+ valid = list.iter_next (ref iter)) {
225+
226+ list.@get (iter, 3, out file);
227+
228+ if (file != null && match.file != null && file.equal (match.file)) {
229+ already_added = true;
230+ break;
231+ }
232+ }
233+ }
234+
235 if (already_added) {
236 continue;
237 }
238 } else if (parent == local_results) {
239- for (var valid = list.iter_nth_child (out iter, global_results, 0); valid;
240+ /* remove current search result from global if in global results */
241+ for (var valid = list.iter_nth_child (out iter, zeitgeist_results, 0); valid;
242 valid = list.iter_next (ref iter)) {
243
244 list.@get (iter, 3, out file);
245@@ -693,13 +754,25 @@
246 break;
247 }
248 }
249- }
250+ } else if (parent == deep_results) {
251+ /* remove deep search result from from global if in global results */
252+ for (var valid = list.iter_nth_child (out iter, zeitgeist_results, 0); valid;
253+ valid = list.iter_next (ref iter)) {
254+
255+ list.@get (iter, 3, out file);
256+
257+ if (file != null && match.file != null && file.equal (match.file)) {
258+ list.remove (ref iter);
259+ break;
260+ }
261+ }
262+ }
263
264 var location = "<span %s>%s</span>".printf (get_pango_grey_color_string (),
265 Markup.escape_text (match.path_string));
266
267 list.append (out iter, parent);
268- list.@set (iter, 0, Markup.escape_text (match.name), 1, match.icon, 2, location, 3, match.file, 4, true);
269+ list.@set (iter, 0, match.name, 1, match.icon, 2, location, 3, match.file, 4, true, 5, match.sortkey);
270 n_results++;
271
272 view.expand_all ();
273@@ -823,21 +896,22 @@
274 FileAttribute.STANDARD_TYPE + "," +
275 FileAttribute.STANDARD_ICON;
276
277- void visit (string term, bool include_hidden, Cancellable cancel) {
278-
279- FileEnumerator enumerator;
280+ void visit (string term, bool include_hidden, Cancellable cancel, File root_folder) {
281 var folder = directory_queue.poll ();
282
283 if (folder == null) {
284 return;
285 }
286
287+ bool in_root = folder.equal (root_folder);
288+ var category_count = in_root ? current_count : deep_count;
289+
290 var depth = 0;
291
292 File f = folder;
293 var path_string = "";
294
295- while (!f.equal (current_root)) {
296+ while (f != null && !f.equal (current_root)) {
297 path_string = f.get_basename () + (path_string == "" ? "" : Path.DIR_SEPARATOR_S + path_string);
298 f = f.get_parent ();
299 depth++;
300@@ -847,6 +921,7 @@
301 return;
302 }
303
304+ FileEnumerator enumerator;
305 try {
306 enumerator = folder.enumerate_children (ATTRIBUTES, 0, cancel);
307 } catch (Error e) {
308@@ -856,10 +931,12 @@
309 var new_results = new Gee.LinkedList<Match> ();
310
311 FileInfo info = null;
312+ Category cat;
313
314 try {
315 while (!cancel.is_cancelled () &&
316- (info = enumerator.next_file (null)) != null) {
317+ (info = enumerator.next_file (null)) != null &&
318+ category_count < MAX_RESULTS) {
319
320 if (info.get_is_hidden () && !include_hidden) {
321 continue;
322@@ -869,43 +946,45 @@
323 directory_queue.add (folder.resolve_relative_path (info.get_name ()));
324 }
325
326- if (term_matches (term, info.get_display_name ())) {
327- new_results.add (new Match (info, path_string, folder));
328- }
329+ bool begins_with;
330+ if (term_matches (term, info.get_display_name (), out begins_with)) {
331+ if (in_root) {
332+ cat = begins_with ? Category.CURRENT_BEGINS : Category.CURRENT_CONTAINS;
333+ } else {
334+ cat = begins_with ? Category.DEEP_BEGINS : Category.DEEP_CONTAINS;
335+ }
336+ new_results.add (new Match (info, path_string, folder, cat));
337+ category_count++;
338+ }
339 }
340 } catch (Error e) {warning ("Error enumerating in visit");}
341
342 if (new_results.size < 1) {
343+ cat = in_root ? Category.CURRENT_ELLIPSIS : Category.DEEP_ELLIPSIS;
344+ new_results.add (new Match.ellipsis (cat));
345 return;
346- }
347-
348- if (!cancel.is_cancelled ()) {
349- var new_count = display_count + new_results.size;
350- if (new_count > MAX_RESULTS) {
351- cancel.cancel ();
352-
353- var num_ok = MAX_RESULTS - display_count;
354- if (num_ok < new_results.size) {
355- var count = 0;
356- var it = new_results.iterator ();
357- while (it.next ()) {
358- count++;
359- if (count > num_ok)
360- it.remove ();
361- }
362- }
363-
364- new_results.add (new Match.ellipsis ());
365-
366- display_count = MAX_RESULTS;
367- } else
368- display_count = new_count;
369+ } else if (!cancel.is_cancelled ()) {
370+ if (in_root) {
371+ current_count = category_count;
372+ } else {
373+ deep_count = category_count;
374+ }
375
376 /* use a closure here to get vala to pass the userdata that we actually want */
377 Idle.add (() => {
378- add_results (new_results, local_results);
379+ add_results (new_results, in_root ? local_results : deep_results);
380 return false;
381 });
382+
383+ if (category_count >= MAX_RESULTS) {
384+ cat = in_root ? Category.CURRENT_ELLIPSIS : Category.DEEP_ELLIPSIS;
385+ new_results.add (new Match.ellipsis (cat));
386+ return;
387+ }
388+
389+ if (current_count >= MAX_RESULTS && deep_count >= MAX_RESULTS) {
390+ cancel.cancel ();
391+ }
392 }
393 }
394
395@@ -915,11 +994,11 @@
396
397 Zeitgeist.ResultSet results;
398 try {
399- results = yield zg_index.search (term,
400+ results = yield zg_index.search ("name:" + term + "*",
401 new Zeitgeist.TimeRange.anytime (),
402 templates,
403 0, /* offset */
404- MAX_RESULTS + 1,
405+ MAX_RESULTS * 3,
406 Zeitgeist.ResultType.MOST_POPULAR_SUBJECTS,
407 current_operation);
408 } catch (IOError.CANCELLED e) {
409@@ -935,54 +1014,80 @@
410
411 var matches = new Gee.LinkedList<Match> ();
412 var home = File.new_for_path (Environment.get_home_dir ());
413+ Category cat;
414 var i = 0;
415- while (results.has_next () && !current_operation.is_cancelled ()) {
416+
417+ while (results.has_next () && !current_operation.is_cancelled () && !global_search_finished) {
418 var result = results.next_value ();
419 foreach (var subject in result.subjects.data) {
420 if (i == MAX_RESULTS) {
421- matches.add (new Match.ellipsis ());
422+ matches.add (new Match.ellipsis (Category.ZEITGEIST_ELLIPSIS));
423+ global_search_finished = true;
424 break;
425 }
426
427 try {
428 var file = File.new_for_uri (subject.uri);
429- var path_string = "";
430- var parent = file;
431- while ((parent = parent.get_parent ()) != null) {
432- if (parent.equal (current_root))
433- break;
434-
435- if (parent.equal (home)) {
436- path_string = "~/" + path_string;
437- break;
438- }
439-
440- if (path_string == "")
441- path_string = parent.get_basename ();
442- else
443- path_string = parent.get_basename () + Path.DIR_SEPARATOR_S + path_string;
444- }
445-
446- var info = yield file.query_info_async (ATTRIBUTES, 0, Priority.DEFAULT, current_operation);
447- matches.add (new Match (info, path_string, file.get_parent ()));
448-
449- i++;
450+ /* Zeitgeist search finds search term anywhere in path. We are only interested
451+ * when the search term is in the basename */
452+ while (file != null && !file.get_basename ().contains (term)) {
453+ file = file.get_parent ();
454+ }
455+
456+ if (file != null) {
457+ var path_string = "";
458+ var parent = file;
459+ while ((parent = parent.get_parent ()) != null) {
460+ if (parent.equal (current_root)) {
461+ break;
462+ }
463+
464+ if (parent.equal (home)) {
465+ path_string = "~/" + path_string;
466+ break;
467+ }
468+
469+ if (path_string == "") {
470+ path_string = parent.get_basename ();
471+ } else {
472+ path_string = Path.build_path (Path.DIR_SEPARATOR_S, parent.get_basename (), path_string);
473+ }
474+ }
475+
476+ /* Eliminate duplicate matches */
477+ bool found = false;
478+ foreach (Match m in matches) {
479+ if (m.path_string == path_string) {
480+ found = true;
481+ break;
482+ }
483+ }
484+
485+ if (!found) {
486+ var info = yield file.query_info_async (ATTRIBUTES, 0, Priority.DEFAULT, current_operation);
487+ var name = info.get_display_name ();
488+ cat = name.has_prefix (term) ? Category.ZEITGEIST_BEGINS : Category.ZEITGEIST_CONTAINS;
489+ matches.add (new Match (info, path_string, file.get_parent (), cat));
490+ i++;
491+ }
492+ }
493 } catch (Error e) {}
494 }
495 }
496
497 if (!current_operation.is_cancelled ()) {
498- add_results (matches, global_results);
499+ add_results (matches, zeitgeist_results);
500 }
501
502 global_search_finished = true;
503 Idle.add (send_search_finished);
504 }
505
506- bool term_matches (string term, string name) {
507- /**TODO** improve */
508+ bool term_matches (string term, string name, out bool begins_with ) {
509 /* term is assumed to be down */
510- return name.normalize ().casefold ().contains (term);
511+ var n = name.normalize ().casefold ();
512+ begins_with = n.has_prefix (term);
513+ return n.contains (term);
514 }
515
516 string get_category_header (string title)

Subscribers

People subscribed via source and target branches