D22723: Fix RunnerManager::queryFinished()

[David Edmundson]

davidedmundson added a comment.


  > Correctly as in "it's supposed to work" yes, but it's not as it was intended to be AFAICT.
  
  I've been going over ThreadWeaver.
  
  The "decorator" is more of a wrapper, from what I can tell of ThreadWeaver we know the wrapper finishes, we know our wrapped object has finished.
  
  I don't think ThreadWeaver API needs fixing. Merely Krunner's usage.
  
  If we're using the QObjectDecorator pattern, RunnerManager is definitely meant to be starting the decorator and watching the wrapper not starting the underlying job.
  
  That part is how it's meant to be.
  
  -----
  
  @fvogt's new version
  Avoiding the relevant ThreadWeaver API is an equally a viable solution.
  
  > Or the job has to be moved to a different thread, which means that Qt makes the connection a queued one automatically.
  
  The job's thread is irrelevant. 
  In evaluating AutoConnection Qt uses the current thread emitting the signal and the thread of the receiver, which in this case RunnerManager in the main thread. AutoConnect should be fine.
  
  -----
  
  IMHO the "most correct" approach would be for RunnerManager to not have the pointless QSets about that shadow the data the Queue already has.
  
  Once we get there the QObjectDecorator object could be created purely within RunnerManager::startJob and ignored from there on.
  
  -------
  
  Both look fine to me, I would happily accept either. 
  I don't want us to get blocked forever choosing which of two perfectly good options is the best.
  
  If I did have to choose, I'd say I prefer Fabian's.

REPOSITORY
  R308 KRunner

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

To: apol, #frameworks, fvogt, davidedmundson
Cc: aacid, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190730/47b89137/attachment-0001.html>