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

[Frank Reininghaus]

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

Ship it!


Thanks for the new patch!

Ah, now I see that my idea to use indexAfterRangesRemoving for two different purposes has one little problem, sorry about that.

In the current state (and also with your patch version 1), the current index would change from the old value to -1, and then to the new value, and currentChanged was emitted twice. And I think I remember that this was actually important in some situations.

However, with the new version, currentChanged is emitted once, no matter if the current index has actually changed or not or has been -1 in the meantime. This might cause some subtle problems.

I think that there are two ways to fix this:

1) Go back to version 1 of your patch.
2) Do it like this:

m_currentItem = indexAfterRangesRemoving(m_currentItem, itemRanges, DiscardRemovedIndex);
if (m_currentItem != previousCurrent) {
    emit currentChanged(m_currentItem, previousCurrent);
    if (m_currentItem < 0) {
        m_currentItem = indexAfterRangesRemoving(previousCurrent, itemRanges, AdjustRemovedIndex);
        emit currentChanged(m_currentItem, -1);
    }
}

1) Probably needs slightly less code overall, but duplicates some parts of the calculation of the new index. Maybe 1) is better, but I don't have a strong opinion on this. What do you think? Maybe you have another idea.

Moreover, can you fix the unit test failure? Thanks.

- Frank Reininghaus


On Jan. 12, 2013, 7:48 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108356/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2013, 7:48 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> 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 ;)
> 
> 
> This addresses bugs 290736 and 305619.
>     http://bugs.kde.org/show_bug.cgi?id=290736
>     http://bugs.kde.org/show_bug.cgi?id=305619
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistselectionmanager.h 43d0dcb 
>   dolphin/src/kitemviews/kitemlistselectionmanager.cpp 383914d 
> 
> Diff: http://git.reviewboard.kde.org/r/108356/diff/
> 
> 
> 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
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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