D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau abandoned this revision.
kossebau added a comment.


  Discarding for now as developers could not agree on API. Might be picked up 
later in one way or another.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 33152.
kossebau added a comment.


  - make MatchReply a QObject & change isValid to isFinished (+ signal) + 
allows to make MatchReplyPrivate unaware of AbstractRunnerPrivate by using the 
signal for unregistering + closer in API terms to existing *Reply classes in Qt 
spheres + finished signal can be used in eventloop-based processing to cancel
  - cancel for now any still active matchreply once a new match request arrives 
removes the need for implementors to decide how to deal with overlapping 
requests now and in the future, they just can rely on the finished state of the 
MatchReply object to decide whether to complete it

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=33114=33152

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  CMakeLists.txt
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/dbusrunnertest.cpp
  autotests/testremoteasyncrunner.cpp
  autotests/testremoteasyncrunner.h
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusrunner1adaptor.cpp
  src/dbusrunner/dbusrunner1adaptor_p.h
  src/dbusrunner/matchreply.cpp
  src/dbusrunner/matchreply.h
  src/dbusrunner/matchreply_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  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?

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson added a comment.


  > 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/undecide

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  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 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.

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 33114.
kossebau added a comment.


  - change #pragma once to traditional include guards, not yet part of kf 
policies
  - adapt all file names to class names
  - brush over API docs

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=33064=33114

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  CMakeLists.txt
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusrunner1adaptor.cpp
  src/dbusrunner/dbusrunner1adaptor_p.h
  src/dbusrunner/matchreply.cpp
  src/dbusrunner/matchreply.h
  src/dbusrunner/matchreply_p.cpp
  src/dbusrunner/matchreply_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson added a comment.


  > 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 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.

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  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 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.

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson added a comment.


  > 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.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> davidedmundson wrote in abstractrunner_p.cpp:141
> you can't (and don't need to) keep a list here.
> 
> It will break the MatchReply destructor calling submit working

How would you think it does break?

submit() tests if the reply is still valid, and only if it is then does create 
the d-bus message and also unregister from the runner instance.

The runner itself uses this list to keep track of all active replies to cancel 
them if itself set to disabled or destructed (which might be corner-cases, but 
seems fine to have them covered).

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> abstractrunner_p.cpp:141
> +MatchReplyPrivate *p = new MatchReplyPrivate(this, message(), queryTerm);
> +mActiveMatchReplies.append(p);
> +

you can't (and don't need to) keep a list here.

It will break the MatchReply destructor calling submit working

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


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

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 33064.
kossebau added a comment.


  also cancel any still running reply on deleting the runner instance

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=33026=33064

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  CMakeLists.txt
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/matchreply.cpp
  src/dbusrunner/matchreply.h
  src/dbusrunner/matchreply_p.cpp
  src/dbusrunner/matchreply_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 33026.
kossebau added a comment.


  update to MatchReply & handleMatchRequest

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=32916=33026

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  CMakeLists.txt
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/matchreply.cpp
  src/dbusrunner/matchreply.h
  src/dbusrunner/matchreply_p.cpp
  src/dbusrunner/matchreply_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson added a comment.


  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. ++

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  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 );
  
  one has a better idea about what to do, compared to
  
virtual void startMatching(const RunnerContext::Ptr );
  
  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.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> davidedmundson wrote in abstractrunner_p.h:46
> This undermines the advantages of my changes. 
> Its not object oriented anymore.

I tried to reason in the update message why I think after having tried that API 
that some other OO approach feels better to me. Any chance you missed that, 
given you do not refer to the arguments given there here?

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> abstractrunner_p.h:46
> +
> +void cancelMatching(const RunnerContext::Ptr );
> +void finishMatching(const RunnerContext::Ptr );

This undermines the advantages of my changes. 
Its not object oriented anymore.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 32916.
kossebau added a comment.


  Updating with proposed further massaging of the API
  
  - changed some class/method names to closely follow KRunner terms: 
