> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
