Merge lp:~azzar1/unity-lens-files/add-filesystem-icons into lp:unity-lens-files

Proposed by Andrea Azzarone on 2012-08-16
Status: Merged
Approved by: Paweł Stołowski on 2012-09-17
Approved revision: 240
Merged at revision: 237
Proposed branch: lp:~azzar1/unity-lens-files/add-filesystem-icons
Merge into: lp:unity-lens-files
Diff against target: 615 lines (+372/-35)
3 files modified
src/daemon.vala (+95/-4)
src/folder.vala (+236/-31)
tests/manual/folders.txt (+41/-0)
To merge this branch: bzr merge lp:~azzar1/unity-lens-files/add-filesystem-icons
Reviewer Review Type Date Requested Status
Paweł Stołowski 2012-08-16 Approve on 2012-09-17
Review via email: mp+119991@code.launchpad.net

Commit message

Add filesystem icons in the "Folders" category of the files lens.

Description of the change

Manual test added. It's my first time with Vala so forgive me for my errors :)

To post a comment you must log in.
234. By Andrea Azzarone on 2012-08-18

Change uri prefix.

Paweł Stołowski (stolowski) wrote :

Hi Andrea, nice job, it works well and looks good, however it's missing support for recently introduced previews - if you right-click on a device icon in the dash, an empty preview is displayed and an error is printed to the stderr:

(process:3255): unity-files-daemon-WARNING **: daemon.vala:1042: Couldn't stat file: 'device://099A-74A7'

Could you please check with design what would they like to see as a device preview and implement the missing bits in the preview(..) method?

Manual tests should now follow the format requested by QA - see TEST_TEMPLATE.txt in unity source code. I know existing tests don't follow it (and need to be fixed), but when we introduce new tests file we should stick to the new format.

Also some minor comments:

This can fail and throw GLib.Error:
415 + AppInfo.launch_default_for_uri (get_volume_uri (), null);

Formatting issues (spaces missing):
363 + public Volume volume {get; set construct; }

378 + Object (volume:volume, uri:"device://"+uuid, icon:icon_name, display_name:name, dnd_uri:"device://"+uuid);

review: Needs Fixing
Andrea Azzarone (azzar1) wrote :

> Hi Andrea, nice job, it works well and looks good, however it's missing
> support for recently introduced previews - if you right-click on a device icon
> in the dash, an empty preview is displayed and an error is printed to the
> stderr:
>
> (process:3255): unity-files-daemon-WARNING **: daemon.vala:1042: Couldn't stat
> file: 'device://099A-74A7'
>
> Could you please check with design what would they like to see as a device
> preview and implement the missing bits in the preview(..) method?

Hey thank you for the review and sorry for the delay (I was quite busy).
I'll add preview support (already talked with John Lea about it).

>
> Manual tests should now follow the format requested by QA - see
> TEST_TEMPLATE.txt in unity source code. I know existing tests don't follow it
> (and need to be fixed), but when we introduce new tests file we should stick
> to the new format.
>

Will do.

> Also some minor comments:
>
> This can fail and throw GLib.Error:
> 415 + AppInfo.launch_default_for_uri (get_volume_uri (), null);
>
>
> Formatting issues (spaces missing):
> 363 + public Volume volume {get; set construct; }
>
> 378 + Object (volume:volume, uri:"device://"+uuid, icon:icon_name,
> display_name:name, dnd_uri:"device://"+uuid);

Will fix.

235. By Andrea Azzarone on 2012-09-05

Merge trunk.

236. By Andrea Azzarone on 2012-09-05

Add space.

237. By Andrea Azzarone on 2012-09-05

Fix manual test.

238. By Andrea Azzarone on 2012-09-06

Add preview support for file system icons.

239. By Andrea Azzarone on 2012-09-06

Fix warnings.

Andrea Azzarone (azzar1) wrote :

*poke*

Paweł Stołowski (stolowski) wrote :

