Review Request: Make it possible to clear the selection using Esc; make it easier to cancel keyboard searches

[Peter Penz]

> On April 24, 2012, 1:59 p.m., Peter Penz wrote:
> > dolphin/src/kitemviews/kitemlistselectionmanager.h, line 72
> > <http://git.reviewboard.kde.org/r/104709/diff/1/?file=58481#file58481line72>
> >
> >     Do we really need to introduce a new signal? Couldn't we use the existing currentChanged() instead (-> if the first parameter is < 0 it indicates that it has been removed). My concern for introducing this new signal is that we'd also need to introduce a kind of currentItemAdded() for having a complete interface, however also in this case the currentChanged() would be sufficient from my point of view.
> 
> Frank Reininghaus wrote:
>     If the current item is removed, KItemListSelectionManager::itemsRemoved(const KItemRangeList& itemRanges) does the following:
>     
>     m_currentItem = qMin(previousCurrent, m_model->count() - 1);
>     
>     and emits currentChanged() then if m_currentItem != previousCurrent. We could first emit currentChanged() with a negative parameter (rather than adding the new signal), but I thought that this might confuse other classes which listen to this signal and possibly cause unexpected regressions. Therefore, I think that adding a new signal is the safer solution.

I agree that this might confuse receivers of the signal, but I still think it is the cleaner solution from an API point of view. The only receiver of the signal currently is KItemListView::slotCurrentChanged(int current, int previous) and the implementation would still work fine if -1 is passed for 'current'. My guess is that the unit-tests for the selection-manager will (and should :-)) fail. I'm not sure how much time you have, but in case if you somehow agree to my suggestion and you've no time I could port your current patch to use the currentChanged()-signal (I'd of course take care to adjust the unit-tests too).


- Peter


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104709/#review12863
-----------------------------------------------------------


On April 24, 2012, 1:06 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104709/
> -----------------------------------------------------------
> 
> (Updated April 24, 2012, 1:06 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> In Dolphin 2.x, it is currently not possible to clear the selection using the keyboard. In Dolphin 1.x, pressing Esc cleared the selection. This patch restores that behaviour.
> 
> Moreover, I think it makes sense to cancel the current keyboard search when pressing Esc (some users noticed that it is currently not possible to cancel a keyboard search, see https://bugs.kde.org/show_bug.cgi?id=297458).
> 
> Finally, when I had added the function KItemListKeyboardSearchManager::cancelSearch(), I thought I could just make it a slot and invoke it when the current item is removed, which fixes the problem that the keyboard search is not reset when the view's URL is changed.
> 
> 
> This addresses bugs 297458, 297488 and 298742.
>     http://bugs.kde.org/show_bug.cgi?id=297458
>     http://bugs.kde.org/show_bug.cgi?id=297488
>     http://bugs.kde.org/show_bug.cgi?id=298742
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistcontroller.cpp 79a6ecf 
>   dolphin/src/kitemviews/kitemlistkeyboardsearchmanager.cpp 592605a 
>   dolphin/src/kitemviews/kitemlistkeyboardsearchmanager_p.h d6a6686 
>   dolphin/src/kitemviews/kitemlistselectionmanager.h 43d0dcb 
>   dolphin/src/kitemviews/kitemlistselectionmanager.cpp 79c3370 
> 
> Diff: http://git.reviewboard.kde.org/r/104709/diff/
> 
> 
> Testing
> -------
> 
> Works for me.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20120424/485fbc8e/attachment.htm>