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

Proposed by Michal Hruby on 2012-03-06
Status: Merged
Approved by: Michal Hruby on 2012-03-13
Approved revision: 222
Merged at revision: 217
Proposed branch: lp:~angeloc/unity-lens-files/fix-for-773841
Merge into: lp:unity-lens-files
Prerequisite: lp:~mhr3/unity-lens-files/fix-921665
Diff against target: 177 lines (+73/-32)
2 files modified
src/daemon.vala (+31/-13)
src/url-checker.vala (+42/-19)
To merge this branch: bzr merge lp:~angeloc/unity-lens-files/fix-for-773841
Reviewer Review Type Date Requested Status
Michal Hruby (community) Approve on 2012-03-13
Unity Bugs 2012-03-06 Pending
Review via email: mp+96100@code.launchpad.net

This proposal supersedes a proposal from 2012-03-02.

Description of the change

Fixes activation of remotely mountable volumes.

I am merge proposing this on behalf of Angelo since there appears to be some LP hiccups...

To post a comment you must log in.
Michal Hruby (mhr3) wrote : Posted in a previous version of this proposal

Nice work, thanks for the contribution!

A couple of comments:

7 private UrlChecker urls;
8 + private UrlMountChecker murls;

Wouldn't it make sense to extend the UrlChecker class to do this instead of adding yet another class that has almost the same semantics?

130 + regexes.prepend (new Regex ("\\\\.+"));
131 + regexes.prepend (new Regex ("ftp://.+"));
132 + regexes.prepend (new Regex ("ssh://.+"));
133 + regexes.prepend (new Regex ("sftp://.+"));
134 + regexes.prepend (new Regex ("smb://.+"));
135 + regexes.prepend (new Regex ("dav://.+"));

There's no need to use this many regexes - instead you can first preprocess the query by replacing \\ with smb:// (if it has \\ prefix), and then you can use a single regex "(ftp|ssh|sftp|smb|dav)://.+".

review: Needs Fixing
Angelo Compagnucci (angeloc) wrote : Posted in a previous version of this proposal

+1 for grouping regexes, I'll make it!

Unifying UrlChecker and UrlMountChecker could be coumbersone, because we need to switch icon dinamically in case you have an url or a mountable volume. We have to add some sort of side effect and a discrete amount of ifs.

Furthermore we should have a way to distinguish between url or remote location in activate method and I thought using different check_urls method be the most straightforward way to go.

Mikkel Kamstrup Erlandsen (kamstrup) wrote : Posted in a previous version of this proposal

Btw Angelo, have you signed the contributor agreement? We need that in order to be able to take your branch http://www.canonical.com/contributors

On merging of the url checker classes I think Michal meant to have two different instances of the same class, but being able to define which regexes they use in the constructor or something. Michal?

Angelo Compagnucci (angeloc) wrote : Posted in a previous version of this proposal

OK, like adding a flag that switch between url and remotes, think it could be done!
Please wait!

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

We cleared that on IRC already, I wanted the same instance and an enum defining what is what.

Angelo Compagnucci (angeloc) wrote : Posted in a previous version of this proposal

Done!

Unified url-checker, with url-mount-checker, url-mount-checker removed.
Collapsed regexes as much as possible.
Using an enum to identify url_types.
Chenged check_url to return the url type.

It's ok?

Angelo Compagnucci (angeloc) wrote : Posted in a previous version of this proposal

Done!

Merged with fix for bug 921665.
Changed code to naming conventions requirements and code style.

Michal Hruby (mhr3) wrote :

Added prereq branch and resubmitted.

Angelo Compagnucci (angeloc) wrote : Posted in a previous version of this proposal

Added UrlType.UNKNOWN as requested.

Michal Hruby (mhr3) wrote :

Almost done here, just a couple of nitpicks:

16 + private UrlType url_type;

Not used.

