Cool! Dunno if this has to be a separate library? But then it would allow for super thin dependencies as it's just Qt DBus, I don't really mind either.

A first draft of some util code for writing D-Bus KRunner plugins, partially tested to work. See D10079 for the Baloo krunner plugin being adapted to it.

Tries to simulate the KRunner::AbstractRunner API a little, to where the krunner1 D-Bus interface makes that easy to do.

Forking KRunner::QueryMatch::Type to KDBusRunner::QueryMatch::Type is not perfect, but allows to just have QtCore in public link interface and QtDBus in the private, so does not pull in all the deps of KF5::Runner just for this enum definition.

While I put this into a subdir for now, all the code of KDBusRunner is independent, perhaps it makes sense to move it into a separate tier1 framework module?
For the future perhaps there could be a XDG D-Bus interface for such quicksearch plugins, so a separate krunner-independent library would make sense for that as well.

For the future perhaps there could be a XDG D-Bus interface for such quicksearch plugins, so a separate krunner-independent library would make sense for that as well.

Gnome does have an equivalent: org.gnome.Shell.SearchProvider2

It's similar to how I would have done it if I wasn't trying to fit into the existing krunner achitecture.
But it's not feasible do retrofit into here.

Making a client API speak both... /might/ be feasible.

I want to us to make sure this is still usable without a library dependency, but I think having this makes sense for the KDE apps. Definitely +1 to the idea.

I like how you've made it so you can reply in a sync or async way without making the sync version too complex.
I have a proposal that re-arranges a little bit to take it just a bit further.

void match( QSharedPointer<MatchContext> context)
that context object stores the m_mLastMatchDBusMessage and has the addMatches methods and it calls finish in its destructor

If a user wants to reply normally, they just do context.setMatches() in this method, with nothing else.

The baloo/p-b-i code can cache the latest MatchContext as a member variable exactly like they currently cache the lastMessage, and everything else gets automatically taken care of.
(with this API it looks quite easy to send the wrong data as a reply if a user doesn't handle the cancel signal correctly)

If a user wants to reply to all messages, they can too.

@davidedmundson Thanks for blowing new life into this patch :) The OO approach sounds nice and good to have, but no chance yet to look in detail.

While I had started some local changes following your feedback/proposals, I had stalled further activity as I got stuck understanding how krunner currently supports signalling of plugin config changes (from what I found, currently the Plasma::AbstractRunner::reloadConfiguration() is not used, instead krunner app reloads all plugins on config change of one?) and also as I hit bug https://bugs.kde.org/show_bug.cgi?id=389611 ("Milou cancels/resets the search if there are no first result after 500 ms") without any direct clue what to do there. As well as the need to develop multi-agent D-Bus krunner plugin support. Too many road-blocks for my non-urgent needs, so had turned to drive other coding entertainment roads noted on my FUNTODO map with more promising quick ROI :P

Given the use cases I had in mind for this dbus runner lib, I would like to have both things (config change signalling & multi-agent support) first sorted out and accordingly integrated into the API, before going public. With your active attention again, I will see to get active on this as well again, hopefully this or next week :)

If the config modules and runner executables will always be written by the same dev and shipped together I don't think we gain much by trying to generic-ify it.

The config writing and reading and syncing will all custom so they may as well do their own signalling. They can emit an anonymous DBus signal in the relevant config plugin ::save() if needed and watching for that in the search app. It'll be just 2 lines, and gives more granular control if needed.

As well as the need to develop multi-agent D-Bus krunner plugin support

I had promised to do that. I'm just incredibly slow on my promises. I'll get to it unless you beat me to it.

I had promised to do that. I'm just incredibly slow on my promises. I'll get to it unless you beat me to it.

That's now done.

Unless you disagree with my opinion on config signalling, is there anything else we need?

I read your comments.

I had also left a description when I first made the change.

