D6390: Add remote runners over DBus
This revision was automatically updated to reflect the committed changes. Closed by commit R308:8389c530e531: Add remote runners over DBus (authored by davidedmundson). REPOSITORY R308 KRunner CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6390?vs=16245&id=18481 REVISION DETAIL https://phabricator.kde.org/D6390 AFFECTED FILES autotests/CMakeLists.txt autotests/dbusrunnertest.cpp autotests/dbusrunnertest.desktop autotests/testremoterunner.cpp autotests/testremoterunner.h src/CMakeLists.txt src/abstractrunner.cpp src/data/org.kde.krunner1.xml src/data/servicetypes/plasma-runner.desktop src/dbusrunner.cpp src/dbusrunner_p.h src/dbusutils_p.h src/querymatch.cpp src/querymatch.h src/runnermanager.cpp To: davidedmundson, #plasma, broulik Cc: broulik, mart, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D6390: Add remote runners over DBus
broulik accepted this revision. This revision is now accepted and ready to land. REPOSITORY R308 KRunner BRANCH dbusrunner REVISION DETAIL https://phabricator.kde.org/D6390 To: davidedmundson, #plasma, broulik Cc: broulik, mart, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D6390: Add remote runners over DBus
davidedmundson updated this revision to Diff 16245. davidedmundson marked 2 inline comments as done. davidedmundson added a comment. Kai's comments REPOSITORY R308 KRunner CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6390?vs=16231&id=16245 BRANCH dbusrunner REVISION DETAIL https://phabricator.kde.org/D6390 AFFECTED FILES autotests/CMakeLists.txt autotests/dbusrunnertest.cpp autotests/dbusrunnertest.desktop autotests/testremoterunner.cpp autotests/testremoterunner.h src/CMakeLists.txt src/abstractrunner.cpp src/data/org.kde.krunner1.xml src/data/servicetypes/plasma-runner.desktop src/dbusrunner.cpp src/dbusrunner_p.h src/dbusutils_p.h src/querymatch.cpp src/querymatch.h src/runnermanager.cpp To: davidedmundson, #plasma Cc: broulik, mart, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D6390: Add remote runners over DBus
davidedmundson marked 9 inline comments as done. davidedmundson added inline comments. INLINE COMMENTS > broulik wrote in dbusrunnertest.cpp:88 > Why are you using `QString` and not `QStringLiteral` all over the place? because it's in a test... > broulik wrote in dbusrunner.cpp:109 > We cannot have matches with different actions then, right? I don't think we > do that in any other runner, so this is fine. It would be possible to do, we can have RemoteMatch have a list of actionIDs with it. We'd then have to cache that in the queryMatch, which would be easier if I wasn't already using setData as id gets mangled and could fetch them here. > broulik wrote in querymatch.h:77 > ` = nullptr` > > Seems an unrelated (and unneccessary?) change to me, though I have to Q_DECLARE_METATYPE for QSignalSpy. That means it needs a default constructor. 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
D6390: Add remote runners over DBus
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();
`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
D6390: Add remote runners over DBus
davidedmundson retitled this revision from "WIP: Add remote runners over DBus" to "Add remote runners over DBus". REPOSITORY R308 KRunner REVISION DETAIL https://phabricator.kde.org/D6390 To: davidedmundson, #plasma Cc: mart, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
