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 )
{
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
KDE CI: Frameworks plasma-framework kf5-qt5 FreeBSDQt5.7 - Build # 117 - Still Unstable!
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20plasma-framework%20kf5-qt5%20FreeBSDQt5.7/117/ Project: Frameworks plasma-framework kf5-qt5 FreeBSDQt5.7 Date of build: Sat, 23 Sep 2017 17:28:35 + Build duration: 20 min and counting JUnit Tests Name: (root) Failed: 1 test(s), Passed: 13 test(s), Skipped: 0 test(s), Total: 14 test(s)Failed: TestSuite.plasma-packagestructuretest
KDE CI: Frameworks plasma-framework kf5-qt5 XenialQt5.7 - Build # 113 - Still Unstable!
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20plasma-framework%20kf5-qt5%20XenialQt5.7/113/ Project: Frameworks plasma-framework kf5-qt5 XenialQt5.7 Date of build: Sat, 23 Sep 2017 17:28:35 + Build duration: 14 min and counting JUnit Tests Name: (root) Failed: 7 test(s), Passed: 8 test(s), Skipped: 0 test(s), Total: 15 test(s)Failed: TestSuite.dialognativetestFailed: TestSuite.plasma-configmodeltestFailed: TestSuite.plasma-dialogqmltestFailed: TestSuite.plasma-fallbackpackagetestFailed: TestSuite.plasma-iconitemtestFailed: TestSuite.plasma-packagestructuretestFailed: TestSuite.plasma-storagetest Cobertura Report Project Coverage Summary Name PackagesFilesClassesLinesConditionalsCobertura Coverage Report86% (6/7)62% (57/92)62% (57/92)38% (3490/9219)26% (1857/7150)Coverage Breakdown by Package Name FilesClassesLinesConditionalsautotests100% (22/22)100% (22/22)76% (605/795)38% (390/1028)src.declarativeimports.core57% (4/7)57% (4/7)28% (245/880)14% (85/618)src.plasma62% (13/21)62% (13/21)40% (1412/3573)29% (773/2697)src.plasma.private46% (11/24)46% (11/24)39% (649/1649)28% (302/1080)src.plasma.scripting0% (0/3)0% (0/3)0% (0/190)0% (0/126)src.plasmaquick50% (6/12)50% (6/12)27% (548/2019)19% (301/1579)src.plasmaquick.private33% (1/3)33% (1/3)27% (31/113)27% (6/22)
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
KDE CI: Frameworks baloo kf5-qt5 XenialQt5.7 - Build # 30 - Unstable!
BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20baloo%20kf5-qt5%20XenialQt5.7/30/ Project: Frameworks baloo kf5-qt5 XenialQt5.7 Date of build: Sat, 23 Sep 2017 13:59:11 + Build duration: 3 min 45 sec and counting JUnit Tests Name: (root) Failed: 1 test(s), Passed: 38 test(s), Skipped: 0 test(s), Total: 39 test(s)Failed: TestSuite.kinotifytest Cobertura Report Project Coverage Summary Name PackagesFilesClassesLinesConditionalsCobertura Coverage Report100% (12/12)77% (111/144)77% (111/144)73% (5039/6932)50% (2613/5194)Coverage Breakdown by Package Name FilesClassesLinesConditionalsautotests.benchmarks100% (2/2)100% (2/2)100% (42/42)89% (16/18)autotests.integration100% (3/3)100% (3/3)95% (242/255)64% (140/220)autotests.unit.codecs100% (3/3)100% (3/3)100% (40/40)57% (25/44)autotests.unit.engine100% (17/17)100% (17/17)100% (736/736)53% (390/740)autotests.unit.file100% (11/11)100% (11/11)97% (788/809)51% (438/864)autotests.unit.lib100% (6/6)100% (6/6)97% (315/326)52% (156/302)src.codecs100% (5/5)100% (5/5)87% (120/138)76% (32/42)src.engine97% (38/39)97% (38/39)79% (1607/2038)58% (796/1379)src.file39% (17/44)39% (17/44)45% (678/1506)38% (374/980)src.file.extractor100% (2/2)100% (2/2)69% (20/29)58% (7/12)src.file.extractor.autotests100% (1/1)100% (1/1)100% (22/22)61% (11/18)src.lib55% (6/11)55% (6/11)43% (429/991)40% (228/575)
D7948: Only match real MIME types, not e.g. "raw CD image"
ngraham closed this revision. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D7948 To: ngraham, #frameworks, nicolasfella, dfaure, kossebau, vhanda Cc: #frameworks
D7948: Only match real MIME types, not e.g. "raw CD image"
ngraham updated this revision to Diff 19829. ngraham added a comment. Grouping the startsWith() and contains() entries REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7948?vs=19827=19829 BRANCH master REVISION DETAIL https://phabricator.kde.org/D7948 AFFECTED FILES src/file/basicindexingjob.cpp To: ngraham, #frameworks, nicolasfella, dfaure, kossebau, vhanda Cc: #frameworks
D7948: Only match real MIME types, not e.g. "raw CD image"
ngraham added a comment. Ah, excellent idea. REPOSITORY R293 Baloo BRANCH master REVISION DETAIL https://phabricator.kde.org/D7948 To: ngraham, #frameworks, nicolasfella, dfaure, kossebau, vhanda Cc: #frameworks
D7948: Only match real MIME types, not e.g. "raw CD image"
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Yes (although I would then swap document to be after text/, to have all the "groups" together, and then the "substring searches"). In any case feel free to push. REPOSITORY R293 Baloo BRANCH master REVISION DETAIL https://phabricator.kde.org/D7948 To: ngraham, #frameworks, nicolasfella, dfaure, kossebau, vhanda Cc: #frameworks
D7944: Pre-select navigation bar URL when clicking on it to enter edit mode
ngraham abandoned this revision. ngraham added a comment. Fair enough. Your points are stronger than mine, now that I think about it. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7944 To: ngraham, #kde_applications, #frameworks, broulik, dfaure Cc: elvisangelaccio, #frameworks
D7948: Only match real MIME types, not e.g. "raw CD image"
ngraham updated this revision to Diff 19827. ngraham added a comment. Use startsWith() instead of contains() for greater speed and correctness, and do this for text MIME types as well REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7948?vs=19816=19827 BRANCH master REVISION DETAIL https://phabricator.kde.org/D7948 AFFECTED FILES src/file/basicindexingjob.cpp To: ngraham, #frameworks, nicolasfella, dfaure, kossebau, vhanda Cc: #frameworks
D7951: ContextChecker: support '!' context switchting and fallthroughContext
dhaumann created this revision. dhaumann added reviewers: cullmann, vkrause. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Extend the highlighting index checker to support contexts of the form "#pop!otherContext". Additionally, scan for fallthroughContext, if applicable. TEST PLAN make && make test REPOSITORY R216 Syntax Highlighting BRANCH contextchecker (branched from master) REVISION DETAIL https://phabricator.kde.org/D7951 AFFECTED FILES src/indexer/katehighlightingindexer.cpp To: dhaumann, cullmann, vkrause Cc: #frameworks
KDE CI: Frameworks baloo kf5-qt5 XenialQt5.7 - Build # 29 - Fixed!
BUILD SUCCESS Build URL https://build.kde.org/job/Frameworks%20baloo%20kf5-qt5%20XenialQt5.7/29/ Project: Frameworks baloo kf5-qt5 XenialQt5.7 Date of build: Sat, 23 Sep 2017 12:28:47 + Build duration: 12 min and counting JUnit Tests Name: (root) Failed: 0 test(s), Passed: 39 test(s), Skipped: 0 test(s), Total: 39 test(s) Cobertura Report Project Coverage Summary Name PackagesFilesClassesLinesConditionalsCobertura Coverage Report100% (12/12)77% (111/144)77% (111/144)73% (5049/6932)50% (2620/5194)Coverage Breakdown by Package Name FilesClassesLinesConditionalsautotests.benchmarks100% (2/2)100% (2/2)100% (42/42)89% (16/18)autotests.integration100% (3/3)100% (3/3)95% (242/255)64% (140/220)autotests.unit.codecs100% (3/3)100% (3/3)100% (40/40)57% (25/44)autotests.unit.engine100% (17/17)100% (17/17)100% (736/736)53% (390/740)autotests.unit.file100% (11/11)100% (11/11)98% (791/809)51% (441/864)autotests.unit.lib100% (6/6)100% (6/6)97% (315/326)52% (156/302)src.codecs100% (5/5)100% (5/5)87% (120/138)76% (32/42)src.engine97% (38/39)97% (38/39)79% (1607/2038)58% (796/1379)src.file39% (17/44)39% (17/44)45% (685/1506)39% (378/980)src.file.extractor100% (2/2)100% (2/2)69% (20/29)58% (7/12)src.file.extractor.autotests100% (1/1)100% (1/1)100% (22/22)61% (11/18)src.lib55% (6/11)55% (6/11)43% (429/991)40% (228/575)
D7926: Remove pf.path() from container before the reference got screwed up by it.remove()
chehrlic closed this revision. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D7926 To: chehrlic, broulik Cc: #frameworks
D7446: Add a Recent Documents places item to Dolphin and file pickers by default
markg added a comment. In https://phabricator.kde.org/D7446#148135, @ngraham wrote: > It seems odd to have all of these special KIO URLs that we don't actually want to use because they're rough and underdeveloped. They're rough and underdeveloped because they're hidden by default, so nobody sees them, and nobody files bugs or submits patches for them. But I do see your point. > > That said, the advantage to adding this as a Places item is that it shows up in file open/save dialogs for free, which is where it's most useful. If we make this into a whole new panel, we'll have to do a bunch of otherwise unnecessary special work to get it into open/save dialogs. I don't see the advantage. > > If the objection is that the content isn't useful (why does it show URLs?), then I can fix that too, but only if by doing so, folks will be amenable to adding a "Recent Documents" entry by default. It really is useful to have recent documents aggregated somewhere. Regarding the advantage in the file open/save dialog. It's only a advantage because the places panel is there by default. A recently used files/documents entry just doesn't belong in there (in my opinion). So you would have to add a second panel for that. It is the best looking way, but yeah, requires quite a bit more code changes. Recent documents just doesn't fit the open/save dialog imho. How useful is it to see the recently accessed files there? Not very. What you would want (and i would in fact like to have that) is a recent folders in open/save. That would be very useful! A IO slave for that does not exist. In dolphin a "recentfiles" panel would be interesting (i still wouldn't like it on by default..), but as it currently stands, no IO slave can currently do that. You would either have to "fix" recentdocuments to be that or make a new one. Also note that everything i said is merely my opinion. I'm not blocking you (i don't feel like i have the right to with my lack of contributions in recent years). If more people find it useful the way you intend it and if there is no comment on the code change then you are quite simply allowed to commit. But in this case you'd probably want to wait (or ask) for a few more opinions before making that decision. All i'm trying to do is provide constructive feedback :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7446 To: ngraham, #dolphin, #kde_applications, broulik, elvisangelaccio, dfaure, emmanuelp Cc: gregormi, markg, alexeymin, #frameworks, broulik, elvisangelaccio, dfaure, davidedmundson, ltoscano, #konqueror, akrutzler, navarromorales, firef, andrebarros, emmanuelp
D7915: Highlighting indexer: check existence of referenced context names
dhaumann closed this revision. dhaumann added a comment. Closed with https://commits.kde.org/syntax-highlighting/c62be1a9cbf501c7fd6ed1f6ad02ebd461ccb8c6 REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D7915 To: dhaumann, #framework_syntax_highlighting, cullmann, vkrause Cc: vkrause, #frameworks
D7446: Add a Recent Documents places item to Dolphin and file pickers by default
gregormi added a comment. In https://phabricator.kde.org/D7446#148135, @ngraham wrote: > That said, the advantage to adding this as a Places item is that it shows up in file open/save dialogs for free, which is where it's most useful. This would support the use case presented here: https://store.kde.org/p/1156273 and here: https://bugs.kde.org/show_bug.cgi?id=384411 Regarding those use cases, are you planning to add a list of recent folders to the Select Folder dialog, too? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7446 To: ngraham, #dolphin, #kde_applications, broulik, elvisangelaccio, dfaure, emmanuelp Cc: gregormi, markg, alexeymin, #frameworks, broulik, elvisangelaccio, dfaure, davidedmundson, ltoscano, #konqueror, akrutzler, navarromorales, firef, andrebarros, emmanuelp
D7944: Pre-select navigation bar URL when clicking on it to enter edit mode
elvisangelaccio added a comment. In https://phabricator.kde.org/D7944#148134, @ngraham wrote: > You're right that this improves one workflow and impairs another, but the way I see it, the patch simply brings consistency with the behavior you get if you hit ctrl-L/Replace Location. I don't know, it looks already consistent to me: - CTRL+L is the shortcut for the "Replace Location" action, so it makes sense to pre-select the URL in this case. - The tooltip of KUrlNavigator says: "Click to Edit Location", and "Edit" does not necessarily imply "Replace" REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D7944 To: ngraham, #kde_applications, #frameworks, broulik, dfaure Cc: elvisangelaccio, #frameworks
D7948: Only match real MIME types, not e.g. "raw CD image"
dfaure added a comment.
Yes, but then startsWith() would be even more correct (and slightly faster)
than contains().
And application/vnd.oasis.opendocument.text isn't plain text, so
contains("text") should also be changed to startsWith("text/").
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D7948
To: ngraham, #frameworks, nicolasfella, dfaure, kossebau, vhanda
Cc: #frameworks
