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.
Details
Details
Diff Detail
Diff Detail
- Repository
- R223 Okular
- Branch
- create-pageview-documentation
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 12747 Build 12765: arc lint + arc unit
Comment Actions
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) | |
Comment Actions
Oh, does someone know any intended purpose division between setupActions(), setupViewerActions(), and setupBaseActions()?
Comment Actions
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
Comment Actions
Part calls them like this:
- setupActions() If not in printPreview mode nor in viewerWidget mode
- setupViewerActions() If not in printPreview mode.
- setupBaseActions() Always.
Thinking how to describe that...
Comment Actions
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 | |
Comment Actions
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.
Comment Actions
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. | |
Comment Actions
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? | |
Comment Actions
- De-rename parameter out_pagenumber
Some other variable names are insufficient (missing) for @param, so didn’t de-rename them.
Comment Actions
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.
Comment Actions
- Forgot to de-rename out_firstpage in doxygen
- Fix compilation by renaming a parameter I named “default”
- Fix grammar in PageViewItem
This comment was removed by davidhurka.
Comment Actions
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.