This makes it quite easy for a developer to screw up and block krunner.
The RAII approach makes it very very hard for a developer to screw up with any of the multiple usage patterns.

In D10078#252871, @davidedmundson wrote:

This makes it quite easy for a developer to screw up and block krunner.
The RAII approach makes it very very hard for a developer to screw up with any of the multiple usage patterns.

Seems we have two different main design motives then: mine was to keep the classes/concepts close to the in-process krunner plugin ones, yours is to try to lower the chance of blocked calls :)

Guess I am not so burned by blocked krunner plugins to give that so much weight, as I also do not see the danger of people forgetting to do any finishMatching call. And would argue that the D-Bus Krunner.side proxy should protect itself in general against remote D-Bus krunner services locking themselves up in other ways.

Your project, your call/decision :)

I propose two renames though:

1. class "*Context" -> "MatchReply" (or similar/better): the "*Context" class in normal in-process krunner plugins has a different, passive purpose, also might "submit/cancel a context" not make that much sense? (non-native speaker here, might miss out phrase meanings)
2. request handling method name "startMatching" -> "handleMatchRequest"(or similar/better): for a method "start*" I would expect some counterpart methods on the same class/object.

I could not remember a Qt/KF class following the same pattern (something like single-thread service class with a handler method to do async request replies), so no existing naming pattern handy for proposal.

At least I imagine that as developer having to reimplement

virtual void handleMatchRequest(const MatchReply::Ptr &matchReply);

one has a better idea about what to do, compared to

virtual void startMatching(const RunnerContext::Ptr &context);

Even if the mixing of request properties (query string) into the reply class spoils the concepts a little. But splitting that off into another separate object makes the code more complex again, so that trade-off could be fine.

We have lots of KJob API patterns when *making* an async call to something else.
I can't think of any async-ly handling somethign else (except for maybe slavebase) , so we are in a fairly unique position here.

I really like your suggestions with respect to namings. ++

API dox still needs overhaul, just concentrated on code for now to keep feedback loop rolling

How would you think it does break?

If the runner has a reference to a sharedpointer then that object will never be deleted and we'll never hit the auto submit in the runner destructor.

In D10078#253627, @davidedmundson wrote:

How would you think it does break?

If the runner has a reference to a sharedpointer then that object will never be deleted and we'll never hit the auto submit in the runner destructor.

Right, in case of the if :) Just (for this reason) this is not using the sharedpointer reference to the normal MatchReply object, instead using a pointer to the MatchReplyPrivate object. (Current unit test would also not work otherwise, given it relies on auto-submit).
I will add some comments to the code to make this more clear to any future code reader.

Good, seems we found agreed-on codebase :) Will brush over the API dox/code comments some more for a final thumb-up.

Though after yesterday's prototyping of further dbusrunner plugins there is another thing where I might want to suggest some API change to be more future-proof (for some possible org.kde.krunner2):

Currently the match requests have no support for being cancelled (not part of org.kde.krunner1 D-Bus API, and have not found something on the D-Bus layer how to signal the discarding the wait for a reply). The baloo dbusrunner simply only handles the latest request. which surely covers the typical use-case, still, it will continue to handle the latest request even if there is no longer interest in it. That approach might be an issue once there are more cpu-intensive dbusrunners, wasting resources (cpu/io) when not needed.

To prepare for that it might make sense to turn MatchReply into a QObject, so it could get some signal validChanged or similar added once the backend supports discarding. While there currently already is a method to query the valid state, this is more a small convenience method given that right now it is under full control of the plugin developer when a MatchReply will get invalid, it will only happen on actions done via the API. But once the valid state can change by remote activitiy, a signal might be nice to have.
Using a QObject would also allow to switch from the fragile QVector<MatchReplyPrivate*> mActiveMatchReplies to using a QPointer-based approach on the real MatchReply objects, which might be less fragile.

What do you think? Would give this a separate try tonight, to get some idea.

