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:
> > I don't have a touch screen to test with, unfortunately.

To bad. :-)  Thanks for the reviews.


> On Sept. 4, 2016, 10:14 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> > ui/pageview.cpp, line 1490
> > <https://git.reviewboard.kde.org/r/128821/diff/1/?file=475893#file475893line1490>
> >
> >     always {}

Okay


> 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.

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.


> On Sept. 4, 2016, 10:14 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> > ui/pageview.cpp, line 1505
> > <https://git.reviewboard.kde.org/r/128821/diff/1/?file=475893#file475893line1505>
> >
> >     potentially uninitialized, add some default value.

Okay.


> On Sept. 4, 2016, 10:14 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> > ui/pageview.cpp, line 1507
> > <https://git.reviewboard.kde.org/r/128821/diff/1/?file=475893#file475893line1507>
> >
> >     always use {}, even for oneliners.

Okay


> On Sept. 4, 2016, 10:14 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> > ui/pageview.cpp, line 1510
> > <https://git.reviewboard.kde.org/r/128821/diff/1/?file=475893#file475893line1510>
> >
> >     Why put this in a separate variable?

In anticipation of

https://git.reviewboard.kde.org/r/126809/

I removed the separate variable for now.


- Oliver


-----------------------------------------------------------
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/ba69e4ce/attachment.html>