[WIP] Improve class documentation for PageView and PageViewItem
Needs ReviewPublic

Authored by davidhurka on Jun 10 2019, 7:42 PM.

Details

Reviewers
None
Group Reviewers
Okular
Summary

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.

Test Plan

Run doxygen

Diff Detail

Repository
R223 Okular
Branch
create-pageview-documentation
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13280
Build 13298: arc lint + arc unit
davidhurka created this revision.Jun 10 2019, 7:42 PM
Restricted Application added a project: Okular. · View Herald TranscriptJun 10 2019, 7:42 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
davidhurka requested review of this revision.Jun 10 2019, 7:42 PM

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()?

aacid added a subscriber: aacid.Jun 11 2019, 9:50 PM

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:

  • setupActions() If not in printPreview mode nor in viewerWidget mode
  • setupViewerActions() If not in printPreview mode.
  • setupBaseActions() Always.

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?

165

it actually returns more than one area, it returns a list of areas

171

please don't rename the variable?

177

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

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

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
171

Ok.

I thought there was a convention to use out_ for output parameters.

davidhurka updated this revision to Diff 59687.Jun 12 2019, 9:21 PM

Looked over the whole patch again.

Several wording improvements, and made some less specific or more accurate.

davidhurka marked 6 inline comments as done.Jun 12 2019, 9:43 PM
davidhurka added inline comments.
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.

177

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?

davidhurka updated this revision to Diff 59696.Jun 12 2019, 9:44 PM
  • De-rename parameter out_pagenumber

Some other variable names are insufficient (missing) for @param, so didn’t de-rename them.

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

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.

davidhurka updated this revision to Diff 60616.Mon, Jun 24, 8:08 PM
davidhurka marked an inline comment as done.
  • 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.