[WIP] Write Documentation for Okular::Part
Needs ReviewPublic

Authored by davidhurka on May 18 2019, 11:52 PM.

Details

Reviewers
None
Group Reviewers
Okular
Summary

This will describe Okular::Part.

The intention comes from D21195, where I am thinking whether I should
mess up Okular::Part instead of Okular::PageView.

Also adds some information to Okular::Document, which I found relevant for documenting some Part members.

Current discussion is mainly how robust this is to future changes in the code.

Test Plan

Run doxygen

Diff Detail

Repository
R223 Okular
Branch
create-part-and-pageview-class-documentation
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13167
Build 13185: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Okular. · View Herald TranscriptMay 18 2019, 11:52 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
davidhurka requested review of this revision.May 18 2019, 11:52 PM

Didn’t get far yet. Much information is visible in the source code, but what should I put into the class/member documentation, and what is clear just because it’s a Part?

Is there some more KParts literature? I could find KParts on api.kde.org and the tutorials on community.kde.org, but couldn’t really transfer that to Okular.

aacid added a subscriber: aacid.May 19 2019, 10:13 PM

Is there some more KParts literature? I could find KParts on api.kde.org and the tutorials on community.kde.org, but couldn’t really transfer that to Okular.

Why you couldn't transfer it to okular? If you don't say what you don't understand is hard to explain *everything*

part.h
170

parent is an object not a widget ;)

Is there some more KParts literature? I could find KParts on api.kde.org and the tutorials on community.kde.org, but couldn’t really transfer that to Okular.

Why you couldn't transfer it to okular? If you don't say what you don't understand is hard to explain *everything*

I would like to understand how KParts are used usually / how they should be used. Understanding how Okular::Part is used from the source code is difficult, because I didn’t yet find it instantiated anywhere (except in ui/fileprinterpreview.cpp).

Okular::Part has a whole bunch of of methods, many of them interfacing D-Bus. api.kde.org doesn’t cover expanding Parts with additional methods, the tutorial only covers using existing Parts. I also have looked at the D-Bus tutorials (still for KDE4), but don’t know yet how important it is for Parts.

I don’t want to write documentation for the class Okular::Part, before I understand KParts well. Neither for it’s members, because I fear that could be redundant or irritating.

I will look at other projects using KParts. But could you hint me where Okular::Part is instantiated?

part.h
170

Oh, right. Changed that.

aacid added a comment.May 20 2019, 4:33 PM

The part is created by the shell, see around line 85.

The basic idea is that shells and parts don't each other (in this particular case the shell is "cheating" a bit), and just use the query system. For example, ark will say, can i get a part that knows how to show PDF files? And then use that to show you a preview of the file.

Now since our shell cheats because it knows specifically it'll get the okular part, it can cast to stuff like okular viewer interface.

Basically parts are a plug-in system that can be used in random shells.

Does that make sense?

davidhurka updated this revision to Diff 58507.May 22 2019, 6:48 PM
  • Part::Part() 'QObject is a widget' -> 'QObject is an object'
  • Document some D-Bus methods and Part signals, minor improvements in Document
davidhurka marked 2 inline comments as done.May 22 2019, 7:02 PM

Probably it should be described how dropped URLs are handled in general? Sometimes the Part uses them and wildly opens files, sometimes not.

part.h
208

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

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

273–278

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

318

Why doesn’t Part handle the print action itself?

346

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

davidhurka updated this revision to Diff 58664.May 25 2019, 8:53 PM

Did exactly this (and some more)

  • Improve code documentation of updateViewActions() and showMenu()
  • Document random member functions of Part, including showMenu()
  • Document more random member functions of Part, including openUrl(), and improve thats code comments
  • Document even more open*(url) functions, and slotPrint(). Also complain about source code.
  • Remove TODOs (but not all)

Giving up for today, there are sooooo many functions of the format open*(url).

You can now look at my changes and comments and so on. If some of my questions contratict themselves (like questions normally do...), feel free to ask. ;)

I will make a list of open*(url) functions soon, they definitely need a call graph before I understand them.

core/document.h
1280

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.

part.cpp
1381

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

1428

So, swapInsteadOfOpening does not work with encrypted files?

1428

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

1440

This if is effectively executing before the while loop.

1491

And this if is effectively executing after the while loop.

1568

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.

1581–1585

I’d like to rename m_topMessage -> m_embeddedFilesMessage.

1653

Uncompressed files have un-real URLs?

1742

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

1746

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

3072

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

3156

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

3356

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

3363

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.

part.h
169–171

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?

414–420

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?

430

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

468

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

637

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

653

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

801

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.

804

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

Yes, the language is not final.

I’m complaining a lot about the source code. Don’t understand that as arrogant demand to fix it, just tell me to do it. Anyway, the code works, so no need to change it?

Honestly i don't think documenting things at this low level makes sense, it'll break because people will never remember to update comments even if it's just on the line above the code they are changing.

But anyway if some day i find time to spare i may try to answer your questions.

If you say “at this low level”, would being more abstract make more sense?

The documentation of e. g. showMenu() is really low level now, and people are lazy and break it up. If it just explains when it should be called, it should work fine for some time, because people are too lazy to change how it is called, and the purpose keeps the same.

Wrote that I give up for today, so will stop thinking about it now.

davidhurka updated this revision to Diff 58857.May 29 2019, 4:41 PM
  • Add structure to open*(url) methods documentation, shown in Detailed Description of Part
  • Make showMenu() documentation more generic.

Got an overview of open-a-document methods in the Detailed Description of Part. Helped me a lot, can it stay there?

I made some method descriptions a bit more robust to future changes (I think) by making them more abstact and focusing a bit more on the intention.

Also got a thought that the class documentation should deal with the intentions, and less with the implementation. Thanks for your critics, Albert.

part.h
191

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

734–747

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

774–798

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

781

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.

801

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.

804

Should be OK now, also improved language.

davidhurka retitled this revision from [RFC] Write a bit Documentation for Part and PageView to [RFC] Write Documentation for Okular::Part.Jun 2 2019, 6:33 PM
davidhurka edited the summary of this revision. (Show Details)

@jucato This could potentially interest you ;)

aacid added a comment.Jun 2 2019, 9:08 PM

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.

part.h
120

Do we really need to document a 1 line function?

123

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

125

original seems a strange word to use here

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.

191

the parameter wins.

734–747

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.

774–798

good point, it's a pretty terrible name

781

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

801

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

Are you looking at the wrong place?

davidhurka marked 6 inline comments as done.Jun 2 2019, 10:04 PM

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

part.cpp
1746

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

3363

Print Selection will print all selected pages.

part.h
120

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)

123

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.

125

Ok, will change that.

130

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.

346

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

414–420

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.

430

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

468

Will accept and ignore that for now.

653

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

781

If that is intended, ok.

801

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 retitled this revision from [RFC] Write Documentation for Okular::Part to [WIP] Write Documentation for Okular::Part.Jun 2 2019, 10:09 PM
davidhurka edited the summary of this revision. (Show Details)
aacid added inline comments.Jun 22 2019, 1:43 PM
part.h
120

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

davidhurka marked 4 inline comments as done.Jun 22 2019, 3:59 PM
davidhurka added inline comments.
part.h
120

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?

davidhurka marked 19 inline comments as done.Jun 22 2019, 4:46 PM
davidhurka updated this revision to Diff 60380.Jun 22 2019, 4:49 PM

Left some changes lying arround, here they are.
Rename m_topMessage, improve callflow description, improve fitWindowToPage description.