(RunnerContext, query)
  - moved RunnerContext into separate header (for one per class)
  - moving submit/cancel methods back to AbstractRunner
  
  Keeping RunnerContext a pure data container and AbstractRunner as responsible
  for creating and consuming these objects feels more balanced and means less
  spreading of responsibilities/functionality.
  
  The auto-submit in the destructor of RunnerContext left a feel of lack of
  control to the API user given the passing of RunnerContext as sharedpointer.
  Also reading the code and seeing only startMatching, one would wonder how
  actually the matching is completed. Having to read up in the API dox might
  not make up for the otherwise unneeded additional call.
  
  One challenge I found when tinkering around with this:
  a D-Bus runner could in theory receive multiple calls at the same time, from
  one or multiple processes. While the RunnerContext now would allow handling
  multiple overlapping requests easily, I have yet to get an idea how/if
  overlapping D-Bus calls work in general, with QtDBus in detail and even more
  how the D-Bus proxy krunner acts when a newer query is asked for (e.g. on
  quick typing). The current code at least should be prepared in theory to
  some bit.

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=31871=32916

BRANCH
  kdbusrunnerlib2

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  CMakeLists.txt
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/querymatch.h
  src/dbusrunner/runnercontext.cpp
  src/dbusrunner/runnercontext.h
  src/dbusrunner/runnercontext_p.h
  src/querymatch.h

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau commandeered this revision.
kossebau edited reviewers, added: davidedmundson; removed: kossebau.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, broulik, davidedmundson
Cc: bruns, michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson added a comment.


  > 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?

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: davidedmundson, broulik, kossebau
Cc: michaelh, ngraham, #frameworks, bruns

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson updated this revision to Diff 31871.
davidedmundson added a comment.


  Rebase

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=29364=31871

BRANCH
  dbus_runner

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: davidedmundson, broulik, kossebau
Cc: michaelh, ngraham, #frameworks, bruns

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson marked an inline comment as done.
davidedmundson added a comment.


  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.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: davidedmundson, broulik, kossebau
Cc: michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  @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 :)

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: davidedmundson, broulik, kossebau
Cc: michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson updated this revision to Diff 29364.
davidedmundson added a comment.


  Rewritten with the second class I suggested.
  
  Not trying to comandeer your work, if you think it was better before, go for 
that.
  
  IMHO it's nice and OO now with the submit/cancel methods being with the data.
  
  It allows a client to:
  
  - easily respond in a sync fashion,  with one less line than before
  - easily keep track of the latest request, without having to track signals
  - optionally respond to every request in any arbitrary order (if using 
threads)
  
  Context needs some docs, but I thought I'd let you comment first.

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=29362=29364

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: davidedmundson, broulik, kossebau
Cc: michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson updated this revision to Diff 29362.
davidedmundson added a comment.


  Add the two really boring changes I wanted
  (virtual hook + class rename)

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10078?vs=25888=29362

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: davidedmundson, broulik, kossebau
Cc: michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson commandeered this revision.
davidedmundson edited reviewers, added: kossebau; removed: davidedmundson.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: davidedmundson, broulik, kossebau
Cc: michaelh, ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[David Edmundson]

davidedmundson added a comment.


  
  
  > 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 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.

INLINE COMMENTS

> abstractrunner.h:161
> +void matchingCancelled();
> +
> +private:

please add a virtual hook for future expansion

