broulik added a comment.
Cool stuff! A bunch of nitpicks but then it's good to go. INLINE COMMENTS > dbusrunnertest.cpp:45 > + void testMatch(); > +// > +private: Remove > dbusrunnertest.cpp:52 > +{ > + m_process = new QProcess(this); > + m_process->start(QFINDTESTDATA("testremoterunner")); Put in initializer list DBusRunnerTest::DBusRunnerTest() : m_process(new QProcess(this)) > dbusrunnertest.cpp:65 > +{ > + QStandardPaths::enableTestMode(true); > + > QDir(QStandardPaths::writableLocation(QStandardPaths::GenericDataLocation)).mkpath("kservices5"); This is deprecated, use `setTestModeEnabled` > dbusrunnertest.cpp:88 > + //see testremoterunner.cpp > + QCOMPARE(result.id(), QString("dbusrunnertest_id1")); //note the runner > name is prepended > + QCOMPARE(result.text(), QString("Match 1")); Why are you using `QString` and not `QStringLiteral` all over the place? > dbusrunnertest.cpp:99 > + > + QCOMPARE(action->text(), QString("Action 1")); > + `QStringLiteral` > dbusrunnertest.desktop:14 > +X-Plasma-API=DBus > +X-Plasma-DBusRunner-Service=net.dave > +X-Plasma-DBusRunner-Path=/dave :D > testremoterunner.cpp:27 > + > +//Test DBus runner, if the search term is "foo" it returns a match, > otherwise nothing > +//Run prints a line to stdout you say "is" but below you do `contains` > testremoterunner.cpp:34 > + QDBusConnection::sessionBus().registerService("net.dave"); > + QDBusConnection::sessionBus().registerObject("/dave", this); > + qDBusRegisterMetaType<RemoteMatch>(); `registerObject` before `registerService` (Thiago iirc said that should be done with the new threaded dbus thing to avoid a service being exported in an inconsistent state) > dbusrunner.cpp:98 > + > + //split is essential items are as native DBus types, optional extras > are in the property map (which is obviously a lot slower to parse) > + > m.setUrls(QUrl::fromStringList(match.properties.value("urls").toStringList())); I see. subtext is widely used from what I can tell, whereas setMatchCategory is only used by baloo runner but that one typically spawns a ton of results for a query. (Just a comment, doesn't mean you neccessarily should change this) > dbusrunner.cpp:109 > +{ > + return actions().values(); > +} We cannot have matches with different actions then, right? I don't think we do that in any other runner, so this is fine. > dbusrunner_p.h:38 > + > +protected slots: > +private: Remove > querymatch.h:77 > */ > - explicit QueryMatch(AbstractRunner *runner); > + explicit QueryMatch(AbstractRunner *runner=0); > ` = nullptr` Seems an unrelated (and unneccessary?) change to me, though REPOSITORY R308 KRunner REVISION DETAIL https://phabricator.kde.org/D6390 To: davidedmundson, #plasma Cc: broulik, mart, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas