Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing
Hugo Pereira Da Costa
hugo.pereira at free.fr
Fri Dec 11 13:56:03 GMT 2015
> On Dec. 10, 2015, 10: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.
> 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)
- Hugo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89321
-----------------------------------------------------------
On Dec. 11, 2015, 12:59 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, 12:59 p.m.)
>
>
> Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo Pereira Da Costa.
>
>
> 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/20151211/4841ff7e/attachment.htm>
More information about the kde-core-devel
mailing list