D21266: [WIP] Improve documentation of area classes

[Albert Astals Cid]

aacid added a comment.


  You really need to remove all the mentions of page you're adding, this is geometry that has nothing to do with pages.
  
  Yes in Okular in 99.99% of the cases it's used in conjunction with a page, but it is not about pages, it just so happen that most of the needs okular has regarding geometry is about pages and its contents.

INLINE COMMENTS

> area.h:63
> + *
> + * NormalizedRect provides additional geometric operations recarding rectangles.
> + *

recarding typeo

> davidhurka wrote in area.h:98
> I think in this case it is strictly page //size// related, because it unifies vertical and horizontal distance using Phytagoras.
> 
> Without the page size, the result would be meaningless in many cases.
> 
> Besides that, NormalizedPoint classes could be used as abstract coordinate system convenience classes, for anything else with varying absolute sizes. Can we expect anything else than pages?
> 
> If you wish to keep the documemtation compatible to future use cases, I could just clarify the scaling. Examle:
> 
>   * @par Scaling
>   * To convert a NormalizedPoint to a point with absolute coordinates,
>   * the normalized coordinate system needs to be mapped to the reference area.
>   * This can be done with two scaling factors, xScale and yScale.
>   * These will just be equal to the width and height of the reference area.
>   *

It is not page size related *at all*

It's just two points, one of them has a scaling. Nothing here is necessarily about pages.

> davidhurka wrote in area.h:108
> Is there a reason to make this one function `static`?

i don't know, you may want to ask Peter, but it was 6 years ago, i doubt he remembers.

What's your problem with it being static?

> davidhurka wrote in area.h:192
> This is implemented with double == 0. Won’t this always return true?
> 
>   bool NormalizedRect::isNull() const
>   {
>       return left == 0 && top== 0 && right == 0 && bottom == 0;
>   }

Why would it always return true?

When left is 0.5 it will obviously return false, no?

> davidhurka wrote in area.h:229
> What’s the purpose of rounding? The application code looks like it doesn’t matter.

I don't understand the question. The purpose it is "it's rounded and thus potentially more accurate than without rounding because 0.9 will be 1 instead of 0"? i mean it's like you're asking "what is rounding"?

> davidhurka wrote in area.h:308
> Adjectives like ‘left’ or ‘top’ aren’t very precise. Is the rectangle left of the point or is the point left of the rectangle?
> 
> Additionally, this is inconsistent to ‘isTop’ <-> ‘isTopOrLevel’. I suggest to rename the functions to names like:
> 
> - isAboveOrLevel
> - isLeftOrAmong
> - isLeftOfOrAlong

Breaking API is a big no no just because you don't like the name.

The documentation clearly says what left means.

> davidhurka wrote in area.h:379
> “A reference to an ‘object’” is very broad.
> 
> This is an area on a page (like RegularAreaRect), and links to an Annotation or SourceReference, or something “not owned”.
> 
> Some bad alternative ideas to ‘ObjectRect’: ;)
> 
> - InterestingArea
> - AreaToClick
> - DetailArea
> - ActiveArea
> - IntelligentArea
> - SpeciaArea
> - ReferencingArea

We're not renaming ObjectRect.

> davidhurka wrote in area.h:386
> What does this mean?

No idea, you're the first peson ever that probably reads this line :D

I used the power of git history and i think tab means table and the table tries to explain which kind of object the object() function returns.

> davidhurka wrote in area.h:412
> What’s the purpose of ‘ellipse’? Currently it’s not used.
> 
> Are any of these parameters used at all?

What do you mean ellipse is not used?

It is used here, isn't it?

https://cgit.kde.org/okular.git/tree/core/area.cpp#n314

> davidhurka wrote in area.h:456
> The base class implementation just ignores the page size and takes the point as normalized.
> 
> This method is not used anywhere. Maybe it should be fixed, ore be pure virtual?

What do you mean it's not used anywhere? Have you tried marking it as pure virtual?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21266

To: davidhurka, #okular
Cc: aacid, okular-devel, joaonetto, tfella, ngraham, darcyshen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20190602/c6012231/attachment-0001.html>