D21281: [RFC] Write a bit Documentation for Part and PageView

[David Hurka]

davidhurka added a comment.


  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.

INLINE COMMENTS

> document.h:1280
>           */
>          DocumentViewport( const QString &description );
>  

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
>          *isCompressedFile = true;
>          uncompressOk = handleCompressed( fileNameToOpen, localFilePath(), compressionType );
>          mime = db.mimeTypeForFile( fileNameToOpen );

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

> part.cpp:1428
>  #ifdef WITH_KWALLET
>          // if the file didn't open correctly it might be encrypted, so ask for a pass
>          QString walletName, walletFolder, walletKey;

So, `swapInsteadOfOpening` does not work with encrypted files?

> part.cpp:1428
>  #ifdef WITH_KWALLET
>          // if the file didn't open correctly it might be encrypted, so ask for a pass
>          QString walletName, walletFolder, walletKey;

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

> part.cpp:1440
>              // 1.A. try to retrieve the first password from the kde wallet system
>              if ( !triedWallet && !walletKey.isNull() )
>              {

This `if` is effectively executing //before// the while loop.

> part.cpp:1491
>  
>              if ( openResult == Document::OpenSuccess )
>              {

And this `if` is effectively executing //after// the while loop.

> part.cpp:1568
> +    bool canSearch = m_document->supportsSearching();
>      const bool ok = openResult == Document::OpenSuccess;
>      emit enableCloseAction( ok );

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.

> part.cpp:1583
> +    // Show a message if embedded files are present.
>      m_topMessage->setVisible( hasEmbeddedFiles && Okular::Settings::showOSD() );
> +

I’d like to rename m_topMessage -> m_embeddedFilesMessage.

> part.cpp:1653
>          {
>              m_realUrl = url();
>          }

Uncompressed files have un-real URLs?

> part.cpp:1742
>       * to read it */
>      m_swapInsteadOfOpening = swapInsteadOfOpening;
>  

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

> part.cpp:1746
>      // We want to save them and restore them later.
>      const KParts::OpenUrlArguments args = arguments();
>  

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

> part.cpp:3063
> +    // The existing actions are expected in some other KXMLGuiClient.
>      if (!m_actionsSearched)
>      {

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

> part.cpp:3147
>                  else
>                      m_document->bookmarkManager()->addBookmark( page->number() );
>              }

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

> part.cpp:3347
>  
>      if ( printDialog )
>      {

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

> part.cpp:3354
>  
>          // If the user has bookmarked pages for printing, then enable Selection
>          if ( !m_document->bookmarkedPageRange().isEmpty() )

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:123
>          /**
> -         * If one element of 'args' contains one of the strings "Print/Preview" or "ViewerWidget",
> +         * @param parentWidget TODO
> +         * @param parent The parent object.

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?

> part.h:329
>      protected Q_SLOTS:
>          // connected to actions
> +

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?

> part.h:345
> +         * @warning
> +         * This will not notify the parent application about the URL change.
> +         */

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

> part.h:386
> +         *
> +         * This slot must only be triggered by actions, which hold a viewport as data.
> +         * The bookmark is determined using this data.

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

> part.h:513
> +        /**
> +         * Maybe hides the incremental find bar.
> +         * Find bar is not hidden if a search is ongoing.

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

> part.h:529
> +
> +        // can be connected to widget elements 
> +

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

> part.h:644
> +
>          bool eventFilter(QObject * watched, QEvent * event) override;
> +

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.

> part.h:647
> +        /**
> +         * Tries to open a file.
> +         *

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

Yes, the language is not final.

REPOSITORY
  R223 Okular

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

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/20190525/850282ff/attachment-0001.html>