This shall improve the documentation of PageView and PageViewItem,
with a focus on geometries.
This shall make it easier to understand which
geometries, coordinate systems, etc. are used.
No Linters Available |
No Unit Test Coverage |
Buildable 12747 | |
Build 12765: arc lint + arc unit |
Documenting these classes wasn’t really difficult, they already have helpful inline comments and member names.
I think PageViewItem should store page rotation, along with already stored geometry information, which is rotation dependent.
That will make it easier to implement both Bug 307224 (Ability to rotate single pages) and 169847 (split view feature, second most wished feature request :) ).
Page could store the information too, but that is semantically less correct.
ui/pageview.cpp | ||
---|---|---|
429 | Wrong cursor? Dragging the page activates a closed hand, not size all. | |
485 | This has usually 17 items. Any reasons to limit to 14 instead? | |
3717 | Suggestion: QRect::contains(x, y, proper = true) |
Oh, does someone know any intended purpose division between setupActions(), setupViewerActions(), and setupBaseActions()?
someone does yes.
it has to do with part's m_embedMode check part.cpp it's actually pretty easy to see the use case for them
Part calls them like this:
Thinking how to describe that...
i got bored of reviewing before reaching 25%, sorry
core/page.h | ||
---|---|---|
103 ↗ | (On Diff #59553) | i see why you would like to mention this here, but i don't think it makes any sense. As far as the page is concerned there's no "trim away" or no "trim away". That's done much earlier, so that is just "the size of the page" whatever the generator did with that page happened before and the page doesn't really know nor care about it. |
ui/pageview.h | ||
56 | i kind of know how this works, and i don't understand what "shown pages" is supposed to mean. | |
130 | what does "can modify the viewport" mean for you? | |
158 | it actually returns more than one area, it returns a list of areas | |
164 | please don't rename the variable? | |
170 | what do you mean by uncropped geometry? | |
237 | toggles is the verb you're looking for | |
255 | you don't document what signals will be used for, signals can be used for whatever the person connecting to the signal wants. | |
286 | enables is not the verb you want, handles is better |
i don't really see what's the need to describe it, there's N modes and each mode has more or less actions depending how "full okular" the mode is.
Thanks for your comments, I will look over my patch soon.
I wrote some parts of this before really understanding this, these parts will get more work.
core/page.h | ||
---|---|---|
103 ↗ | (On Diff #59553) | Agreed, doesn’t belong here. Only important in PageViewItem, as that class stores how margins are trimmed. |
ui/pageview.h | ||
164 | Ok. I thought there was a convention to use out_ for output parameters. |
Looked over the whole patch again.
Several wording improvements, and made some less specific or more accurate.
ui/pageview.h | ||
---|---|---|
56 | Made the item “Content area” more general, to explain its purpose not before the item “Viewport area”. | |
130 | Didn’t make sense. fitPageWidth() relayouts the pages, so mentioning that instead. | |
170 | That’s a coordinate system of PageViewItem. Uncropped geometry is now explained in the Detailed Description of PageViewItem. Do you think this is unclear, or didn’t you read so far yet? |
Some other variable names are insufficient (missing) for @param, so didn’t de-rename them.
I think I figured that out now.
ViewerWidgetMode limits editing actions, and PrintPreviewMode additionally limits zoom action shortcuts. I don’t understand why viewer embedding modes forbid text selection.
Please move as a Merge Request in https://invent.kde.org/kde/okular
We have pre-commit CI and lots of checks including clazy and clang-tidy there so it's a much better place for doing the review/approval/merge of the code.