D7954: Fix repaint loop in KToolBar

2018-02-21 Thread Jean-Baptiste Mardelle
mardelle abandoned this revision.

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D7954

To: mardelle, #frameworks, ilic, dfaure
Cc: michaelh


D7954: Fix repaint loop in KToolBar

2017-10-07 Thread David Faure
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  You can discard this, I killed the code completely, with my change to 
kwidgetaddons.

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D7954

To: mardelle, #frameworks, ilic, dfaure


D7954: Fix repaint loop in KToolBar

2017-10-05 Thread David Faure
dfaure added a comment.


  Ah right, my comment was wrong, it assumed that `text` was the current text 
of `tb`, but it's not (necessarily).
  
  So your patch is ok, it's just that I'd rather remove all this code with my 
other patch instead ;)
  But until it's approved, let's have yours in...

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D7954

To: mardelle, #frameworks, ilic, dfaure


D7954: Fix repaint loop in KToolBar

2017-10-05 Thread Jean-Baptiste Mardelle
mardelle added a comment.


  So did some more tests, and the loop is caused by the tb->settext() call. The 
QAbstractButton setText method checks if :
  d->text == text
  
  But d->text contains the & accel marker and not text, so we end up comparing 
'ren&der' with 'render' on paint, thus the loop. I will review & update my 
patch tomorrow

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D7954

To: mardelle, #frameworks, ilic, dfaure


D7954: Fix repaint loop in KToolBar

2017-09-24 Thread David Faure
dfaure added a comment.


  Uploaded my patch, now with unittest: https://phabricator.kde.org/D7964

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D7954

To: mardelle, #frameworks, ilic, dfaure


D7954: Fix repaint loop in KToolBar

2017-09-23 Thread David Faure
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  OK, let's get the quick fix in, but I still think my alternative patch 
(removing all this code and applying 
http://www.davidfaure.fr/2017/0001-KAcceleratorManager-set-icon-text-on-actions-to-remo.patch
 instead) is a better solution.
  It works in my tests, although the discussion in 
https://git.reviewboard.kde.org/r/129663/ got a little confusing.

INLINE COMMENTS

> ktoolbar.cpp:1340
> +const QString modText = i18nc("@action:intoolbar Text 
> label of toolbar button", "%1", text);
> +if (modText != text) {
> +tb->setText(modText);

This if() serves no purpose, setText already does this:

  void QAbstractButton::setText(const QString &text)

  
  {
  Q_D(QAbstractButton);
  if (d->text == text)
  return;

> ktoolbar.cpp:1345
> +const QString modToolTip = i18nc("@info:tooltip Tooltip 
> of toolbar button", "%1", toolTip);
> +if (modToolTip != toolTip) {
> +tb->setToolTip(modToolTip);

QWidget::setToolTip however lacks the equality check, how strange. On the other 
hand that just generates a ToolTipChange event, surely not the cause for the 
recursion here. You can keep the check, but for the record, that's not the fix.

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D7954

To: mardelle, #frameworks, ilic, dfaure


D7954: Fix repaint loop in KToolBar

2017-09-23 Thread Jean-Baptiste Mardelle
mardelle created this revision.
mardelle added reviewers: Frameworks, ilic, dfaure.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Patch is based on https://git.reviewboard.kde.org/r/128421/
  This issue - endless repaint loop when a QToolButton is embedded in a 
KToolBar - causes major battery and performance drain. Just opening an 
application causes a 15-20% CPU doing nothing. With the patch, CPU usage goes 
down to 0%.
  Bugs: 
  https://bugs.kde.org/show_bug.cgi?id=365050
  https://bugs.kde.org/show_bug.cgi?id=377859

REPOSITORY
  R263 KXmlGui

REVISION DETAIL
  https://phabricator.kde.org/D7954

AFFECTED FILES
  src/ktoolbar.cpp

To: mardelle, #frameworks, ilic, dfaure