The preview looks fine, but I found a general problem with how uri for device item is constructed: if you plug a device with more than 1 partition, you get same uris for all partitions. I've a usb stick with 2 partitions (each partition has a different name though), and get 2 "device://0012-D687" uris for them. Left clicking on such item in the dash will always activate the same partition; also preview navigation (left/right arrow in the preview) is misbehaving in such case.

review: Needs Fixing
Andrea Azzarone (azzar1) wrote :

Maybe we can use mount point to build the uri. It should be easy enough to change.

240. By Andrea Azzarone on 2012-09-17

Use mount point as IDs.

Andrea Azzarone (azzar1) wrote :

Ok I've updated the branch to use mount point as IDs. I need to update unity trunk too.

Paweł Stołowski (stolowski) wrote :

yes, that sounds like a good fix.

Paweł Stołowski (stolowski) wrote :

Yay, it works now! Thanks!

review: Approve
Andrea Azzarone (azzar1) wrote :

MMM we cannot use the mount point because if the volume is not mounted volume.get_activation_root() always return null (it shouldn't, but it does here). Btw I've a pen drive with two partitions and they have two different uris.

Paweł Stołowski (stolowski) wrote :

Uh, how about falling back to volume name or something else?
My pendrive may be a special case as it has two *factory* made partitions that you cannot remove (it's a MySQL-branded usb stick I got during last UDS).

Andrea Azzarone (azzar1) wrote :

Let's try with label :)

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 2012-09-13 15:01:52 +0000
3+++ src/daemon.vala 2012-09-17 19:57:19 +0000
4@@ -34,6 +34,7 @@
5 private Zeitgeist.Monitor monitor;
6
7 private Bookmarks bookmarks;
8+ private Devices devices;
9 private UrlChecker urls;
10
11 private Unity.Lens lens;
12@@ -94,6 +95,13 @@
13
14 bookmarks = new Bookmarks ();
15 urls = new UrlChecker ();
16+
17+ devices = new Devices ();
18+ devices.changed.connect (() => {
19+ /* make sure our results are fresh */
20+ scope.queue_search_changed (SearchType.DEFAULT);
21+ scope.queue_search_changed (SearchType.GLOBAL);
22+ });
23
24 /* Listen for filter changes */
25 scope.filters_changed.connect (() => {
26@@ -453,6 +461,18 @@
27 0, int64.MAX, result_flags,
28 bookmark_uris, category_id);
29
30+ if (has_search)
31+ {
32+ GLib.List<Device> matching_devices;
33+
34+ if (has_search)
35+ matching_devices = devices.search (search.search_string);
36+ else
37+ matching_devices = devices.list ();
38+
39+ append_devices (matching_devices, results_model, Categories.FILES_AND_FOLDERS);
40+ }
41+
42 /* Add downloads catagory if we don't have a search */
43 if (has_search == false)
44 {
45@@ -603,6 +623,16 @@
46 min_size, max_size,
47 ResultFlags.USE_ORIGIN,
48 bookmark_uris);
49+
50+ GLib.List<Device> matching_devices;
51+
52+ if (has_search)
53+ matching_devices = devices.search (search.search_string);
54+ else
55+ matching_devices = devices.list ();
56+
57+ append_devices (matching_devices, results_model,
58+ Categories.FOLDERS);
59 }
60 else
61 {
62@@ -799,6 +829,18 @@
63 }
64 }
65
66+ private void append_devices (GLib.List<Device> devices,
67+ Dee.Model results_model,
68+ Categories category)
69+ {
70+ foreach (var device in devices)
71+ {
72+ results_model.append (device.uri, device.icon_name, category,
73+ "", device.display_name,
74+ device.dnd_uri);
75+ }
76+ }
77+
78 private async void perform_locate (uint timeout,
79 string query,
80 Dee.Model results_model,
81@@ -861,7 +903,7 @@
82 }
83 }
84
85- private void count_directory_items (string top_path, ref int count, ref int64 total_size, bool recurse=false) throws GLib.Error
86+ private void count_directory_items (string top_path, ref int count, ref uint64 total_size, bool recurse=false) throws GLib.Error
87 {
88 var dir = GLib.File.new_for_path (top_path);
89 var file_iter = dir.enumerate_children (GLib.FileAttribute.STANDARD_SIZE, GLib.FileQueryInfoFlags.NONE); //follow symlinks
90@@ -913,6 +955,11 @@
91 {
92 real_uri = Bookmark.uri_from_bookmark (uri);
93 }
94+ else if (Device.is_device_uri (uri))
95+ {
96+ preview = create_device_preview (uri);
97+ return preview;
98+ }
99 else
100 {
101 real_uri = uri;
102@@ -959,7 +1006,7 @@
103 open_action.activated.connect (on_preview_open);
104 preview.add_action (open_action);
105
106- int64 total_size = 0;
107+ uint64 total_size = 0;
108 if (finfo.get_file_type () == GLib.FileType.DIRECTORY)
109 {
110 if (file.get_path () != null)
111@@ -1010,6 +1057,50 @@
112 return preview;
113 }
114
115+
116+ private Unity.GenericPreview? create_device_preview (string uri)
117+ {
118+ var device = devices.get_device_from_uri (uri);
119+
120+ if (device == null)
121+ return null;
122+
123+ Unity.GenericPreview preview = new Unity.GenericPreview (device.display_name, "", device.icon);
124+
125+ int contents_count = 0;
126+ uint64 contents_size = 0;
127+ string filesystem_type = "";
128+ uint64 total_capacity = 0;
129+ uint64 free_capacity = 0;
130+
131+ try {
132+ var file = device.get_root_file (device.volume);
133+ var file_info = file.query_filesystem_info (GLib.FileAttribute.FILESYSTEM_SIZE + "," +
134+ GLib.FileAttribute.FILESYSTEM_TYPE + "," +
135+ GLib.FileAttribute.FILESYSTEM_FREE, null);
136+
137+ count_directory_items (file.get_path (), ref contents_count, ref contents_size, false);
138+ filesystem_type = file_info.get_attribute_as_string (GLib.FileAttribute.FILESYSTEM_TYPE);
139+ total_capacity = file_info.get_attribute_uint64 (GLib.FileAttribute.FILESYSTEM_SIZE);
140+ free_capacity = file_info.get_attribute_uint64 (GLib.FileAttribute.FILESYSTEM_FREE);
141+
142+ contents_size = total_capacity - free_capacity;
143+
144+ } catch (Error e) {
145+ warning ("Failed to query filesystem info: %s", e.message);
146+ }
147+
148+ preview.subtitle = _("Total capacity %s").printf (GLib.format_size(total_capacity));
149+ preview.add_info (new InfoHint ("filesytem_type", _("Filesystem type"), null, filesystem_type));
150+ preview.add_info (new InfoHint ("contents", _("Contents"), null, _("%s, %u items").printf (GLib.format_size (contents_size), contents_count)));
151+
152+ var open_action = new Unity.PreviewAction ("open", _("Open"), null);
153+ open_action.activated.connect (on_preview_open);
154+ preview.add_action (open_action);
155+
156+ return preview;
157+ }
158+
159 public Unity.ActivationResponse on_email_file (string uri)
160 {
161 debug (@"Emailing file: $uri");
162@@ -1033,7 +1124,7 @@
163 {
164 try
165 {
166- if (bookmarks.launch_if_bookmark (uri) || GLib.AppInfo.launch_default_for_uri (uri, null))
167+ if (bookmarks.launch_if_bookmark (uri) || devices.launch_if_device (uri) || GLib.AppInfo.launch_default_for_uri (uri, null))
168 {
169 return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
170 }
171@@ -1066,7 +1157,7 @@
172 {
173 debug (@"Activating: $uri");
174 try {
175- if (bookmarks.launch_if_bookmark (uri))
176+ if (bookmarks.launch_if_bookmark (uri) || devices.launch_if_device (uri))
177 return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
178
179 /* this code ensures that a file manager will be used
180
181=== modified file 'src/folder.vala'
182--- src/folder.vala 2012-09-03 07:07:56 +0000
183+++ src/folder.vala 2012-09-17 19:57:19 +0000
184@@ -21,18 +21,18 @@
185
186 public class Bookmarks : Object
187 {
188-
189+
190 private List<Bookmark> bookmarks;
191 private string bookmarks_file;
192 private FileMonitor monitor;
193-
194+
195 public signal void changed ();
196-
197+
198 public Bookmarks ()
199 {
200 bookmarks_file = @"$(Environment.get_home_dir())/.gtk-bookmarks";
201 update();
202-
203+
204 /* Update the bookmarks list whener the bookmarks file changes */
205 try {
206 monitor = File.new_for_path (bookmarks_file).monitor (FileMonitorFlags.NONE);
207@@ -47,7 +47,7 @@
208 bookmarks_file, e.message);
209 }
210 }
211-
212+
213 private void update ()
214 {
215 bookmarks = new List<Bookmark> ();
216@@ -57,31 +57,32 @@
217
218 File desktop_file = File.new_for_path (Environment.get_user_special_dir (UserDirectory.DESKTOP));
219 desktop_uri = desktop_file.get_uri ();
220-
221+
222 try {
223 FileUtils.get_contents (bookmarks_file, out contents);
224-
225+
226 } catch (FileError e) {
227 warning ("Failed to read favorites: %s", e.message);
228 return;
229 }
230-
231+
232 string[] favorites = contents.split ("\n");
233 string mimetype = "inode/directory";
234-
235+
236 foreach (var uri in favorites)
237 {
238 if (uri == "")
239 continue;
240+
241 // Filter out useless bookmark that is created by Nautilus.
242 // https://bugs.launchpad.net/unity-lens-files/+bug/1044309
243 if (uri.has_prefix("x-nautilus-desktop:")) continue;
244-
245+
246 if (uri == desktop_uri) has_desktop_in_favourites = true;
247-
248+
249 string[] parts = uri.split (" ", 2);
250 string display_name;
251-
252+
253 if (parts.length == 1)
254 {
255 display_name = Uri.unescape_string (uri);
256@@ -98,11 +99,11 @@
257 uri);
258 display_name = uri;
259 }
260-
261+
262 var bookmark = new Bookmark (uri, mimetype, display_name);
263 bookmarks.append (bookmark);
264 }
265-
266+
267 /* Add desktop statically */
268 if (!has_desktop_in_favourites)
269 {
270@@ -164,12 +165,12 @@
271 {
272 return filter_bookmarks (bookmarks);
273 }
274-
275+
276 public List<Bookmark> prefix_search (string search)
277 {
278 var prefix = Utils.normalize_string (search);
279 var matches = new List<Bookmark> ();
280-
281+
282 foreach (var bookmark in bookmarks)
283 {
284 foreach (var term in bookmark.get_index_terms ())
285@@ -182,35 +183,35 @@
286 }
287 }
288 }
289-
290+
291 return filter_bookmarks (matches);
292 }
293-
294+
295 public bool launch_if_bookmark (string uri) throws Error
296 {
297 if (!uri.has_prefix ("bookmark:"))
298 return false;
299-
300+
301 var launcher = AppInfo.get_default_for_type ("inode/directory", true);
302-
303+
304 uri = uri.offset (9); // Remove "bookmark:" prefix from uri
305-
306+
307 if (launcher == null)
308 {
309 warning ("No default handler for inode/directory. Unable to open bookmark '%s'", uri);
310 throw new IOError.FAILED ("No default handler for inode/directory. Unable to open bookmark '%s'", uri);
311 }
312-
313+
314 var uris = new List<string> ();
315 uris.append (uri);
316-
317+
318 launcher.launch_uris (uris, null);
319-
320+
321 return true;
322 }
323-
324+
325 }
326-
327+
328 public class Bookmark : Object
329 {
330 public string uri { get; set construct; }
331@@ -218,19 +219,19 @@
332 public string mimetype { get; set construct; }
333 public string display_name { get; set construct; }
334 public string dnd_uri { get; set construct; }
335-
336+
337 private List<string> index_terms;
338-
339+
340 public Bookmark (string uri, string mimetype, string display_name)
341 {
342 Object (uri:"bookmark:"+uri, icon:Utils.get_icon_for_uri (uri, mimetype),
343 mimetype:mimetype, display_name:display_name, dnd_uri:uri);
344-
345+
346 index_terms = new List<string> ();
347 index_terms.append (Utils.normalize_string (Path.get_basename (uri)));
348 index_terms.append (Utils.normalize_string (display_name));
349 }
350-
351+
352 public unowned List<string> get_index_terms ()
353 {
354 return index_terms;
355@@ -246,6 +247,210 @@
356 return uri.substring (9);
357 }
358 }
359-
360+
361+
362+ public class Devices : Object
363+ {
364+ private List<Device> devices;
365+ private VolumeMonitor volume_monitor;
366+
367+ public signal void changed ();
368+
369+ public Devices ()
370+ {
371+ volume_monitor = VolumeMonitor.get ();
372+
373+ update();
374+
375+ volume_monitor.volume_added.connect ((mon, volume) => {
376+ update ();
377+ changed ();
378+ });
379+
380+ volume_monitor.volume_removed.connect ((mon, volume) => {
381+ update ();
382+ changed ();
383+ });
384+
385+ volume_monitor.volume_changed.connect ((mon, volume) => {
386+ update ();
387+ changed ();
388+ });
389+ }
390+
391+ private void update ()
392+ {
393+ devices = new List<Device> ();
394+
395+ foreach ( Volume volume in volume_monitor.get_volumes ())
396+ {
397+ var device = new Device (volume);
398+ devices.append (device);
399+ }
400+ }
401+
402+ public List<Device> list ()
403+ {
404+ var result = new GLib.List<Device> ();
405+
406+ foreach (var device in devices)
407+ {
408+ result.append (device);
409+ }
410+
411+ return result;
412+ }
413+
414+ public List<Device> search (string search)
415+ {
416+ var normalized_search = Utils.normalize_string (search);
417+ var matches = new List<Device> ();
418+
419+ foreach (var device in devices)
420+ {
421+ foreach (var term in device.get_index_terms ())
422+ {
423+ if (term.contains (normalized_search))
424+ {
425+ matches.append (device);
426+ break;
427+ }
428+ }
429+ }
430+
431+ return matches;
432+ }
433+
434+ public bool launch_if_device (string uri) throws Error
435+ {
436+ if (!Device.is_device_uri (uri))
437+ return false;
438+
439+ foreach (var device in devices)
440+ {
441+ if (device.uri == uri)
442+ {
443+ device.mount_and_open ();
444+ return true;
445+ }
446+ }
447+
448+ return false;
449+ }
450+
451+ public Device? get_device_from_uri (string uri)
452+ {
453+ if (!Device.is_device_uri (uri))
454+ return null;
455+
456+ foreach (var device in devices)
457+ {
458+ if (device.uri == uri)
459+ return device;
460+ }
461+
462+ return null;
463+ }
464+ }
465+
466+
467+ public class Device : Object
468+ {
469+ public Volume volume { get; set construct; }
470+ public string name { get; set construct; }
471+ public string uri { get; set construct; }
472+ public Icon icon { get; set construct; }
473+ public string icon_name { get; set construct; }
474+ public string display_name { get; set construct; }
475+ public string dnd_uri { get; set construct; }
476+
477+ private List<string> index_terms_;
478+
479+ public Device (Volume volume)
480+ {
481+ var name = volume.get_name ();
482+ var icon = volume.get_icon ();
483+ var icon_name = icon.to_string ();
484+
485+ var identifier = get_volume_uri (volume);
486+ if (identifier != null && identifier.has_prefix ("file://"))
487+ identifier = identifier.substring(7);
488+
489+ Object (volume:volume, name:name, uri:"device://"+identifier, icon:icon,icon_name:icon_name, display_name:name, dnd_uri:"device://"+"ciao");
490+
491+ index_terms_ = new List<string> ();
492+ index_terms_.append (Utils.normalize_string (name));
493+ }
494+
495+ public unowned List<string> get_index_terms ()
496+ {
497+ return index_terms_;
498+ }
499+
500+ public static bool is_device_uri (string uri)
501+ {
502+ return uri.has_prefix ("device://");
503+ }
504+
505+ public void mount_and_open () throws Error
506+ {
507+ if (is_mounted (volume))
508+ {
509+ open_in_file_manager ();
510+ }
511+ else
512+ {
513+ volume.mount.begin (MountMountFlags.NONE, null, null, (obj, res) => {
514+ try {
515+ if (volume.mount.end (res))
516+ open_in_file_manager ();
517+ } catch (Error e) {
518+ warning ("Failed to mount %s: %s", display_name, e.message);
519+ }
520+ });
521+ }
522+ }
523+
524+ private static bool is_mounted (Volume volume)
525+ {
526+ var mount = volume.get_mount ();
527+ return mount != null;
528+ }
529+
530+ private void open_in_file_manager () throws Error
531+ {
532+ if (!is_mounted (volume))
533+ return;
534+
535+ AppInfo.launch_default_for_uri (get_volume_uri (volume), null);
536+ }
537+
538+ private static string? get_volume_uri (Volume volume)
539+ {
540+ var root = get_root_file (volume);
541+
542+ if (root == null)
543+ return null;
544+
545+ return root.get_uri ();
546+ }
547+
548+ public static File? get_root_file (Volume volume)
549+ {
550+ if (is_mounted(volume))
551+ {
552+ var mount = volume.get_mount ();
553+
554+ if (mount == null)
555+ return null;
556+
557+ return mount.get_root ();
558+ }
559+ else
560+ {
561+ return volume.get_activation_root ();
562+ }
563+ }
564+ }
565
566 } /* end: namespace */
567
568=== modified file 'tests/manual/folders.txt'
569--- tests/manual/folders.txt 2012-04-05 14:58:27 +0000
570+++ tests/manual/folders.txt 2012-09-17 19:57:19 +0000
571@@ -8,3 +8,44 @@
572 Outcome
573 You should see "Files and Folders" category with both folders starting with 'd' (including "Desktop"), followed by files containing 'd' or residing in a folder with 'd' in name.
574
575+
576+File system icons - Plug
577+----------------------------------------------------
578+Tests that file system icons are shown in the "Folder" category.
579+
580+Actions:
581+#. Plug a USB flash drive.
582+#. Open files lens.
583+#. Expand "Folder" category.
584+
585+Expected Result:
586+You should see the icon of the USB flash drive in the "Folders" category.
587+
588+
589+File system icons - Click
590+----------------------------------------------------
591+Test that file system icons can be mounted and are opened in the file manager.
592+
593+Actions:
594+#. Plug a USB flash drive.
595+#. Open files lens.
596+#. Expand "Folder" category.
597+#. Click on the icon of the USB flash drive.
598+
599+Expected Result:
600+The file manager should be opened showing the content of the USB flash drive.
601+
602+
603+File system icons - Unplug
604+----------------------------------------------------
605+Test that file system icons are removed from the "Folder" category.
606+
607+Actions:
608+#. Plug a USB flash drive.
609+#. Open files lens.
610+#. Expand "Folder" category.
611+#. Make sure you see the icon of the USB flash drive.
612+#. Unplug the USB flash drive.
613+
614+Expected Result:
615+Make sure the icon is correctly removed from the "Folder" category.

Subscribers

People subscribed via source and target branches