Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

[David Faure]

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



src/filewidgets/kurlnavigator.cpp (line 156)
<https://git.reviewboard.kde.org/r/126295/#comment61132>

    Please rename to retrievePlaceUrl, otherwise when looking at the calling code it still seems like it returns a path ;)



src/filewidgets/kurlnavigator.cpp (line 368)
<https://git.reviewboard.kde.org/r/126295/#comment61133>

    The issue was there already, but I don't see why this calls retrievePlacePath() again, it could just do placeUrl.toDisplayString...



src/filewidgets/kurlnavigator.cpp (line 369)
<https://git.reviewboard.kde.org/r/126295/#comment61135>

    This comment is misleading. That method doesn't return an empty url for local files. The path is empty, that's what you meant, right?
    
    But then no point in calling toDisplayString(PreferLocalFile), if we know that for local files we'll need "/" anyway, right?
    IIUC this could be simplified/clarified to
    
        if (placeUrl.isLocalFile()) {
            dirName = QStringLiteral("/");
        } else {
            dirName = placeUrl.toDisplayString();
        }
    
    no?



src/filewidgets/kurlnavigator.cpp (line 784)
<https://git.reviewboard.kde.org/r/126295/#comment61134>

    setPath(QString()) slightly faster


- David Faure


On Dec. 12, 2015, 9:35 a.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126295/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2015, 9:35 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> 9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on startIndex. The argument index passed to buttonUrl is based number of '/'in full url. Because of that, url like "sftp://a@b.com/a/b" is now shown as "sftp:a at b.com" / "b" / "b" in dolphin url bar.
> 
> This patch changes all relevant code that calls buttonUrl() to use url.path() instead of url.toDisplayString() to count the number of "/".
> 
> 
> Diffs
> -----
> 
>   src/filewidgets/kurlnavigator.cpp 848e98b 
> 
> Diff: https://git.reviewboard.kde.org/r/126295/diff/
> 
> 
> Testing
> -------
> 
> Remote url -> ok
> Local url -> ok
> Remote url in places and try browse some sub folder -> ok
> Local url in places and try browse some sub folder -> ok
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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