- offers to cancel an accidental startup of an already running job
- kill all running instances instead of one by one
- fix: do not ask again if "No" was already selected
Details
- start second instance of an already running job
- select
- Kill All Instances (No) (kills all running instances)
- Start Another (Yes) (just starts a new instance)
- Cancel (closes the dialog and drops the new job)
Diff Detail
- Repository
- R32 KDevelop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
plugins/execute/nativeappjob.cpp | ||
---|---|---|
137–138 | I'd prefer QSet and then you could use the more expressive !contains here instead of !count | |
138 | use QSet, or unordered_set - the order isn't needed and the hash-based containers are faster | |
138 | please put the QMessageBox::question call into a separate line, and break it up. Save the return value in a temp var and switch over it then Also reword the message to '%1' is already being executed.. Then set proper texts on the button such that it's clear what they do: Yes -> Kill I'd also say that the default action should be cancel here, or maybe "start another", but not a destructive kill | |
140 | why this line? you do this, to check whether the job is still alive? If so, store it in a QPointer and check its validity |
The QPointer was a good hint, it actually simplifies the whole loop.
I switched to
- Yes->Start Another
- No->Kill,
since the question about killing vanished.
looking at the old code, I wonder why it did what it did... Most notably, it refetched the current jobs after the message box. This looks intentional and is now gone. I mean sure, while a dialog is shown, anything could happen, including running another job... Do we need to take care of this? Does anyone remember why it was done in this way? Does the git history show up anything?
If we don't need that anymore, can we get rid of the loop altogether? I mean either we want to kill all jobs, or none. Showing one dialog per job is confusing as there is not indication which job we are going to kill out of multiple ones...
plugins/execute/nativeappjob.cpp | ||
---|---|---|
138–166 | use a QVector please |
I introduced that restart of the whole loop, when fixing the unit test behaviour (D6593). While the bug was still present, three instances of the same process could be launched in the background, which revealed what could go wrong with that dialog. That's not happening any longer and even if it does: The use of QPointer guards against access to deleted jobs, I just didn't think of that solution.
If we don't need that anymore, can we get rid of the loop altogether? I mean either we want to kill all jobs, or none. Showing one dialog per job is confusing as there is not indication which job we are going to kill out of multiple ones...
There only is a job name and the jobs in RunController::currentJobs() have no particular order, so showing a counter or so wouldn't help. Probably your suggestion is the best solution. If the user wants to kill a specific job, he will probably do that manually...
Uh, what the hell happened here... My commit accidentally overwrote & closed this Diff.
Phab tells me as cause:
We didn't find a "Differential Revision" field in the commit message. This commit and the active diff of D8199 had the same tree hash (fc22669292a8d6e76b63b6a298abf97e07703071) so we linked this commit to D8199.
@croick Can you re-upload your patch by updating this Diff?
I did a rebase before the last time I updated the diff and didn't really check which files were included. Maybe Phabricator was not yet aware of your commit. It supports rebasing, right?