Improve documentation of area classes
ClosedPublic

Authored by davidhurka on May 17 2019, 11:06 PM.

Details

Summary

This shall improve the documentation of several area classes,
including NormalizedPoint, NormalizedRect, RegularArea, RegularAreaRect.

This shall also clarify when absolute coordinates and when normalized
coordinates are used.

Describes the normalized coordinate system in NormalizedPoint, with a new term “reference area” do describe mapping.

TODO:
Page view rotation.
This is not done consistently in Okular, but can be changed later. I think this documentation will help (me) with that then.

Test Plan

Run doxygen

Diff Detail

Repository
R223 Okular
Branch
improve-area-classes-documentation
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13242
Build 13260: arc lint + arc unit
davidhurka created this revision.May 17 2019, 11:06 PM
Restricted Application added a project: Okular. · View Herald TranscriptMay 17 2019, 11:06 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
davidhurka requested review of this revision.May 17 2019, 11:06 PM

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

core/area.h
164

Is there a reason to make this one function static?

251

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

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

366

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
437

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

What does this mean?

475

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

Are any of these parameters used at all?

519–520

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?

aacid added a subscriber: aacid.May 19 2019, 10:09 PM
aacid added inline comments.
core/area.h
35

i don't see why your version of the text is better.

45–117

zoom doesn't seem the appropiate word here, there's no zooming involved

123

why "the" ?

134

I don't like that you mention a page here.

152–153

same, this is not striclty page related

181

Again this is not strictly page related, can be used for anything please don't refer to pages other than as "for example"

184

This information makes no sense here.

187

What a regularAreaRect is doesn't belong to the description of NormalizedRect (imho, the @see is fine)

939

This comment is weird, it seems to imply such functionality exists in this class, when it doesn't.

Should the section “Coordinate System” of NormalizedPoint mention the “Trimmed Margins” feature? To prevent that someone sloppily codes a feature, which draws something on the page and fails if Trim Margins is active.

core/area.h
35

Not sure. Shall I change it back?

The paragraph “Coordinate System” should make it clear anyway.

45–117

Hmm, agreed. Will change it back.

123

I thought that all instances will be equal points, so “the” would be appropiate.

Usually it goes “creates a null ...”, so “a” is probaby better. BTW: There is no NormalizedPoint::isNull(), so why this constructor?

134

One can scale with a factor or with a divisor, and will probaby call both scaling factor. So what should xScale or yScale be? I think saying “page size” makes it clear. Using a PageSize or ScalingFactor struct would make it clear as well.

152–153

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.
*
184

TextEntity provides example usage, so someone who wonders why this class exists will be happy.

Throwing in this link, because it mentions examples.
https://community.kde.org/Guidelines_and_HOWTOs/API_Documentation#Writing_APIDOX_in_New_Code

I will remove the “glyphs, words, line...”, that’s too specific.

187

It provides an alternative to NormalizedRect. NormalizedRect is already pretty good for describing shapes on a page, but in some cases RegularAreaRect will be better.

939

Doesn’t RegularAreaRect::geometry() do that?

davidhurka updated this revision to Diff 58380.May 20 2019, 6:41 PM
  • NormalizedPoint: Revert some stuff and mention mouse click events

Turns out that the coordinate system of RegularAreaRect, as it is used by TextPage::textArea(TextSelection), is not independent of the page display. textArea() transforms the whole RegularAreaRect with the current totalRotation() of the Page.

Turns out that nothing is independent of page rotation, just discovered PagePrivate::rotateAt(Okular::Rotation), which transforms everything what is known to rotateAt().

Turns out that two facts turned out which let me wonder about the limited purpose of the normalized coordinate system. Why doesn’t it normalize rotation?

aacid added a comment.Jun 2 2019, 8:33 PM

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.

core/area.h
61

recarding typeo

152–153

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.

164

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?

251

Why would it always return true?

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

287

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

366

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

The documentation clearly says what left means.

437

We're not renaming ObjectRect.

445

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.

475

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

519–520

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

davidhurka marked 10 inline comments as done.Jun 2 2019, 10:51 PM

I will remove page sizes from member documentations and describe the coordinate system with a more abstract reference rectangle.

Page sizes and zoom and so on will just be an application of the normalized coordinate system.

Furthermore, it will not be discussed yet whether normalized coordinates should contain page rotation information.

core/area.h
251

