<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126308/">https://git.reviewboard.kde.org/r/126308/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 10th, 2015, 10:11 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">What tells you that this is a dialog buttonbox pushbutton?</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">What happens if the button has no text?</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The bug is in QDialogButtonBox (or rather the K variant, QDialogButtonBoxPrivate::createButton() seems to incorporate the hint correctly)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was interpreted by KPushButton <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">and</em> kstyle for the hint, but the hint only covers Q'DialogButtonBox'es - there's simply no global rule for this like AA_DontShowIconsInMenus</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">=> KDialogButtonBox shall respect the hint for its buttons (there're two special creation routines).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For the rest, the platform plugin ideally picks the kdeglobals setting and exports it to the application object (dyn property?) where the style can pick it and incorporate it into its calculations (ie. if no icons are wanted and there's text or arrow, omit the icon in size calculation and painting)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Fixing" that in deprecated KPushButton doesn't really fix anything. We'll face the mix we had, just that users of QPushButton were far less prone to pass them an icon in pre-KF5 times.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please also attach Hugo Pereira Da Costa.</p></pre>
</blockquote>
<p>On December 10th, 2015, 10:51 p.m. UTC, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1: why should one care? It is said nowhere that the setting defined as "show icons on buttons" in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">systemsettings(5)</code> applies only to dialogs. Rather, the tooltip says "when this is enabled, KDE applications will show icons alongside [sic!] some important buttons".
In other words, when "this" is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> enabled, there should be no icons, period.
I have found no sign in the code that the ShowIconsOnPushButtons hint is to be used only for dialogs; <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">SH_DialogButtonBox_ButtonsHaveIcons</code> indeed carries the suggestion in its name but I would not be surprised if Qt really thinks of it in a more general sense. Probably also because the notion of what a dialog is has become a lot vaguer.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And that also happens to be what Qt does; buttons show their icons on Linux (and other Unix variants?) but on OS X or MS Windows displaying of those icons is deactivated unless you use a style that enables it. In fact, the default setting for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">SH_DialogButtonBox_ButtonsHaveIcons</code> is false except in the generic Unix theme (= it does work globally like <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">AA_DontShowIconsInMenus</code> everywhere else).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">2: a user who indicates s/he doesn't want to see icons will get an empty button. That's also what can happen with QPushButton, and app developers have to take this into consideration. Cf. toolbars (and Qt's position on the use of "texted separators").
I don't think I've ever come across a standard button showing only an icon, except possibly the arrow button next to the progress indicators in KMail and KDevelop.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to fixing it here: as it turns out, "here" is the main source for annoying icons rearing their silly heads on buttons on my screen ;) and it was also something of a challenge to understand why they kept appearing despite the fact that all code appeared to return the value of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ShowIconsOnPushButtons</code>. Deprecated or not, it doesn't look like all applications are going to stop using it anytime soon.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I looked into fixing the issue in KDialogButtonBox but saw that it does nothing to override the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ShowIconsOnPushButtons</code> setting. The only way to respect the setting through that class (or a modern equivalent) would be to set an empty icon if <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ShowIconsOnPushButtons=false</code>. That introduces another regression: changes in this setting are supposed to be reflected by running applications without requiring a restart (or a recreation of dialogs). If it were just me I'd decree that buttons can have either text or an icon, but right now we have to make do with this mixed situation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't mind making this an OS X (and MS Windows) specific modification, of course, but on those platforms</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1: why should one care?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Because, as explained, that is what the hint says. Nothing else.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have found no sign in the code that the ShowIconsOnPushButtons hint is to be used only for dialogs</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">AND</em> KPushButton.
ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but NOT vv.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The approach is wrong, since you're abusing the hint for generalisation.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">but on OS X or MS Windows</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">... Qt uses native elements which might simply globally downforce the pushbutton icon nonsense (as could any style - I was more than once close to doing that in virtuality)
Eg. Breeze might do that on favor of the HIG, but that's not relevant here.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Downforcing in KPushButton means to operate on legacy code only and fixes nothing.
Downforcing in a style (only) is the styles choice only.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We simply need to ensure to
a) respect SH_DialogButtonBox_ButtonsHaveIcons in KDialogButtonBox
b) forward the offered variable to a position where the style can pick it. (As alternative, the style can read it directly, but that "requires" it to link KDE in order to <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">correctly</em> read kdeglobals)</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">in KDialogButtonBox but saw that it does nothing to override the ShowIconsOnPushButtons setting</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Err... what? The problem will be in "KDialogButtonBox::addButton(const KGuiItem &guiitem, .)", you need to get rid of the icon after KGuiItem::assign() if SH_DialogButtonBox_ButtonsHaveIcons is false (for the other function, Qt should do correctly)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And to repeat myself: that has <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">nothing</em> to do with ShowIconsOnPushButtons - you can ignore that value in this position, it feeds the stylehint via the platform plugin, but in this position <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">only</em> the stylehint matters.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">ShowIconsOnPushButtons is orthogonal and supposed to cover <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">all</em> Q/KPushButtons and must therefore be forwarded to and caught by the style as suggested (or by any equivalent implementation)</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On December 10th, 2015, 10:54 p.m. UTC, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo Pereira Da Costa.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Dec. 10, 2015, 10:54 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs4support
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KF5 applications have long had a habit of drawing icons on buttons even when this feature was turned off in the user's setting. This was mostly noticeable in applications built on kdelibs4support.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It seems that the actual culprit is in Qt's QPushButton implementation (https://bugreports.qt.io/browse/QTBUG-49887), but it is possible to work around it in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KPushButton::paintEvent</code>, by removing the icon (forcing it to the null icon) in the option instance, before handing off control to the painter.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On Kubuntu 14.04 and OS X 10.9.5 with Qt 5.5.1 and KF5 frameworks 5.16.0 .</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have not yet verified if there are other classes where this modification would be relevant too.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/kdeui/kpushbutton.cpp <span style="color: grey">(98534fa)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126308/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>