Add remote runners over DBus
ClosedPublic

Authored by davidedmundson on Jun 26 2017, 11:34 AM.

Details

Summary

This patch allows adding runners which, rather than being in-process
receive a search term and provide results over DBus.

A metadata file should be installed with metadata for the service and
path, and the remote app should implement the supplied interface.

By using metadata files all the current krunner architecture and config
management works seamlessly, and it allows the possibility for a runner
to be DBus activated if it wants to be.

The goal of this patch is threefold:

  • To make it considerably easier to write runners where the data exists

in an already running application. The plasma-browser-integration is a
good example, it currently has it's own custom protocol, and ends up
sending data on all the tabs only to filter it inside krunner, somewhat
wasteful. With this adding searchable tabs to konversation (for example)
would be a ten minute patch.

  • Make it easy to write a runner in any other language. An app just

needs to speak DBus with no other dependencies. Potentially even shipped
via the store.

  • This approach could work for runners in sandboxes.
Test Plan

Wrote new sample app
Got my runner showing results
Brief profiling had roundtrip time as 0ms.

Diff Detail

Repository
R308 KRunner
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptJun 26 2017, 11:34 AM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
mart added a subscriber: mart.Jun 29 2017, 3:31 PM

love the idea.
when this goes in, at least one using this should already exist, to make sure the code gets well tested from day one.

this makes me think...
it's not exacrtly the same thing, maybe kindof a layer, but would be possible to start from this and have a process that contains all current runners which krunner and plasmashell then query? (this at least would get rid of things like the thousands of plasmashell crashes caused by baloo once for all, even tough it moves them to another process, it's a less mission critical one)

Added unit test, support actions

davidedmundson retitled this revision from WIP: Add remote runners over DBus to Add remote runners over DBus.Jul 6 2017, 11:24 AM

Cool stuff!

A bunch of nitpicks but then it's good to go.

autotests/dbusrunnertest.cpp
46

Remove

53

Put in initializer list

DBusRunnerTest::DBusRunnerTest()
    : m_process(new QProcess(this))
66

This is deprecated, use setTestModeEnabled

89

Why are you using QString and not QStringLiteral all over the place?

100

QStringLiteral

autotests/dbusrunnertest.desktop
15

:D

autotests/testremoterunner.cpp
28

you say "is" but below you do contains

35

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)

src/dbusrunner.cpp
99

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)

110

We cannot have matches with different actions then, right? I don't think we do that in any other runner, so this is fine.

src/dbusrunner_p.h
39

Remove

src/querymatch.h
77

= nullptr

Seems an unrelated (and unneccessary?) change to me, though

davidedmundson marked 9 inline comments as done.Jul 6 2017, 2:31 PM
davidedmundson added inline comments.
autotests/dbusrunnertest.cpp
89

because it's in a test...

src/dbusrunner.cpp
110

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.

src/querymatch.h
77

I have to Q_DECLARE_METATYPE for QSignalSpy.

That means it needs a default constructor.

davidedmundson marked 2 inline comments as done.

Kai's comments

broulik accepted this revision.Jul 6 2017, 4:16 PM
This revision is now accepted and ready to land.Jul 6 2017, 4:16 PM
This revision was automatically updated to reflect the committed changes.