resortAllItems findings

[Mark]

Hi list,

I'm sending this mail because of this [1] reviewrequest.

The change in resortAllItems made me look into the resortAllItems() code
since it looks odd. I found a few things that might be interesting to take
a look at and would make it a lot faster. I didn't do much testing so i
don't know if i'm right, but from a visual "does re-sorting work by
pressing on the name in the header" it seems to be working just fine.

The first things that i noticed are:
- oldUrls is being created, is that needed?
- m_groups is cleared but never filled again, why?
- m_items is cleared and refilled, but is that actually needed?

For m_items, i think there is no need to clear it. It's being re-sorted.
Nothing gets added or removed thus m_items should be the same before and
after sorting. If m_items where to be iterated somewhere it "might" make a
difference, but a quick code inspection didn't show me any places where
m_items is being iterated. Thus i doubt if it should be cleared and
refilled, i don't think it has to.

Actually, the only thing that seems to be really needed there is:
sort(m_itemData.begin(), m_itemData.end());

and an emit to notify the view of changed items.

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. However, the
layout has not changed. 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.

To conclude, my resortAllItems function looks like:
void KFileItemModel::resortAllItems()
{
    m_resortAllItemsTimer->stop();

    const int itemCount = count();
    if (itemCount <= 0) {
        return;
    }

#ifdef KFILEITEMMODEL_DEBUG
    QElapsedTimer timer;
    timer.start();
    kDebug() <<
"===========================================================";
    kDebug() << "Resorting" << itemCount << "items";
#endif

    // Resort the items
    sort(m_itemData.begin(), m_itemData.end());

    // Don't check whether items have really been moved and always emit a
    // itemsMoved() signal after resorting: In case of grouped items
    // the groups might change even if the items themselves don't change
their
    // position. Let the receiver of the signal decide whether a check for
moved
    // items makes sense.
    emit itemsMoved(KItemRange(0, itemCount), QList<int>());

#ifdef KFILEITEMMODEL_DEBUG
    kDebug() << "[TIME] Resorting of" << itemCount << "items:" <<
timer.elapsed();
#endif
}

and my slotItemsMoved(...) looks like:
void KItemListView::slotItemsMoved(const KItemRange& itemRange, const
QList<int>& movedToIndexes)
{
    const int firstVisibleMovedIndex = qMax(firstVisibleIndex(),
itemRange.index);
    const int lastVisibleMovedIndex = qMin(lastVisibleIndex(),
itemRange.index + itemRange.count - 1);

    for (int index = firstVisibleMovedIndex; index <=
lastVisibleMovedIndex; ++index) {
        KItemListWidget* widget = m_visibleItems.value(index);
        if (widget) {
            updateWidgetProperties(widget, index);
            initializeItemListWidget(widget);
        }
    }

    doLayout(NoAnimation);
    updateSiblingsInformation();
}

My spreedup.. Way faster as usual with my patches ;) You can expect it to
be ~4x faster then the patch provided in [1]. However, i don't know if i'm
right in all my assumptions so i hope the people on this list can take a
look at this as well to see where it makes sense and if i'm right at all. I
can't give you a diff since my branch is currently not playing nice with
me, but if you folks want a diff i will post one as soon as i get my branch
sorted out.

Kind regards,
Mark
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130729/663eccda/attachment.htm>