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

[Martin Tobias Holmedahl Sandsmark]

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



I don't have a touch screen to test with, unfortunately.


ui/pageview.cpp (line 1490)
<https://git.reviewboard.kde.org/r/128821/#comment66563>

    always {}



ui/pageview.cpp (line 1500)
<https://git.reviewboard.kde.org/r/128821/#comment66559>

    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.



ui/pageview.cpp (line 1505)
<https://git.reviewboard.kde.org/r/128821/#comment66562>

    potentially uninitialized, add some default value.



ui/pageview.cpp (line 1507)
<https://git.reviewboard.kde.org/r/128821/#comment66561>

    always use {}, even for oneliners.



ui/pageview.cpp (line 1510)
<https://git.reviewboard.kde.org/r/128821/#comment66560>

    Why put this in a separate variable?


- Martin Tobias Holmedahl Sandsmark


On Sept. 3, 2016, 5:49 p.m., Oliver Sander wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128821/
> -----------------------------------------------------------
> 
> (Updated Sept. 3, 2016, 5:49 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/23fd55f0/attachment-0001.html>