Clean up KRunner
Open, Needs TriagePublic

Description

Currently KRunner uses mental multi-threading for everything. QIcon::fromTheme isn't thread-safe, for instance. It also doesn't have a concept of what a result is, so you can end up with duplicate results from different runners, like Baloo, Recent Documents, etc. Furthermore, writing (and distributing on the store!) 3rd party runners could be a lot easier by not requiring one to write a binary C++/Qt plugin.

The idea is to:

  • Reduce the thread usage overall. It is still needed for some runners but certainly not having every single runner in its own thread all the time
  • Add a concept for entity type, e.g. "file at ~/foo.txt with mime type text/plain", "application org.kde.foo", also check out Core Spotlight CSSearchableItemAttributeSet for inspiration. This way it could also provide richer results and additional actions automatically for certain results, e.g. "open containing folder", preview, file attributes/metadata for "files", etc
  • Come up with better scoring for results. Currently each runner has to set a global number from 0 to 1 which just doesn't scale
  • Make dbus runner a first-class citizen, so people can easily write custom runners just by slapping together a python script.
  • Out of process stuff. krunnerd external process queried by Kickoff, Krunner, whoever? Being able to query multiple krunner dbus services could also be used for compat, i.e. have one "krunnerd5" which is just the old KRunner with all its plugins and then gradually migrate over to the new thing without breaking the whole runner ecosystem again?

That entity type could also be useful for Kicker which currently uses a heuristic to guess the entity type from a kactivitymanagerd UUID. Maybe schema.org / kitinerary stuff could make sense in this context overall? I surely don't want to go down that whole nepomuk ontology rabbit hole, though :)

broulik created this task.Nov 14 2019, 9:48 AM
vkrause moved this task from Needs Input to Backlog on the KF6 board.Nov 23 2019, 3:23 PM
vkrause added a subscriber: vkrause.EditedNov 23 2019, 3:37 PM

Other notes from the sprint:

  • KProtocolInfo::classInfo is the only thing needed from KIO
  • depends on KService, with is then the only thing keeping this at tier 3+ -> can this be ported to K/QPluginLoader plugin loader?
  • plasma plugin version check seems no longer necessary, covered by Qt plugin meta info
  • move classes out of plasma namespace
  • cleanup qdebug usage
nicolasfella moved this task from Backlog to Metatasks on the KF6 board.Nov 25 2019, 9:27 PM
alex added a subscriber: alex.May 3 2020, 8:45 AM
meven added a subscriber: meven.Sep 8 2020, 9:35 AM

@alex should be interested in this.

alex added a comment.Sep 8 2020, 9:41 AM

@alex should be interested in this.

I am aware of this and we have a BoF this week https://community.kde.org/Akademy/2020/Thursday#Room_02_-_10th_September where some of the mentioned point will also be discussed :)

alex added a comment.Sep 8 2020, 10:40 AM

depends on KService, with is then the only thing keeping this at tier 3+ -> can this be ported to K/QPluginLoader plugin loader?

kossebau Has already done this

cleanup qdebug usage

When you are referring to the ton of unused/commented out qDebug statements that this has been done

alex claimed this task.Sep 8 2020, 4:01 PM
alex added a comment.EditedSep 11 2020, 7:31 AM

To give you all an update from the BoF:

  • The actions for each match should be set in the properties map => no interface changes required
  • The prepare/teardown/reload configuration methods should be added to the D-Bus interface, this will require a V2
  • Also the syntaxes should be added to the interface, this way the dbus runners can return the syntaxes at runtime. This is required if the runner uses configurable triggerwords or any settings that are not known at build time
  • We should add some lib for the dbus runners in C++, see https://phabricator.kde.org/D10078 for a first attempt. This way we reduce boilerplate and provide a standardized way on how to write there runners.
  • The shortcut activation is currently pretty horrible, because the krunner executable is started each time. It should be directly invoked using dbus.
  • Add a minimum letter count property. Often we spawn a ton of threads and they just get rejected by the runner, because the query is to short.
  • Also add a match regex. This solves the issue that there are a lot of threads spawned, but the runner has trigger word which does not match the query -> we shouldn't have to spawn a thread to figure this out.
  • >This will also help us prevent D-Bus activation of runners it it is not needed. (We still have to think about how to handle the prepare/teardown/actions calls)
