Review Request 128821: [frameworks] Implement zooming with a pinch gesture on a touch screen

[Martin Tobias Holmedahl Sandsmark]

> On Sept. 4, 2016, 10:14 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> > ui/pageview.cpp, line 1500
> > <https://git.reviewboard.kde.org/r/128821/diff/1/?file=475893#file475893line1500>
> >
> >     Don't put assignments in the if condition.
> >     
> >     Also, prefer to do an early exit, i. e.
> >     
> >         if (!pinchGesture)
> >         {
> >             return false;
> >         }
> >     
> >     or similar.
> >     
> >     Lastly, don't use static_cast here, it doesn't do runtime type checking. 
> >     
> >     And always prefer qobject_cast over dynamic_cast if you can (that is, if you're casting QObjects). It is faster than dynamic_cast and works across dynamic libraries.
> 
> Oliver Sander wrote:
>     I moved the assignment out of the conditional.
>     
>     IMO the early exit is not a good idea.  Eventually, the gestureEvent method may support other gestures than just pinches.  In that case exiting is not the right thing to do when the pinch variable is nulptr.
>     
>     The static_cast is taken from the Qt documentation (e.g., http://doc.qt.io/qt-5/qtwidgets-gestures-imagegestures-example.html). As I understand it, event->gesture(Qt::PinchGesture) does the runtime type checking for you.
>     
>     QGestureEvents are not QObjects, there qobject_cast is not appropriate.

>  IMO the early exit is not a good idea.  Eventually, the gestureEvent method may support other gestures than just pinches.  In that case exiting is not the right thing to do when the pinch variable is nulptr.

Fair enough. But then I guess it makes more sense to check gestureType() in the if() and then cast inside the conditional block.

> As I understand it, event->gesture(Qt::PinchGesture) does the runtime type checking for you.

I'm not sure from the documentation, so I think it makes sense to check gestureType().

> QGestureEvents are not QObjects, there qobject_cast is not appropriate.

My bad, I was looking at QPinchGesture.


- Martin Tobias Holmedahl


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128821/#review98864
-----------------------------------------------------------


On Sept. 4, 2016, 3:16 p.m., Oliver Sander wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128821/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2016, 3:16 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 366781
>     http://bugs.kde.org/show_bug.cgi?id=366781
> 
> 
> Repository: okular
> 
> 
> Description
> -------
> 
> The attached patch implements document zoom controlled by a two-finger pinch gesture on a touch screen.
> 
> The actual zooming was done by copy'n'paste, I don't understand the full details of how zooming works in okular, yet.  So a bit of review would be appreciated.
> 
> 
> Diffs
> -----
> 
>   ui/pageview.h 01c39bf 
>   ui/pageview.cpp cd53407 
> 
> Diff: https://git.reviewboard.kde.org/r/128821/diff/
> 
> 
> Testing
> -------
> 
> I tested it on my Lenovo Thinkpad Yoga, and it works nicely there.
> 
> 
> Thanks,
> 
> Oliver Sander
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20160904/23c723d5/attachment-0001.html>