[KDE/Mac] Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

Fri Dec 11 15:09:31 UTC 2015

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 36
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line36>
> >
> >     unrelated and not required

Not required indeed, but related in the sense that it removes any ambiguity on what method is being called ;)

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line39>
> >
> >     QDialogButtonBox::addButton should do correctly anyway, so please don't workaround things that are not broken.

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

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 57
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line57>
> >
> >     you can completely spare this, there's no reason to manipulate a copy of the GuiItem, just burns CPU

In that case I'll have to remove the `const` from guiitem, meaning a change to the API.

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 70
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line70>
> >
> >     Setting the icon is sufficient, please do not mess around with other attributes.

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

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 75
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line75>
> >
> >     this is really the only thing you should need to do here.

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 the icon completely.

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kpushbutton.cpp, line 257
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421662#file421662line257>
> >
> >     still wrong and again, please don't mess with the icon size - you're just tempting DIV zero segfaults.

what?? Code that doesn't check integer div 0 should be encouraged to crash. A different bug than the one we're addressing here, but not one I have any patience with/for.

I could use QSize() instead of QSize(0,0); the former would mean iconSize.isValid() will return false while with the latter it'll return true. But note that functions like QPushButton::sizeHint() do not check isValid. A bit of a conundrum.
Am I right that a button that never had an icon will have `iconSize() == QSize()` ?

- 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, 3:03 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, 3:03 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20151211/25503922/attachment-0001.html>