You’re right, I thought this converts to int, tuncating the value, but it will convert to double.
https://en.cppreference.com/w/cpp/language/operator_arithmetic

287

Rounding is like a statistical offset of 0.5.
And in the application (TextEntity reordering/concatenation), the calculation is just taking place 0.5 pixels to the bottom-right.
Hmm, I consider this as related to page related - not page related.

366

Didn’t realize OKULARCORE_EXPORT, bad idea to rename this. So I agree.

This should also answer whether to replace scaleX and scaleY arguments by a struct. Wouldn’t have done that in this patch anyway.

Bug 334297 is indirectly related, have to think about that.

475

I meant whether it is used by anything which calls ObjectRect. But everything seems to implement own stuff to populate the QPainterPath.

davidhurka marked 4 inline comments as done.Jun 2 2019, 11:34 PM
davidhurka added inline comments.
core/area.h
164

It would make sense to make it non static, because the first parameter is a NormalizedPoint (consists of x and y, xScale and yScale don’t scale the point).

Anyway, OKULARCORE_EXPORT is a reason to keep it static, right?

519–520

pure virtual does not work becaues ObjectRect is instantiated.

I still don’t find it used anywhere.

davidhurka updated this revision to Diff 59149.Jun 4 2019, 6:49 PM
davidhurka marked an inline comment as done.
davidhurka edited the summary of this revision. (Show Details)

Removed references to pages from methods.

Now they refer to a reference area, and the description of the normalized coordinate system now explains how mapping to a “reference area” works, and how parameters xScale and yScale are related.

Also added another example section, now covers transformation both to and from a page coordinate system. The second example touches rotation, I can remove that if demanded.

ObjectRect still uses pages as reference area, because I consider ObjectRect coordinates page related. (Document objects are located on pages.) For consistency, I can remove these references there too, if demanded.

Annotation::Quad is related, but is already pretty well documented. Just clarified some things I consider relevant. (Only 4 points instead of 8 points, and things which are directly related to Bug 334297.)

I think it makes sense to move text reordering to another patch.

aacid accepted this revision.Jun 22 2019, 1:30 PM

There's a few typos and i think i don't really agree with the "rectangles should not have negative width/height" wording, they don't make much sense, but on principle don't tell people what they can or can not do.

But let's commit this, i think it's not worse of what we have in general.

Thanks :)

core/area.h
94

click->press

97

seach -> search, click -> press

164

it's exported api, so yes let's not change it, and less in a "improve documentation" MR ;)

184

i still don't like the "is used" wording, it's like binds it forever, if you really want to reference a class (can't people use grep or find all in an IDE), i'd prefer a wording like "for example is used in" or similar

214

what if they want it to be negative for some reason? Does anything break? i mean the lines above clearly say left, top, and bottom.

225

same as above, does it really need to have a positive width and height?

This revision is now accepted and ready to land.Jun 22 2019, 1:30 PM
davidhurka marked 12 inline comments as done.Jun 22 2019, 6:14 PM

Thanks for marking the typos.

I tried to use your latest comments for improvements, without changing much else. (In the end I changed only 3 sentences.)

core/area.h
184

is used -> can be used e. g.

Better?

214

Yes, e. g. if left is right of right, the geometry operations (|=, &=, etc) don’t behave really ...obviously defined. It will probably work, but it might work differently as soon as someone tries to fix a bug.

The problem is mainly that the behaviour will not be defined even after this patch, so I simply write it this way.

davidhurka updated this revision to Diff 60388.Jun 22 2019, 6:15 PM
  • Small improvements based on latest comments by Albert.
davidhurka marked 14 inline comments as done.Jun 23 2019, 8:01 PM
davidhurka updated this revision to Diff 60536.Jun 23 2019, 8:08 PM
  • Describe consequences of negative width/height
  • Add note to RegularArea::contains() concerning simplify()
davidhurka marked 2 inline comments as done.Jun 24 2019, 2:50 PM

Changed 3 more lines yesterday.
Now removing [WIP]; I have some remaining questions (inline coments without “done”), but these are not important for this patch.

core/area.h
225

Added a sloppy note why it should be positive.

davidhurka retitled this revision from [WIP] Improve documentation of area classes to Improve documentation of area classes.Jun 24 2019, 2:54 PM
davidhurka edited the summary of this revision. (Show Details)
aacid closed this revision.Jul 21 2019, 6:00 PM