> 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)
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)
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89321
-----------------------------------------------------------
On Dec. 10, 2015, 11:54 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. 10, 2015, 11:54 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/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
