Review Request 111144: Fix Bug 320897 - selection files with selection rectangle doesn't work if the rectangle has 0 pixel height

[Frank Reininghaus]

> On June 20, 2013, 12:51 p.m., Frank Reininghaus wrote:
> > Thanks for the patch! Looks good to me! Just one thing - can we guarantee that the floating point offset of 1.0 (in scene coordinates) will always be translated into a 1 pixel offset on the screen? In principle, this looks like a reasonable assumption, but if this is applied to the 4.10 branch (with no possibility to ship further bug fix updates after 4.10.5), we should be 100% sure that this is the case.
> 
> Emmanuel Pescosta wrote:
>     Hmm good question. I don't know, sry.
>     
>     I googled a little bit about the dpi awareness in Qt, but found nothing. (Ok maybe this link: https://blog.qt.digia.com/blog/category/macosx/)
>     
>     But when I draw a rectangle with QRectF(1.0, 1.0) the rectangle drawn on the screen is always "1 screen px" high and "1 screen px". Am I right?

OK, I had a look, and it seems that the only way to break the 1:1.0 correspondence between (integer) screen pixels and (floating point) QGraphicsScene coordinates is to explicitly set a transform for the QGraphicsView: http://qt-project.org/doc/qt-4.8/qgraphicsview.html#setTransform

We don't do that anywhere at the moment. If someone ever does it, then your patch might lead to some strange visual effects, but probably it's unlikely that this ever happens, and finding out the right coordinate <-> screen pixel correspondence from KItemListRubberBand would require so many rather ugly changes that we should better not do it.

So I'd say that this is fine to go in, but actually, I like version #2 better. Even though this part of the code is probably not really performance-critical, it seems unnecessary to apply a translation in the most common case that the rectangle does not have zero width or height. Moreover, in version #2, you see quite quickly that nothing happens if the width/height is not zero. In version #3, you have to figure out first what the 'translation' is about.

So from my point of view, version #2 is OK for KDE/4.10. Thanks for your work!


- Frank


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


On June 20, 2013, 1:31 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111144/
> -----------------------------------------------------------
> 
> (Updated June 20, 2013, 1:31 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Prevent the selection rectangle from being reduced to a 0px width
> or a 0px height. So the selected items can not be accidently
> unselected when the rectangle width/height becomes 0px.
> 
> 
> This addresses bug 320897.
>     http://bugs.kde.org/show_bug.cgi?id=320897
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/private/kitemlistrubberband.cpp ae023d2 
> 
> Diff: http://git.reviewboard.kde.org/r/111144/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/20130620/d8d5d16b/attachment.htm>