What do you think? Would give this a separate try tonight, to get some idea.

Forwarding AbstractRunner::teardown is something I'd fully support.
At which point you don't need a signal in the context. IMO that's making things overly complex.

Note that the ThreadWeaver stuff in Krunner client is pretty messed up, so cancelling and whatnot doesn't really work as-is.

Using a QObject would also allow to switch from the fragile QVector<MatchReplyPrivate*> mActiveMatchReplies to using a QPointer-based approach on the real MatchReply objects, which might be less

You can use QWeakPointer already. I don't think it's particularly needed though.

In D10078#253730, @davidedmundson wrote:

What do you think? Would give this a separate try tonight, to get some idea.

Forwarding AbstractRunner::teardown is something I'd fully support.

Forwarding AbstractRunner::teardown would be nice to have as well.

Though I have been thinking of forwarding some (not yet existing) RunnerContext::isValidChanged() signal, once krunner has decided to no longer be interested in a context, due to query string having been changed by user,

From my prototyping experiments I can tell that the current approach of recommending in API docs to discard-existing-request-if-new-one-arrived-as-we-assume-just-one-client-who-simply-upgraded-to-a-new-request feels incomplete/undecided. As implemtor I want to know which requests should be handled and which not.

So a choice should be made:
a) this should be codified as policy in KDBusRunner::AbstractRunner, to e.g. set any tracked existing MatchReply to invalid once a new request arrived,
b) the concept of potentially parallel independent request should be officially supported (think e.g. stand-alone runner applets showing results in permanent non-popup list, updating automatically to some data source used as query string)

In case b) it would be then good to have a way to tell which requests can be discarded and which should still get a full match reply.

Actually I think we should hard-code a) for now, while at the same time leaving the option for b) once people start to have motivation to see b) supported.

At which point you don't need a signal in the context. IMO that's making things overly complex.

Note that the ThreadWeaver stuff in Krunner client is pretty messed up, so cancelling and whatnot doesn't really work as-is.

Yes, the RunnerContext currently becomes suddenly invalid but without signalling to the runner plugins when it happens, one has to actively query. But this could be fixed, given RunnerContext is a QObject.

Using a QObject would also allow to switch from the fragile QVector<MatchReplyPrivate*> mActiveMatchReplies to using a QPointer-based approach on the real MatchReply objects, which might be less

You can use QWeakPointer already. I don't think it's particularly needed though.

Did not know about QWeakPointer, might have a closer look if things could be implemented less fragile with it.

Though making MatchReply a QObject for the mentioned purpose continues to make sense to me, even more now.

From my prototyping experiments I can tell that the current approach of recommending in API docs to discard-existing-request-if-new-one-arrived-as-we-assume-just-one-client-who-simply-upgraded-to-a-new-request feels incomplete/undecided

Not really. It leaves it up to the implementor. It covers the simple case well, and any advanced case is still do-able quite easily.

Adding QObjects in context doesn't make sense. If you're processing stuff you won't process the signal. If you're not processing things then you don't need the signal.

In D10078#253982, @davidedmundson wrote:

From my prototyping experiments I can tell that the current approach of recommending in API docs to discard-existing-request-if-new-one-arrived-as-we-assume-just-one-client-who-simply-upgraded-to-a-new-request feels incomplete/undecided

Not really. It leaves it up to the implementor. It covers the simple case well, and any advanced case is still do-able quite easily.

? As implementor, I can report you first hand: I want to know whether it makes sense to continue handling a MatchReply or if not. As in: what is the minimum I need to do to correctly and completely serve the requests.

Having an API which has me doing potentially useless stuff because it does not tell me whether something is useful to do or not, would we not call this a broken API?

Adding QObjects in context doesn't make sense. If you're processing stuff you won't process the signal. If you're not processing things then you don't need the signal.

Well, it makes sense once you process things in other threads with event loops due to further async processing, no?