Merge lp:~mhr3/unity-lens-files/folder-fixes into lp:unity-lens-files

Proposed by Michal Hruby on 2011-09-22
Status: Merged
Merged at revision: 190
Proposed branch: lp:~mhr3/unity-lens-files/folder-fixes
Merge into: lp:unity-lens-files
Diff against target: 245 lines (+103/-45)
2 files modified
src/daemon.vala (+50/-40)
src/folder.vala (+53/-5)
To merge this branch: bzr merge lp:~mhr3/unity-lens-files/folder-fixes
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) 2011-09-22 Approve on 2011-09-22
Review via email: mp+76525@code.launchpad.net

This proposal supersedes a proposal from 2011-09-21.

Description of the change

Fixes Folder filter and bookmarks displaying deleted directories.

To post a comment you must log in.
Mikkel Kamstrup Erlandsen (kamstrup) wrote : Posted in a previous version of this proposal

I can confirm that it works :-) However, I'd like to refactor this a bit. The idea so far has been to keep all bookmarks logic contained in the Bookmarks class in folders.vala.

I suggest that the filtering be moved into that class an automatically taken care of when you call bookmarks.prefix_search() or bookmarks.list().

Also - there's no reason to do async IO here since we have no UI to block :-). In fact async IO is to be taken cautiously inside the lens daemons because it warrants the need for reentrancy safety for fast changing queries (the daemons protect somewhat against this at a higher level, but still).

Otherwise good!

review: Needs Fixing
Michal Hruby (mhr3) wrote : Posted in a previous version of this proposal

> I can confirm that it works :-) However, I'd like to refactor this a bit. The
> idea so far has been to keep all bookmarks logic contained in the Bookmarks
> class in folders.vala.
>
> I suggest that the filtering be moved into that class an automatically taken
> care of when you call bookmarks.prefix_search() or bookmarks.list().
>

Fair enough, I'll fix that.

> Also - there's no reason to do async IO here since we have no UI to block :-).
> In fact async IO is to be taken cautiously inside the lens daemons because it
> warrants the need for reentrancy safety for fast changing queries (the daemons
> protect somewhat against this at a higher level, but still).
>
> Otherwise good!

TBH, I'm not sure doing everything synchronously is such a good idea, true we do not have a UI to block, but we make the subsequent queries execute slower, imo the correct solution would be to add a Cancellable instance to each query and call cancel on it if the lens gets a new query. But perhaps this would be something for P.

ok, I like :-)

