D22723: Fix RunnerManager::queryFinished()

apol abandoned this revision. apol added a comment. https://commits.kde.org/krunner/ae5a477b9d41e33137c4c8447e3fbe6e4b72cf32 REPOSITORY R308 KRunner REVISION DETAIL https://phabricator.kde.org/D22723 To: apol, #frameworks, fvogt, davidedmundson Cc: aacid, kde-frameworks-devel, LeGast00n,

D22723: Fix RunnerManager::queryFinished()

davidedmundson added a comment. Makes sense, thanks. (well makes sense, within the confines of the mess that is RunnerManager) This morning I accepted Fabian's patch, but it wasn't complete. Can you merge these two lines from your patch: https://phabricator.kde.org/P455 REPOSIT

D22723: Fix RunnerManager::queryFinished()

apol added inline comments. INLINE COMMENTS > davidedmundson wrote in runnermanager.cpp:360 > can you explain why we need this? In checkTearDown we have the code that reacts to the job being done. If we don't set it to true we'll never be looking at whether all jobs are done because we'll exit

D22723: Fix RunnerManager::queryFinished()

davidedmundson added inline comments. INLINE COMMENTS > runnermanager.cpp:360 > > +teardownRequested = true; > checkTearDown(); can you explain why we need this? REPOSITORY R308 KRunner REVISION DETAIL https://phabricator.kde.org/D22723 To: apol, #frameworks, fvogt, dav

D22723: Fix RunnerManager::queryFinished()

apol updated this revision to Diff 62851. apol added a comment. Hopefully make it more understandable by having the job run from a lambda rather than subclassing a whole new object REPOSITORY R308 KRunner CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22723?vs=62491&id=62851 BRA

D22723: Fix RunnerManager::queryFinished()

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 wrappe

D22723: Fix RunnerManager::queryFinished()

apol added a comment. Can someone chim in? because we're in a crazy place where we have a fix for a known bug and we're not moving in any direction. REPOSITORY R308 KRunner REVISION DETAIL https://phabricator.kde.org/D22723 To: apol, #frameworks, fvogt, davidedmundson Cc: aacid, kde-fra

D22723: Fix RunnerManager::queryFinished()

fvogt added a comment. > QObjects live in their own thread and shouldn't be used outside. > https://doc.qt.io/qt-5/qobject.html#thread > > In your patch we are emitting the signal from the run thread instead of the actual object's thread. This is wrong. Needs a `Qt::QueuedConnecti

D22723: Fix RunnerManager::queryFinished()

apol added a comment. > If you don't like it I won't object landing this internal class, but please add a long comment explaining why it was done like this. QObjects live in their own thread and shouldn't be used outside. https://doc.qt.io/qt-5/qobject.html#thread In your patch we

D22723: Fix RunnerManager::queryFinished()

fvogt added a comment. In D22723#502365 , @aacid wrote: > I honestly don't see the problem with this patch, one may argue that the ThreadWeaver API is awkward, ok, but this is using it correctly AFAICS, i.e. have a ThreadWeaver::QObjectDecorator

D22723: Fix RunnerManager::queryFinished()

aacid added a comment. I honestly don't see the problem with this patch, one may argue that the ThreadWeaver API is awkward, ok, but this is using it correctly AFAICS, i.e. have a ThreadWeaver::QObjectDecorator, give it a ThreadWeaver::Job on its constructor, and go on from there. REPOSITOR

D22723: Fix RunnerManager::queryFinished()

apol added a dependent revision: D22514: Show if KRunner is still searching for more things. REPOSITORY R308 KRunner REVISION DETAIL https://phabricator.kde.org/D22723 To: apol, #frameworks, fvogt, davidedmundson Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22723: Fix RunnerManager::queryFinished()

fvogt added a comment. In D22723#501907 , @apol wrote: > In D22723#501690 , @fvogt wrote: > > > Looks like a hack still, with two Job objects for each job... > > > > What about just merging `QObje

D22723: Fix RunnerManager::queryFinished()

apol added a comment. In D22723#501690 , @fvogt wrote: > Looks like a hack still, with two Job objects for each job... > > What about just merging `QObjectDecorator` into `FindMatchesJobInternal` by basically just adding a custom `done` signa

D22723: Fix RunnerManager::queryFinished()

fvogt added a reviewer: davidedmundson. fvogt added a comment. Looks like a hack still, with two Job objects for each job... What about just merging `QObjectDecorator` into `FindMatchesJobInternal` by basically just adding a custom `done` signal and ignoring the entire "decorators which

D22723: Fix RunnerManager::queryFinished()

apol created this revision. apol added reviewers: Frameworks, fvogt. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. apol requested review of this revision. REVISION SUMMARY This actually looks like the port to the QObjectDecorator was done wrong. QObjectDec