Review Request 111382: Make sure that KItemListSizeHintResolver is always in a consistent state

[Frank Reininghaus]

> On July 4, 2013, 11:32 a.m., Emmanuel Pescosta wrote:
> > dolphin/src/kitemviews/kitemlistview.cpp, line 1549
> > <http://git.reviewboard.kde.org/r/111382/diff/2/?file=167956#file167956line1549>
> >
> >     Why not use m_sizeHintResolver->clearCache() here and change clearCache() to call m_sizeHintCache.clear()? 
> >     
> >     I think it would make it more obvious at first sight and removes the "if (!m_sizeHintCache.isEmpty() && m_itemListView->model()) {" in KItemListSizeHintResolver::itemsRemoved.

Because clearCache() does not actually remove the items from the cache. It just replaces their calculated size hints by QSizeF(), which indicates that a new size hint has to be calculated. Note that clearCache() is called by KItemListView every time anything that affects the item size changes.

However, if the model changes, we really want to remove the size hints from the cache, and add a new QSizeF() for every item of the new model.


> On July 4, 2013, 11:32 a.m., Emmanuel Pescosta wrote:
> > dolphin/src/kitemviews/private/kitemlistsizehintresolver.cpp, line 64
> > <http://git.reviewboard.kde.org/r/111382/diff/2/?file=167958#file167958line64>
> >
> >     Maybe replace it with "m_sizeHintCache.insert(m_sizeHintCache.end(), insertedCount, QSizeF());"?

Thanks for the hint, that looks better indeed. I'll change it.


- Frank


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


On July 3, 2013, 7:47 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111382/
> -----------------------------------------------------------
> 
> (Updated July 3, 2013, 7:47 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> The other day, I found out what the real cause for bug 317827 is. The assert verifies that the layouter is "dirty", i.e., that no function that triggers a layouting has been called in the loop in KItemListView::slotItemsRemoved(const KItemRangeList&). This is done because the item ranges are removed step by step from KItemListSizeHintResolver, such that it is in an inconsistent state inside the loop, and any attempt to start a layout may yield incorrect results. However, if an animation is ongoing, calling "m_animation->stop(widget)" will trigger KItemListView::slotAnimationFinished(), which calls m_layouter->firstVisibleIndex(), which then triggers KItemListViewLayouter::doLayout().
> 
> I think the best way to fix this is to make sure that all item ranges are always inserted to and removed from KItemListSizeHintResolver at once. A nice side-effect is that there is one source less for O(N^2) behaviour if many item ranges are inserted/removed (but there are still more, which I will look at).
> 
> Moreover, this fixed some more inconsistencies in KItemListView::setModel(), which caused KItemListSizeHintResolver to have more items than actually needed .
> 
> 
> This addresses bug 317827.
>     http://bugs.kde.org/show_bug.cgi?id=317827
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistview.cpp b5e1058 
>   dolphin/src/kitemviews/private/kitemlistsizehintresolver.h 6f50d67 
>   dolphin/src/kitemviews/private/kitemlistsizehintresolver.cpp e075b46 
> 
> Diff: http://git.reviewboard.kde.org/r/111382/diff/
> 
> 
> Testing
> -------
> 
> Works. Note that the patch still contains some debug output that makes it easier to see if the items are moved and initialized correctly. I'll remove it before pushing the patch.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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