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

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[René J . V . Bertin]

> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[René J . V . Bertin]

> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[René J . V . Bertin]

> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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`?

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 )
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 , 
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(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.


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

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

[René J . V . Bertin]

> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[Hugo Pereira Da Costa]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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 )
> 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 
> , 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(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 

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

[David Faure]

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



src/kdeui/kpushbutton.cpp (line 256)


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


- David Faure


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

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

[René J . V . Bertin]

> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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(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?

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


- René J.V.


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[René J . V . Bertin]

> On Dec. 10, 2015, 11: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 

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

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


> 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.
> 
> 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.
> 
> René J.V. Bertin wrote:
> 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?)

There's no deep copy of an icon, it's implicitly shared. And if there was, 
copying the KGuiItem would still only make things worse ...


> 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.
> 
> 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)
> 
> René J.V. Bertin wrote:
> No, I haven't seen such a role (and client code bug, 

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

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 

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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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 

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 

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

[René J . V . Bertin]

> On Dec. 10, 2015, 11: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 

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 

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 

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 

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

[Hugo Pereira Da Costa]

> 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 

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

[René J . V . Bertin]

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

(Updated Dec. 11, 2015, 1:42 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
Pereira Da Costa.


Changes
---

This adds the changes to KDialogButtonBox that seem required to respect 
`SH_DialogButtonBox_ButtonsHaveIcons` regardless of `ShowIconsOnPushButtons` (= 
if the former could be independent of the latter e.g. when using a style that 
does not use the latter to determine the value of the former).

What is the point in allowing `KDialogButtonBox::addButton` to create a button 
that is not added because of an invalid role? It seems that button wouldn't 
appear (or in an unexpected place), and be leaked?


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 (updated)
-

  src/kdeui/kpushbutton.cpp 98534fa 
  src/kdeui/kdialogbuttonbox.cpp 0f6649b 

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

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

[Hugo Pereira Da Costa]

> 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 

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

[René J . V . Bertin]

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

(Updated Dec. 11, 2015, 1:59 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
Pereira Da Costa.


Changes
---

This patch for KDialogButtonBox actually builds.


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 (updated)
-

  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

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 

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

[René J . V . Bertin]

> On Dec. 10, 2015, 11: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 

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 

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

[René J . V . Bertin]

> On Dec. 10, 2015, 11: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 

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

[René J . V . Bertin]

> On Dec. 10, 2015, 11: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 

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

[René J . V . Bertin]

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


Changes
---

Thomas, what exactly did you mean with "QDialogButtonBox::addButton should do 
correctly"? Looking at Qt's code again, I can confirm that 
QDB::addButton(StandardButton) is the only one invoking the private 
createButton method which in turn sets an icon if ButtonsHaveIcons is true. The 
other QDB::addButton methods simply call the private addButton method which 
will do a layout step by default, but I don't see where an icon would be added.

Should I understand that `style->standardIcon()` is invoked during drawing as a 
function of button's role?

I cannot find evidence of that in QtCurve. But if that is the case nonetheless, 
why are we patching KDialogButtonBox again? And how do you explain that 
removing the icon after a button is created has the effect it has (when the 
style will continue to see the button role)?


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 (updated)
-

  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

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 61
> > 
> >
> > unrelated and it won't leak, since the cleanup is done by the 
> > parent/child relation ("this" passed to KPushButton)

Maybe it won't leak, but the question remains why what buttons with an invalid 
role are good for.


- 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

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

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

[René J . V . Bertin]

> On Dec. 10, 2015, 11: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 

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

[Thomas Lübking]

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


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.

- Thomas Lübking


On Dec. 10, 2015, 9:01 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. 10, 2015, 9:01 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Qt KDE.
> 
> 
> 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/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

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

[René J . V . Bertin]

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

Review request for KDE Software on Mac OS X, KDE Frameworks and Qt KDE.


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

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

[René J . V . Bertin]

> On Dec. 10, 2015, 11: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.

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


- René J.V.


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


On Dec. 10, 2015, 11:12 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. 10, 2015, 11:12 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
> Pereira Da Costa.
> 
> 
> 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 
> 

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

[René J . V . Bertin]

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

(Updated Dec. 10, 2015, 11:54 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
Pereira Da Costa.


Changes
---

Easy enough to avoid empty buttons!


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 (updated)
-

  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

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

> 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 operate on legacy code only and fixes 
nothing.
Downforcing in a style (only) is the styles choice only.

We simply need to ensure to
a)