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
Details
Details
- Reviewers
ilic dfaure - Group Reviewers
Frameworks
Diff Detail
Diff Detail
- Repository
- R263 KXmlGui
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
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.
| src/ktoolbar.cpp | ||
|---|---|---|
| 1340 | This if() serves no purpose, setText already does this: void QAbstractButton::setText(const QString &text)
{
Q_D(QAbstractButton);
if (d->text == text)
return; | |
| 1345 | 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. | |
Comment Actions
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
Comment Actions
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...