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

[Michal Hruby]

Fixes activation of remotely mountable volumes.

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

[Michal Hruby (mhr3)]

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)]

+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)]

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)]

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

[Michal Hruby (mhr3)]

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

[Angelo Compagnucci (angeloc)]

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)]

Done!

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

[Michal Hruby (mhr3)]

Added prereq branch and resubmitted.

[Angelo Compagnucci (angeloc)]

Added UrlType.UNKNOWN as requested.

[Michal Hruby (mhr3)]

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)]

Great, thanks a lot!

review: Approve

[Unity Merger (unity-merger)]

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

[Unity Merger (unity-merger)]

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

text conflict in src/daemon.vala

[Michal Hruby (mhr3)]

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

[lp:~angeloc/unity-lens-files/fix-for-773841]

222. By Angelo Compagnucci on 2012-03-12

Merged with trunk, fix for bug 773841

[Angelo Compagnucci (angeloc)]

Done!

Merged with trunk

[Michal Hruby (mhr3)]

review: Approve

=== 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 @@
  *
  * Authored by Mikkel Kamstrup Erlandsen <[e-mail]>
  *             Michal Hruby <[e-mail]>
+ *             Angelo Compagnucci <[e-mail]>
  *
  */
 using Zeitgeist;
@@ -390,14 +391,16 @@
 
         results_model.clear ();
 
-        /* check if the thing typed isn't a url (like facebook.com) */
-        var checked_url = urls.check_url (search.search_string);
+        /* check if the thing typed isn't a url (like facebook.com) 
+         * or isn't a remote mountable url (like ftp://ftp.ubuntu.com)*/
+        var url_type = UrlType.UNKNOWN;
+        var checked_url = urls.check_url (search.search_string, out url_type);
         if (checked_url != null)
         {
-          results_model.append (checked_url, urls.icon,
-                                Categories.FILES_AND_FOLDERS,
-                                "text/html", search.search_string,
-                                checked_url, checked_url);
+          results_model.append (checked_url, urls.get_icon_for_type(url_type),
+                Categories.FILES_AND_FOLDERS,
+                "text/html", search.search_string,
+                checked_url, checked_url);
         }
 
         var category_id = has_search ?
@@ -460,13 +463,16 @@
 
         txn.clear ();
 
-        /* check if the thing typed isn't a url (like facebook.com) */
-        var checked_url = urls.check_url (search.search_string);
+        /* check if the thing typed isn't a url (like facebook.com) 
+         * or isn't a remote mountable url (like ftp://ftp.ubuntu.com)*/
+        var url_type = UrlType.UNKNOWN; 
+        var checked_url = urls.check_url (search.search_string, out url_type);
         if (checked_url != null)
         {
-          txn.append (checked_url, urls.icon, Categories.RECENT,
-                      "text/html", search.search_string,
-                      checked_url, checked_url);
+          txn.append (checked_url, urls.get_icon_for_type(url_type), 
+                Categories.RECENT,
+                "text/html", search.search_string,
+                checked_url, checked_url);
         }
 
         /* apply filters to results found by zeitgeist */
@@ -858,8 +864,20 @@
     {
       debug (@"Activating: $uri");
       try {
-        if (bookmarks.launch_if_bookmark (uri))
-          return new Unity.ActivationResponse (Unity.HandledType.HIDE_DASH);
+        if (bookmarks.launch_if_bookmark (uri)) 
+          return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
+          
+        /* this code ensures that a file manager will be used
+         * * if uri it's a remote location that should be mounted */
+        var url_type = UrlType.UNKNOWN;
+        var checked_url = urls.check_url (uri, out url_type);
+        if (checked_url != null && url_type == UrlType.MOUNTABLE) {
+          var muris = new GLib.List<string>();
+          muris.prepend (uri);
+          var file_manager = AppInfo.get_default_for_type("inode/directory", true);
+          file_manager.launch_uris(muris,null);
+          return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
+        }
       } catch (GLib.Error error) {
         warning ("Failed to launch URI %s", uri);
       }
 
=== 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 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by Mikkel Kamstrup Erlandsen <[e-mail]>
+ * Modified by Angelo Compagnucci <[e-mail]>
  *
  */
 
 namespace Unity.FilesLens {
+  public enum UrlType {
+   UNKNOWN,
+   WEB,
+   MOUNTABLE;
+  }
 
   public class UrlChecker : Object
   {
     /* A string serialized GIcon */
-    public string icon { get; private set; }
+    public string web_icon { get; private set; }
+    public string mountable_icon { get; private set; }
     
     /* Regexes URLs must match */
-    private List<Regex> regexes;
+    private Regex web_regex;
+    private Regex mountable_regex;
     
     public UrlChecker ()
     {
-      icon = new ThemedIcon ("web-browser").to_string ();
-      regexes = new List<Regex> ();
+      web_icon = new ThemedIcon ("web-browser").to_string ();
+      mountable_icon = new ThemedIcon ("folder-remote").to_string ();
       
       try {
-        regexes.prepend (new Regex (".+\\...+")); // contains a dot + >= 2 chars
-        regexes.prepend (new Regex ("www.*"));
-        regexes.prepend (new Regex ("http://.+"));
-        regexes.prepend (new Regex ("https://.+"));
+        web_regex = new Regex ("([a-zA-Z0-9\\-]+\\...+|http[s]{0,1}://.+)");
+        mountable_regex = new Regex ("(\\\\|(ftp|ssh|sftp|smb|dav)://).+");
       } catch (RegexError e) {
         warning ("Error compiling regular expressions for URL matching. URL launching will not work: %s", e.message);
       }
     }
     
-    /* Returns a valid HTTP/HTTPS URL if the input looks like it,
-     * or null otherwise */
-    public string? check_url (string sample)
+    /* Returns a valid URL if the input looks like it or null otherwise,
+     * returns also url type which can after be used
+     * to retrive corresponding icon
+     */
+    public string? check_url (string sample, out UrlType url_type)
     {
       if (sample.strip () == "") return null;
-
-      foreach (var regex in regexes)
-      {
-        if (regex.match (sample))
-          {
-            return sample.has_prefix ("http") ? sample : ("http://" + sample);
-          }
-      }
       
+      if (mountable_regex.match (sample))
+      {
+        url_type = UrlType.MOUNTABLE;
+        return sample.replace("\\\\","smb://");
+      }
+      else if (web_regex.match (sample))
+      {
+        url_type = UrlType.WEB;
+        return sample.has_prefix ("http") ? sample : ("http://" + sample);
+      }
+      url_type = UrlType.UNKNOWN;
       return null;
     }
     
+    public string get_icon_for_type (UrlType url_type)
+    {
+      switch (url_type)
+      {
+        case UrlType.WEB:
+          return web_icon;
+        case UrlType.MOUNTABLE:
+          return mountable_icon;
+      }
+      return web_icon;
+  }
+    
   } /* end: class UrlChecker */
 
 } /* end: namespace */