D6390: Add remote runners over DBus

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170706/a2e3732e/attachment-0001.html>