Review Request 112678: Fix Bug 311099 - View the underscore when using Ctrl + PagDown

[Frank Reininghaus]

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

Ship it!


Thanks for looking into this problem! The approach looks good, and I can confirm that it works most of the time. I have a small suggestion for improvement though that fixes the following remaining problem:

In Icons View, press and hold "Down" to reach the last item. Then reduce the height of the window by a few pixels, such that the line below the item text just disappears. Then press "Up" and "Down" and notice that the view is not scrolled back to the bottom. Similar things happen when scrolling to the top, or to the left and right in Compact View.

This is because you check "if (currentRect.top() < viewGeometry.top())" and then only take the margin into account if this check fails. If you account for the margin in advance instead by replacing

const QRectF currentRect = itemRect(index);

by

QRectF currentRect = itemRect(index);
currentRect.adjust(-m_styleOption.horizontalMargin, -m_styleOption.verticalMargin, m_styleOption.horizontalMargin, m_styleOption.verticalMargin);

it should work better and always scroll to the edge of the view if the first/last item becomes the "current" item.

If you agree that this small change makes your good patch even better, please include that in your commit. Thanks.

- Frank Reininghaus


On Sept. 11, 2013, 7:50 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112678/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2013, 7:50 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Fix Bug 311099 - View the underscore when using Ctrl + PagDown
>     
> Take the style option vertical/horizontal margin into account
> for the calculation of the new scroll offset.
> 
> 
> This addresses bug 311099.
>     http://bugs.kde.org/show_bug.cgi?id=311099
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistview.cpp 7f15261 
> 
> Diff: http://git.reviewboard.kde.org/r/112678/diff/
> 
> 
> Testing
> -------
> 
> Works.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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