unity-lens-files

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

  1. fix-for-773841
  2. Merge into trunk
Proposed by Michal Hruby
Status: Merged
Approved by: Michal Hruby
Approved revision: 222
Merged at revision: 217
Proposed branch: lp://staging/~angeloc/unity-lens-files/fix-for-773841
Merge into: lp://staging/unity-lens-files
Prerequisite: lp://staging/~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://staging/~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.staging.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?

lp://staging/~angeloc/unity-lens-files/fix-for-773841 updated
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
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches