D21266: [WIP] Improve documentation of area classes

[David Hurka]

davidhurka added a comment.


  This is part of my goal to understand how TextEntity reordering works. There will probably be more patches like this soon.
  
  I included some examples about how/where these classes are used. But why are there both RegularAreaRect (with its base classes) and ObjectRect?
  
  What is a special purpose of ObjectRect? As it is used now, it’s indeed rectangular and RegularAreaRect could do most of it. ObjectRect derived objects do store their type, but are not stored mixed. An annotation or a source reference could simply link to an own RegularAreaRect or NormalizedPoint.
  
  There are some more things I couldn't figure out. See my inline comments.
  
  How can I add inline comments to unmodified files...?

INLINE COMMENTS

> area.h:108
>           */
>          static double distanceSqr( double x, double y, double xScale, double yScale, const NormalizedPoint& start, const NormalizedPoint& end );
>  

Is there a reason to make this one function `static`?

> area.h:192
>           */
>          bool isNull() const;
>  

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

> area.h:229
>           */
>          QRect roundedGeometry( int xScale, int yScale ) const;
>  

What’s the purpose of rounding? The application code looks like it doesn’t matter.

> area.h:308
>           */
>          bool isLeft(const NormalizedPoint& pt) const
>          {

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

> area.h:379
>  /**
>   * @short NormalizedRect that contains a reference to an object.
>   *

“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

> area.h:386
>   *
>   * Type / Class correspondence tab:
>   *  - Action    : class Action: description of an action

What does this mean?

> area.h:412
>           * @param bottom The bottom coordinate of the rectangle.
>           * @param ellipse If true the rectangle describes an ellipse.
>           * @param type The type of the storable object @see ObjectType.

What’s the purpose of ‘ellipse’? Currently it’s not used.

Are any of these parameters used at all?

> area.h:456
>          /**
> -         * Returns whether the object rectangle contains the point @p x, @p y for the
> -         * scaling factor @p xScale and @p yScale.
> +         * Returns whether the object rectangle contains the point with absolute coordinates
> +         * (@p x, @p y) at a page size of @p xScale x @p yScale.

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?

REPOSITORY
  R223 Okular

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

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