<table><tr><td style="">sitter 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/D29526">View Revision</a></tr></table><br /><div><div><p>You do seem to calculate the deviceWidth and height an awful lot, it makes reading a bit clunky. I'd much rather have the multiplication done once and then always use the var instead. In fact, perhaps it'd make sense to have createV3 feed the values into the implementations? Currently they all repate the same two lines over and over.</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/D29526#inline-169992">View Inline</a><span style="color: #4b4d51; font-weight: bold;">djvucreator.cpp:61</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">QByteArray</span> <span class="n">sizearg</span><span class="p">,</span> <span class="n">fnamearg</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">  <span class="n">sizearg</span> <span style="color: #aa2211">=</span> <span class="n">QByteArray</span><span style="color: #aa2211">::</span><span class="n">number</span><span class="p">(</span><span class="n">width</span><span class="p">)</span> <span style="color: #aa2211">+</span> <span style="color: #766510">'x'</span> <span style="color: #aa2211">+</span> <span class="n">QByteArray</span><span style="color: #aa2211">::</span><span class="n">number</span><span class="p">(</span><span class="n">height</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">  <span class="n">sizearg</span> <span style="color: #aa2211">=</span> <span class="n">QByteArray</span><span style="color: #aa2211">::</span><span class="n">number</span><span class="p">(</span><span class="n">width<span class="bright"></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">devicePixelRatio</span></span><span class="p">)</span> <span style="color: #aa2211">+</span> <span style="color: #766510">'x'</span> <span style="color: #aa2211">+</span> <span class="n">QByteArray</span><span style="color: #aa2211">::</span><span class="n">number</span><span class="p">(</span><span class="n">height<span class="bright"></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">devicePixelRatio</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; ">  <span class="n">fnamearg</span> <span style="color: #aa2211">=</span> <span class="n">QFile</span><span style="color: #aa2211">::</span><span class="n">encodeName</span><span class="p">(</span> <span class="n">path</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Surely multiplication results need converting to int. That being said, you do multiply below again, so maybe just move maxWidth/Height up here.</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/D29526#inline-170009">View Inline</a><span style="color: #4b4d51; font-weight: bold;">thumbnail.cpp:774</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: #aa4000">void</span> <span class="n">ThumbnailProtocol</span><span style="color: #aa2211">::</span><span class="n">scaleDownImage</span><span class="p">(</span><span class="n">QImage</span><span style="color: #aa2211">&</span> <span class="n">img</span><span class="p">,</span> <span style="color: #aa4000">int</span> <span class="n">maxWidth</span><span class="p">,</span> <span style="color: #aa4000">int</span> <span class="n">maxHeight</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Var naming is a bit inconsistent across the code base now. In the implementations there are maxWidth/H that are device-adjusted but in here they are not. Not a huge concern, just noticed.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R320 KIO Extras</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D29526">https://phabricator.kde.org/D29526</a></div></div><br /><div><strong>To: </strong>meven, Frameworks, dfaure, broulik, sitter, ngraham<br /><strong>Cc: </strong>kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov<br /></div>