resortAllItems findings

[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.

>
>
> > 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.

I didn't know about that. Thank you very much for explaining that.

>
> > 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 ;-)

I did notice that, but it didn't seem to matter from a visual
perspective. But i did not include grouping or even a wide variance of
file names where both would probably have shown an issue when the
above code was commented out. But that's exactly the reason why i'm
sending this to the list, to report my (simplistic) finding where you
can then kick them down and tell me that i've made a mistake :) No
joking here, that is really appreciated!
>
> You should NEVER remove code that you do not understand.

That is why i'm mailing it to the list as well. Otherwise it would
have been a reviewrequest. ;-)
>
> Cheers,
> Frank

As a rule of thumb, whenever i report any findings about possible
performance improvements i usually am really on to something that
improves the performance. However, i am also - nearly always - going a
bit to far by disabling functionality. That might still work exactly
the same for me with no visual difference but will probably break
other configs (eg. where grouping is involved). I will try to take
that into consideration whenever i report other findings about
performance improvements (which i do from time to time).

Cheers,
Mark