<table><tr><td style="">sebas requested changes to this revision.<br />sebas 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/D4689" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>I think a problem with using roundToIconSize as isolated property is that it really isn't. It has intended effects on the sizing of the item, but the current version of the patch doesn't reflect that. I think it needs to trigger a series of size change signals. See my comments inline.</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/D4689#inline-19358" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iconitemtest.cpp:524</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: #d0ffd0;">    <span class="n">item</span><span style="color: #aa2211">-></span><span class="n">setProperty</span><span class="p">(</span><span style="color: #766510">"roundToIconSize"</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;">Just checking the values here is not enough, the property change results in changes in paintedWidth, but we're currently not signalling that these props have changed. See also my comment below.</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/D4689#inline-19357" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iconitem.cpp:313</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: #d0ffd0;"><span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #aa4000">void</span> <span class="n">IconItem</span><span style="color: #aa2211">::</span><span class="n">setRoundToIconSize</span><span class="p">(</span><span style="color: #aa4000">bool</span> <span class="n">roundToIconSize</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Changing roundToIconSize may change the size of the icon, so should we trigger size changes (paintedWidth / paintedHeight / boundingRect likely? Perhaps others.) and re-rendering here as well? Right now, judging from the code, changing this property mid-flight is broken. We may even have to go as far as loading a new pixmap (in loadPixmap() from a quick glance).</p>

<p style="padding: 0; margin: 8px;">Please also add unit tests for the effects on the iconitem's size properties.</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/D4689#inline-19355" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iconitem.h:147</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: #d0ffd0;">    <span style="color: #aa4000">bool</span> <span style="color: #004012">isRoundingToIconSize</span><span class="p">()</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">    <span style="color: #aa4000">void</span> <span style="color: #004012">setRoundToIconSize</span><span class="p">(</span><span style="color: #aa4000">bool</span> <span class="n">roundToIconSize</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">bool roundToIconSize() const;</p>

<p style="padding: 0; margin: 8px;">we generally don't use "is" in new code where we can avoid it, and the ing makes this even more inconsistent. I'd much prefer the above suggestion.</p>

<p style="padding: 0; margin: 8px;">Please also add api docs, at least for new code.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R242 Plasma Framework (Library)</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D4689" rel="noreferrer">https://phabricator.kde.org/D4689</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>drosca, Plasma, sebas<br /><strong>Cc: </strong>sebas, mart, davidedmundson, plasma-devel, Frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol<br /></div>