Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[René J . V . Bertin]

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> >

For the record: I've raised a few interrogations that are preventing me from 
following up and addressing the open issues raised by Thomas.


- René J.V.


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89351
---


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
> 
>

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > 
> >
> > QDialogButtonBox::addButton should do correctly anyway, so please don't 
> > workaround things that are not broken.
> 
> René J.V. Bertin wrote:
> No, I've looked at Qt 5.5.1 . The only QDialogButtonBox::addButton that 
> "does correctly" is the one that takes a StandardButton. I haven't had time 
> to test this (will need to rebuild QtBase first) but I'm pretty sure that 
> that method creates a button with an icon with its sequence
> 
> ```
> QPushButton *button = new QPushButton(text, this);
> d->addButton(button, role);
> ```
> 
> My approach here is to avoid adding an icon if ButtonsHaveIcons is false, 
> or remove the icon if one was already added. That's what QDB does with its 
> ::addButton(StandardButton btn) method (calling a private createButton() 
> method). Any other approach is useless without a style supporting and 
> enforcing ButtonsHaveIcons, but which such a style KDialogButtonBox doesn't 
> need to be fixed in the first place...

::addButton(text, role) creates "new QPushButton(text, this)" - those should 
seriously not have any icons.

> whith such a style KDialogButtonBox doesn't need to be fixed in the first 
> place

If it's broken, it needs to be fixed - you cannot bail out with "the style is 
correcting that for us" (I've been fixing far too many kdelibs/qtgui bugs in 
the style ;-)


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 57
> > 
> >
> > you can completely spare this, there's no reason to manipulate a copy 
> > of the GuiItem, just burns CPU
> 
> René J.V. Bertin wrote:
> In that case I'll have to remove the `const` from guiitem, meaning a 
> change to the API.

No, you do not touch the KGuiItem that comes (it's not yours!) and it's 
pointless to create a copy, strip the icon from that, assign it to the button 
(and maybe even strip the icon from the button) - you just apply the GuiItem 
that enters and strip the icon from the button. The interim (deep) GuiItem copy 
is just detour in the path to remove the icon from the button.


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 61
> > 
> >
> > unrelated and it won't leak, since the cleanup is done by the 
> > parent/child relation ("this" passed to KPushButton)
> 
> René J.V. Bertin wrote:
> Maybe it won't leak, but the question remains why what buttons with an 
> invalid role are good for.

Did you see such role being used? It would be a client code bug (the symbol is 
juts for completeness sake, so that you can eg. assign it to a role, pipe that 
to some transformation functions, check whether it's still invalid and then 
react to that)


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 70
> > 
> >
> > Setting the icon is sufficient, please do not mess around with other 
> > attributes.
> 
> René J.V. Bertin wrote:
> Are you sure? setIcon() doesn't call setIconSize() nor does it reset any 
> size information already present. Is it a good idea to replace an icon and 
> leaving the size information from the previous icon)? NB: should the icon 
> from the KGuiItem override the role's standard icon or should it be the other 
> way round (provided icon as a default when the role doesn't provide an icon, 
> for instance)?

setIcon *shall* not setIconSize, the two values are completely orhtogonal. 
Dropping the size information will just get you into trouble when you should 
require it again.
If eg. a style would reserve icon space of iconSize despite there actually is 
no icon to paint, the style is simply broken.

The icon size refers to the wanted size in the widget, not what the icon 
provides. Resolving that is job of the icon loader (or painting routine, eg. 
the style)

