<table><tr><td style="">leinir requested changes to this revision.<br />leinir added a comment.<br />This revision now requires changes to proceed.
</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/D15775">View Revision</a></tr></table><br /><div><div><p>Looks pretty good to me. It'd be nice if we could avoid exposing DocumentImpl (it's supposed to be sort of hidden), if you could have a think on a way to avoid that, i'd appreciate it :)</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/D15775#inline-85483">View Inline</a><span style="color: #4b4d51; font-weight: bold;">View.cpp:139</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: #aa4000">if</span> <span class="p">(</span><span class="n">pageCache</span><span class="p">()</span> <span style="color: #aa2211">==</span> <span class="n">withCache</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #aa4000">return</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">i'm always very uncomfortable with introducing returns inside statements... i'd very much like if you could refactor this so it doesn't just return like this (perhaps merge the two if statements, something like <tt style="background: #ebebeb; font-size: 13px;">if (pageCache() != withCache && d->document && d->document->impl())</tt>?). i know it's semantically identical, but maintainability of early returns is just... super awkward ;)</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/D15775#inline-85486">View Inline</a><span style="color: #4b4d51; font-weight: bold;">KWCanvasBase.h:106</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 class="n">virtual</span> <span style="color: #aa4000">void</span> <span class="n">setCacheEnabled</span><span class="p">(</span><span style="color: #aa4000">bool</span> <span class="n">enabled</span><span class="p">,</span> <span style="color: #aa4000">int</span> <span class="n">cacheSize</span> <span style="color: #aa2211">=</span> <span style="color: #601200">50</span><span class="p">,</span> <span class="n">qreal</span> <span class="n">maxZoom</span> <span style="color: #aa2211">=</span> <span style="color: #601200">2.0</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">bool</span> <span style="color: #004012">isCacheEnabled</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;">It'd be nice to have docs on new public APIs, please :) i realise it's trivial and "just right there", but the api docs page doesn't show stuff quite so conveniently without a bit of help ;)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R8 Calligra</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D15775">https://phabricator.kde.org/D15775</a></div></div><br /><div><strong>To: </strong>dcaliste, leinir, danders, anthonyfieroni, Calligra: 3.0<br /><strong>Cc: </strong>boemann, Calligra-Devel-list, dcaliste, cochise, vandenoever<br /></div>