There's a small gotcha - that we can address in a later patch: When showing all folder I can see my "ratings-query" directory. But when searching for "rat" it doesn't show in the result set. I think we may be matching against the wrong field...

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/daemon.vala'
2--- src/daemon.vala 2011-09-21 09:05:51 +0000
3+++ src/daemon.vala 2011-09-22 08:51:29 +0000
4@@ -376,7 +376,7 @@
5 get_current_size_limits (out min_size, out max_size);
6
7 Unity.FilesLens.append_events_sorted (results, results_model,
8- min_size, max_size);
9+ min_size, max_size, false);
10
11 } catch (GLib.Error e) {
12 warning ("Error performing global search '%s': %s",
13@@ -414,8 +414,9 @@
14
15 string type_id = get_current_type ();
16
17- var result_type = type_id == "folder" ? ResultType.MOST_RECENT_ORIGIN
18- : ResultType.MOST_RECENT_SUBJECTS;
19+ bool origin_grouping = type_id == "folders";
20+ var result_type = origin_grouping ? ResultType.MOST_RECENT_ORIGIN
21+ : ResultType.MOST_RECENT_SUBJECTS;
22
23 /* Grab the pre-compiled template we're going to use */
24 var templates = new PtrArray.sized(1);
25@@ -457,7 +458,8 @@
26 /* FIXME: Add downloads */
27
28 Unity.FilesLens.append_events_sorted (results, results_model,
29- min_size, max_size);
30+ min_size, max_size,
31+ origin_grouping);
32
33 } catch (GLib.Error e) {
34 warning ("Error performing global search '%s': %s",
35@@ -582,8 +584,10 @@
36
37 string type_id = get_current_type ();
38
39- var result_type = type_id == "folder" ? ResultType.MOST_RECENT_ORIGIN
40- : ResultType.MOST_RECENT_SUBJECTS;
41+ bool origin_grouping = type_id == "folders";
42+ var result_type = origin_grouping ? ResultType.MOST_RECENT_ORIGIN
43+ : ResultType.MOST_RECENT_SUBJECTS;
44+
45
46 /* Grab the pre-compiled template we're going to use */
47 var templates = new PtrArray.sized(1);
48@@ -606,8 +610,10 @@
49
50 results_model.clear ();
51
52- if (type_id == "all" || type_id == "folder")
53- append_bookmarks (bookmarks.list(), results_model);
54+ if (type_id == "all" || type_id == "folders")
55+ {
56+ append_bookmarks (bookmarks.list (), results_model, Categories.FOLDERS);
57+ }
58
59 int64 min_size, max_size;
60 get_current_size_limits (out min_size, out max_size);
61@@ -615,7 +621,8 @@
62 yield update_downloads_async (results_model);
63
64 Unity.FilesLens.append_events_sorted (results, results_model,
65- min_size, max_size);
66+ min_size, max_size,
67+ origin_grouping);
68
69 } catch (GLib.Error e) {
70 warning ("Error performing empty search: %s",
71@@ -636,7 +643,7 @@
72
73 private void append_bookmarks (GLib.List<Bookmark> bookmarks,
74 Dee.Model results_model,
75- Categories category = Categories.FOLDERS)
76+ Categories category)
77 {
78 foreach (var bookmark in bookmarks)
79 {
80@@ -727,7 +734,8 @@
81 * these events are already sorted with descending timestamps */
82 public void append_events_sorted (Zeitgeist.ResultSet events,
83 Dee.Model results,
84- int64 min_size, int64 max_size)
85+ int64 min_size, int64 max_size,
86+ bool use_origin)
87 {
88 foreach (var ev in events)
89 {
90@@ -735,13 +743,33 @@
91 {
92 // FIXME: We only use the first subject...
93 Zeitgeist.Subject su = ev.get_subject(0);
94-
95- string uri = su.get_uri ();
96- string display_name = su.get_text ();
97- string mimetype = su.get_mimetype () != null ?
98- su.get_mimetype () : "application/octet-stream";
99+
100+ string uri;
101+ string display_name;
102+ string mimetype;
103+
104+ if (use_origin)
105+ {
106+ uri = su.get_origin ();
107+ display_name = "";
108+ mimetype = "inode/directory";
109+ }
110+ else
111+ {
112+ uri = su.get_uri ();
113+ display_name = su.get_text ();
114+ mimetype = su.get_mimetype ();
115+ mimetype = su.get_mimetype () != null ?
116+ su.get_mimetype () : "application/octet-stream";
117+ }
118+ if (uri == null) continue;
119 File file = File.new_for_uri (uri);
120-
121+
122+ if (display_name == null || display_name == "")
123+ {
124+ display_name = Path.get_basename (file.get_parse_name ());
125+ }
126+
127 bool check_size = min_size > 0 || max_size < int64.MAX;
128 /* Don't check existence on non-native files as http:// and
129 * friends are *very* expensive to query */
130@@ -762,34 +790,16 @@
131 continue;
132 }
133 }
134-#if 0
135- if (section == Section.FOLDERS)
136- {
137- File dir = File.new_for_uri(uri).get_parent();
138- uri = dir.get_uri ();
139- try{
140- FileInfo info = dir.query_info (FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME,
141- FileQueryInfoFlags.NONE);
142- display_name = info.get_display_name ();
143- } catch (GLib.Error e) {
144- /* Bugger, we fall back to basename, which might not be
145- * valid UTF8... */
146- warning ("Unable to get display name for %s", uri);
147- display_name = dir.get_basename ();
148- }
149- mimetype = "inode/directory";
150- }
151-#endif
152 string icon = Utils.get_icon_for_uri (uri, mimetype);
153-
154+
155 uint category_id;
156- string comment = "";
157+ string comment = file.get_parse_name ();
158
159- category_id = file.query_file_type (0, null) == FileType.DIRECTORY ? Categories.FOLDERS
160- : Categories.RECENT;
161+ category_id = file.query_file_type (0, null) == FileType.DIRECTORY ?
162+ Categories.FOLDERS : Categories.RECENT;
163 results.append (uri, icon, category_id, mimetype,
164 display_name, comment);
165-
166+
167 }
168 }
169 }
170
171=== modified file 'src/folder.vala'
172--- src/folder.vala 2011-08-04 15:26:32 +0000
173+++ src/folder.vala 2011-09-22 08:51:29 +0000
174@@ -94,10 +94,58 @@
175 bookmarks.append (bookmark);
176 }
177 }
178-
179- public unowned List<Bookmark> list ()
180- {
181- return bookmarks;
182+
183+ /* makes sure the uris exist on the filesystem (checks only native uris) */
184+ private GLib.List<Bookmark> filter_bookmarks (List<Bookmark> bm_list)
185+ {
186+ var result = new GLib.List<Bookmark> ();
187+
188+ foreach (var bookmark in bm_list)
189+ {
190+ File f = File.new_for_uri (bookmark.uri);
191+ if (f.is_native () && !f.query_exists ()) continue;
192+
193+ result.prepend (bookmark);
194+ }
195+
196+ result.reverse ();
197+ return result;
198+ }
199+#if 0
200+ private async GLib.List<Bookmark> filter_bookmarks_async (owned GLib.List<Bookmark> bm_list)
201+ {
202+ var result = new GLib.List<Bookmark> ();
203+
204+ foreach (var bookmark in bm_list)
205+ {
206+ File f = File.new_for_uri (bookmark.uri);
207+ if (f.is_native ())
208+ {
209+ bool exists;
210+ try
211+ {
212+ var info = yield f.query_info_async (FILE_ATTRIBUTE_STANDARD_TYPE,
213+ 0, Priority.DEFAULT, null);
214+ exists = info.get_file_type () != FileType.UNKNOWN;
215+ }
216+ catch (Error err)
217+ {
218+ exists = false;
219+ }
220+ if (!exists)
221+ continue;
222+ }
223+
224+ result.prepend (bookmark);
225+ }
226+ result.reverse ();
227+ return result;
228+ }
229+#endif
230+
231+ public List<Bookmark> list ()
232+ {
233+ return filter_bookmarks (bookmarks);
234 }
235
236 public List<Bookmark> prefix_search (string search)
237@@ -118,7 +166,7 @@
238 }
239 }
240
241- return (owned) matches;
242+ return filter_bookmarks (matches);
243 }
244
245 }

Subscribers

People subscribed via source and target branches