Review Request 111486: Fixed the wrong calculation of the first and last visible index

[Frank Reininghaus]

> On July 14, 2013, 9:41 a.m., Frank Reininghaus wrote:
> > Thanks for your efforts to fix these bugs! You said in bug 302373 that you found a way to reproduce the problem. Could you please share it with us? I'm afraid you patch doesn't really make it obvious to me what the real problem is. It seems that I'm also unable to reproduce bug 319951 in master.
> 
> Emmanuel Pescosta wrote:
>     > You said in bug 302373 that you found a way to reproduce the problem
>     
>     1. Switch to the icons view mode
>     2. Scroll to the bottom
>     3. Verify that the last row has maximal "Items per Row - 1" items
>     4. Delete all items in the last row
>     
>     Result: See Images initial.png, master.png and master_with_patch.png
>     
>     > It seems that I'm also unable to reproduce bug 319951 in master.
>     Hmm I can reproduce it. See snapshot9.png and snapshot10.png

Thanks, I can reproduce bug 302373 now (at least if the first row in the view is only visible partially).

However, even after staring at your patch for quite some time, I don't see what part of it fixes it, to be honest. I haven't written any of this code myself, and therefore, it would really help me (and also others, I think) if you could try to describe what exactly goes wrong in current master. Maybe some variable is assigned an incorrect value?

What I can see in your patch is:

* You are saying that updating m_maximumScrollOffset in every row improves the performance. But how can that be better than only looking at the last row after the layouting is finished? Am I missing something here?

* KItemListView::doLayout() should be changed to do the following things:

(a) Set the scroll offset to 'maximumScrollOffset()' if it's too large. This looks wrong because you have to subtract the view height (but then you do this in KItemListViewLayouter::scrollOffset()).

(b) Call m_animation->setScrollOffset(m_layouter->scrollOffset());

(c) Call emitOffsetChanges(), even though this is called at the end of the function already. Why does it make sense to add another call in the middle of the function?

* You subtract m_headerHeight from m_maximumScrollOffset, and add it back in KItemListViewLayouter::setScrollOffset(qreal offset). What does this have to do with the bug? Or is this change about the other bug? Let's please keep fixes for different bugs separate (unless they have the same root cause).

* You add m_groupHeaderHeight to maxScrollOffset in KItemListViewLayouter::setScrollOffset(qreal offset). I don't understand why. The group header height is already taken into consideration in doLayout(), right?

I don't see which of these changes fixes it. Maybe I could figure it out if I looked at the code for a few hours and tested how it behaves if I only apply some parts of your patch, but to be honest, I think that it would be better if you could try to describe what the root cause of the bug is about. Thanks.


- Frank


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


On July 14, 2013, 12:42 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111486/
> -----------------------------------------------------------
> 
> (Updated July 14, 2013, 12:42 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Fixed the wrong calculation of the first and last visible
> index in KItemListViewLayouter, which happened under some special
> circumstances (esp. when deleting items at the end of the view).
> 
> Also made a small performance improvement in KItemListViewLayouter::doLayout.
> We use the already calculated y bottom position instead of looping over
> all items, to determine the items "greatest" bottom position.
> 
> BUG: 319951
> BUG: 302373
> FIXED-IN: 4.11.0
> 
> 
> This addresses bugs 302373 and 319951.
>     http://bugs.kde.org/show_bug.cgi?id=302373
>     http://bugs.kde.org/show_bug.cgi?id=319951
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistview.cpp d2b3fa1 
>   dolphin/src/kitemviews/private/kitemlistviewlayouter.cpp da569b3 
> 
> Diff: http://git.reviewboard.kde.org/r/111486/diff/
> 
> 
> Testing
> -------
> 
> Yep. No more icons rendered on top of each other. All items are visible after deleting some items at the end.
> 
> 
> File Attachments
> ----------------
> 
> Initial folder
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/07/14/initial.png
> Bug 302373 with Dolphin Master
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/07/14/master.png
> Bug 302373 with Dolphin Master + Patch
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/07/14/master_with_patch.png
> Bug 319951 with Dolpin Master (5 Items removed)
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/07/14/snapshot9.png
> Bug 319951 with Dolphin Master + Patch (Same initial setup as snapshot 9 - 5 Items removed)
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/07/14/snapshot10.png
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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