Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

[David Faure]

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



I forgot to review this, sorry for the delay.


src/filewidgets/urlutil.h (line 20)
<https://git.reviewboard.kde.org/r/127111/#comment67719>

    This file isn't installed so it should be renamed to urlutil_p.h



src/filewidgets/urlutil.h (line 24)
<https://git.reviewboard.kde.org/r/127111/#comment67726>

    no longer used



src/filewidgets/urlutil.h (line 65)
<https://git.reviewboard.kde.org/r/127111/#comment67720>

    The len=5 in the comment is strange, given that this code doesn't look at lengths.
    
    The /home==/home comment is strange too, because this code is hit in many many other cases too, like
    /a and /b, or any other case where currentUrl isn't a parent of lastUrl.
    
    I'd just remove the 3 lines of comments, the if() is clear enough.



src/filewidgets/urlutil.h (line 71)
<https://git.reviewboard.kde.org/r/127111/#comment67721>

    If path2 requires a comment saying "the child one", how about renaming path2 to childPath and path1 to parentPath? :-)



src/filewidgets/urlutil.h (line 92)
<https://git.reviewboard.kde.org/r/127111/#comment67722>

    // keeps the scheme
    
    would be a more correct comment.
    
    It could be file:// or anything else, but for sure there is one, we only work with absolute URLs in KIO.



src/filewidgets/urlutil.h (line 93)
<https://git.reviewboard.kde.org/r/127111/#comment67723>

    remove useless comment ;)



tests/CMakeLists.txt (line 53)
<https://git.reviewboard.kde.org/r/127111/#comment67718>

    yes but you should move this to the autotests subfolder.
    
    "tests" is for interactive test programs.
    
    And there you'll also be able to integrate better with the cmakelists for the other autotests ;-)



tests/urlutiltest.cpp (line 32)
<https://git.reviewboard.kde.org/r/127111/#comment67724>

    Using the pre-processor is ugly and unnecessary here. Make this an inline function instead?
    
    static inline QUrl lUrl(const QString &path) { return QUrl::fromLocalFile(path); }



tests/urlutiltest.cpp (line 44)
<https://git.reviewboard.kde.org/r/127111/#comment67725>

    Calling QUrl::fromLocalFile("file:///home/aaa") is just plain wrong, either remove the scheme or don't call fromLocalFile.
    
    OTOH you should add tests with '#' in the path, since that would hit any path/url confusion in the code.


- David Faure


On Sept. 6, 2016, 9:59 a.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2016, 9:59 a.m.)
> 
> 
> Review request for KDE Frameworks, Emmanuel Pescosta and Frank Reininghaus.
> 
> 
> Bugs: 335616
>     https://bugs.kde.org/show_bug.cgi?id=335616
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> Moved logic from https://git.reviewboard.kde.org/r/123253 to here.
> 
> Provides a signal to implement bug https://bugs.kde.org/show_bug.cgi?id=335616: "Dolphin: Navigate to parent folder selects child folder".
> 
> 
> Diffs
> -----
> 
>   src/filewidgets/kurlnavigator.h 4ffe56acf9ef7a80ba27ba3a08327e363f98fb0d 
>   src/filewidgets/kurlnavigator.cpp 3c045c5aaadb429fd22d28b55ad20d31b086ef48 
>   src/filewidgets/urlutil.h PRE-CREATION 
>   tests/CMakeLists.txt bc94a4aefd77fb9ce9b2a24075415aac7c30e5ca 
>   tests/urlutiltest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127111/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

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