D21281: [RFC] Write Documentation for Okular::Part

[Albert Astals Cid]

aacid added a comment.


  Some comments.
  
  Anyhow i feel that you asking all these questions over here is weird, phabricator is terrible for this kind communication and i can't keep up with the questions you have or the ones i've answered or whatnot.
  
  I really feel this would benefit from you jumping on IRC and making the questions live.

INLINE COMMENTS

> part.h:120
> + *  - openUrl( const QUrl &url )
> + *    Overrides ReadOnlyPart::openUrl( const QUrl& ) to redirect to the following method:
> + *  - openUrl( const QUrl &url, bool swapInsteadOfOpening )

Do we really need to document a 1 line function?

> part.h:123
> + *    Uses the original implementation of ReadOnlyPart::openUrl( const QUrl& ),
> + *    but allows to reload the file without affecting the user interface.
> + *  - openFile()

Can you explain what you mean with that sentence about reload?

> part.h:125
> + *  - openFile()
> + *    Called by the original implementation of ReadOnlyPart::openUrl( const QUrl& ).
> + *    Tries to determine the mimetype, and handles UI updating.

original seems a strange word to use here

> part.h:130
> + *    To actually open the file, this calls Document, which will get the correct Generator for the mimetype.
> + *
> + * Similarly to openDocument(), these protected slots are used for special use cases of opening. They also call openUrl().

I'm really sorry but i don't see any value in this documentation here.

You just described the calling flow. This is something anyone with a debugger or a nice IDE can do. And it is very easy this will get out of sync once someone renames a method.

Please try to convince me why having this is useful.

> davidhurka wrote in part.h:191
> What if the fragment of the URL contains a page number?

the parameter wins.

> davidhurka wrote in part.h:739
> A duration of 0 milliseconds seems a bit short to me. Why is this default?

It's a silly default, the nice thing is that it's never the default since this is conneected from document::error and that one requires for a value for duration, so this = 0 can be just removed without any issue.

> davidhurka wrote in part.h:773
> This is a function, but sounds like a signal. How about “addBookmarkActions”?

good point, it's a pretty terrible name

> davidhurka wrote in part.h:780
> One view action is “Fit Width”. Unlike Fit Width from the menu bar, this Fit Width will not only set the zoom to Fit Width, but also set the view mode to Single Page, and center the viewport on page N.

Yes, because this is "fit page N to width"

> davidhurka wrote in part.h:644
> This opens a context menu for context menu items, or what?
> 
> It accepts events of type ContextMenu, that’s clear. To open the context menu, it seems to need a QMenu as event source.

It's an event filter, this is a basic way of filtering events in Qt.

Are you looking at the wrong place?

F6865175: imatge.png <https://phabricator.kde.org/F6865175>

REPOSITORY
  R223 Okular

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

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