fvogt added a comment.
In D22723#501907 <https://phabricator.kde.org/D22723#501907>, @apol wrote: > In D22723#501690 <https://phabricator.kde.org/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` signal and ignoring the entire "decorators which are actually wrappers" business? > > > > IMO this new `FindMatchesJobInternal` class makes it even less obvious what's actually going on. > > > This is how ThreadWeaver and especially QObjectDecorator is meant to be used. The way `ThreadWeaver::QObjectDecorator` is apparently meant to be used is to wrap the custom job inside a `QObjectDecorator` and use only the wrapper from there on: https://github.com/KDE/threadweaver/blob/239cf8fffe687c0a758f5170a40b26ae0acef7b0/autotests/QueueTests.cpp#L157 autoDeleteJob = new QObjectDecorator(new AppendCharacterJob(QChar('a'), &sequence)); [...] QVERIFY(autoDeleteJob != nullptr); QVERIFY(connect(autoDeleteJob, SIGNAL(done(ThreadWeaver::JobPointer)), SLOT(deleteJob(ThreadWeaver::JobPointer)))); which is not great, to say the least. What this patch does on the surface is wrap the `QObjectDecorator` inside an object that fakes being the custom job itself. That's actually a slightly better design than the code above does as now the pointer passed from the `done` signal is not the `QObjectDecorator` pointer but the custom class. Still, IMO much better would be to just merge the `QObjectDecorator` into the custom job as that would both avoid creating two job objects per job and make the code clearer and shorter. > I don't really know why you say it's confusing. The confusing part so far was that jobDone slot was never called. Which likely happened because the author was confused by the code... REPOSITORY R308 KRunner REVISION DETAIL https://phabricator.kde.org/D22723 To: apol, #frameworks, fvogt, davidedmundson Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns
