D7954: Fix repaint loop in KToolBar
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
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
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
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
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
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
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