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

[Frank Reininghaus]

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


Thanks for the update, and sorry for the late reply.

If I understand correctly, the remaining problem (after the recent changes) is the following:

If a scroll bar is visible in Details View, the view is scrolled to the bottom, and items are removed, the scroll bar looks like it's not at the bottom any more (even though the view is at the bottom). Is that correct?

If that is the case, then I think (after having spent some time reading all the relevant code and figuring out what it's supposed to do) that the root cause of the problem is that KItemListContainer sets an incorrect maximum value for the scroll bar. The confusing thing is that the "scroll offset" of the view always refers to the top part of the view, which is hidden behind the header in Details View. Therefore, the maximum value of the scroll bar must be calculated using the full height of the view, and not the height minus the header height:

http://paste.kde.org/pfa226314/

Your patch works around the problem in another way. It works, but I think that one part of it could possibly cause other problems in the future: KItemListViewLayouter can modify its member m_scrollOffset. The possibly dangerous thing about this is that usually the view sets this member and then notifies other objects about the change via its scrollOffsetChanged() signal. If, however, the layouter modifies the member, then we could run into a situation where m_scrollOffset is no longer consistent with the value that the objects which listen to the view's signal know (it might not happen in the code paths that are executed right now, but if other parts of KItemListView are changed in the future, it could happen).

There are some nice ideas in your patch though. The new function KItemListViewLayouter::pageStep() does make some things easier. But I think that such a change should better only go into master at this point, because it does not fix a bug. Maybe we can also figure out other ways to make the code less confusing (because confusing code can easily lead to bugs, as we see here).

What do you think?

About the 'performance improvement': I must admit that I still don't get it. If there are N items in the view, then your approach updates the member m_maximumScrollOffset N times. The current code, OTOH, only examines the items in the last row. The number of these items is small (limited by the screen size!). I mean, there probably isn't any measurable difference, but I cannot believe that your approach is more efficient if N is large.

- Frank Reininghaus


On July 21, 2013, 2:20 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111486/
> -----------------------------------------------------------
> 
> (Updated July 21, 2013, 2:20 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
> FIXED-IN: 4.11.0
> 
> 
> This addresses bug 319951.
>     http://bugs.kde.org/show_bug.cgi?id=319951
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistcontainer.cpp 3bd8067 
>   dolphin/src/kitemviews/kitemlistview.h 6467b8c 
>   dolphin/src/kitemviews/kitemlistview.cpp d2b3fa1 
>   dolphin/src/kitemviews/private/kitemlistviewlayouter.h 306fcd3 
>   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/20130724/9107fa6f/attachment.htm>