Review Request 120606: Properly parse URL in KUrlNavigator

[David Faure]

> On Oct. 16, 2014, 2:43 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.cpp, line 717
> > <https://git.reviewboard.kde.org/r/120606/diff/1/?file=320044#file320044line717>
> >
> >     This looks wrong.
> >     
> >     After this line, path can contain either a path or a URL.
> >     
> >     Which means that transforming this back to a URL becomes rather tricky. And in particular QUrl(path) is wrong, for the case of a local file.
> >     
> >     Try a directory with a '#' in the name to make sure the conversions are correct. It will fail in QUrl(localpath).path() because QUrl(localpath) will interpret the '#' as a query delimiter.
> >     
> >     I don't know the KUrlNavigator code so I can't suggest a proper fix right now, but for sure this should match the code that creates the buttons in the first place (as you say), while at the same time avoid strings that can contain "a local path or a url", or if it can't avoid that, then use QUrl::fromUserInput(), which handles that correctly (just make sure that function never gets passed a relative path).
> 
> Jan Grulich wrote:
>     You are right, it doesn't work with a folder with a '#' in the name. What about testing if the URL is a local file and if so, use newUrl.setPath(path) and if not use newUrl.setPath(QUrl(path))? Also using QUrl::fromUserInput() seems to work correctly and I think it should't get a relative path, otherwise the path wouldn't work in KUrlNavigator if you would like to go back.
> 
> David Faure wrote:
>     "What about..." -> that's exactly the logic inside QUrl::fromUserInput (and it does it correctly, including on Windows where this isn't just startsWith('/')), so let's use that :-)

Ah and in that case, please rename the variable (back) to pathOrUrl.


- David


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


On Oct. 16, 2014, 1:56 p.m., Jan Grulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120606/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2014, 1:56 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> When using an URL with a scheme, like sftp://foo@bar.com/home/foo/, then KUrlNavigator doesn't properly parse it. At the beginning it tries to count the number of slashes from sftp://foo@bar.com, which is 2, then it tries to contruct buttons using names from particular sections separated by slashes, but when we use only QUrl::path() for URL above, we will always get only "/home/foo/" path and therefore we will have bigger index then the number of sections, which leads to have same URLs for all buttons. We need to parse sections from full URL including first two slashes.
> 
> 
> Diffs
> -----
> 
>   src/filewidgets/kurlnavigator.cpp e96d914 
> 
> Diff: https://git.reviewboard.kde.org/r/120606/diff/
> 
> 
> Testing
> -------
> 
> Tested in dolphin and works fine now.
> 
> 
> File Attachments
> ----------------
> 
> Screenshot of the problem
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/10/16/eda6fb7e-a75e-4c28-ac87-5d16a39a366d__kurlnavigator.png
> 
> 
> Thanks,
> 
> Jan Grulich
> 
>

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