> On Dec. 19, 2015, 10:23 a.m., David Faure wrote: > > src/kdeui/kpushbutton.cpp, line 256 > > <https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256> > > > > This patch looks wrong because KPushButton can be used outside of > > "dialog button boxes", while the styleHint is supposed to be only about > > dialog button boxes. > > > > QPushButton::sizeHint does this: > > bool showButtonBoxIcons = > > qobject_cast<QDialogButtonBox*>(parentWidget()) > > && > > style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons); > > which is a solution for testing the parent widget. > > > > I still don't fully understand the issue though, at painting time both > > QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why > > these two would be working differently. > > Is this a workaround for KPushButton which doesn't fix QPushButton? > > René J.V. Bertin wrote: > David, I'm dropping this whole RR, leaving it open only in case Thomas > wants to pursue his view on fixing what there is to fix. > > You are right though, it's a workaround for KPushButton which doesn't > depend on fixing QPushButton. And whether or not QPushButton requires fixing > is apparently the big question. What is your idea on the scope of > `ShowIconsOnButtons`? > > Thomas Lübking wrote: > The "problem" is that this is really scattered around everywhere :( > > One major catch should be (frameworksintegration) > > ``` > diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp > b/src/platformtheme/kdeplatformfiledialoghelper.cpp > index 11e7efb..9cd374c 100644 > --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp > +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp > @@ -161,6 +161,11 @@ void > KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode) > m_fileWidget->setMode(KFile::File); > break; > } > + // ::setOperationMode happily adds icons to our buttonbox, so we > clear them everytime the mode is set > + if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, > nullptr, this)) { > + foreach (QAbstractButton *button, m_buttons->buttons()) > + button->setIcon(QIcon()); > + } > } > > void > KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, > const QString &text) > diff --git a/src/platformtheme/kdirselectdialog.cpp > b/src/platformtheme/kdirselectdialog.cpp > index 0a65cd3..13d7dc7 100644 > --- a/src/platformtheme/kdirselectdialog.cpp > +++ b/src/platformtheme/kdirselectdialog.cpp > @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl > &startDir, bool localOnly, QWidget > m_buttons->setStandardButtons(QDialogButtonBox::Ok | > QDialogButtonBox::Cancel); > connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept())); > connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject())); > + if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, > nullptr, this)) { > + foreach (QAbstractButton *button, m_buttons->buttons()) > + button->setIcon(QIcon()); > + } > topLayout->addWidget(m_buttons); > > QHBoxLayout *hlay = new QHBoxLayout(page); > ``` > > But that somehow doesn't scale. > > KGuiItem::apply would have to catch that, but doesn't. > QDialogButtonBox::addButton *could* catch things, but doesn't (also it's > not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, > so QPushButton::setIcon() would have to catch it and QPushButton would have > to catch it on reparenting. > > > => For a complete solution, QPushButton actually needs painting code to > check the parent & style hint when setting up the style option - or we simply > rely on the style testing (widget && > qobject_cast<QDialogButtonBox*>(widget->parentWidget())) when calculating > sizes and painting the button. > The only alternative is to walk a long way and tell everyone to please > check the hint and fix their buttonboxes.... eewww. > > > > ==> QPushButton will require a "fix" to handle the StyleHint, but we > cannot expect *Q*PushButton to honor some global KDE setting, that's really a > job for the platform integration and in this case ultimately the style. > > > > > => the platform integration plugin (KDE part in all Qt applications) > needs to read that setting and expose it to the styles. > Alternatively the styles could read the setting from kdeglobals directly, > but that requires them to link kconfig (to do it correctly, just grabbing it > from the users kdeglobals ini isn't that much of a problem ;-), ie. rules out > Qt-only styles (and somehow contrasts w/ the KF5 approach to things) > > > > > Ideally™ there would be an official "Qt::AA_PushButtonTextOnly" (rename > me) flag for the QPA to set and QPushButtons to pick. > > René J.V. Bertin wrote: > Hear hear ... > > I have raised that final question in the Qt bug report I filed on this > (https://bugreports.qt.io/browse/QTBUG-49887) > > Maybe the best approach is to ensure that for now ShowIconsOnButtons is > handled the best way possible in the main current KDE styles (Breeze, Oxygen, > QtCurve) and then wait to see what position Qt adopts before trying to > implement something better (= less CPU intensive)?
What "hear hear" - I hardly changed my opinion, just figured that the hint is ignored all over KDE (client) code on migration from KDialogButtonBox to QDialogButtonBox and in other places and thus considered we -unfortunately- will best try to "fix" that (broken client code) in QPushButton. I had simply underestimated that this is not a bug in KDialogButtonBox only, but broken all over the place. Please nota bene that QPushButton is /not/ broken and does /not/ need to be fixed. One would request it to catch and cover broken client code. They may very well just respond "why not simply fix your shit?" > Maybe the best approach is to ensure that for now ShowIconsOnButtons is > handled [...] see what position Qt adopts In that case I'd propose to PoC a working infrastructure (QPA reads setting and sets a dynamic property on the QApplication instance, best before the style is constructed, so that finally the style(s) adopt that) and then ask "can we please make this a canonical and documented application attribute?" - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126308/#review89737 ----------------------------------------------------------- On Dec. 11, 2015, 4: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, 4: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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
