<table><tr><td style="">mwolff 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/D8642" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>I started with a cursory glance at the changes, but the change set is huge, which makes it really hard to review, especially for people who have no extensive prior experience with okular. I think some of my comments could be "solved" by adding a comment here or there, as it seems like they are non-issues but rather arise from misunderstanding or lack of information from my side. Help future people looking at this code by writing some more comments. And consider splitting up such monster commits in the future into smaller patches, one building on the other - if possible. Or is this the case already? I.e. could I instead review a feature branch? I know that phabricator sucks in that regard, but at least it would simplify my life at reviewing, even if I'd read patches on the command line and then add comments here</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/D8642#inline-38987" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">documenttest.cpp:105</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Pretend the user has done the migration</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_document</span><span style="color: #aa2211">-></span><span class="n">docdataMigrationDone</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">can we test the actual migration, too? i.e. instead of pretending, actually do 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/D8642#inline-38988" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">documenttest.cpp:112</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QCOMPARE</span><span class="p">(</span> <span class="n">m_document</span><span style="color: #aa2211">-></span><span class="n">openDocument</span><span class="p">(</span> <span class="n">testFilePath</span><span class="p">,</span> <span class="n">testFileUrl</span><span class="p">,</span> <span class="n">mime</span> <span class="p">),</span> <span class="n">Okular</span><span style="color: #aa2211">::</span><span class="n">Document</span><span style="color: #aa2211">::</span><span class="n">OpenSuccess</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QCOMPARE</span><span class="p">(</span> <span class="n">m_document</span><span style="color: #aa2211">-></span><span class="n">page</span><span class="p">(</span> <span style="color: #601200">0</span> <span class="p">)</span><span style="color: #aa2211">-></span><span class="n">annotations</span><span class="p">().</span><span class="n">size</span><span class="p">(),</span> <span style="color: #601200">0</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QCOMPARE</span><span class="p">(</span> <span class="n">m_document</span><span style="color: #aa2211">-></span><span class="n">isDocdataMigrationNeeded</span><span class="p">(),</span> <span style="color: #304a96">false</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">shouldn't this have an <tt style="background: #ebebeb; font-size: 13px;">QEXECTED_FAIL</tt>, considering that in principle the annotations should have been migrated?</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/D8642#inline-38989" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">parttest.cpp:827</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">closeDialogHelper</span><span class="p">.</span><span class="n">reset</span><span class="p">(</span><span style="color: #aa4000">new</span> <span class="n">CloseDialogHelper</span><span class="p">(</span> <span style="color: #aa2211">&</span><span class="n">part</span><span class="p">,</span> <span class="n">QDialogButtonBox</span><span style="color: #aa2211">::</span><span class="n">No</span> <span class="p">));</span> <span style="color: #74777d">// this is the "you're going to lose the annotations" dialog</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">this overrides the helper from above, is this desired? do you need a stack of helpers? or do you maybe miss a wait step before (signal spy?) to ensure the previous dialog has been shown and closed?</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/D8642#inline-38990" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">parttest.cpp:854</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// so we want to give her a last chance to save on close with the "you have changes dialog"</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">closeDialogHelper</span><span class="p">.</span><span class="n">reset</span><span class="p">(</span><span style="color: #aa4000">new</span> <span class="n">CloseDialogHelper</span><span class="p">(</span> <span style="color: #aa2211">&</span><span class="n">part</span><span class="p">,</span> <span class="n">QDialogButtonBox</span><span style="color: #aa2211">::</span><span class="n">No</span> <span class="p">));</span> <span style="color: #74777d">// this is the "do you want to save or discard" dialog</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">dito, this again potentially overrides the close helper, no?</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/D8642#inline-38991" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">document.cpp:1162</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">// 2.1. Save page attributes (bookmark state, annotations, ... ) to DOM</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">QDomElement</span></span><span class="bright"> </span><span class="n"><span class="bright">pageList</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">doc</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">createElement</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span class="n"><span class="bright">QStringLiteral</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"pageList"</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">);</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">root</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">appendChild</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span class="n"><span class="bright">pageList</span></span><span class="bright"> </span><span class="p"><span class="bright">);</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// -> do this there are not-yet-migrated annots or forms in docdata/</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span class="n"><span class="bright">m_docdataMigrationNeeded</span></span><span class="bright"> </span><span class="p"><span class="bright">)</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">missing "if"</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/D8642#inline-38992" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">document.cpp:1170</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> <span class="bright"> * annotations,</span> we <span class="bright">save</span> the<span class="bright">m back unmodified in</span> the <span class="bright">original</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> <span class="bright"> * document's metadata, so that it appears that it was not changed */</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">saveWhat</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">|=</span></span><span class="bright"> </span><span class="n"><span class="bright">OriginalA</span>nnotation<span class="bright">PageItems</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// read when</span> we <span class="bright">opened</span> the<span class="bright"> file and ignore any change made by</span> the <span class="bright">user.</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// Since we don't store annotations and forms docdata/ any more, this is</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// necessary to preserve a</span>nnotation<span class="bright">s/forms that previous Okular version</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">missing "in"</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/D8642#inline-38996" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator.h:296</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">virtual</span> <span class="n">SwapBackingFileResult</span> <span style="color: #004012">swapBackingFile</span><span class="p">(</span> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">newFileName</span><span class="p">,</span> <span class="n">QVector</span><span style="color: #aa2211"><</span><span class="n">Okular</span><span style="color: #aa2211">::</span><span class="n">Page</span><span style="color: #aa2211">*></span> <span style="color: #aa2211">&</span> <span class="n">newPagesVector</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">whitespace issue after &</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/D8642#inline-38997" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">page.cpp:502</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">Annotation</span> <span style="color: #aa2211">*</span> <span class="n">Page</span><span style="color: #aa2211">::</span><span class="n">annotation</span><span class="p">(</span> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span> <span class="n">uniqueName</span> <span class="p">)</span> <span style="color: #aa4000">const</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">ws issues like above</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/D8642#inline-38995" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">page.h:256</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Annotation</span> <span style="color: #aa2211">*</span> <span style="color: #004012">annotation</span><span class="p">(</span> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span> <span class="n">uniqueName</span> <span class="p">)</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">whitespace issue after &</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/D8642" rel="noreferrer">https://phabricator.kde.org/D8642</a></div></div><br /><div><strong>To: </strong>aacid, mlaurent, rkflx<br /><strong>Cc: </strong>mwolff, rkflx, lueck, mlaurent, michaelweghorn, ngraham, Okular, aacid<br /></div>