> On Dec. 19, 2015, 11: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)
>
> Thomas Lübking wrote:
> 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);
> }
>
> René J.V. Bertin wrote:
> > What "hear hear" - I hardly changed my opinion
>
> Wasn't what I meant, just that you too now realise there isn't a single
> easy place to fix this.
>
> @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.
>
> 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.
>
> Thomas Lübking wrote:
> 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 =)
>
>
> Yes, I had really not imagined how deeply broken this was - mostly due to
> invocation of KGuiItem, altering the standard buttons everywhere :-(
>
>
> 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.
The icon doesn't require space if it isn't supposed to be visible, or else
there's no point in determining `showButtonBoxIcons` in QPushButton::sizeHint()
... and I can certainly not see the reason why it be used in `if
(!icon().isNull() || showButtonBoxIcons)` instead of something like `if
(!icon().isNull() && (showButtonBoxIcons || empty))`.
I just discovered something that I'll need to add to the Qt bug report too. It
is apparently not the OS X ("Cocoa") style which omits drawing icons in QDBBs.
I tweaked my Qt build so that it picks up my XDG icon theme directories and has
a default theme ... and all of a sudden I got buttons in lots of dialog button
boxes. Annoyingly enough not in all applications: KCM modules (displayed
through `kcmshell5` or `systemsettings5`) get buttoned icons, but Kate doesn't
in its dialogs.
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89737
-----------------------------------------------------------
On Dec. 11, 2015, 5: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, 5: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
>
>
