<table><tr><td style="">dhaumann 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/D7163" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>I think this is already a very good patch. I just have some minor comments. Could you have another look?</p>
<p>By the way: Given you have more patches that build on this one, you should consider applying for a KDE developer account, if you don't have one yet, see: <a href="https://community.kde.org/Infrastructure/Get_a_Developer_Account" class="remarkup-link" target="_blank" rel="noreferrer">https://community.kde.org/Infrastructure/Get_a_Developer_Account</a></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/D7163#inline-29184" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ksqueezedtextlabelautotest.cpp:39</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">void</span> <span class="n">KSqueezedTextLabelAutotest</span><span style="color: #aa2211">::</span><span class="n">squeezeLabel</span><span class="p">(</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">KSqueezedTextLabel</span> <span style="color: #aa2211">*</span><span class="n">label</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span style="color: #aa4000">int</span> <span class="n">amount</span> <span style="color: #aa2211">=</span> <span style="color: #601200">1</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;">To me, it was not immediately clear whether amount referts to pixel or to characters. So you may want to rename the parameter to pixelAmount or pxAmout or amountInPixels, or maybe even simply "pixels".</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/D7163#inline-29182" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ksqueezedtextlabelautotest.cpp:46</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">KSqueezedTextLabel</span> <span style="color: #aa2211">*</span><span class="n">label</span> <span style="color: #aa2211">=</span> <span class="n">createLabel</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">""</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Use QString() instead of QStringLiteral("") for empty strings.</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/D7163#inline-29185" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ksqueezedtextlabelautotest.cpp:140</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">KSqueezedTextLabel</span> <span style="color: #aa2211">*</span><span class="n">label</span> <span style="color: #aa2211">=</span> <span class="n">createLabel</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">""</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Same here: Prefer QString() over QStringLiteral("")</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/D7163#inline-29186" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ksqueezedtextlabelautotest.cpp:217-234</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">void</span> <span class="p">(</span><span class="n">KSqueezedTextLabel</span><span style="color: #aa2211">::*</span><span class="n">attributeFn</span><span class="p">)(</span><span style="color: #aa4000">int</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// TODO: Does Q_DECLARE_METATYPE support pointers to member functions?</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Then the switch statement and the enum could be removed.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">switch</span><span class="p">(</span><span class="n">attribute</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">case</span> <span style="color: #a0a000">setIndent</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">attributeFn</span> <span style="color: #aa2211">=</span> <span style="color: #aa2211">&</span><span class="n">KSqueezedTextLabel</span><span style="color: #aa2211">::</span><span class="n">setIndent</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">margin, indent, and lineWidth are all Q_PROPERTYs, maybe you can use QObject::setProperty(const char *, QVariant) to set the values instead of using a function pointer? The function pointer is a bit ugly...</p>
<p style="padding: 0; margin: 8px;">Example:<br />
label->setProperty("indent", 40); // will automatically call label->setIndent(40);</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/D7163#inline-29183" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ksqueezedtextlabelautotest.h:35-36</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: #a0a000">private</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">KSqueezedTextLabel</span><span style="color: #aa2211">*</span> <span class="n">createLabel</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">text</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: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">void</span> <span style="color: #004012">squeezeLabel</span><span class="p">(</span><span class="n">KSqueezedTextLabel</span> <span style="color: #aa2211">*</span><span class="n">label</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span style="color: #aa4000">int</span> <span class="n">amount</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;">Given you don't use any member variables, these could also be free functions in e.g. an anonymous namespace in the .cpp file. This is just a comment though, no strict requirement. I guess it is fine as is :-)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R236 KWidgetsAddons</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7163" rel="noreferrer">https://phabricator.kde.org/D7163</a></div></div><br /><div><strong>To: </strong>rkflx, Frameworks<br /><strong>Cc: </strong>dhaumann<br /></div>