> 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.
> 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. - 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, 1: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, 1: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 > >
