resortAllItems findings

[Frank Reininghaus]

Hi Mark,

2013/7/29 Mark:
> The first things that i noticed are:
> - oldUrls is being created, is that needed?

Yes, is needed for the itemsMoved signal.

> - m_groups is cleared but never filled again, why?

It is filled again lazily when the view requests information about the groups.

> - m_items is cleared and refilled, but is that actually needed?

In principle, it is not necessary to clear m_items. We could just
update the entries without removing them first. However, that could
lead to subtle bugs if not done correctly.

Assume that there is one file 'a' in the directory. m_items will then
have one entry with the key 'a' and the value 0 (the index of the
item).

If 'a' is renamed to 'b', and we do not remove the entry 'a', then
m_items would have two entries 'a' and 'b' which both point to the
same index 0. This could cause trouble later on. The unit tests would
even crash because they verify the consistency of the model with an
assert.

> About the emit. The movedToIndexes only seems to be used (correct me if i'm
> wrong, i don't know this code) to re-calculate the layout.

It is also needed to update the indexes of the selected items.

> However, the layout has not changed.

This is, in general, wrong. If grouping is enabled, then changing the
order of the items will change the layout most of the time. Moreover,
in Icons and Compact View, the space required by each item depends on
the file name. Therefore, changing the order of the items can modify
the layout even if grouping is disabled.

> It's just moving of items so i don't think you need
> to have those either. The signal requires it to be there so i just provided
> an empty list. In the slotItemsMoved i then simply commented out the lines:
>
>
> (kitemlistciew.cpp)
> //    m_sizeHintResolver->itemsMoved(itemRange, movedToIndexes);
> //    m_layouter->markAsDirty();
>
> //    if (m_controller) {
> //        m_controller->selectionManager()->itemsMoved(itemRange,
> movedToIndexes);
> //    }
>
> When testing this in a visual manner it also seems to be working very well.
> No visual regressions at all as far as i can tell.

If you had actually looked at the code you commented out, you might
have noticed that the signal is required to update the selection ;-)

You should NEVER remove code that you do not understand.

Cheers,
Frank