<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<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/D19431">View Revision</a></tr></table><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/D19431#inline-108721">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kwidgetitemdelegatetest.cpp:254</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: #304a96">#else</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #aa4000">int</span> <span class="n">widthUninstall</span> <span style="color: #aa2211">=</span> <span class="n">QApplication</span><span style="color: #aa2211">::</span><span class="n">style</span><span class="p">()</span><span style="color: #aa2211">-></span><span class="n">sizeFromContents</span><span class="p">(</span><span class="n">QStyle</span><span style="color: #aa2211">::</span><span class="n">CT_ToolButton</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">toolButtonOpt</span><span class="p">,</span> <span class="n">QSize</span><span class="p">(</span><span class="n">option</span><span class="p">.</span><span class="n">fontMetrics</span><span class="p">.</span><span class="n">horizontalAdvance</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"Uninstall"</span><span class="p">))</span> <span style="color: #aa2211">+</span> <span class="n">HARDCODED_BORDER</span> <span style="color: #aa2211">*</span> <span style="color: #601200">3</span><span class="p">,</span> <span class="n">option</span><span class="p">.</span><span class="n">fontMetrics</span><span class="p">.</span><span class="n">height</span><span class="p">()),</span> <span class="n">toolButton</span><span class="p">).</span><span class="n">width</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#endif</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think the whole point of the name horizontalAdvance() is that characters can actually be drawn outside of that width. I guess boundingRect().width() is a better port for width() in general.</p>

<p style="padding: 0; margin: 8px;">Heh actually I just read the documentation for width and it agrees with that.</p>

<p style="padding: 0; margin: 8px;">So yes this is a "same behaviour" port, but the whole point of the renaming is to make us realize these things. Sometimes horizontalAdvance is correct (like in kwordwrap.cpp, where they get added together), sometimes boundingRect is what we need, like here when it's to resize something and make sure all the pixels fit.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R276 KItemViews</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D19431">https://phabricator.kde.org/D19431</a></div></div><br /><div><strong>To: </strong>mlaurent, dfaure<br /><strong>Cc: </strong>kde-frameworks-devel, michaelh, ngraham, bruns<br /></div>