Test failures with krunner 5.39.0
Hallo, I'm packaging krunner 5.39.0 for GNU Guix [1]. GNU Guix builds all software in an isolated environment (a container), which includes only the packages and tools specified explicitly. When running the tests, dbusrunnertest faills, with this notable warning and the error show below: DBusRunnerTest::testMatch() org.kde.krunner: Invalid entry: "DBus runner test" Any idea what may be missing, what needs to be set up (env-var, directory, package, paths)? Any idea hos to debug this further? All required packages and all optional/feature packages (except of QCH) are installed. I tried running the tests with both dbus-launch and without. The external program "testremoterunner" gets started. strace -e trace=file" shows nothing relevant (as far as I can tell). * Start testing of DBusRunnerTest * Config: Using QtTest library 5.9.2, Qt 5.9.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.4.0) PASS : DBusRunnerTest::initTestCase() QWARN : DBusRunnerTest::testMatch() inotify_add_watch("/etc/mtab") failed: "No such file or directory" QWARN : DBusRunnerTest::testMatch() inotify_add_watch("/etc/fstab") failed: "No such file or directory" QWARN : DBusRunnerTest::testMatch() org.kde.krunner: Invalid entry: "DBus runner test" FAIL! : DBusRunnerTest::testMatch() 'spy.wait()' returned FALSE. () Loc: [/tmp/guix-build-krunner-5.39.0.drv-0/krunner-5.39.0/autotests/dbusrunnertest.cpp(82)] PASS : DBusRunnerTest::cleanupTestCase() Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 4931ms * Finished testing of DBusRunnerTest * -- Regards Hartmut Goebel | Hartmut Goebel | h.goe...@crazy-compilers.com | | www.crazy-compilers.com | compilers which you thought are impossible |
D8098: Strip down and re-write the tags KIO slave.
dfaure added inline comments. INLINE COMMENTS > kio_tags.cpp:64 > +QString tag; > +tag = url.path().section(QLatin1Char('?'), 0, 0); > +while (tag.startsWith(QDir::separator())) - merge with previous line - can path() really contain '?' ? I thought that wasn't possible (? delimits the query). > kio_tags.cpp:119 > +Q_UNUSED(permissions); > +Q_UNUSED(flags); > missing KIO::Overwrite support? Can the dest already exist? > kio_tags.cpp:156 > > -case FileUrl: > -ForwardingSlaveBase::get(QUrl::fromLocalFile(fileUrl)); > -return; > +if ((result.urlType == TagFileUrl) || (result.urlType == LocalUrl)) { > +ForwardingSlaveBase::get(QUrl::fromLocalFile(result.filePath)); And otherwise? what happens if it's neither TagFileUrl nor LocalUrl? should error(not supported) be called? The job would never finish otherwise. > kio_tags.cpp:163 > { > -Q_UNUSED(permissions); > Q_UNUSED(flags); > same question, can dest already exist? (if so, then the right thing to do is error(), unless `flags & Overwrite`) > kio_tags.cpp:203 > +for (const QString& tag : md.tags()) { > +if (tag == srcResult.tag || > (tag.startsWith(QString(srcResult.tag + QLatin1Char('/') { > +QString newTag = tag; the QString() isn't needed (repeats) > kio_tags.cpp:262 > +} else { > +ForwardingSlaveBase::mimetype(QUrl::fromLocalFile(result.filePath)); > } missing return, to avoid emitting finished() twice (like you did after the other calls to ForwardingSlaveBase) > kio_tags.cpp:292 > +TagListJob* tagJob = new TagListJob(); > +tagJob->exec(); > can this fail? > kio_tags.cpp:331 > +// Extract any file path and determine tag from url > +if (url.scheme() == QLatin1String("file")) { > +result.urlType = LocalUrl; That's better written as if (url.isLocalFile()) > kio_tags.cpp:337 > +bool validTag = relaxValidation; > +if (url.path().contains(QLatin1Char('?')) || chopLastSection) { > +QUrl choppedUrl = url; same question as before > kio_tags.cpp:356 > +QString query; > +const QStringList tagSections = > result.tag.split(QLatin1Char('/')); > +for (int i = 0; i < tagSections.size(); ++i) { could this whole splitting be optimized by saying something like this? result.tag.prepend("tag:"); result.tag.replace('/', " AND tag:"); It's just that split is slow (allocates a temporary container etc.). If this is called often, and the above isn't good enough, then something like what I did in kcontacts, commit https://phabricator.kde.org/R174:5de361ff80ce0a015b62a3a76fe676355d8d3a52, can be used. But maybe split is fine here, I guess it's likely not time critical. > kio_tags.cpp:393 > +result.pathUDSResults << createUDSEntryForTag(QStringLiteral("."), > result.tag); > +int index = result.tag.count(QLatin1Char('/')) + !result.tag.isEmpty(); > +QStringList tagPaths; This conversion from bool to int sounds ... fragile. I would do + (result.tag.isEmpty() ? 0 : 1) > kio_tags.cpp:397 > +for (const QString& tag : tags) { > +QString tagSection = tag.section(QLatin1Char('/'), index, index, > QString::SectionSkipEmpty); > +if (result.tag.isEmpty() || (tag.startsWith(result.tag, > Qt::CaseInsensitive) && !result.tag.isEmpty())) { `tagSection` isn't used by the if condition itself, so it could move into the if(). > kio_tags.cpp:398 > +QString tagSection = tag.section(QLatin1Char('/'), index, index, > QString::SectionSkipEmpty); > +if (result.tag.isEmpty() || (tag.startsWith(result.tag, > Qt::CaseInsensitive) && !result.tag.isEmpty())) { > +if (!tagPaths.contains(tagSection, Qt::CaseInsensitive) && > !tagSection.isEmpty()) { The `&& !result.tag.isEmpty()` is useless. If we're evaluating that part of the condition (the part after the "||"), then result.tag is NOT empty, by definition. > kio_tags.cpp:422 > +KIO::UDSEntry uds; > +if (KIO::StatJob* job = KIO::stat(match, KIO::HideProgressInfo)) { > +// we do not want to wait for the event loop to delete the job KIO::stat never returns nullptr, so the if() isn't useful. REPOSITORY R293 Baloo BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin, ngraham Cc: dfaure, nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
dfaure requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin, ngraham, dfaure Cc: dfaure, nicolasfella, ngraham
D8084: KAutoSaveFile breaks if source file name contains a space!
dfaure added a comment. Thanks for the fix and autotest ! INLINE COMMENTS > kautosavefiletest.cpp:83 > { > -// TODO > +QUrl normalFile = QUrl::fromLocalFile(QStringLiteral("/tmp/test > directory/tîst me.txt")); > + Should use QDir::tempPath() for portability. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D8084 To: mardelle, #frameworks, shaforostoff Cc: dfaure, ngraham, cfeck, ltoscano
D8544: KTextEditor : avoiding QML crashes
rjvbb set the repository for this revision to R39 KTextEditor. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb updated this revision to Diff 21724. rjvbb added a comment. This simpler version also seems to do the trick for me (read: I haven't been able to get "it" to crash). CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8544?vs=21519=21724 REVISION DETAIL https://phabricator.kde.org/D8544 AFFECTED FILES src/utils/kateglobal.cpp To: rjvbb, #frameworks Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, dhaumann
D8566: Add API to retrieve the screen id for a screen name
amantia added a dependent revision: D8493: Make Folder View screen aware. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8566 To: amantia, #plasma, ervin, dvratil, mlaurent, hein, davidedmundson Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D8332: Added baloo urls into places model
ngraham added a comment. @dvratil and @ervin, any more concerns? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8434: Created 'remote' section
ngraham accepted this revision. ngraham added a comment. Lovely. Looks good to me, too! REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin, mwolff Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8348: Add a section for removable devices
renatoo added a dependent revision: D8434: Created 'remote' section. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8348 To: renatoo, #dolphin, #frameworks, #vdg, ervin Cc: mlaurent, anthonyfieroni, ngraham, #frameworks
D8434: Created 'remote' section
renatoo edited the summary of this revision. renatoo added dependencies: D8332: Added baloo urls into places model, D8348: Add a section for removable devices. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin, mwolff Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8332: Added baloo urls into places model
renatoo added a dependent revision: D8434: Created 'remote' section. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8332 To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
D8434: Created 'remote' section
ngraham added a comment. Can you add "Depends on D" for each other patch that this requires? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin, mwolff Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8434: Created 'remote' section
mwolff accepted this revision. mwolff added a comment. This revision is now accepted and ready to land. lgtm, but please wait a bit, maybe someone else wants to chime in? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin, mwolff Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8592: Update default colors to match new colors in D7424
ngraham created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY If https://phabricator.kde.org/D7424 goes in, we'll have to update the default colors in KConfigWidgets. This does that. Depends on https://phabricator.kde.org/D7424 TEST PLAN Compiled and deployed in KDE Neon, then ran a bunch of programs. Could not see any differences. REPOSITORY R265 KConfigWidgets BRANCH master REVISION DETAIL https://phabricator.kde.org/D8592 AFFECTED FILES src/kcolorscheme.cpp To: ngraham Cc: #frameworks
D8592: Update default colors to match new colors in D7424
ngraham added reviewers: Frameworks, broulik, VDG. REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D8592 To: ngraham, #frameworks, broulik, #vdg Cc: #frameworks
D8434: Created 'remote' section
renatoo updated this revision to Diff 21697. renatoo added a comment. Added more test cases Renamed enum to match group name REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8434?vs=21694=21697 REVISION DETAIL https://phabricator.kde.org/D8434 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8434: Created 'remote' section
mwolff added inline comments. INLINE COMMENTS > kfileplacesmodeltest.cpp:784 > +// insert a new network url > +m_places->addPlace(QStringLiteral("My Shared"), QUrl( > QStringLiteral("ftp://192.168.1.1/ftp;)), QString(), QString(), > QModelIndex()); > + please add URLs for the following schemas, too: smb sftp fish webdav > kfileplacesitem_p.h:48 > PlacesType = 0, > -RecentlySavedType = 1, > -SearchForType = 2, > -DevicesType = 3, > -RemovableDevicesType = 4 > +NetworkType = 1, > +RecentlySavedType = 2, this should probably also be called `RemoteType` now, no? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8434: Created 'remote' section
renatoo updated this revision to Diff 21694. renatoo added a comment. Created unit test for remote ulrs REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8434?vs=21693=21694 REVISION DETAIL https://phabricator.kde.org/D8434 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D7787: Set KPageListView width properly
volkov added a comment. Could you try to test it without + 5 and with the following change applied? https://phabricator.kde.org/D8590 ? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D7787 To: wojnilowicz, #frameworks Cc: volkov, cfeck, tbaumgart, #kmymoney
D8590: KPageListView: Update width on font change
wojnilowicz resigned from this revision. wojnilowicz added a comment. I resign because I don't maintain KPageListView. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D8590 To: volkov, cfeck, wojnilowicz Cc: #frameworks
D8434: Created 'remote' section
ngraham added a comment. +1 on Remote. We really need to remove the duplicated Places test that's the header for the whole widget, though. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8434: Created 'remote' section
renatoo added a comment. In https://phabricator.kde.org/D8434#162786, @mwolff wrote: > Well, but if we use `Network` for `remote://` already, then the group should also have this label, no? I don't see an issue with this, really. On the contrary - maybe we could in the future remove the `remote://` link and let the category header react to a click, such that it will show you e.g. all remote devices. That would remove the duplication then. I think duplicate In https://phabricator.kde.org/D8434#162002, @ngraham wrote: > +1 for the idea! Needs more screenshots. :) screenshot added REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8590: KPageListView: Update width on font change
volkov added a reviewer: wojnilowicz. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D8590 To: volkov, cfeck, wojnilowicz Cc: #frameworks
D8434: Created 'remote' section
renatoo retitled this revision from "Created 'shared' section" to "Created 'remote' section". renatoo edited the summary of this revision. renatoo edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8434: Created 'shared' section
renatoo updated this revision to Diff 21693. renatoo added a comment. Renamed section from 'sared' to 'remote' REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8434?vs=21649=21693 REVISION DETAIL https://phabricator.kde.org/D8434 AFFECTED FILES autotests/kfileplacesmodeltest.cpp src/filewidgets/kfileplacesitem.cpp src/filewidgets/kfileplacesitem_p.h To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8590: KPageListView: Update width on font change
volkov added a reviewer: cfeck. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D8590 To: volkov, cfeck Cc: #frameworks
D8434: Created 'shared' section
mwolff added a comment. But thinking 5s more about it, I personally would say that using "Remote" for the category would be OK for now. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8434: Created 'shared' section
mwolff added a comment. Well, but if we use `Network` for `remote://` already, then the group should also have this label, no? I don't see an issue with this, really. On the contrary - maybe we could in the future remove the `remote://` link and let the category header react to a click, such that it will show you e.g. all remote devices. That would remove the duplication then. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8590: KPageListView: Update width on font change
volkov created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY In particular its font can be changed by Breeze style that modifies bold fonts to regular when it polishes list views. REPOSITORY R236 KWidgetsAddons BRANCH KPageListView-updateWidth REVISION DETAIL https://phabricator.kde.org/D8590 AFFECTED FILES src/kpageview_p.cpp src/kpageview_p.h To: volkov Cc: #frameworks
D8434: Created 'shared' section
renatoo added a comment. In https://phabricator.kde.org/D8434#162727, @mwolff wrote: > lgtm overall. but I wonder about the naming choice. "Shared" is confusing, to me personally at least. Why not call it "Remote" or "Network"? The reasoning is that "shared" for me is only LAN/WLAN shared stuff, while "remote://" allows arbitrary remote links. I use it to connect via (S)FTP to machines on the internet e.g. I first thought of using "Network" but this is already used as label for "remote://" url. "Remote" can be a option since it is not used yet. But "Shared" came from MacOS images posted in another review, looks like is is used by "Finder". REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8434: Created 'shared' section
elvisangelaccio added a comment. In https://phabricator.kde.org/D8434#162727, @mwolff wrote: > lgtm overall. but I wonder about the naming choice. "Shared" is confusing, to me personally at least. Why not call it "Remote" or "Network"? The reasoning is that "shared" for me is only LAN/WLAN shared stuff, while "remote://" allows arbitrary remote links. I use it to connect via (S)FTP to machines on the internet e.g. +1 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: elvisangelaccio, mwolff, mlaurent, #frameworks
D8566: Add API to retrieve the screen id for a screen name
davidedmundson accepted this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8566 To: amantia, #plasma, ervin, dvratil, mlaurent, hein, davidedmundson Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D8434: Created 'shared' section
mwolff added a comment. lgtm overall. but I wonder about the naming choice. "Shared" is confusing, to me personally at least. Why not call it "Remote" or "Network"? The reasoning is that "shared" for me is only LAN/WLAN shared stuff, while "remote://" allows arbitrary remote links. I use it to connect via (S)FTP to machines on the internet e.g. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8434 To: renatoo, ngraham, #frameworks, #dolphin Cc: mwolff, mlaurent, #frameworks
D8566: Add API to retrieve the screen id for a screen name
amantia updated this revision to Diff 21682. amantia added a comment. Add signals instead of a new virtual method (that is not BIC). This also makes us not rely on screen name, but only on plasma-internal screen id. REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8566?vs=21679=21682 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8566 AFFECTED FILES src/plasma/corona.h To: amantia, #plasma, ervin, dvratil, mlaurent, hein Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D8566: Add API to retrieve the screen id for a screen name
amantia added a comment. @broulik Very good point, luckily I reworked it. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8566 To: amantia, #plasma, ervin, dvratil, mlaurent, hein Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D8566: Add API to retrieve the screen id for a screen name
broulik added a comment. This class is exported, you cannot add a new virtual to it. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8566 To: amantia, #plasma, ervin, dvratil, mlaurent, hein Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D8566: Add API to retrieve the screen id for a screen name
amantia updated this revision to Diff 21679. amantia added a comment. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. Fix API docs text REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8566?vs=21617=21679 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8566 AFFECTED FILES src/plasma/corona.cpp src/plasma/corona.h To: amantia, #plasma, ervin, dvratil, mlaurent, hein Cc: plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D8566: Add API to retrieve the screen id for a screen name
dhaumann added inline comments. INLINE COMMENTS > ervin wrote in corona.h:223 > "if there is no such screen" I guess? And it is @return and not @returns. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D8566 To: amantia, #plasma, ervin, dvratil, mlaurent, hein Cc: dhaumann, apol, #frameworks