D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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 clas

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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 KRu

D10078: Add separate lib KF5::DBusRunner

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 `

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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 mor

D10078: Add separate lib KF5::DBusRunner

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

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

D10078: Add separate lib KF5::DBusRunner

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.

D10078: Add separate lib KF5::DBusRunner

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,

D10078: Add separate lib KF5::DBusRunner

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);

D10078: Add separate lib KF5::DBusRunner

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

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&id=33064 BRANCH kdbusrunnerlib2 REVISION DETAIL h

D10078: Add separate lib KF5::DBusRunner

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&id=33026 BRANCH kdbusrunnerlib2 REVISION DETAIL https://phabricator.kde.org

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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 usa

D10078: Add separate lib KF5::DBusRunner

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 u

D10078: Add separate lib KF5::DBusRunner

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 b

D10078: Add separate lib KF5::DBusRunner

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 &context); > +void finishMatching(const RunnerContext::Ptr &c

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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

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 REVIS

D10078: Add separate lib KF5::DBusRunner

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&id=31871 BRANCH dbus_runner REVISION DETAIL https://phabricator.kde.org/D10078 AFFECTED FILES K

D10078: Add separate lib KF5::DBusRunner

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 wil

D10078: Add separate lib KF5::DBusRunner

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 st

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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&id=29362 BRANCH master REVISION

D10078: Add separate lib KF5::DBusRunner

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

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

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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 krunne

D10078: Add separate lib KF5::DBusRunner

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

D10078: Add separate lib KF5::DBusRunner

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

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 plug