Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > <https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256>
> >
> >     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.
> >     
> >     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.
> >     
> >     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?
> 
> René J.V. Bertin wrote:
>     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.
>     
>     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 `ShowIconsOnButtons`?

The "problem" is that this is really scattered around everywhere :(

One major catch should be (frameworksintegration)

```
diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp b/src/platformtheme/kdeplatformfiledialoghelper.cpp
index 11e7efb..9cd374c 100644
--- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
+++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
@@ -161,6 +161,11 @@ void KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
         m_fileWidget->setMode(KFile::File);
         break;
     }
+    // ::setOperationMode happily adds icons to our buttonbox, so we clear them everytime the mode is set
+    if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, nullptr, this)) {
+        foreach (QAbstractButton *button, m_buttons->buttons())
+            button->setIcon(QIcon());
+    }
 }
 
 void KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, const QString &text)
diff --git a/src/platformtheme/kdirselectdialog.cpp b/src/platformtheme/kdirselectdialog.cpp
index 0a65cd3..13d7dc7 100644
--- a/src/platformtheme/kdirselectdialog.cpp
+++ b/src/platformtheme/kdirselectdialog.cpp
@@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl &startDir, bool localOnly, QWidget
     m_buttons->setStandardButtons(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
     connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
     connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
+    if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, nullptr, this)) {
+        foreach (QAbstractButton *button, m_buttons->buttons())
+            button->setIcon(QIcon());
+    }
     topLayout->addWidget(m_buttons);
 
     QHBoxLayout *hlay = new QHBoxLayout(page);
```

But that somehow doesn't scale.

KGuiItem::apply would have to catch that, but doesn't.
QDialogButtonBox::addButton *could* 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.


=> 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.


==> QPushButton will require a "fix" to handle the StyleHint, but we cannot expect *Q*PushButton to honor some global KDE setting, that's really a job for the platform integration and in this case ultimately the style.


=> 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)


Ideally™ there would be an official "Qt::AA_PushButtonTextOnly" (rename me) flag for the QPA to set and QPushButtons to pick.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89737
-----------------------------------------------------------


On Dec. 11, 2015, 4:26 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126308/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2015, 4:26 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, Hugo Pereira Da Costa, and Yichao Yu.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> -------
> 
> 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.
> 
> 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 `KPushButton::paintEvent`, by removing the icon (forcing it to the null icon) in the option instance, before handing off control to the painter.
> 
> 
> Diffs
> -----
> 
>   src/kdeui/kdialogbuttonbox.cpp 0f6649b 
>   src/kdeui/kpushbutton.cpp 98534fa 
> 
> Diff: https://git.reviewboard.kde.org/r/126308/diff/
> 
> 
> Testing
> -------
> 
> On Kubuntu 14.04 and OS X 10.9.5 with Qt 5.5.1 and KF5 frameworks 5.16.0 .
> 
> I have not yet verified if there are other classes where this modification would be relevant too.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20151221/f4862f74/attachment.htm>