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