Review Request 127111: kurlnavigator: add new signal selectParentOfPreviousUrl

[David Faure]

> On Feb. 20, 2016, 7:46 p.m., David Faure wrote:
> > src/filewidgets/kurlnavigator.h, line 428
> > <https://git.reviewboard.kde.org/r/127111/diff/1/?file=444588#file444588line428>
> >
> >     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.
> 
> Frank Reininghaus wrote:
>     I also thought first that something like urlChangedToParent(QUrl) would be a good signal name, but it might make people think that this signal will always be emitted when navigating from a URL to its (possibly indirect) parent.
>     
>     However, this is not the case - the new signal will only be emitted if the URL change of the KUrlNavigator was triggered by a call of setLocationUrl(const QUrl&). If the URL change was caused by invoking the goBack() or goForward() slots, then the signal will not be emitted. The reason is that the user of KUrlNavigator will try to restore the view state (using the result from KUrlNavigator's locationState() function) then, and selecting any parents would interfere with that (see the discussion in https://git.reviewboard.kde.org/r/123253/ ).
>     
>     So the idea is that the new signal is only emitted if no history action was responsible for the URL change. This makes it possible for Dolphin and other users of KUrlNavigator to behave like some other file managers, which also select the parent of the previous URL, unless the URL change was caused by back/forward. So anything with 'changed' is not really accurate, because a change is not sufficient to emit this signal.
>     
>     Still, it might make sense to have an 'ed' in the name. 'requested' appears often in signals which are not so much about state changes, but have the purpose to make the receiver do something. How about
>     
>     urlSelectionRequested(const QUrl &) ?

OK, I see. Yes, this is better. The comment would still explain this use case for requesting selection, but in theory this lets the door open for more use cases for the url navigator requesting that a selection be performed. In any case the signal name is much easier to understand this way ;)


- David


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


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: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20160223/367fde4d/attachment.htm>
-------------- next part --------------
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel at kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel