Skip to content

fix for "Cannot Create keybinding to restart a specific Task" #36474

Closed
vemoo wants to merge 9 commits intomicrosoft:masterfrom
vemoo:master
Closed

fix for "Cannot Create keybinding to restart a specific Task" #36474
vemoo wants to merge 9 commits intomicrosoft:masterfrom
vemoo:master

Conversation

@vemoo
Copy link
Contributor

@vemoo vemoo commented Oct 18, 2017

Issue: #35910.

Also fixes a couple of related issues in src/vs/workbench/parts/tasks/node/processTaskSystem.ts task termination.

@msftclas
Copy link

msftclas commented Oct 18, 2017

CLA assistant check
All CLA requirements met.

@dbaeumer dbaeumer added this to the October 2017 milestone Oct 19, 2017
@vemoo
Copy link
Contributor Author

vemoo commented Nov 12, 2017

I fixed some typos in some translation keys in this commit becase at the time I could't find any translations in the repository. But now I see there are translations, should I revert that change? or update the keys in the translations also?

@dbaeumer
Copy link
Member

There is no need to revert nor to update the translation. This happens automatically when we pull the next time from our translation database.

@dbaeumer dbaeumer modified the milestones: October 2017, November 2017 Nov 13, 2017
@alexdima alexdima modified the milestones: November 2017, December 2017 Dec 8, 2017
@vemoo
Copy link
Contributor Author

vemoo commented Dec 17, 2017

After using vscode for a while I realised this pull request introduces some changes when stopping and restarting tasks.

Right now when terminating or restarting a task, the following happens:

  • in the old task system (0.1.0) the task is terminated or restarted directly
  • in the new task system (2.0.0) even if there's only one task running a quick pick is always show

In this pull request I unified the code so both stop or terminate the task without asking the user if there's only one running.
What's the intended behaviour?

Also, if an argument is passed but no task is found, should there be some kind of notification for the user?

@dbaeumer
Copy link
Member

@vemoo

In tasks 2.0 we always prompt on terminate even if only one task is running. Since task 2.0.0 can run multiple task in parallel we need a dialog for n >= 2 and to make things consistent we always show the dialog. However if a keyboard short cut is bound to terminate a specific task no picker should be shown.

If an argument is passed but the task is not found I would show a picker with the currently running tasks. This is consistent with a short cut to executing a task.

@alexdima alexdima modified the milestones: January 2018, February 2018 Feb 2, 2018
@vemoo
Copy link
Contributor Author

vemoo commented Feb 10, 2018

Is there anything else I can do to help this pull request get merged?

@dbaeumer
Copy link
Member

@vemoo actually I was not sure if your PR addresses these two issues:

However if a keyboard short cut is bound to terminate a specific task no picker should be shown.

If an argument is passed but the task is not found I would show a picker with the currently running tasks. This is consistent with a short cut to executing a task.

@vemoo
Copy link
Contributor Author

vemoo commented Feb 19, 2018

Yes, with this PR, when terminating a task with an argument

  • If the task is found: it's terminated without showing the picker
  • If the task is not found: the currently running tasks are show in the picker

@bpasero bpasero modified the milestones: February 2018, March 2018 Mar 8, 2018
@vemoo
Copy link
Contributor Author

vemoo commented Mar 11, 2018

I've rebased onto current upstream master, and fixed the conflict.
Now the changes in this PR also use the "notificationService" instead of the "messageService".

@bpasero bpasero removed this from the March 2018 milestone Apr 6, 2018
@bpasero bpasero added this to the April 2018 milestone Apr 6, 2018
@bpasero bpasero modified the milestones: April 2018, May 2018 May 8, 2018
@dbaeumer
Copy link
Member

The fix for the arg is OK, however there is #47853 which adds a more generic way to skip zero task prompts. Will look into merging the two PRs.

@dbaeumer dbaeumer modified the milestones: May 2018, June 2018 May 30, 2018
@dbaeumer
Copy link
Member

dbaeumer commented Jun 7, 2018

A lot has changed here especially the fact to be able to reference a task via a task identifier. So I merged the PR by hand and modified it to:

  • use the picker directly when no arg is provided. This ensures that we have proper progress
  • use task identifier
  • only available for task 2.0.0

@dbaeumer dbaeumer closed this Jun 7, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants