Review Request 111630: Do not render icons on top of each other anymore

[Emmanuel Pescosta]

> On July 22, 2013, 3:55 p.m., Frank Reininghaus wrote:
> > Thanks for looking into this bug.
> > 
> > I looked a bit at the code (I haven't written it myself, and it was not really obvious to me what it does at first sight). I have the impression that the root cause of the problem happens a few lines earlier: the code tries to create the widget at its 'imaginary old position' and determines this position by calling "m_layouter->itemRect(i - changedCount)" where changedCount is negative, and its absolute value is the number of removed items. However, "i - changedCount" is the "old index" of the item only if the item is *behind* the removed item range. In the other case, the item became visible because the view was forced to scroll up, and its index and "itemRect" are actually unchanged. Using "m_layouter->itemRect(i - changedCount)" does not make any sense then - this is the completely 
 wrong position at which the item appears when this bug is triggered.
> > 
> > Later on, inside "if (animate) {...}", it checks if the item is behind the removed range and calls "moveWidget(widget, newPos)" only then. Therefore, the animation is never started, and the widget remains at the wrong position.
> > 
> > IMHO, the correct fix is to check if the item is behind the removed range, and do the "create at imaginary old position, move it to newPos with an animation" thing only then. If that is not the case, applyNewPos just remains true
> > 
> > Moreover, I think that the check "if (itemsRemoved && (i >= changedIndex + changedCount + 1))" inside "if (animate) {...}" to find out if the item is wrong. It should be "i >= changedIndex".
> > 
> > http://paste.kde.org/pbba4849f/
> > 
> > What do you think?
> > 
> >

> and it was not really obvious to me what it does at first sight
I had the same problem

> http://paste.kde.org/pbba4849f/
Tested and works fine :) Fixes also the bug 319951 (only a small problem with the scrollbar offset is left which is fixed by my patch for bug 319951 ;)

> What do you think?
Great work as usual. Ship it from my side!


- Emmanuel


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


On July 21, 2013, 2:28 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111630/
> -----------------------------------------------------------
> 
> (Updated July 21, 2013, 2:28 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Do not render icons on top of each other anymore.
> 
> Set applyNewPos to false here is wrong in my mind, because the "if (applyNewPos) { widget->setPos(newPos); }" case will be never called, when items got removed and no move animation is running.
> 
> 
> This addresses bug 302373.
>     http://bugs.kde.org/show_bug.cgi?id=302373
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistview.cpp d2b3fa1 
> 
> Diff: http://git.reviewboard.kde.org/r/111630/diff/
> 
> 
> Testing
> -------
> 
> Works for me.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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