> dbusadaptor_p.h:31
> + */
> +class DBusRunnerAdaptor: public QDBusAbstractAdaptor
> +{

Can you put a 1 in the class name for future proofing.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, davidedmundson, broulik
Cc: ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> broulik wrote in dbusadaptor_p.h:1
> Isn't this file compile-time generated from xml?

It can be, yes. I had switched for reason* to  manually written one, but with 
the public class API no longer affected by reason* I could/should perhaps 
switch back.
Nothing really won or lost though by having a non-generated class.

*qdbusxml2cpp picks up the casing of the method names in the D-Bus interface 
and expects them also to be used in the wrapped class. D-Bus method naming 
standard seems to be "UpperCase", which also used in the krunner1 interface 
("Actions", "Run", "Query").
While in our Qt-style code we prefer method names to be with lowerCaseStart.
And given the purpose of the lib is "being nice to use", I switched to a 
manually adapted version of the adaptor code. Compare e.g. changed to API of 
TestRemoteRunner.
Later though, when I went to use QDBusContext for enabling the async match 
collection, I avoided leaking that detail in the public API and moved all D-Bus 
stuff to the pimpl object. Where having "UpperCaseStart" methods is not harming 
public consumers.
Whatever people prefer, no own preference.

> broulik wrote in querymatch.h:35
> Should be pimpl this? once we commit to this we can never add new fields but 
> then the DBus signature is also now or never.. I think we should add subtext 
> and url to the main struct since I always use the two. Originally David and I 
> thought we could put them in properties as they're seldom used but turns out 
> they're not.

Indeed, DBus signature being fixed is also why I wrote the simple struct for 
now instead of setter/getter pimpl API.
Having learned how to use c++11 initlalizer lists recently I also now fancy 
their use where possible ;) and in the little sample cpde I played for now 
using initializer lists worked nicely, no need for explicit setters. No getters 
used even. After all the runners are just producing that content usually and do 
no own further processing, but just deliver it.

Putting subtext and url to the main struct if you like. No idea yet how to 
adapt the marshalling, given those two need to be in the a{sv}, but possibly 
can be done with some temporary variantmap. No big runtime costs, and will be 
nicer API.

No strong opinion here as well. You decide and I will write :)

> broulik wrote in querymatch.h:53
> Can we static assert this in the dbus runner somehow? But I guess since it's 
> a separate lib you cannot

No idea yet how to automatically ensure this :/ Why are there  no guides with 
tips & tricks for D-Bus interface types?!

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, davidedmundson, broulik
Cc: ngraham, #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a comment.


  A first draft of some util code for writing D-Bus KRunner plugins, partially 
tested to work. See https://phabricator.kde.org/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.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, davidedmundson, broulik
Cc: #frameworks

D10078: Add separate lib KF5::DBusRunner

[Kai Uwe Broulik]

broulik added a comment.


  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.

INLINE COMMENTS

> dbusadaptor_p.h:1
> +/*
> + *   Copyright 2018 Friedrich W. H. Kossebau 

Isn't this file compile-time generated from xml?

> querymatch.h:35
> + */
> +struct KDBUSRUNNER_EXPORT QueryMatch
> +{

Should be pimpl this? once we commit to this we can never add new fields but 
then the DBus signature is also now or never.. I think we should add subtext 
and url to the main struct since I always use the two. Originally David and I 
thought we could put them in properties as they're seldom used but turns out 
they're not.

> querymatch.h:53
>   * The type of match. Value is important here as it is used for 
> sorting
> + * @internal Needs to match KDBusRunner::QueryMatch::Type.
>   */

Can we static assert this in the dbus runner somehow? But I guess since it's a 
separate lib you cannot

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, davidedmundson, broulik
Cc: #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau added a dependent revision: D10079: Port baloo krunner plugin to 
KDBusRunner.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D10078

To: kossebau, davidedmundson, broulik
Cc: #frameworks

D10078: Add separate lib KF5::DBusRunner

[Friedrich W . H . Kossebau]

kossebau created this revision.
kossebau added reviewers: davidedmundson, broulik.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
kossebau requested review of this revision.

REVISION SUMMARY
  Enables writing remote-via-D-Bus krunner plugins without
  having to care for any boilerplate of D-Bus calls.

REPOSITORY
  R308 KRunner

BRANCH
  kdbusrunnerlib

REVISION DETAIL
  https://phabricator.kde.org/D10078

AFFECTED FILES
  KF5DBusRunnerConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/testremoterunner.cpp
  autotests/testremoterunner.h
  src/CMakeLists.txt
  src/dbusrunner/CMakeLists.txt
  src/dbusrunner/abstractrunner.cpp
  src/dbusrunner/abstractrunner.h
  src/dbusrunner/abstractrunner_p.cpp
  src/dbusrunner/abstractrunner_p.h
  src/dbusrunner/action.h
  src/dbusrunner/dbusadaptor.cpp
  src/dbusrunner/dbusadaptor_p.h
  src/dbusrunner/querymatch.h
  src/querymatch.h

To: kossebau, davidedmundson, broulik
Cc: #frameworks