> 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`?
>
> Thomas Lübking wrote:
> 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.
>
> René J.V. Bertin wrote:
> Hear hear ...
>
> I have raised that final question in the Qt bug report I filed on this
> (https://bugreports.qt.io/browse/QTBUG-49887)
>
> 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)?
>
> Thomas Lübking wrote:
> 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.
>
> 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?"
>
> > Maybe the best approach is to ensure that for now ShowIconsOnButtons is
> handled [...] see what position Qt adopts
>
> 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?"
>
> Hugo Pereira Da Costa wrote:
> 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 ...
> Comments welcome (especially if I missed something)
This is the unconditional patch for virtuality - it does indeed this (I had
similar for bespin, I really dislike those icons myself ;-)
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 )
```
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 *option,
QPainter *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);
}
- 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
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