About GuiItem ./. StdRole: FIFO, ie. the last setter should usually win (if 
you've a button with a dedicated icon and role "ok" and switch the role to 
"delete", the "ok" icon is most certainly no longer correct ;-)


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 75
> > 
> >
> > this is really the only thing you should need to do here.
> 
> René J.V. Bertin wrote:
> Cf. the previous comment about icon priority: this method can provide 2 
> icons that the button will have to chose from.
> 
> And I think that it's probably a good idea to set the icon size to 0 when 
> the intent is to remove t

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> 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 ope

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[René J . V . Bertin]

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > 
> >
> > QDialogButtonBox::addButton should do correctly anyway, so please don't 
> > workaround things that are not broken.
> 
> René J.V. Bertin wrote:
> No, I've looked at Qt 5.5.1 . The only QDialogButtonBox::addButton that 
> "does correctly" is the one that takes a StandardButton. I haven't had time 
> to test this (will need to rebuild QtBase first) but I'm pretty sure that 
> that method creates a button with an icon with its sequence
> 
> ```
> QPushButton *button = new QPushButton(text, this);
> d->addButton(button, role);
> ```
> 
> My approach here is to avoid adding an icon if ButtonsHaveIcons is false, 
> or remove the icon if one was already added. That's what QDB does with its 
> ::addButton(StandardButton btn) method (calling a private createButton() 
> method). Any other approach is useless without a style supporting and 
> enforcing ButtonsHaveIcons, but which such a style KDialogButtonBox doesn't 
> need to be fixed in the first place...
> 
> Thomas Lübking wrote:
> ::addButton(text, role) creates "new QPushButton(text, this)" - those 
> should seriously not have any icons.
> 
> > whith such a style KDialogButtonBox doesn't need to be fixed in the 
> first place
> 
> If it's broken, it needs to be fixed - you cannot bail out with "the 
> style is correcting that for us" (I've been fixing far too many kdelibs/qtgui 
> bugs in the style ;-)

> ::addButton(text, role) creates "new QPushButton(text, this)" - those should 
> seriously not have any icons.

Agreed, they shouldn't *show* any if the user doesn't want to see them. AFAIC 
they can have a whole bunch of icons as long as they're not displayed. 

This argument is a bit useless as long as we don't know if an interface should 
stop showing icons the moment the user unticks the corresponding setting in 
systemsettings (and start showing them again when the setting is ticked). If 
it's ok to tell the user that "the new setting will only be respected after an 
application restart", then fine, let's simply not add icons when they're not 
wanted. In all those countless places where the setting will have to be applied.

But didn't you point out yourself that the style is in the best position to 
avoid drawing any unwanted icons?


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 57
> > 
> >
> > you can completely spare this, there's no reason to manipulate a copy 
> > of the GuiItem, just burns CPU
> 
> René J.V. Bertin wrote:
> In that case I'll have to remove the `const` from guiitem, meaning a 
> change to the API.
> 
> Thomas Lübking wrote:
> No, you do not touch the KGuiItem that comes (it's not yours!) and it's 
> pointless to create a copy, strip the icon from that, assign it to the button 
> (and maybe even strip the icon from the button) - you just apply the GuiItem 
> that enters and strip the icon from the button. The interim (deep) GuiItem 
> copy is just detour in the path to remove the icon from the button.

I must be getting on with other real-life stuff now; I agree with not touching 
the incoming item of course. I'll see if there isn't a way to avoid adding the 
unwanted icon at all, to avoid the icon deep copy as well (probably the most 
expensive part of a GuiItem deep copy, no?)


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 61
> > 
> >
> > unrelated and it won't leak, since the cleanup is done by the 
> > parent/child relation ("this" passed to KPushButton)
> 
> René J.V. Bertin wrote:
> Maybe it won't leak, but the question remains why what buttons with an 
> invalid role are good for.
> 
> Thomas Lübking wrote:
> Did you see such role being used? It would be a client code bug (the 
> symbol is juts for completeness sake, so that you can eg. assign it to a 
> role, pipe that to some transformation functions, check whether it's still 
> invalid and then react to that)

No, I haven't seen such a role (and client code bug, agreed on that). I did see 
however that there is no trivial way to check for it; you apparently have to 
look at the role before adding the button, or else query the KDialogButtonBox 
if it owns the button (if that's even possible).


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 70
> > 
> >
> > Setting the icon is sufficient, please do not mess around with other 
> > attributes.
> 
> René J.V. Bertin wrote:
> Are you sure? setIcon() doesn't call setIconSize(