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

2013-06-23 Thread Commit Hook 

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

(Updated June 23, 2013, 5:22 p.m.)


Status
--

This change has been marked as submitted.


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 79fd965 
  core/annotations.cpp 353759b 
  core/annotations_p.h f9a342f 
  core/area.h 4f63759 
  core/area.cpp d772fc0 
  core/page.cpp a6fa623 
  tests/CMakeLists.txt 63c5c35 
  tests/annotationstest.cpp PRE-CREATION 

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

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-06-23 Thread Commit Hook 

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


This review has been submitted with commit 
4d4dd68ca2c7eea47216c134c84eec85588890e1 by Albert Astals Cid on behalf of 
Peter Grasch to branch master.

- Commit Hook


On June 17, 2013, 11:33 a.m., Peter Grasch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109627/
> ---
> 
> (Updated June 17, 2013, 11:33 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 79fd965 
>   core/annotations.cpp 353759b 
>   core/annotations_p.h f9a342f 
>   core/area.h 4f63759 
>   core/area.cpp d772fc0 
>   core/page.cpp a6fa623 
>   tests/CMakeLists.txt 63c5c35 
>   tests/annotationstest.cpp PRE-CREATION 
> 
> 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
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-06-17 Thread Peter Grasch 


> On March 21, 2013, 11:22 p.m., Albert Astals Cid wrote:
> > core/area.h, line 28
> > 
> >
> > Don't think this belongs to area.h to be honest
> 
> Peter Grasch wrote:
> Would you prefer something like this:
> bool ObjectRect::boundedContains( double x, double y, double xScale, 
> double yScale ) const
> 
> I don't really use the constant for anything other than that except for 
> the eraser but - if that were to be extended for a configurable brush size - 
> that would need to be changed anyway.
> 
> Albert Astals Cid wrote:
> What I basically don't like is that you've added it to a public header, 
> since you are only using it in page.cpp put it there for the moment and we'll 
> see how to export it out if/when we need it.

Ah, I actually remembered why I had it in the public header as soon as I 
uploaded the "fixed" patch right now: The eraser (review 109632) needs it.

If not in area.h, where would you rather have it?


- Peter


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


On June 17, 2013, 11:33 a.m., Peter Grasch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109627/
> ---
> 
> (Updated June 17, 2013, 11:33 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 79fd965 
>   core/annotations.cpp 353759b 
>   core/annotations_p.h f9a342f 
>   core/area.h 4f63759 
>   core/area.cpp d772fc0 
>   core/page.cpp a6fa623 
>   tests/CMakeLists.txt 63c5c35 
>   tests/annotationstest.cpp PRE-CREATION 
> 
> 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
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-06-17 Thread Peter Grasch 

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

(Updated June 17, 2013, 11:33 a.m.)


Review request for Okular.


Changes
---

Rebased off of current master and moved distanceConsideredEqual to page.cpp


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 79fd965 
  core/annotations.cpp 353759b 
  core/annotations_p.h f9a342f 
  core/area.h 4f63759 
  core/area.cpp d772fc0 
  core/page.cpp a6fa623 
  tests/CMakeLists.txt 63c5c35 
  tests/annotationstest.cpp PRE-CREATION 

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

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-06-17 Thread Peter Grasch 

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

(Updated June 17, 2013, 11:33 a.m.)


Review request for Okular.


Changes
---

Rebased off of current master and moved distanceConsideredEqual to page.cpp


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 (updated)
-

  core/annotations.h 79fd965 
  core/annotations.cpp 353759b 
  core/annotations_p.h f9a342f 
  core/area.h 4f63759 
  core/area.cpp d772fc0 
  core/page.cpp a6fa623 
  tests/CMakeLists.txt 63c5c35 
  tests/annotationstest.cpp PRE-CREATION 

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

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-04-04 Thread Albert Astals Cid 


> On March 21, 2013, 11:22 p.m., Albert Astals Cid wrote:
> > core/area.h, line 28
> > 
> >
> > Don't think this belongs to area.h to be honest
> 
> Peter Grasch wrote:
> Would you prefer something like this:
> bool ObjectRect::boundedContains( double x, double y, double xScale, 
> double yScale ) const
> 
> I don't really use the constant for anything other than that except for 
> the eraser but - if that were to be extended for a configurable brush size - 
> that would need to be changed anyway.

What I basically don't like is that you've added it to a public header, since 
you are only using it in page.cpp put it there for the moment and we'll see how 
to export it out if/when we need it.


- Albert


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


On March 23, 2013, 7:52 p.m., Peter Grasch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109627/
> ---
> 
> (Updated March 23, 2013, 7:52 p.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 
>   tests/CMakeLists.txt 6a26b3e 
>   tests/annotationstest.cpp PRE-CREATION 
> 
> 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
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-04-04 Thread Peter Grasch 


> On March 21, 2013, 11:22 p.m., Albert Astals Cid wrote:
> > core/area.h, line 28
> > 
> >
> > Don't think this belongs to area.h to be honest

Would you prefer something like this:
bool ObjectRect::boundedContains( double x, double y, double xScale, double 
yScale ) const

I don't really use the constant for anything other than that except for the 
eraser but - if that were to be extended for a configurable brush size - that 
would need to be changed anyway.


> On March 21, 2013, 11:22 p.m., Albert Astals Cid wrote:
> > core/area.h, line 82
> > 
> >
> > don't particulary like the distanceSqr name, but it seems we where 
> > already using it, so let it be i guess :D

Agreed, but changing this in ObjectRect would break BC :)


- Peter


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


On March 23, 2013, 7:52 p.m., Peter Grasch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109627/
> ---
> 
> (Updated March 23, 2013, 7:52 p.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 
>   tests/CMakeLists.txt 6a26b3e 
>   tests/annotationstest.cpp PRE-CREATION 
> 
> 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
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-04-03 Thread Albert Astals Cid 

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


you say "all but the one about distanceConsideredEqual where I'm waiting for a 
reply to my earlier comment" but I don't see an earlier comment, are you sure 
you published it?

- Albert Astals Cid


On March 23, 2013, 7:52 p.m., Peter Grasch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109627/
> ---
> 
> (Updated March 23, 2013, 7:52 p.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 
>   tests/CMakeLists.txt 6a26b3e 
>   tests/annotationstest.cpp PRE-CREATION 
> 
> 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
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-03-23 Thread Peter Grasch 

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

(Updated March 23, 2013, 7:52 p.m.)


Review request for Okular.


Changes
---

Fixed reported issues (all but the one about distanceConsideredEqual where I'm 
waiting for a reply to my earlier comment) and added test case.


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 (updated)
-

  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 
  tests/CMakeLists.txt 6a26b3e 
  tests/annotationstest.cpp PRE-CREATION 

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

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

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

2013-03-21 Thread 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


Make the function static



core/annotations.cpp


function static and & to last param



core/annotations.cpp


please make as many of this vars as you can const



core/annotations.cpp


const & for quad



core/area.h


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



core/area.h


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



core/area.h


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

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel