| fvogt | |
| davidedmundson |
| Frameworks |
This actually looks like the port to the QObjectDecorator was done wrong. QObjectDecorator::done was never emitted because the decorator doesn't notify about changes in the decoratee (which is potentially something to improve in ThreadWeaver).
This patch changes the internal FindMatchesJob to be a QObjectDecorator which will allow us to get notified when the jobs get done.
Deprecates D21606 which forced emitting done from the thread.
Tested with D22514
| No Linters Available |
| No Unit Test Coverage |
| Buildable 14612 | |
| Build 14630: arc lint + arc unit |
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.
I don't really know why you say it's confusing. The confusing part so far was that jobDone slot was never called.
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:
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...
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.
Correctly as in "it's supposed to work" yes, but it's not as it was intended to be AFAICT.
This now adds a custom private and friend class (ugh) which means that now there's a sandwich out of three classes:
FindMatchesJob -(inherits)> QObjectDecorator -(uses)> FindMatchesJobInternal while only a single one would do the job.
I did a PoC of dropping use of QObjectDecorator: https://phabricator.kde.org/D22758
Here, one may argue that it reinvents the wheel, but if the current iteration of the wheel is square I'd say it's allowed.
It also gets rid of a heap allocation.
What do you think?
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.
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 are emitting the signal from the run thread instead of the actual object's thread. This is wrong.
Also I'd say there's value in using tools (i.e. Threadweaver) as it's meant to be used, despite you don't liking its semantics.
QObjects live in their own thread and shouldn't be used outside.
https://doc.qt.io/qt-5/qobject.html#threadIn your patch we are emitting the signal from the run thread instead of the actual object's thread. This is wrong.
Needs a Qt::QueuedConnection indeed. Or the job has to be moved to a different thread, which means that Qt makes the connection a queued one automatically.
Also I'd say there's value in using tools (i.e. Threadweaver) as it's meant to be used, despite you don't liking its semantics.
Yes, improving the Threadweaver API is the best option here.
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.
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.
Hopefully make it more understandable by having the job run from a lambda rather than subclassing a whole new object
| src/runnermanager.cpp | ||
|---|---|---|
| 360 | can you explain why we need this? | |
| src/runnermanager.cpp | ||
|---|---|---|
| 360 | 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 early the function. Note that so far it was only being set to true in single runner mode, so it was impossible it we'd ever emit queryFinished() otherwise. | |
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