Merge lp:~angeloc/unity-lens-files/fix-for-837810 into lp:unity-lens-files

Proposed by Angelo Compagnucci on 2012-03-16
Status: Rejected
Rejected by: Michal Hruby on 2012-04-05
Proposed branch: lp:~angeloc/unity-lens-files/fix-for-837810
Merge into: lp:unity-lens-files
Diff against target: 41 lines (+12/-1)
1 file modified
src/folder.vala (+12/-1)
To merge this branch: bzr merge lp:~angeloc/unity-lens-files/fix-for-837810
Reviewer Review Type Date Requested Status
Michal Hruby (community) 2012-03-16 Needs Fixing on 2012-03-20
Review via email: mp+97924@code.launchpad.net

Description of the change

Added desktop to favourites in files and folders lens.

To post a comment you must log in.
Michal Hruby (mhr3) wrote :

Thanks for this! A couple of comments:

8 + string uri_desktop = "file://" + @"$(Environment.get_user_special_dir(UserDirectory.DESKTOP))";

Don't do this pls, use Gio's File.new_for_path and the get_uri () method in the File interface.

17 + var desktop_display_name = Uri.unescape_string (uri_desktop);
18 + desktop_display_name = Filename.display_basename (desktop_display_name);

Eeek, again, we have File.get_parse_name()

I also think that the desktop bookmark (which isn't a bookmark) should be last in the list, so it doesn't stick out.

review: Needs Fixing
Angelo Compagnucci (angeloc) wrote :

Done!

modified:
    src/folder.vala
    - Using File.new_for_path instead path string concatenation
    - Moved desktop folder to the end of favourites list

223. By Angelo Compagnucci on 2012-03-16

modified:
  src/folder.vala
  - Using File.new_for_path instead path string concatenation
  - Moved desktop folder to the end of favourites list

224. By Angelo Compagnucci on 2012-03-16

modified:
  src/folder.vala
  - Using get_parse_name() to get desktop folder name.

Angelo Compagnucci (angeloc) wrote :

Done

modified:
  src/folder.vala
  - Using get_parse_name() to get desktop folder name.

Michal Hruby (mhr3) wrote :

10 + File uri_desktop_file = File.new_for_path (@"$(Environment.get_user_special_dir(UserDirectory.DESKTOP))");

Can you please remove the string template, it's useless in this case.

36 + var desktop_display_name = Filename.display_basename (uri_desktop_file.get_parse_name());

This should be just Path.get_basename, not Filename.display_basename (Filename.display_basename doesn't expect utf-8 string).

25 + if (uri == uri_desktop)
26 + continue;

If the user has Desktop in favourites, we shouldn't mess with the order, and this should be just a flag "if (uri == uri_desktop) add_desktop_bookmark = false;".

review: Needs Fixing
Michal Hruby (mhr3) wrote :

Fixed the remaining issues and proposed in https://code.launchpad.net/~mhr3/unity-lens-files/fix-837810/+merge/100986 Thanks!

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/folder.vala'
2--- src/folder.vala 2012-03-05 09:36:13 +0000
3+++ src/folder.vala 2012-03-16 18:15:22 +0000
4@@ -52,6 +52,10 @@
5 {
6 bookmarks = new List<Bookmark> ();
7 string contents;
8+ string uri_desktop;
9+
10+ File uri_desktop_file = File.new_for_path (@"$(Environment.get_user_special_dir(UserDirectory.DESKTOP))");
11+ uri_desktop = uri_desktop_file.get_uri ();
12
13 try {
14 FileUtils.get_contents (bookmarks_file, out contents);
15@@ -64,11 +68,13 @@
16 string[] favorites = contents.split ("\n");
17 string mimetype = "inode/directory";
18
19-
20 foreach (var uri in favorites)
21 {
22 if (uri == "")
23 continue;
24+
25+ if (uri == uri_desktop)
26+ continue;
27
28 string[] parts = uri.split (" ", 2);
29 string display_name;
30@@ -93,6 +99,11 @@
31 var bookmark = new Bookmark (uri, mimetype, display_name);
32 bookmarks.append (bookmark);
33 }
34+
35+ /* Add desktop statically */
36+ var desktop_display_name = Filename.display_basename (uri_desktop_file.get_parse_name());
37+ var desktop_bookmark = new Bookmark (uri_desktop, mimetype, desktop_display_name);
38+ bookmarks.append (desktop_bookmark);
39 }
40
41 /* makes sure the uris exist on the filesystem (checks only native uris) */

Subscribers

People subscribed via source and target branches