<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 19th, 2015, 10:23 a.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256" style="color: black; font-weight: bold; text-decoration: underline;">src/kdeui/kpushbutton.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void KPushButton::paintEvent(QPaintEvent *)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">256</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">256</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">text</span><span class="p">().</span><span class="n">isEmpty</span><span class="p">()</span> <span class="o">&&</span> <span class="o">!</span><span class="n">style</span><span class="p">()</span><span class="o">-></span><span class="n">styleHint</span><span class="p">(</span><span class="n">QStyle</span><span class="o">::</span><span class="n">SH_DialogButtonBox_ButtonsHaveIcons</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="k">this</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<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;">This patch looks wrong because KPushButton can be used outside of "dialog button boxes", while the styleHint is supposed to be only about dialog button boxes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QPushButton::sizeHint does this:
bool showButtonBoxIcons = qobject_cast<QDialogButtonBox*>(parentWidget())
&& style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
which is a solution for testing the parent widget.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I still don't fully understand the issue though, at painting time both QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why these two would be working differently.
Is this a workaround for KPushButton which doesn't fix QPushButton?</p></pre>
</blockquote>
<p>On December 19th, 2015, 11:05 a.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;">David, I'm dropping this whole RR, leaving it open only in case Thomas wants to pursue his view on fixing what there is to fix.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You are right though, it's a workaround for KPushButton which doesn't depend on fixing QPushButton. And whether or not QPushButton requires fixing is apparently the big question. What is your idea on the scope of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ShowIconsOnButtons</code>?</p></pre>
</blockquote>
<p>On December 21st, 2015, 1:49 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The "problem" is that this is really scattered around everywhere :(</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One major catch should be (frameworksintegration)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #000080; font-weight: bold">diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp b/src/platformtheme/kdeplatformfiledialoghelper.cpp</span>
<span style="color: #000080; font-weight: bold">index 11e7efb..9cd374c 100644</span>
<span style="color: #A00000">--- a/src/platformtheme/kdeplatformfiledialoghelper.cpp</span>
<span style="color: #00A000">+++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -161,6 +161,11 @@ void KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)</span>
m_fileWidget->setMode(KFile::File);
break;
}
<span style="color: #00A000">+ // ::setOperationMode happily adds icons to our buttonbox, so we clear them everytime the mode is set</span>
<span style="color: #00A000">+ if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, nullptr, this)) {</span>
<span style="color: #00A000">+ foreach (QAbstractButton *button, m_buttons->buttons())</span>
<span style="color: #00A000">+ button->setIcon(QIcon());</span>
<span style="color: #00A000">+ }</span>
}
void KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, const QString &text)
<span style="color: #000080; font-weight: bold">diff --git a/src/platformtheme/kdirselectdialog.cpp b/src/platformtheme/kdirselectdialog.cpp</span>
<span style="color: #000080; font-weight: bold">index 0a65cd3..13d7dc7 100644</span>
<span style="color: #A00000">--- a/src/platformtheme/kdirselectdialog.cpp</span>
<span style="color: #00A000">+++ b/src/platformtheme/kdirselectdialog.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl &startDir, bool localOnly, QWidget</span>
m_buttons->setStandardButtons(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
<span style="color: #00A000">+ if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, nullptr, this)) {</span>
<span style="color: #00A000">+ foreach (QAbstractButton *button, m_buttons->buttons())</span>
<span style="color: #00A000">+ button->setIcon(QIcon());</span>
<span style="color: #00A000">+ }</span>
topLayout->addWidget(m_buttons);
QHBoxLayout *hlay = new QHBoxLayout(page);
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But that somehow doesn't scale.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KGuiItem::apply would have to catch that, but doesn't.
QDialogButtonBox::addButton <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">could</em> catch things, but doesn't (also it's not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, so QPushButton::setIcon() would have to catch it and QPushButton would have to catch it on reparenting.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">=> For a complete solution, QPushButton actually needs painting code to check the parent & style hint when setting up the style option - or we simply rely on the style testing (widget && qobject_cast<QDialogButtonBox*>(widget->parentWidget())) when calculating sizes and painting the button.
The only alternative is to walk a long way and tell everyone to please check the hint and fix their buttonboxes.... eewww.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">==> QPushButton will require a "fix" to handle the StyleHint, but we cannot expect <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Q</em>PushButton to honor some global KDE setting, that's really a job for the platform integration and in this case ultimately the style.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">=> the platform integration plugin (KDE part in all Qt applications) needs to read that setting and expose it to the styles.
Alternatively the styles could read the setting from kdeglobals directly, but that requires them to link kconfig (to do it correctly, just grabbing it from the users kdeglobals ini isn't that much of a problem ;-), ie. rules out Qt-only styles (and somehow contrasts w/ the KF5 approach to things)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ideally™ there would be an official "Qt::AA_PushButtonTextOnly" (rename me) flag for the QPA to set and QPushButtons to pick.</p></pre>
</blockquote>
<p>On December 21st, 2015, 2:06 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;">Hear hear ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have raised that final question in the Qt bug report I filed on this (https://bugreports.qt.io/browse/QTBUG-49887)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe the best approach is to ensure that for now ShowIconsOnButtons is handled the best way possible in the main current KDE styles (Breeze, Oxygen, QtCurve) and then wait to see what position Qt adopts before trying to implement something better (= less CPU intensive)?</p></pre>
</blockquote>
<p>On December 21st, 2015, 3:18 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What "hear hear" - I hardly changed my opinion, just figured that the hint is ignored all over KDE (client) code on migration from KDialogButtonBox to QDialogButtonBox and in other places and thus considered we -unfortunately- will best try to "fix" that (broken client code) in QPushButton. I had simply underestimated that this is not a bug in KDialogButtonBox only, but broken all over the place.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please nota bene that QPushButton is /not/ broken and does /not/ need to be fixed. One would request it to catch and cover broken client code. They may very well just respond "why not simply fix your shit?"</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;">Maybe the best approach is to ensure that for now ShowIconsOnButtons is handled [...] see what position Qt adopts</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In that case I'd propose to PoC a working infrastructure (QPA reads setting and sets a dynamic property on the QApplication instance, best before the style is constructed, so that finally the style(s) adopt that) and then ask "can we please make this a canonical and documented application attribute?"</p></pre>
</blockquote>
<p>On December 21st, 2015, 4:25 p.m. UTC, <b>Hugo Pereira Da Costa</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;">Hi,
Jumping in.
I have played with honouring ShowIconsOnButtons in Breeze style directly (on a local branch)
Now: it is easy to get the icon not being shown, but the buttons are still "large", as if they would contain the icon. The reason is that, as already pointed by René, QPushButton::SizeHint already include the size of the icon to be drawn in the size it calculates and passes to QStyle::sizeFromContents.
So that you would need to either "undo" the size calculated by QPushButton in the sizeHint method (which sounds quite fragile I think), or indeed, patch QPushButton in some way ...<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Comments welcome (especially if I missed something)</p></pre>
</blockquote>
<p>On December 21st, 2015, 4:44 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is the unconditional patch for virtuality - it does indeed this (I had similar for bespin, I really dislike those icons myself ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There's no other way since the label is handled opaquely (is that a word? ;-)
The contents size is text+icon size, any padding or margin needs to be added by the style (so you actually should be testing for whether there's an icon anyway ... ;-P )</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">```
diff --git a/buttons.cpp b/buttons.cpp
index 78422e7..b3f433f 100644
--- a/buttons.cpp
+++ b/buttons.cpp
@@ -261,9 +261,7 @@ Style::drawPushButtonLabel(const QStyleOption <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">option, QPainter </em>painter, const
}
drawItemText(painter, ir, Qt::AlignCenter | BESPIN_MNEMONIC, PAL, isEnabled, btn->text, QPalette::NoRole, &ir);
RESTORE_PAINTER
- }
-
- if (!btn->icon.isNull()) { // The ICON ================================================
+ } else if (!btn->icon.isNull()) { // The ICON ================================================
QIcon::Mode mode = isEnabled ? QIcon::Normal : QIcon::Disabled;
if (mode == QIcon::Normal && hasFocus)
mode = QIcon::Active;
diff --git a/sizefromcontents.cpp b/sizefromcontents.cpp
index 2cee67f..06ec571 100644
--- a/sizefromcontents.cpp
+++ b/sizefromcontents.cpp
@@ -156,7 +156,8 @@ Style::sizeFromContents(ContentsType ct, const QStyleOption *option, const QSize
} else {
w += h + F(4);
if (!btn->icon.isNull())
- w += F(4) + btn->iconSize.width(); // we want symmetry and need 2px padding (+the blind icon and it's blind padding)
+ w -= btn->iconSize.width();
+// w += F(4) + btn->iconSize.width(); // we want symmetry and need 2px padding (+the blind icon and it's blind padding)
// if (w < F(80))
// w = F(80);
}</p></pre>
</blockquote>
<p>On December 21st, 2015, 6:15 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;"><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;">What "hear hear" - I hardly changed my opinion</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Wasn't what I meant, just that you too now realise there isn't a single easy place to fix this.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Hugo: yes, QPushButton includes the icon size in button size calculation, but I'm pretty sure there's a bug in that code. That's actually what the bug report referenced above was about initially.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What's curious here is that buttons don't make the impression of being too large on OS X. Then again, everything in standard Qt interfaces makes that impression on OS X, so it's quite likely the extra size for icons is drowned in the rest over the oversizedness.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">It's not a bug, the icon (if present) is part of the content and requires space. Since there's only one ContentsType in QStyle, it correctly contains combined sizes of icon and text (in width)
If you don't want the icon, you need to remove it from the contents size for the returned size, that's how it works =)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I had really not imagined how deeply broken this was - mostly due to invocation of KGuiItem, altering the standard buttons everywhere :-(</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No impact on the "no superfluous icons at all please" stance - that must and can be (fairly easily, see patch) covered by the style.
The one and only "tricky" thing is to get the users wish down to the style.</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On December 11th, 2015, 4:26 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, Hugo Pereira Da Costa, and Yichao Yu.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Dec. 11, 2015, 4:26 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/kdialogbuttonbox.cpp <span style="color: grey">(0f6649b)</span></li>
<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>