alex added a comment.EditedSep 12 2020, 8:30 AM

Regarding the categories feature: It is really messy and also kind of useless. An alternative would be to introduce a third runner mode where we specify a list of runners we want to query(and /just/ for this query and not like the allowed runners). This would behave like the normal mode, but would only return queries from the selected runners.

This way we could remove this messy feature, but would not loose functionality(which is currently unused anyway). Also we could easily expose this to the DBus interface of KRunner, this is currently not possible. Then it will be very useful to launch "groups" of runners.

We could achieve this by adding another overload to the launchQuery method. And if we use the new mode with only one runner we could default to the single runner mode. Then the impl would actually be really easy.

alex added a comment.EditedSep 12 2020, 4:32 PM

Out of process stuff. krunnerd external process queried by Kickoff, Krunner, whoever?

This sounds a bit like such an architecture(arrow means this process queries the other process):

Or to avoid making a dbus call to a process which makes a dbus call and return a result we could do sth. like:

In both cases we have the advantage of having only one instance of the traditional runners.

dfaure added a subscriber: dfaure.Sep 12 2020, 7:00 PM

The second architecture seems better/simpler.

The problem with the first architecture is that krunnerd will be waiting for the reply before being able to reply itself, and if it does so blockingly, this will prevent it from processing other incoming calls, which will themselves block meanwhile. When it all works performance will be not great, and if anything stops working, everything will grind to a halt.
This problem with nested DBus calls is solvable with DBus transactions and async APIs, but that's painful to write, IMHO.
Direct calls seem much simpler to me.
Ideally async too, but at least without the need for DBus transactions.

alex added a subscriber: davidre.Sep 14 2020, 7:48 AM

Or to avoid making a dbus call to a process which makes a dbus call and return a result we could do sth. like:

That's how I always envisioned things when people propose a "krunnerd". Effectively it's one code path. Some just happen to be in one binary, but that's an implementation detail that neither the client code nor the server code need to care about.

An alternative would be to introduce a third runner mode where we specify a list of runners we want to query

One advantage of categories is that we could abstract clients from needing to know the client names. From a client POV, I can say I want to show files, and if the backend suddenly changes from baloo to tracker we can roll that out without the views being affected.

Though as you point out, we don't use them.. so no objections to killing it.

An alternative would be to introduce a third runner mode where we specify a list of runners we want to query(and /just/ for this query and not like the allowed runners).

That idea makes a lot of sense. It'd be cleaner than the way the single runner mode works +++

alex added a comment.Sep 28 2020, 6:27 AM

That's how I always envisioned things when people propose a "krunnerd".

But one detail I am not sure about: When we were talking about krunnerd we also talked about making the KF5 runners work to not again break the ecosystem. Should there be a krunnerd for the KDE5 krunner plugins too? Or just for the general putting the traditional plugins in a separate process?

We were breaking the KRunner ecosystem in KF5, because from KF4->KF5 we removed support for the python runners entirely. But in KF6 the transition should not be that big, especially the D-Bus runners should be fine.

Though as you point out, we don't use them.. so no objections to killing it.

https://invent.kde.org/frameworks/krunner/-/merge_requests/33

ivan added a subscriber: ivan.Feb 20 2021, 8:48 AM

