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

Proposed by Michal Hruby
Status: Merged
Approved by: Michal Hruby
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
Unity Bugs 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.
Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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?

Revision history for this message
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!

Revision history for this message
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.

Revision history for this message
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?

Revision history for this message
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.

Revision history for this message
Michal Hruby (mhr3) wrote :

Added prereq branch and resubmitted.

Revision history for this message
Angelo Compagnucci (angeloc) wrote : Posted in a previous version of this proposal

Added UrlType.UNKNOWN as requested.

Revision history for this message
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 ...;
}

Revision history for this message
Michal Hruby (mhr3) wrote :

Great, thanks a lot!

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

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

Revision history for this message
Unity Merger (unity-merger) wrote :

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

text conflict in src/daemon.vala

Revision history for this message
Michal Hruby (mhr3) wrote :

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

222. By Angelo Compagnucci

Merged with trunk, fix for bug 773841

Revision history for this message
Angelo Compagnucci (angeloc) wrote :

Done!

Merged with trunk

Revision history for this message
Michal Hruby (mhr3) :
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 2012-03-12 14:40:44 +0000
3+++ src/daemon.vala 2012-03-12 23:29:17 +0000
4@@ -15,6 +15,7 @@
5 *
6 * Authored by Mikkel Kamstrup Erlandsen <mikkel.kamstrup@canonical.com>
7 * Michal Hruby <michal.hruby@canonical.com>
8+ * Angelo Compagnucci <angelo.compagnucci@gmail.com>
9 *
10 */
11 using Zeitgeist;
12@@ -390,14 +391,16 @@
13
14 results_model.clear ();
15
16- /* check if the thing typed isn't a url (like facebook.com) */
17- var checked_url = urls.check_url (search.search_string);
18+ /* check if the thing typed isn't a url (like facebook.com)
19+ * or isn't a remote mountable url (like ftp://ftp.ubuntu.com)*/
20+ var url_type = UrlType.UNKNOWN;
21+ var checked_url = urls.check_url (search.search_string, out url_type);
22 if (checked_url != null)
23 {
24- results_model.append (checked_url, urls.icon,
25- Categories.FILES_AND_FOLDERS,
26- "text/html", search.search_string,
27- checked_url, checked_url);
28+ results_model.append (checked_url, urls.get_icon_for_type(url_type),
29+ Categories.FILES_AND_FOLDERS,
30+ "text/html", search.search_string,
31+ checked_url, checked_url);
32 }
33
34 var category_id = has_search ?
35@@ -460,13 +463,16 @@
36
37 txn.clear ();
38
39- /* check if the thing typed isn't a url (like facebook.com) */
40- var checked_url = urls.check_url (search.search_string);
41+ /* check if the thing typed isn't a url (like facebook.com)
42+ * or isn't a remote mountable url (like ftp://ftp.ubuntu.com)*/
43+ var url_type = UrlType.UNKNOWN;
44+ var checked_url = urls.check_url (search.search_string, out url_type);
45 if (checked_url != null)
46 {
47- txn.append (checked_url, urls.icon, Categories.RECENT,
48- "text/html", search.search_string,
49- checked_url, checked_url);
50+ txn.append (checked_url, urls.get_icon_for_type(url_type),
51+ Categories.RECENT,
52+ "text/html", search.search_string,
53+ checked_url, checked_url);
54 }
55
56 /* apply filters to results found by zeitgeist */
57@@ -858,8 +864,20 @@
58 {
59 debug (@"Activating: $uri");
60 try {
61- if (bookmarks.launch_if_bookmark (uri))
62- return new Unity.ActivationResponse (Unity.HandledType.HIDE_DASH);
63+ if (bookmarks.launch_if_bookmark (uri))
64+ return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
65+
66+ /* this code ensures that a file manager will be used
67+ * * if uri it's a remote location that should be mounted */
68+ var url_type = UrlType.UNKNOWN;
69+ var checked_url = urls.check_url (uri, out url_type);
70+ if (checked_url != null && url_type == UrlType.MOUNTABLE) {
71+ var muris = new GLib.List<string>();
72+ muris.prepend (uri);
73+ var file_manager = AppInfo.get_default_for_type("inode/directory", true);
74+ file_manager.launch_uris(muris,null);
75+ return new Unity.ActivationResponse(Unity.HandledType.HIDE_DASH);
76+ }
77 } catch (GLib.Error error) {
78 warning ("Failed to launch URI %s", uri);
79 }
80
81=== modified file 'src/url-checker.vala'
82--- src/url-checker.vala 2012-01-30 11:26:40 +0000
83+++ src/url-checker.vala 2012-03-12 23:29:17 +0000
84@@ -14,51 +14,74 @@
85 * along with this program. If not, see <http://www.gnu.org/licenses/>.
86 *
87 * Authored by Mikkel Kamstrup Erlandsen <mikkel.kamstrup@canonical.com>
88+ * Modified by Angelo Compagnucci <angelo.compagnucci@gmail.com>
89 *
90 */
91
92 namespace Unity.FilesLens {
93+ public enum UrlType {
94+ UNKNOWN,
95+ WEB,
96+ MOUNTABLE;
97+ }
98
99 public class UrlChecker : Object
100 {
101 /* A string serialized GIcon */
102- public string icon { get; private set; }
103+ public string web_icon { get; private set; }
104+ public string mountable_icon { get; private set; }
105
106 /* Regexes URLs must match */
107- private List<Regex> regexes;
108+ private Regex web_regex;
109+ private Regex mountable_regex;
110
111 public UrlChecker ()
112 {
113- icon = new ThemedIcon ("web-browser").to_string ();
114- regexes = new List<Regex> ();
115+ web_icon = new ThemedIcon ("web-browser").to_string ();
116+ mountable_icon = new ThemedIcon ("folder-remote").to_string ();
117
118 try {
119- regexes.prepend (new Regex (".+\\...+")); // contains a dot + >= 2 chars
120- regexes.prepend (new Regex ("www.*"));
121- regexes.prepend (new Regex ("http://.+"));
122- regexes.prepend (new Regex ("https://.+"));
123+ web_regex = new Regex ("([a-zA-Z0-9\\-]+\\...+|http[s]{0,1}://.+)");
124+ mountable_regex = new Regex ("(\\\\|(ftp|ssh|sftp|smb|dav)://).+");
125 } catch (RegexError e) {
126 warning ("Error compiling regular expressions for URL matching. URL launching will not work: %s", e.message);
127 }
128 }
129
130- /* Returns a valid HTTP/HTTPS URL if the input looks like it,
131- * or null otherwise */
132- public string? check_url (string sample)
133+ /* Returns a valid URL if the input looks like it or null otherwise,
134+ * returns also url type which can after be used
135+ * to retrive corresponding icon
136+ */
137+ public string? check_url (string sample, out UrlType url_type)
138 {
139 if (sample.strip () == "") return null;
140-
141- foreach (var regex in regexes)
142- {
143- if (regex.match (sample))
144- {
145- return sample.has_prefix ("http") ? sample : ("http://" + sample);
146- }
147- }
148
149+ if (mountable_regex.match (sample))
150+ {
151+ url_type = UrlType.MOUNTABLE;
152+ return sample.replace("\\\\","smb://");
153+ }
154+ else if (web_regex.match (sample))
155+ {
156+ url_type = UrlType.WEB;
157+ return sample.has_prefix ("http") ? sample : ("http://" + sample);
158+ }
159+ url_type = UrlType.UNKNOWN;
160 return null;
161 }
162
163+ public string get_icon_for_type (UrlType url_type)
164+ {
165+ switch (url_type)
166+ {
167+ case UrlType.WEB:
168+ return web_icon;
169+ case UrlType.MOUNTABLE:
170+ return mountable_icon;
171+ }
172+ return web_icon;
173+ }
174+
175 } /* end: class UrlChecker */
176
177 } /* end: namespace */

Subscribers

People subscribed via source and target branches