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

[Oliver Sander]

> 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.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     >  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.

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

You mean

if (event->gesture(Qt::PinchGesture))
{
  QPinchGesture *pinch = your_favorite_cast<QPinchGesture*>(event->gesture(Qt::PinchGesture));
  ...
  
? That would mean two calls to event->gesture(Qt::PinchGesture), unless we move the assignment into the conditional again.  Are the two calls what you want?  (I have no opinion here).


- Oliver


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


On Sept. 4, 2016, 7:48 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, 7:48 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/31744504/attachment.html>