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/#review92587
-----------------------------------------------------------




src/filewidgets/kurlnavigator.h (line 425)
<https://git.reviewboard.kde.org/r/127111/#comment63117>

    I had to read the whole patch to understand "url is set to the first path segment that leads from N to O".
    
    First, N and O don't help readability, I first thought the O was 0... maybe just spell out "the new url" and "the old url".
    
    Also, a url is not a path. So this could say something like:
    
    \a url is set to the child directory of the new url which is an ancestor of the old url
    
    I would then add "this signal allows file managers to pre-select the directory that the user is navigating up from".



src/filewidgets/kurlnavigator.h (line 428)
<https://git.reviewboard.kde.org/r/127111/#comment63118>

    Signal names usually end with "ed", they reflect a state change or an action change.
    
    "select" here is what you want the app to do (now that I read the bug report; otherwise it was very unclear just from the API docs), but you have no guarantee that the app will do that.
    
    void urlChangedToParent(const QUrl &ancestorOfPreviousUrl)
    
    ?
    
    An alternative is to let the app do the "finding the immediate child" logic by just emitting
    
    void urlChangedToParent(const QUrl &oldUrl, const QUrl &newUrl)
    
    It seems to me that this is a better signal, because it's more generic. On the other hand, if you are afraid that multiple apps would then need to implement the "first child" logic, then this could be done by a helper method in this class. But at least the signal has a much more generic purpose than being geared towards this specific feature,
    which increases the chances that it is useful for other things later.


- David Faure


On Feb. 18, 2016, 9:53 p.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127111/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 9:53 p.m.)
> 
> 
> Review request for Dolphin, 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 64d2a6d1d5cf8ca2e0aaa61d00ac1ffb1a9866b3 
>   src/filewidgets/urlutil.h PRE-CREATION 
>   tests/CMakeLists.txt dc88ce9fd5c5ae6ad135b72785370c0094969b5c 
>   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/20160220/1eb2fc13/attachment.html>