101 + public enum UrlType {
102 + UNKNOWN,

Missing spaces in front of the enum values

173 + switch (url_type){
174 + case UrlType.WEB:
175 + return web_icon;

Wrong indent, should be:
switch (..)
{
  case ...:
    return ...;
}

Michal Hruby (mhr3) wrote :

Great, thanks a lot!

review: Approve
Unity Merger (unity-merger) wrote :

The prerequisite lp:~mhr3/unity-lens-files/fix-921665 has not yet been merged into lp:unity-lens-files.

Unity Merger (unity-merger) wrote :

Attempt to merge into lp:unity-lens-files failed due to conflicts:

text conflict in src/daemon.vala

Michal Hruby (mhr3) wrote :

Could you please re-merge with trunk and fix the conflict?

222. By Angelo Compagnucci on 2012-03-12

Merged with trunk, fix for bug 773841

Angelo Compagnucci (angeloc) wrote :

Done!

Merged with trunk

Michal Hruby (mhr3) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/daemon.vala'
--- src/daemon.vala 2012-03-12 14:40:44 +0000
+++ src/daemon.vala 2012-03-12 23:29:17 +0000
@@ -15,6 +15,7 @@
15 *15 *
16 * Authored by Mikkel Kamstrup Erlandsen <mikkel.kamstrup@canonical.com>16 * Authored by Mikkel Kamstrup Erlandsen <mikkel.kamstrup@canonical.com>
17 * Michal Hruby <michal.hruby@canonical.com>17 * Michal Hruby <michal.hruby@canonical.com>
18 * Angelo Compagnucci <angelo.compagnucci@gmail.com>
18 *19 *
19 */20 */
20using Zeitgeist;21using Zeitgeist;
@@ -390,14 +391,16 @@
390391
391 results_model.clear ();392 results_model.clear ();
392393
393 /* check if the thing typed isn't a url (like facebook.com) */394 /* check if the thing typed isn't a url (like facebook.com)
394 var checked_url = urls.check_url (search.search_string);395 * or isn't a remote mountable url (like ftp://ftp.ubuntu.com)*/
396 var url_type = UrlType.UNKNOWN;
397 var checked_url = urls.check_url (search.search_string, out url_type);
395 if (checked_url != null)398 if (checked_url != null)
396 {399 {
397 results_model.append (checked_url, urls.icon,400 results_model.append (checked_url, urls.get_icon_for_type(url_type),
398 Categories.FILES_AND_FOLDERS,401 Categories.FILES_AND_FOLDERS,
399 "text/html", search.search_string,402 "text/html", search.search_string,
400 checked_url, checked_url);403 checked_url, checked_url);
401 }404 }
402405
403 var category_id = has_search ?406 var category_id = has_search ?
@@ -460,13 +463,16 @@
460463
461 txn.clear ();464 txn.clear ();
462465
463 /* check if the thing typed isn't a url (like facebook.com) */466 /* check if the thing typed isn't a url (like facebook.com)
464 var checked_url = urls.check_url (search.search_string);467 * or isn't a remote mountable url (like ftp://ftp.ubuntu.com)*/
468 var url_type = UrlType.UNKNOWN;
469 var checked_url = urls.check_url (search.search_string, out url_type);
465 if (checked_url != null)470 if (checked_url != null)
466 {471 {
467 txn.append (checked_url, urls.icon, Categories.RECENT,472 txn.append (checked_url, urls.get_icon_for_type(url_type),
468 "text/html", search.search_string,473 Categories.RECENT,
469 checked_url, checked_url);474 "text/html", search.search_string,
475 checked_url, checked_url);
470 }476 }
471477
472 /* apply filters to results found by zeitgeist */478 /* apply filters to results found by zeitgeist */
@@ -858,8 +864,20 @@
858 {864 {
859 debug (@"Activating: $uri");865 debug (@"Activating: $uri");
860 try {866 try {
861 if (bookmarks.launch_if_bookmark (uri))867 if (bookmarks.launch_if_bookmark (uri))
862 return new Unity.ActivationResponse (Unity.HandledType.HIDE_DASH);868 return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
869
870 /* this code ensures that a file manager will be used
871 * * if uri it's a remote location that should be mounted */
872 var url_type = UrlType.UNKNOWN;
873 var checked_url = urls.check_url (uri, out url_type);
874 if (checked_url != null && url_type == UrlType.MOUNTABLE) {
875 var muris = new GLib.List<string>();
876 muris.prepend (uri);
877 var file_manager = AppInfo.get_default_for_type("inode/directory", true);
878 file_manager.launch_uris(muris,null);
879 return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
880 }
863 } catch (GLib.Error error) {881 } catch (GLib.Error error) {
864 warning ("Failed to launch URI %s", uri);882 warning ("Failed to launch URI %s", uri);
865 }883 }
866884
=== modified file 'src/url-checker.vala'
--- src/url-checker.vala 2012-01-30 11:26:40 +0000
+++ src/url-checker.vala 2012-03-12 23:29:17 +0000
@@ -14,51 +14,74 @@
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *15 *
16 * Authored by Mikkel Kamstrup Erlandsen <mikkel.kamstrup@canonical.com>16 * Authored by Mikkel Kamstrup Erlandsen <mikkel.kamstrup@canonical.com>
17 * Modified by Angelo Compagnucci <angelo.compagnucci@gmail.com>
17 *18 *
18 */19 */
1920
20namespace Unity.FilesLens {21namespace Unity.FilesLens {
22 public enum UrlType {
23 UNKNOWN,
24 WEB,
25 MOUNTABLE;
26 }
2127
22 public class UrlChecker : Object28 public class UrlChecker : Object
23 {29 {
24 /* A string serialized GIcon */30 /* A string serialized GIcon */
25 public string icon { get; private set; }31 public string web_icon { get; private set; }
32 public string mountable_icon { get; private set; }
26 33
27 /* Regexes URLs must match */34 /* Regexes URLs must match */
28 private List<Regex> regexes;35 private Regex web_regex;
36 private Regex mountable_regex;
29 37
30 public UrlChecker ()38 public UrlChecker ()
31 {39 {
32 icon = new ThemedIcon ("web-browser").to_string ();40 web_icon = new ThemedIcon ("web-browser").to_string ();
33 regexes = new List<Regex> ();41 mountable_icon = new ThemedIcon ("folder-remote").to_string ();
34 42
35 try {43 try {
36 regexes.prepend (new Regex (".+\\...+")); // contains a dot + >= 2 chars44 web_regex = new Regex ("([a-zA-Z0-9\\-]+\\...+|http[s]{0,1}://.+)");
37 regexes.prepend (new Regex ("www.*"));45 mountable_regex = new Regex ("(\\\\|(ftp|ssh|sftp|smb|dav)://).+");
38 regexes.prepend (new Regex ("http://.+"));
39 regexes.prepend (new Regex ("https://.+"));
40 } catch (RegexError e) {46 } catch (RegexError e) {
41 warning ("Error compiling regular expressions for URL matching. URL launching will not work: %s", e.message);47 warning ("Error compiling regular expressions for URL matching. URL launching will not work: %s", e.message);
42 }48 }
43 }49 }
44 50
45 /* Returns a valid HTTP/HTTPS URL if the input looks like it,51 /* Returns a valid URL if the input looks like it or null otherwise,
46 * or null otherwise */52 * returns also url type which can after be used
47 public string? check_url (string sample)53 * to retrive corresponding icon
54 */
55 public string? check_url (string sample, out UrlType url_type)
48 {56 {
49 if (sample.strip () == "") return null;57 if (sample.strip () == "") return null;
50
51 foreach (var regex in regexes)
52 {
53 if (regex.match (sample))
54 {
55 return sample.has_prefix ("http") ? sample : ("http://" + sample);
56 }
57 }
58 58
59 if (mountable_regex.match (sample))
60 {
61 url_type = UrlType.MOUNTABLE;
62 return sample.replace("\\\\","smb://");
63 }
64 else if (web_regex.match (sample))
65 {
66 url_type = UrlType.WEB;
67 return sample.has_prefix ("http") ? sample : ("http://" + sample);
68 }
69 url_type = UrlType.UNKNOWN;
59 return null;70 return null;
60 }71 }
61 72
73 public string get_icon_for_type (UrlType url_type)
74 {
75 switch (url_type)
76 {
77 case UrlType.WEB:
78 return web_icon;
79 case UrlType.MOUNTABLE:
80 return mountable_icon;
81 }
82 return web_icon;
83 }
84
62 } /* end: class UrlChecker */85 } /* end: class UrlChecker */
6386
64} /* end: namespace */87} /* end: namespace */

Subscribers

People subscribed via source and target branches