Allow abortion of starting another NativeAppJob
ClosedPublic

Authored by croick on Oct 8 2017, 1:43 PM.

Details

Summary
  • 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
Test Plan
  • 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
Branch
multijob
Lint
No Linters Available
Unit
No Unit Test Coverage
croick created this revision.Oct 8 2017, 1:43 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 8 2017, 1:43 PM
mwolff requested changes to this revision.Oct 8 2017, 7:09 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
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
No -> Start Another
Cancel -> Cancel (can probably stay)

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

This revision now requires changes to proceed.Oct 8 2017, 7:09 PM
croick updated this revision to Diff 20499.Oct 8 2017, 9:24 PM
  • use QPointer instead of checking for existence of jobs
  • redraft message box
croick marked 4 inline comments as done.Oct 8 2017, 9:31 PM

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.

mwolff added a comment.Oct 9 2017, 8:40 PM

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 ↗(On Diff #20499)

use a QVector please

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?

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...

croick updated this revision to Diff 20543.Oct 9 2017, 11:27 PM
  • offer kill all jobs at a time
croick edited the summary of this revision. (Show Details)Oct 9 2017, 11:29 PM
croick edited the test plan for this revision. (Show Details)
croick updated this revision to Diff 20544.Oct 9 2017, 11:35 PM
croick marked an inline comment as done.
  • remove patch from rebase...
This revision was automatically updated to reflect the committed changes.
kfunk reopened this revision.Oct 13 2017, 8:28 AM
kfunk added a subscriber: kfunk.

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?

croick updated this revision to Diff 20699.Oct 13 2017, 9:24 PM
croick edited the test plan for this revision. (Show Details)

reopen diff

In D8199#154842, @kfunk wrote:

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?

mwolff accepted this revision.Oct 18 2017, 7:06 PM

ok, lgtm

This revision is now accepted and ready to land.Oct 18 2017, 7:06 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!