Review Request 120138: kshorturifilter : Treat "///..." as "/"

[David Faure]

> On Sept. 14, 2014, 12:26 p.m., Mark Gaiser wrote:
> > My attempt at this. Feel free to use it as you see fit.
> > 
> > diff --git a/src/urifilters/shorturi/kshorturifilter.cpp b/src/urifilters/shorturi/kshorturifilter.cpp
> > index 00668d9..6c35393 100644
> > --- a/src/urifilters/shorturi/kshorturifilter.cpp
> > +++ b/src/urifilters/shorturi/kshorturifilter.cpp
> > @@ -130,6 +130,24 @@ bool KShortUriFilter::filterUri( KUriFilterData& data ) const
> >    //QUrl url = data.uri();
> >    QString cmd = data.typedString();
> >  
> > +  // If a string starts with forward slashes, replace it by one forward slash.
> > +  if (cmd.startsWith('/')) {
> > +      const int cmdLength = cmd.length();
> > +      for (int i = 0; i < cmdLength; i++) {
> > +          if (cmd[i] != '/') {
> > +              QString slashes = cmd.left(i).replace(QRegExp("/+"), "/");
> > +              cmd.replace(0, i, slashes);
> > +              break;
> > +          }
> > +
> > +          // The full string contains slashes..
> > +          if (i == cmdLength - 1) {
> > +              cmd = "/";
> > +              break;
> > +          }
> > +      }
> > +  }
> > +
> >    // Replicate what KUrl(cmd) did in KDE4. This could later be folded into the checks further down...
> >    QUrl url(cmd);
> >    if (QDir::isAbsolutePath(cmd)) {
> > 
> > 
> > I'm not sure if this approach is always OK. It works with the current test cases and those two that Arjun added.
> 
> Mark Gaiser wrote:
>     Reviewboard kinda screwed that paste up.. Here is a better paste: http://paste.kde.org/p96gvcrcp (1 month lifetime)
> 
> Alexander Richardson wrote:
>     Wouldn't it be more efficent to find the index of the first non-slash char and then use QString::mid(lastSlashIndex) instead of QRegExp?
> 
> Mark Gaiser wrote:
>     Yes, that would be better.

Urgh, asking for speed and seeing a QRegExp in the answer... :-)

I was thinking simply of
    bool onlySlashes = true;
    for (int i = 0; i < cmd.length() && onlySlashes; ++i) {
        onlySlashes = cmd.at(i) == '/';
    }
    if (onlySlashes) {
        cmd.resize(1);
    }
    
But indeed this leaves the question of what to do with //something.
I think I was wrong previously, there's no particular reason to assume file://host or smb://host from that (it's not \host).
So we could also remove leading slashes even if the string isn't only just slashes.

    int firstNonSlash = 0;
    while (firstNonSlash < cmd.length() && cmd.at(firstNonSlash) == '/') {
        ++firstNonSlash;
    }
    if (firstNonSlash > 1) {
        cmd = cmd.mid(firstNonSlash - 1);
    }


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120138/#review66454
-----------------------------------------------------------


On Sept. 11, 2014, 8:50 a.m., Arjun Ak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120138/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2014, 8:50 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> "///..." should produce `QUrl("file:///")` not `QUrl("file://")`
> 
> 
> Diffs
> -----
> 
>   autotests/kurifiltertest.cpp b84dcd0 
>   src/urifilters/shorturi/kshorturifilter.cpp 00668d9 
> 
> Diff: https://git.reviewboard.kde.org/r/120138/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140915/c1a11a07/attachment.html>