D21734: [WIP] Improve class documentation for PageView and PageViewItem

[Albert Astals Cid]

aacid added a comment.


  i got bored of reviewing before reaching 25%, sorry

INLINE COMMENTS

> page.h:103
> +         *
> +         * If Trim Margins is active, returns the width without the trimmed-away margins.
>           */

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.

> pageview.h:56
> + * This is a scroll area, so it uses two straightforward coordinate systems:
> + *  * Content area: Contains all shown pages, which will be arranged in a layout.
> + *  * Viewport area: Is the currently visible part of the content area.

i kind of know how this works, and i don't understand what "shown pages" is supposed to mean.

> pageview.h:104
> +        /**
> +         * Returns true when fitPageWidth() can modify the viewport.
> +         */

what does "can modify the viewport" mean for you?

> pageview.h:132
> +        /**
> +         * Returns a text selection area, using text-style selection.
> +         *

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

> pageview.h:138
> +         */
> +        QList< Okular::RegularAreaRect * > textSelections( const QPoint& start, const QPoint& end, int& out_firstpage );
> +

please don't rename the variable?

> pageview.h:144
> +         * @param item The page on which to select.
> +         * @param startPoint Where to start the selection, in uncropped geometry coordinates of the page.
> +         * @param endPoint Where to end the selection, in uncropped geometry coordinates of the page.

what do you mean by uncropped geometry?

> pageview.h:211
> +        /**
> +         * Enables or disables the change colors feature.
> +         */

toggles is the verb you're looking for

> pageview.h:229
> +         *
> +         * Shall be used to open a context menu at the cursor position.
> +         *

you don't document what signals will be used for, signals can be used for whatever the person connecting to the signal wants.

> pageview.h:264
> +        /**
> +         * Enables pinch zoom and rotation.
> +         */

enables is not the verb you want, handles is better

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21734

To: davidhurka, #okular
Cc: aacid, okular-devel, joaonetto, tfella, ngraham, darcyshen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20190611/0d66e802/attachment-0001.html>