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.
aacid |
Okular |
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.
Run doxygen
No Linters Available |
No Unit Test Coverage |
Buildable 13169 | |
Build 13187: arc lint + arc unit |
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 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. |
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? |
core/textpage.cpp | ||
---|---|---|
1880–1883 | Looks like I messed up my comments.
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. |
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. |
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? |
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. |
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 :) |
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). |
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 |
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. :) |