parent is an object not a widget ;)

Oh, right. Changed that.

This creates a GotoAction and lets m_document “process” it.

I still don’t understand how (Goto)Action works and how source locations work.

How to set the display used for presentation? It’s possible from the config dialog, but not from D-Bus?

Why doesn’t Part handle the print action itself?

What do pageViewPortSize and pageSize mean? Or why are these two important? I don’t understant the slot in Shell.

I had a note that this would be called with a source location, and something with GotoAction. No idea what I meant with that.

Anyway, I have no idea how Actions or GotoActions work, and also am not sure what is meant with “source location”. Someone who knows more about them? Or where I can find documentation fragments about them? Doxygen reveals nothing (except that they describe “actions”), and the code is to obscure for me.

If uncompressOk is false now, this could directly return openResult, which is now OpenError.
Pro: Reduces nesting level and makes if tests smaller.

So, swapInsteadOfOpening does not work with encrypted files?

Assume, the document did opened correctly. Why do all this wallet stuff then?

This if is effectively executing before the while loop.

And this if is effectively executing after the while loop.

This ok variable is effectively enabling the whole rest of the function. Why not return directly if openResult != Document::OpenSuccess?
There is very few code being executed without ok == true, mostly disabling several actions.

I’d like to rename m_topMessage -> m_embeddedFilesMessage.

Uncompressed files have un-real URLs?

Because openFile() is called by ReadWritePart::openUrl(), which won’t pass arguments, right?

Seems pointless, because closeUrl() will be called some more times.

m_actionsSearched is not used outside this function. Make it a static local variable?

This could cause a segfault. Who says that page will be valid?

printDialog was just constructed. So, why test it?
If it could be invalid, who will clean up the memory then?

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.

parentWidget is what will be a parent of the Part::widget(), so it can receive QEvents which are not used by the Part? Assuming that, a Part receives its QEvents through its widget(), right?

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?

Does this make sense? The parent application will loose control about the URL change. (Tried with KDevelop.)

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

Sounds funny, but that is how it works...
Even calls FindBar::maybeHide(). ;)

Not really an idea what this means. And to which members does it belong?

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.

Uh-oh, this is complicated. Would someone look over it?

Yes, the language is not final.

What if the fragment of the URL contains a page number?

A duration of 0 milliseconds seems a bit short to me. Why is this default?

This is a function, but sounds like a signal. How about “addBookmarkActions”?

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.

Seems like it opens a context menu for bookmark entries in the Bookmarks menu, to rename or delete a single bookmark. But such a menu does not open in my Okular.

Should be OK now, also improved language.

Do we really need to document a 1 line function?

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

original seems a strange word to use here

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.

the parameter wins.

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.

good point, it's a pretty terrible name

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

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

Are you looking at the wrong place?

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

Print Selection will print all selected pages.

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)

The Reload action (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.

Ok, will change that.

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.

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

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.

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

Will accept and ignore that for now.

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

If that is intended, ok.

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.

the thing is, that's not true, calling openUrl is fine, it's what the okular shell does, no?

Yes and yes. The shell has an URL that doesn’t need to be tolerantly parsed.

The purpose of openDocument(QUrl) is to be called by other applications, where the quality of the URL is not defined (only to be expected when Okular::Part is not used though the KPart interface), right?