resortAllItems findings

[Frank Reininghaus]

Hi,

2013/7/30 Mark:
> On Tue, Jul 30, 2013 at 10:45 AM, Frank Reininghaus
> <frank78ac at googlemail.com> wrote:
>>
>> 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.
>
> my assumption with looking at that code was that it's being called
> when you press one of the header columns. It's should be just
> resorting - like the name implies, right?
>
> To check if my assumption is right i debugged this. A rename is
> causing KFileItemModel::slotRefreshItems to be called which is
> executed before resortAllItems and is already updating m_items when
> needed. I didn't do more testing like deletion or moving files, but
> i'm fairly sure that those all trigger some other function prior to
> resortAllItems that update m_items as well which leads me to think
> that resortAllItems should have no need to rebuild m_items as it's
> just not needed and causes overhead for the resorting operation.

When inline renaming is used, DolphinView calls
KFileItemModel::setData(), which does not update m_items AFAICS. It
might still be that we receive a refreshItems signal later on, which
then updates m_items. But I'm not sure, and I don't want to risk
regressions at this point of the release cycle. Moreover, how much
time does it save, actually? Did you do some benchmarking and see that
not clearing m_items does improve the performance significantly? If
yes, how does it compare to the improvement you get if you keep
m_items.clear(), but then reserve the (already known) amount of space
that is needed for all items?

Regards,
Frank