Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing
René J.V. Bertin
rjvbertin at gmail.com
Thu Dec 17 17:52:12 GMT 2015
> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> >
>
> René J.V. Bertin 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.
I'm stepping out, patching icons out of buttons elsewhere but in the style no longer has any urgency for me.
A final word of advice: try to get the Qt devs to indicate what they really intend with `SH_DialogButtonBox_ButtonsHaveIcons` : the end result on the screen (i.e. ButtonsShowIcons) or the result at the code/data level (i.e. ButtonsHaveIconMemberVariablesThatAreNotNull). If the former, there's no bug to patch (except in the style).
> 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.
>
> 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 ;-)
>
> René J.V. Bertin wrote:
> > ::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?
>
> Thomas Lübking wrote:
> If you create a PushButton with this constructor, the button has at this point no icon assigned. This has absolutely nothing to do with some setting or the display of anything - where should the button have obtained an icon? None is set here in the first place. QPushButton::icon().isNull()
Yes, apparently my assumption was wrong. It beats me where that icon gets set then, and how it'll be possible ultimately to prevent one from getting set. I'm going to hand this one back to you - I've solved my personal icon gripe in the style I use so as far as I'm concerned there is no longer a bug here O:-)
> 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.
>
> 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)?
>
> Thomas Lübking wrote:
> 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 ;-)
>
> René J.V. Bertin wrote:
> OK for iconSize ... but how should the user know the order of precedence in this case?
>
> Thomas Lübking wrote:
> > how should the user know the order of precedence in this case
>
> Hah? By "user" you likely mean "client code developer" and he's in charge to get his calls in proper order and the buttonbox just operates fifo. That's a completely natural and expectable behavior.
As a client code developer I wouldn't be very charmed to have to delve into the source code of each and every library I use to figure out if something shows "completely natural and expectable behaviour" by design or by accident.
- 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20151217/4ef599be/attachment.htm>
-------------- next part --------------
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel at kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
More information about the kde-core-devel
mailing list