Review Request: Bug 290736 - Wrong current item after deleting files

[Frank Reininghaus]

The ReviewBoard server seems to have trouble right now, therefore,
I'll just reply by e-mail:

2013/1/12 Emmanuel Pescosta <emmanuelpescosta099 at gmail.com>
>
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108356/
>
> Review request for Dolphin and Frank Reininghaus.
> By Emmanuel Pescosta.
>
> Description
>
> Select right item as current item (first item after the deletion) after
> deleting files
>
> When the current item is one of the deleted items, then the first item
> after the deletion range gets selected.
>
> Example:
> * Folders: A, B, C, D, E
> * Select: E, B -> B is current item (because it was the last selected
> item)
> * Remove them -> In this example we have two deletion ranges [Range 1: B,
> Range 2: E]
> * C is the new current item (Because it is the first item after the
> deletion range, in which the previous current item was placed - in this
> example B)
>
> I hope this is the right behavior? ;) Or should the item, after the last
> deletion range, be the new current item?
>
>
> Btw.:
> How can I decide if items were added or removed in void
> KItemListSelectionManager::itemsMoved(const KItemRange& itemRange, const
> QList<int>& movedToIndexes)?
> If this is possible, we can replace this function by:
>
> void KItemListSelectionManager::itemsMoved(const KItemRange& itemRange,
> const QList<int>& movedToIndexes)
> {
>     const KItemRangeList itemRanges = KItemRangeList() << itemRange;
>
>     if (/*moved away -> deleted*/) {
>         itemsRemoved(itemRanges);
>     } else {
>         itemsInserted(itemRanges);
>     }
> }
>
> -> Save a few lines of source code + Fix Bug 305619 ;)
>
> Testing
>
> Yep. Works fine so far in Dolphin
>
> But the SelectionManager test failed -> Maybe because I implemented the
> behavior of Dolphin a little bit different
>
> Bugs: 290736
>
> Diffs
>
> dolphin/src/kitemviews/kitemlistselectionmanager.cpp (383914d)
>
> View Diff

Thanks Emmanuel for the patch! It seems that this was one of the
selection manager issues that we did not fix before the Dolphin 2.0
release and then forgot because there were always so many other bug
reports.

Your approach looks good, but it duplicates some of the functionality
from indexAfterRangesRemoving(), so I wonder if we can achieve the
same result with less code.

If I understand correctly, indexAfterRangesRemoving() would yield the
new current item position if the "return -1;" was replaced by "index =
itemRange.index", and the "dec += itemRange.count;" was put into an
"else" branch for the preceding if, right? Maybe we can modify the
function such that it returns the correct result both for the "current
item" case and for the case that the index really should be -1 if the
item was removed (which is also needed, e.g., for selected items).
Essentially, one would have to add an "if" and two branches that
either return -1 or set index to itemRange.index. Then the function
would need an additional parameter. Boolean parameters are evil, so
maybe an enum might be the way to go. Maybe an enum type
"RangesRemovingBehaviour" that takes the values "AdjustRemovedIndex"
and "DiscardRemovedIndex"? Not sure, it seems I'm not very creative
right now.

Note that I haven't actually tested this idea yet. If it contains some
nonsense, feel free to tell me.

About the unit test failure: it seems that the "Remove around current
item" case of testDeleteCurrentItem() documents the current (wrong)
behaviour. This should be modified - the new current item in that case
should be 45 and not 50, I think.

About itemsMoved(const KItemRange& itemRange, const QList<int>&
movedToIndexes): that function is actually called if the number of
items in the model remains the same, but only the order of the items
in itemRange changes (due to resorting, renaming, or something like
that). The list movedToIndexes then contains the new positions of the
items in itemRange. Therefore, this function cannot be used for the
'items removed' case, I'm afraid.