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