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

[David Hurka]

davidhurka marked 6 inline comments as done.
davidhurka added a comment.


  Sometimes I’m working on PageView, but I move it out of this patch because it’s less related than I thought. And this is enough stuff for a single patch for now...

INLINE COMMENTS

> davidhurka wrote in part.cpp:1746
> Seems pointless, because closeUrl() will be called some more times.

closeUrl() is called through openUrl(), which probably knows what it does to the arguments.

> davidhurka wrote in part.cpp:3354
> How does the Selection option work? Print all pages with bookmarks? Print all pages between bookmarks? Print the page with the selected bookmark? Could not find that in the user documentation.

Print Selection will print all selected pages.

> aacid wrote in part.h:120
> Do we really need to document a 1 line function?

openUrl() is the function which is usually used by Parts to open something. Someone (me) who wants to understand how documents are opened will want to know what will happen to a call to this function.

It doesn’t need do be documentation of this function, just describing what will happen to the usual call is enough.

In the documentation of this function, I consider the following approiate:
To open a document, the parent application should call openDocument(*) instead of openUrl(). @see openDocument(const QString &url)

> aacid wrote in part.h:123
> Can you explain what you mean with that sentence about reload?

The Reload action (F5 <https://phabricator.kde.org/F5>) also reloads the UI, but SaveAs doesn’t. It also reloads the document, so it is backed by the newly saved file. It uses swapInsteadOfOpening to achieve that.

At least that is how I understand it.

> aacid wrote in part.h:125
> original seems a strange word to use here

Ok, will change that.

> aacid wrote in part.h:130
> 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.

I needed this calling flow to understand how Okular handles document opening. For someone else (don’t know who that could be, someone of the future...), this //could// give an overview. And that is what I consider the purpose of documentation.

People who prever a debugger or the capabilities of an IDE won’t neccessarily need that.

I think it is save to document this, because many of these functions are exposed and probably won’t be renamend. doOpenFile() sounds a little strange, but is accurate.

The following three functions will more likely be changed. And they are pretty trivial and simple to understand with a normal IDE, so I’m ok with removing this part.

> aacid wrote in part.h:780
> Yes, because this is "fit page N to width"

If that is intended, ok.

> davidhurka wrote in part.h:329
> What does this mean? Are these the functions which are called when the user clicks an action, and they will call the bunch of other open*(url) functions?

Already understand that they will call the bunch of other open*(url) functions, see Okular::Part Detailed Description.

But I don’t see how they are connected to QActions, so I suggest to remove that line or clarify that these AreaToClick (ObjectRect) things in the Document are meant.

> davidhurka wrote in part.h:345
> Does this make sense? The parent application will loose control about the URL change. (Tried with KDevelop.)

Actually calls openUrl(), which informs the parent application. KDevelop just ignores that. Need to look how exactly that is done.

> davidhurka wrote in part.h:386
> These slots use QObject::sender() and cast it to QAction* to extract the data, to convert it to a string and then to a DocumentViewport.
> This seems overly specific to me, and screws up my documentation.
> Why not simply connect to slotRenameBookmark(DocumentViewport) and pass the viewport as argument? (This is only used by showMenu() (?), which constructs actions on-demand anyway.)

Will accept and ignore that for now.

> davidhurka wrote in part.h:529
> Not really an idea what this means. And to which members does it belong?

Belongs to updateViewActions() and enableTOC(bool). Connecting to widgets is not done and makes little sense, so I will remove that line.

> aacid wrote in part.h:644
> 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>

Doesn’t work here. Other popup menus can open a context menu for action collection actions, but didn’t notice that in a menubar menu so far.

So it opens a context menu for a menubar menu, almost guessed.

> davidhurka wrote in part.h:271
> What do `pageViewPortSize` and `pageSize` mean? Or why are //these// two important? I don’t understant the slot in Shell.

pageViewPortSize is the current viewport size + visible scollbars,
and pageSize is the size of the page + the intended margins to the viewport edges.

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/8a521b92/attachment-0001.html>