[Okular-devel] Review Request 109627: Outline based selection for annotation elements

[Albert Astals Cid]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109627/#review29666
-----------------------------------------------------------


Quick and mostly "stylistic" review, had a look at how it works for a line and to be honest i found it a bit hard (i.e. i had to be quite on top of the line) to select it, but i guess it is just a matter of getting used to.

I am trying to slowly get new features to come with some kind of tests to make sure stuff does not break. Do you have any knowledge of QTest? Do you think you'd be able to create a test for this? In my mind it would be like 
 * Open pdf
 * Add annotations of all types
 * Programatically move the mouse to the locations around the annotations and right click and then check if the menu correctly detects we are on the object or not

What do you think?


core/annotations.cpp
<http://git.reviewboard.kde.org/r/109627/#comment22104>

    Make the function static



core/annotations.cpp
<http://git.reviewboard.kde.org/r/109627/#comment22105>

    function static and & to last param



core/annotations.cpp
<http://git.reviewboard.kde.org/r/109627/#comment22106>

    please make as many of this vars as you can const



core/annotations.cpp
<http://git.reviewboard.kde.org/r/109627/#comment22107>

    const & for quad



core/area.h
<http://git.reviewboard.kde.org/r/109627/#comment22110>

    Don't think this belongs to area.h to be honest



core/area.h
<http://git.reviewboard.kde.org/r/109627/#comment22108>

    Please add @p var in your docu like we do in the other places



core/area.h
<http://git.reviewboard.kde.org/r/109627/#comment22109>

    don't particulary like the distanceSqr name, but it seems we where already using it, so let it be i guess :D


- Albert Astals Cid


On March 21, 2013, 12:50 a.m., Peter Grasch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109627/
> -----------------------------------------------------------
> 
> (Updated March 21, 2013, 12:50 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> -------
> 
> Multiple features in Okular require to determine what object is at a given position. Traditionally, this relied on the bounding boxes of the given object.
> These do not necessarily correlate with the user would expect (for example, a diagonal line of 1px has a very large bounding box).
> 
> This patch implementes shape based selection for the following annotation types:
> Ink, Geometric, Line, Highlight.
> Other objects default to the old behaviour.
> 
> 
> Diffs
> -----
> 
>   core/annotations.h 72abdff 
>   core/annotations.cpp 49ab5bd 
>   core/annotations_p.h 221572d 
>   core/area.h 4f63759 
>   core/area.cpp d772fc0 
>   core/page.cpp 1db2763 
> 
> Diff: http://git.reviewboard.kde.org/r/109627/diff/
> 
> 
> Testing
> -------
> 
> I tested the annotation objects above and a couple of special cases mentioned in the IRC.
> 
> 
> Thanks,
> 
> Peter Grasch
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20130321/e2b3c729/attachment.html>