Improve documentation of TextEntity stuff
ClosedPublic

Authored by davidhurka on May 18 2019, 12:26 PM.

Details

Summary

This adds some important documentation on TextEntity and other classes, and improves some of the existing documentation.

This includes changing parameter names from ‘rect’ to ‘area’, because I found ‘rect’ misleading.

Test Plan

Run doxygen

Diff Detail

Repository
R223 Okular
Branch
improve-documentation-of-textentity-stuff
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13169
Build 13187: arc lint + arc unit
davidhurka created this revision.May 18 2019, 12:26 PM
Restricted Application added a subscriber: okular-devel. · View Herald TranscriptMay 18 2019, 12:26 PM
davidhurka requested review of this revision.May 18 2019, 12:26 PM

Thanks for fixing these typos.

core/textpage.h
174
core/textpage_p.h
22

Typo: it's bounding box -> its bounding box

TextPagePrivate::correctTextOrder() calls some complex functions, which are yet undocumented. Interesting stuff is happening in them, so they should get some documentation. I added their prototypes to core/textpage_p.h, so I can add documentation to them.

Thanks for fixing these typos.

Thanks for spotting these typos. :)

core/textpage.cpp
1880–1883

This is some interesting information, which should be documented more visible. Is the information still true?

1883

Is this enough for documents with high text density? At least, this is a good reason for NormalizedRect::roundedGeometry().

core/textpage.h
52

Documentation can’t fix https://bugs.kde.org/show_bug.cgi?id=407133. As soon as TextPagePrivate::correctTextOrder handles vertical text, this paragraph can be removed.

aacid added a subscriber: aacid.May 19 2019, 8:45 PM
aacid added inline comments.
core/textpage.cpp
1880–1883

Why should it documented more visible? Who else cares?

core/textpage.h
39–52

s/an/its ?

44

entities

163

i don't see the extra value added by this addition, the "Retuns" below already says that this function does, but if you want to have a sentence here i'd prefer "and returns it as a string" over "and concatenates it to a string".

174

same as above

core/textpage_p.h
32

What?

89

What?

davidhurka updated this revision to Diff 58322.May 19 2019, 9:00 PM
davidhurka marked 2 inline comments as done.
  • Fix typos spotted by yurchor
  • Fix typos spotted by Albert
davidhurka added inline comments.May 19 2019, 9:31 PM
core/textpage.cpp
1880–1883

Looks like I messed up my comments.

This is some interesting information, which should be documented more visible. Is the information still true?

belongs only to line 1880:

// m_page->width() and m_page->height() are in pixels at
//100% zoom level, and thus depend on display DPI.

(Information from line 1880) Should be documented because any Generator has to return the page size at some time. When I was reading about Generator implementation, I wondered what this size should be, and how it is related to the pixmap size. Should I think in 1px/pt? 1px/mm? A completely different unit?

This (line 1880) is not directly about the Generator, but it might be interesting what will be considered “default size” after the Generator returned the page size.

core/textpage.h
163

Agreed, will remove that sentence.

This is related to textArea( TextSelction * ), I will mention that instead.

core/textpage_p.h
32

Need this to document the following function prototypes, would not compile otherwise.

aacid added inline comments.Jun 2 2019, 8:39 PM
core/textpage.cpp
1880–1883

pagesSizeMetric defines what pagesize means for your generator.

core/textpage_p.h
32

Why are there new functions in an "improve documentation" merge request?

davidhurka marked 4 inline comments as done.Jun 2 2019, 9:04 PM
davidhurka added inline comments.
core/textpage.cpp
1880–1883

Ok, if it is documented directly in Generator, it should be sufficient. Maybe I was looking at PageSize only.

core/textpage_p.h
32

As mentioned somewhere else, the algorithms in these functions are interesting. (Besides they don’t do a good job with vertical text. I hope by writing documentation I will get some understanding what changes could be appropiate.)

I just didn’t write the documentation yet (Blahblah placeholders), but doxygen won’t compile it if they don’t appear in this header file. Maybe there is some other way to compile it, just tell me which way is better.

I think that these functions should be private members of TextPagePrivate, because that is where they are used. If I understand PIMPL correctly, that would be no problem for binary compability.

aacid added inline comments.Jun 2 2019, 9:13 PM
core/textpage_p.h
32

Ah, you're trying to export already existing functions :D

Didn't realize that.

Why do you want to export them? Yes their are interesting, but where would you use them that is not textpage itself?

davidhurka marked an inline comment as done.Jun 2 2019, 10:08 PM
davidhurka added inline comments.
core/textpage_p.h
32

I have exported them to find them in this header file later. But I prefer to do the opposite by moving them to TextPagePrivate.

Anyway, they could, maybe, be useful for an OCR application which uses Okularpart to show input and result. Not planning to do that.

aacid added inline comments.Jun 22 2019, 1:39 PM
core/textpage.h
53

I'm not convinced we should mention this (and even less in this class), sure it's a bug, but hopefully it'll be fixed and when it does noone will remember to remove this since the bug is not even in this class.

163

concatenate is not the word you want here.

Concactenate means "apppend" and there's no string passed in to get it appended to.

"returns it as a string", which as pointed out is already below, but if you really want that sentence to exist please use return as not concatenate it to

core/textpage_p.h
32

You're trying to do to different things here then, this says "improve documentation" so please don't move code around, let's try to focus on the documentation :)

davidhurka updated this revision to Diff 60383.Jun 22 2019, 5:34 PM
davidhurka marked 10 inline comments as done.
  • Remove comment on vertical text, remove the word concatenate
  • Don’t export text entity reordering functions
davidhurka added inline comments.Jun 22 2019, 5:35 PM
core/textpage.h
53

Ok, then I will remove it.

163

How do you get from concatenate to append?

I’m not the best english speaker, but I think concatenate (as verb) does not take that kind of object.

Well, I will avoid it completely.

core/textpage_p.h
32

Moving that to a future patch (not defining future right now).

aacid accepted this revision.Jun 23 2019, 3:28 PM

Thanks for putting up with me.

You don't have a committer account, right?

core/textpage.h
163

concatenate = to put things together as a connected series

https://en.wikipedia.org/wiki/Concatenation says 'For example, the concatenation of "snow" and "ball" is "snowball".'

That's why concatenate to me implies more than one thing, and here's there just one, the string itself.

But i'm not a native english speaker either so may be totally wrong

This revision is now accepted and ready to land.Jun 23 2019, 3:28 PM
davidhurka marked 11 inline comments as done.Jun 23 2019, 5:20 PM

You don't have a committer account, right?

No.

core/textpage.h
163

It was meant differently. It concatenates the TextEntities, which are kind of strings, and more than only one. The result is the one string which is returned. :)

davidhurka marked 2 inline comments as done.Jun 23 2019, 5:29 PM
davidhurka retitled this revision from [WIP] Improve documentation of TextEntity stuff to Improve documentation of TextEntity stuff.Jun 23 2019, 5:35 PM
aacid closed this revision.Jul 21 2019, 8:50 AM