<table><tr><td style="">davidhurka marked 6 inline comments as done.<br />davidhurka added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D21281">View Revision</a></tr></table><br /><div><div><p>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...</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-120258">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidhurka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.cpp:1746</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Seems pointless, because closeUrl() will be called some more times.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">closeUrl() is called through openUrl(), which probably knows what it does to the arguments.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-120263">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidhurka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.cpp:3354</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Print Selection will print all selected pages.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-121095">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aacid</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:120</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Do we really need to document a 1 line function?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">It doesn’t need do be documentation of this function, just describing what will happen to the usual call is enough.</p>
<p style="padding: 0; margin: 8px;">In the documentation of this function, I consider the following approiate:<br />
To open a document, the parent application should call openDocument(*) instead of openUrl(). <span class="phabricator-remarkup-mention-unknown">@see</span> openDocument(const QString &url)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-121096">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aacid</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:123</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Can you explain what you mean with that sentence about reload?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The Reload action (<a href="https://phabricator.kde.org/F5" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F5</a>) 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.</p>
<p style="padding: 0; margin: 8px;">At least that is how I understand it.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-121097">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aacid</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:125</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">original seems a strange word to use here</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ok, will change that.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-121098">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aacid</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:130</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I'm really sorry but i don't see any value in this documentation here.</p>
<p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">Please try to convince me why having this is useful.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 <em>could</em> give an overview. And that is what I consider the purpose of documentation.</p>
<p style="padding: 0; margin: 8px;">People who prever a debugger or the capabilities of an IDE won’t neccessarily need that.</p>
<p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-121105">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aacid</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:780</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Yes, because this is "fit page N to width"</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">If that is intended, ok.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-120264">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidhurka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:329</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Already understand that they will call the bunch of other open*(url) functions, see Okular::Part Detailed Description.</p>
<p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-120266">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidhurka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:345</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Does this make sense? The parent application will loose control about the URL change. (Tried with KDevelop.)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Actually calls openUrl(), which informs the parent application. KDevelop just ignores that. Need to look how exactly that is done.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-120267">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidhurka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:386</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.<br />
This seems overly specific to me, and screws up my documentation.<br />
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.)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Will accept and ignore that for now.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-120269">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidhurka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:529</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Not really an idea what this means. And to which members does it belong?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Belongs to updateViewActions() and enableTOC(bool). Connecting to widgets is not done and makes little sense, so I will remove that line.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-121106">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aacid</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:644</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">It's an event filter, this is a basic way of filtering events in Qt.</p>
<p style="padding: 0; margin: 8px;">Are you looking at the wrong place?</p>
<p style="padding: 0; margin: 8px;"><a href="https://phabricator.kde.org/F6865175" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F6865175: imatge.png</a></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">So it opens a context menu for a menubar menu, almost guessed.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21281#inline-119878">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidhurka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">part.h:271</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">What do <tt style="background: #ebebeb; font-size: 13px;">pageViewPortSize</tt> and <tt style="background: #ebebeb; font-size: 13px;">pageSize</tt> mean? Or why are <em>these</em> two important? I don’t understant the slot in Shell.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">pageViewPortSize is the current viewport size + visible scollbars,<br />
and pageSize is the size of the page + the intended margins to the viewport edges.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R223 Okular</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D21281">https://phabricator.kde.org/D21281</a></div></div><br /><div><strong>To: </strong>davidhurka, Okular<br /><strong>Cc: </strong>ognarb, jucato, aacid, okular-devel, joaonetto, tfella, ngraham, darcyshen<br /></div>