Missing things for the KAMD use-case (patch https://invent.kde.org/plasma/kactivitymanagerd/-/merge_requests/11):

  • Filtering runners based on their keywords - it is a waste of CPU cycles for each application like KAMD to be pinged via dbus, for said application to do a startsWith, see that nobody mentioned its keyword and return an empty list over DBus. This can be remedied by adding activationKeywords() -> QStringList (which would return i18n and plain keywords... or whatever else that triggers them) to DBus runners' API so that the frontend can query the keywords on startup.
  • There should be a way to request loading of the DBus runner object from an application - lazy loading of the runner. If the activities runner (for example) is disabled, there's no reason why KAMD should instantiate its KRunner plugin and create the DBus object.
alex added a comment.Feb 20 2021, 8:58 AM

This can be remedied by adding activationKeywords() -> QStringList

We already have the X-Plasma-Runner-Match-Regex property which can be defined in the service file. I was thinking of adding a init or reloadConfiguration method to the runner interface, this could return a map where the runner can override the service properties at runtime. (This can also be useful when the runner does some caching and supports the goal of bringing feature parity to the dbus runners).

There should be a way to request loading of the DBus runner object from an application - lazy loading of the runner. If the activities runner (for example) is disabled, there's no reason why KAMD should instantiate its KRunner plugin and create the DBus object.

That is a good point, not sure how it plays in with the kactivities plugin system. For example the baloo runner used DBus activation, but there the runner is a standalone process.

ivan added a comment.Feb 20 2021, 10:03 AM

I assume X-Plasma-Runner-Match-Regex is not localized?

reloadConfiguration or something similar sounds cool. Maybe even a two-way communication - so that the runner can trigger the change when it needs to. There's a org.kde.ActivityManager.Application.loadPlugin dbus method on KAMD which can be used for this. Now, we just need to think how to run it for the krunner plugin :)

In doubt we could make a dummy binary that is DBus-activated that then calls loadPlugin. Dbus doesn't care who registers the service, doesn't need to be the process it spawned.

alex added a comment.Feb 20 2021, 10:23 AM

I assume X-Plasma-Runner-Match-Regex is not localized?

Yep, that is why we need this at runtime and construct the expression with the lokalized string.

Maybe even a two-way communication

That would be done kindof implicitly: The runner can have a config module and if the user changes the settings there the reloadConfiguration method will be called. This is done using the KConfig notify system which means that the changed signal can manually be emitted using DBus. This is already done here: https://invent.kde.org/plasma/plasma-desktop/-/blob/master/kcms/runners/kcm.cpp#L134

In doubt we could make a dummy binary that is DBus-activated that then calls loadPlugin

Yeah that could work, we did the same with the KRunner shortcut before KGlobalAccel could handle the DBus activation itself. But I am not sure if there is that much gain considering that it is just a simple class instance which gets queries on demand and the runner is enabled by default. I will leave it up to you @ivan

ivan added a comment.Feb 20 2021, 10:36 AM

That would be done kindof implicitly

Not sure I get it. What should KAMD runner do when, for example, the system language changes, and it wants to update the keywords accordingly? (this might be a bad example as we don't really handle language changes at runtime, but I hope you get the point)

which gets queries on demand

When it becomes 'on demand' instead of 'let's query all runners for all queries longer than 3 chars', the point will be moot. For the current KAMD runner at least. For other runners, or if KAMD runner decides to become overly complex at some point, this will need a 'recommended' solution.

alex added a comment.EditedFeb 20 2021, 10:42 AM

What should KAMD runner do when, for example, the system language changes, and it wants to update the keywords accordingly?

Then it could emit a dbus signal like the one in plasma-desktop I linked.

And regarding the second point you mentioned: Should I maybe disable the plugin by default and add a DBus activation file which calls dbus-send to load the plugin?

ivan added a comment.Feb 20 2021, 11:08 AM

Then it could emit a dbus signal like the one in plasma-desktop I liked.

Ok, understood.

And regarding the second point you mentioned: Should I maybe disable the plugin by default and add a DBus activation file which calls dbus-send to load the plugin?

Not yet. If the activities runner is enabled by default and the plugin is, let's leave it as it is for the time being.

alex added a comment.Sep 24 2021, 8:04 PM

I take back my plans to provide a krunnerd-daemon to support KF5 runners in KF6. The porting for the Dbus runners only affects the file location, the rest will stay compatible.

I have also done quite a lot of API deprecation and ported all of the KDE Runners, my own plugins and even third party ones away from the deprecated API.