> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> >
> >
> > The bug is in QDialogButtonBox (or rather the K variant,
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint
> > correctly)
> >
> >
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like
> > AA_DontShowIconsInMenus
> >
> > => KDialogButtonBox shall respect the hint for its buttons (there're two
> > special creation routines).
> >
> > 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)
> >
> > "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.
> >
> > Please also attach Hugo Pereira Da Costa.
>
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as
> "show icons on buttons" in `systemsettings(5)` 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 *not* 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; `SH_DialogButtonBox_ButtonsHaveIcons` 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.
>
> 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 `SH_DialogButtonBox_ButtonsHaveIcons` is false except
> in the generic Unix theme (= it does work globally like
> `AA_DontShowIconsInMenus` everywhere else).
>
> 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.
>
> 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
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all
> applications are going to stop using it anytime soon.
>
> I looked into fixing the issue in KDialogButtonBox but saw that it does
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to
> respect the setting through that class (or a modern equivalent) would be to
> set an empty icon if `ShowIconsOnPushButtons=false`. 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.
>
> I don't mind making this an OS X (and MS Windows) specific modification,
> of course, but on those platforms
>
> Thomas Lübking wrote:
> > 1: why should one care?
>
> Because, as explained, that is what the hint says. Nothing else.
>
> > I have found no sign in the code that the ShowIconsOnPushButtons hint
> is to be used only for dialogs
>
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND*
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but
> NOT vv.
>
> The approach is wrong, since you're abusing the hint for generalisation.
>
> > but on OS X or MS Windows
>
> ... 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.
>
> Downforcing in KPushButton means to operate on legacy code only and fixes
> nothing.
> Downforcing in a style (only) is the styles choice only.
>
> 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 *correctly* read kdeglobals)
>
>
> > in KDialogButtonBox but saw that it does nothing to override the
> ShowIconsOnPushButtons setting
>
> 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)
>
> And to repeat myself: that has *nothing* to do with
> ShowIconsOnPushButtons - you can ignore that value in this position, it feeds
> the stylehint via the platform plugin, but in this position *only* the
> stylehint matters.
>
> ShowIconsOnPushButtons is orthogonal and supposed to cover _all_
> Q/KPushButtons and must therefore be forwarded to and caught by the style as
> suggested (or by any equivalent implementation)
>
> René J.V. Bertin wrote:
> With all due respect, this seems like overzealous application of a
> principle that doesn't really apply, making things overly complicated.
>
> So, I was right that *"ShowIconsOnPushButtons is orthogonal and supposed
> to cover all Q/KPushButtons"*. Inside/under a KDE environment, I presume, so
> also for "pure Qt" applications that get to use the KdePlatformPlugin in
> order to blend in?
>
> In that case, it doesn't matter how some central bit of KDE code achieves
> this. *All that counts is the end result that users see*, and that this end
> result is consistent. If they can reasonably expect that the control that
> toggles `ShowIconsOnPushButtons` has an immediate effect on just about all
> buttons, if SH_DialogButtonBox_ButtonsHaveIcons allows to achieve this and if
> Qt itself already uses the parameter in situations outside of a
> QDialogButtonBox context, then I think there is no harm in using that
> parameter. If there were a setting call `HandYourDirtyLaundryOutToDry` which
> has the desired effect (and no undesirable side-effects), then we'd be using
> that (and asking Qt what they think about all that :)).
>
> Am I right that the only buttons Qt provides that can show icons
> automatically as a function of their role are the ones in a QDialogButtonBox?
> Either way, whatever the idea of the developer who introduced
> `SH_DialogButtonBox_ButtonsHaveIcons` was, the code seems to have evolved
> since to use it in a broader context. (Or the opposite is going on, in which
> case I think that this would already have been pointed out to me on the Qt ML
> and/or bug report.)
> The main point here is that I can see no unexpected side-effects of
> generalising `SH_DialogButtonBox_ButtonsHaveIcons`. Qt's idea must have been
> that it should be possible to hide icons assigned automatically to the
> buttons that have this feature, but not the ones assigned by developers to
> buttons that do not provide automatic icons. That idea is not violated in a
> context where the parameter is generalised to hide the icons on all buttons.
>
> Also:
> ```
> /**
> * This function determines if the user wishes to see icons on the
> * push buttons.
> *
> * @return Returns true if user wants to show icons.
> * @deprecated since 5.0, use
> style->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 0, widget);
> */
> static KDELIBS4SUPPORT_DEPRECATED bool showIconsOnPushButtons();
> ```
>
> seems to be very clear...
>
> The KdePlatformPlugin already respects the `ShowIconsOnPushButtons`
> setting and returns it when queried for the corresponding hint. I hope that
> should be enough to ensure that `QDialogButtonBox` buttons don't show their
> icons when so requested, but have yet to confirm that.
>
> Did I say that I agree that patching KPushButton has limited interest,
> even if it does solve part of the problem currently, namely that of
> applications building dialog-like interfaces without using Q/KDialogButtonBox
> (and using KPushButton instead).
> If KDialogButtonBox is not (or less) deprecated I (or someone else) can
> indeed modify it so that it respects `SH_DialogButtonBox_ButtonsHaveIcons`,
> but again, I think that can only be done by ignoring the icon when the
> buttons are created. You'd still get icons though if there is a way to change
> the icon on those buttons, and applications use that.
>
> And that still doesn't address the fact that QPushButtons should also
> respect `ShowIconsOnPushButtons` . The only way I see to achieve that
> currently that does not involve patching Qt is to oblige all code to check
> for the corresponding KDE style hint (or global setting, but see the
> deprecation note above), and assign an icon only when the feature is
> requested by the user. That seems like a strong argument to reintroduce
> KPushButton to me ...
>
> BTW: let me be pedantic for a bit, and point out that
> `ShowIconsOnPushButtons` literal meaning suggests that *all* push buttons
> should show an icon if the parameter is true. Ultimately that means that the
> setting has been violated for ages, maybe not the same way I'm proposing to
> violate `SH_DialogButtonBox_ButtonsHaveIcons` but still :)
>
> > >but on OS X or MS Windows
> > ... Qt uses native elements
>
> Actually, no. Not on OS X at least, and not in the sense that a
> QPushButton is implemented using an NSButton. I was under that impression
> myself, until it was pointed out to me that "QWidgets (and QML) don't use
> native UI views. They draw everything themself (or refrain from drawing, in
> case of button icons). The drawing is done in the style (qmacstyle_mac.mm in
> this case)." And I can confirm that this is true: on OS X I can use the XCB
> QPA and request the macintosh style, which gives me a X11 windows (local *or*
> remote) that have the exact same content as "native" windows, up to a scale
> factor and some colour differences. I'd been wondering how this was possible,
> now I know.
>
> (before anyone jumps at the thought of getting a true Mac theme on their
> KDE desktop: I suppose it still requires basic drawing routines and other
> [ObjC] APIs only available on a Mac :P)
>
> Thomas Lübking wrote:
> With all due respect, anything else simply makes no sense :-P
>
> See, the shortest cut to "please no superfluous icons on pushbuttons" is
> to have the style omit them.
> It's also the only global and with deprecation of KPushButton the only
> relevant approach.
> Agreed?
>
> Now, SH_DialogButtonBox_ButtonsHaveIcons is a hint *from* the style to
> "something" (the ButtonBox in particular) to not add icons to (certain)
> pushbuttons.
> If it's turned into a global hint *from* the style *to* something for
> something that only the style can actually provide, it's pointless. Do we
> agree on that? The style knows "i don't want to see icons on pushbuttons", it
> doesn't really have to share that info with the world (at least not for this
> purpose) - or itself.
>
> Now the only remaining question is "How do we make the style know what it
> wants".
> 1. The behavior is hardcoded in the style or the style has maybe some
> private config key for it
> 2. There's a central KDE setting that is to be honored by anything
> inheriting KStyle and can ideally be read by anything being a plain QStyle as
> well.
>
> The central setting exists and you promote to abuse
> SH_DialogButtonBox_ButtonsHaveIcons to propagate it.
> Leaving the abuse character aside, the patch doesn't really get you what
> you wanted to achieve (QPushButtons still have undesired icons everywhere)
> and will be increasingly useless by KPushButton -> QPushButton replacements.
>
> => To get what you want, you'll have to change KStyle/Breeze/Other styles
> _or_ QPushButton.
> Much luck on the latter but if you resolve this within *Style (what is
> imo the only correct solution, because QStyle is the LAF abstraction in Qt),
> walking across the StyleHint is just CPU burning, therefore the likely abuse
> is pointless.
>
> So my proposal is to
> a) fix KDialogButtonBox (it's a bug and bugs exist to be fixed - even in
> deprecated code, notably if it's still widely used)
> b) Wire up the kglobals setting from the platform plugin into the
> application where QStyle implementations can pick it up.
>
> The alternative to (b) is to let styles deal this locally (and ditch the
> gui config, resp. interpret it for dialogbuttonboxes only since that's the
> only thing we can effectively control - at least for KStyle inheritors that
> do not override the hint...)
>
> I'll provide a patch for (a), you take your preference on (b) and maybe
> Hugo wants to say a word ;-)
>
> Hugo Pereira Da Costa wrote:
> Damn,
> thats a long thread.
> I have no objection against
> 1/ adding (well, me adding, or someone adding) a new option in breeze to
> disable icon on *all* buttons,
> 2/ or honouring the kdeglobals showIconsOnPushButtons in breeze
> 3/ or in oxygen
> 4/ or in kstyle (and let other QStyle based style do the same)
>
> My pereference would go to 2+4
> And I believe this is the best way to achieve whats intended
>
> Hugo Pereira Da Costa wrote:
> Note however that there are subtleties on the style side:
> 1/ there are toolbooton that can be non-autoraised and thus mimic a
> normal pushbutton. Should the icon be hidden on these ?
> 2/ there are pushbutton that can be rendered flat and thus mimic a normal
> toolbutton. Should the icon be rendered on these ?
> best
>
> René J.V. Bertin wrote:
> Well, I *am* taking this up with Qt to see what their actual intentions
> are re: QPushButton.
> So,
>
> > See, the shortest cut to "please no superfluous icons on pushbuttons"
> is to have the style omit them.
> > It's also the only global and with deprecation of KPushButton the only
> relevant approach.
> > Agreed?
>
> No, QPushButton seems to be closer to the source and a more appropriate
> place where a more than just a single parameter can be taken into
> consideration (e.g. draw the icon after all because otherwise we'll show an
> empty button).
>
> How do I know where fix this in the only other style I care about beyond
> the native Mac style? Search for CE_PushButton? Can the style decide to
> force-draw the icon if a button has no text?
> (Hugo: please fix both Breeze & Oxygen while you're at it!)
>
> Thomas Lübking wrote:
> > there are toolbooton that can be non-autoraised and thus mimic a normal
> pushbutton.
>
> No, can't ;-)
> You can use a toolbutton wrongly, but that's just bad client code.
> There's no guarantee it ends up looking anything like a PushButton and also
> ToolButtons have dedicated modes wrt. to icons & text which cannot be ignored.
>
> > there are pushbutton that can be rendered flat and thus mimic a normal
> toolbutton.
>
> No, can't for the same reason. A pushbutton is a pushbutton and cannot be
> expected to look like a toolbutton.
> It's a valid question on whether a "flat" pushbutton should carry an icon
> as additional or even only indicator that this is not only a label. One more
> reason to have this dealt by the style.
>
> Thomas Lübking wrote:
> > where a more than just a single parameter can be taken into
> consideration (e.g. draw the icon after all because otherwise we'll show an
> empty button)
>
> No, the style draws the entire thing. It's the very instance that has the
> most appropriate idea on what things are gonna end up visually.
>
> > How do I know where fix this in the only other style
>
> First we decide "how" to fix it, then we fix it, ok?
> You need to take care of geometry calculations and painting and I'm sure
> Hugo is more than capable of handling it *correctly* on the first shot.
>
> The relevant control element *should* be CE_PushButtonLabel, but their
> can be pitfalls because the button is painted in a chain of elements.
>
> René J.V. Bertin wrote:
> > a "flat" pushbutton should carry an icon as additional or even only
> indicator that this is not only a label
>
> How would that help? That icon could just as well be part of the label
> (or an additional one). Buttons that are iOS-9-style indistinguishable from a
> label are bad interface design IMHO (they're mentally deranged hyperlinks :))
> ; it shouldn't be encouraged by other questionable interface design
> principles like automatic button icons.
>
> Hugo Pereira Da Costa wrote:
> > The relevant control element should be CE_PushButtonLabel, but their
> can be pitfalls because the button is painted in a chain of elements.
> There is also CT_PushButton (size from contents)
> In any case, I'll implement that in breeze "soonish", will CCMAIL You
> René to the commit, and then you can easily dig which changes are necessary
> to port to other styles
> (I can also send summary)
>
> René J.V. Bertin wrote:
> Is Hugo going to take over QtCurve? O:-)
>
> QPushButton::paintEvent "raises" CE_PushButton, which then indeed raises
> CE_PushButtonLabel in QtCurve.
>
> > It's the very instance that has the most appropriate idea on what
> things are gonna end up visually.
>
> I'm not arguing that it knows best what things are going to be shown, but
> I am less convinced that it is at the best place to make the decision. Even
> though indeed a CE_PushButtonLabel handler should have access to both icon
> and text info you still end up in a situation where you limit what's possible
> by subclassing QPushButton. Or so it seems.
>
> Thomas Lübking wrote:
> The style could turn the icon into a watermark-a-like background :)
> (I agree that a button that looks like a label is bad GUI)
>
> Thomas Lübking wrote:
> Yichao is just as capable :-P
>
> > Even though indeed a CE_PushButtonLabel handler should have access to
> both icon and text info you still end up in a situation where you limit
> what's possible by subclassing QPushButton. Or so it seems.
>
> On the contrary. *Everything* that uses the style to render the button
> (inc. QtQuick components or handcrafted button implementations) will be
> caught by this.
>
> René J.V. Bertin wrote:
> Hmmm, Yichao is probably just as capable, but I'm not sure he's as
> motivated or available. I'm using an old QtCurve version for GtK exactly
> because the current code gives me buttons with icons; I reported that and a
> number of other things without ever getting a reaction.
> In short, I prefer to tackle this myself, either with (waiting for)
> Hugo's help or using his feedback to verify my implementation.
>
> My question remains: are we sure that there are (or should be) no
> use-cases where a QPushButton is derived to create a button that overrides
> the style selected by the user? I know, this sounds weird coming from me, and
> is a moot point with platform styles that simply don't draw button icons, but
> is this something you'd want to introduce in KDE's own styles?
>
> Thomas Lübking wrote:
> > I'm using an old QtCurve version for GtK
>
> Please notice that any styling support was removed from gtk3, not sure
> about the gtk2 situation, but given the behavior of gtk developers itr, I'd
> fully understand any GFY stance reg. gtk.
>
> As for gtk2, "gtk-button-images=0" in ~/.gtkrc-2.0 (or the gtk config
> file used on your side) should be sufficient (with all styles)
>
> > QPushButton is derived to create a button that overrides the style
> selected by the user
>
> No, but you can QWidget::setStyle() anytime. If that ultimately enters a
> style that doesn't support the global setting (however we'll do that) you'll
> be screwed. The only relevant case would be QStyleSheetStyle, ie.
> "button->setStyleSheet("color: red;");" but that does rarely touch icons (and
> if it does, the style sheet author likely wanted exactly this.
>
>
> Otoh, it's just as much possible to run into "class MyPushButton : public
> QWidget { // fake pushbutton" that invokes the style routines to paint a
> pushbutton, but doesn't relate to QPushButton at all.
> Please notice that any styling support was removed from gtk3, not sure about
> the gtk2 situation, but given the behavior of gtk developers itr, I'd fully
> understand any GFY stance reg. gtk.
I know. QtCurve never offered a GtK3 version so, so that's not it. For now I've
also kept the GtK3 version on my Mac frozen to maintain theming for the few
apps I use that need it. Not a sustainable solution sadly.
> As for gtk2, "gtk-button-images=0" in ~/.gtkrc-2.0 (or the gtk config file
> used on your side) should be sufficient (with all styles)
Agreed. It should. I suppose QtCurve is proof of your further remarks above. I
think I even know which commit introduced the regression, I just haven't been
bothered (yet) to figure out what aspect is the culprit (it's a rather large
commit).
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89321
-----------------------------------------------------------
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
>
>
