Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execute tasks via keybindings. #5913

Merged

Conversation

Wicked7000
Copy link
Contributor

@Wicked7000 Wicked7000 commented Aug 10, 2019

Signed-off-by: Jason Attwood jason_attwood@hotmail.co.uk

What it does

This addresses issue #5870 , allowing for shortcuts to be made to run tasks.

How to test

Create any tasks (or use the default ones that are provided at the bottom here)
Create a keybinding for that task in the following format:

{
        "keybinding": "ctrl+alt+s",
        "command": "workbench.action.tasks.runTask",
        "args": "[Task] Echo a string"
}

the args is the label of the task to execute.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added keybindings issues related to keybindings tasks issues related to the task system vscode issues related to VSCode compatibility labels Aug 12, 2019
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • please make it a proper PR that others can review and test it as well
  • please also get rid of commits which fix issues in the first commit, it should be one commit then

packages/core/src/common/keybinding.ts Outdated Show resolved Hide resolved
packages/keymaps/src/browser/keymaps-parser.ts Outdated Show resolved Hide resolved
packages/task/src/browser/task-frontend-contribution.ts Outdated Show resolved Hide resolved
@Wicked7000 Wicked7000 marked this pull request as ready for review August 18, 2019 11:19
@Wicked7000 Wicked7000 force-pushed the feature-task-keybinding branch 3 times, most recently from 3c2e33b to 203d267 Compare August 18, 2019 12:48
@akosyakov
Copy link
Member

@Wicked7000 please squash the git history, commits should be only about new features or fixing code before the PR. Commits which refactor or fix code introduced by the PR should be removed

Please see https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#checklist-commit-history

@@ -398,6 +398,17 @@ export class TaskService implements TaskConfigurationClient {
}
}

async runTaskByLabel(taskLabel: string): Promise<Boolean> {
const tasks: TaskConfiguration[] = await this.getConfiguredTasks();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be applied to detected tasks as well or getConfiguredTasks return all tasks, i.e. configured by a user and detected by extensions? cc @theia-ide/task-extension

Copy link
Contributor Author

@Wicked7000 Wicked7000 Aug 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand getConfiguredTasks only returns user configured tasks (defined in tasks.json) so if we wanted them to run detected tasks it should be 'getTasks'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to getTasks as that seems like what would be needed but should their be different priority's? As for example what if someone registers a keybind for a label that matches both a detected task and a user configured one, currently it would be whatever is first in the array I think it should be user configured first then detected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine to just use getTasks, since -IIUC- a configured task is a complete override by the user of a given detected task (cc @elaihau?)

Signed-off-by: Jason Attwood <jason_attwood@hotmail.co.uk>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes are looking good to me. @theia-ide/task-extension please find to verify 🙏

@Wicked7000
Copy link
Contributor Author

I'll revert the change use getTasks then as it seems getConfiguredTasks is sufficient ?

@RomanNikitenko
Copy link
Contributor

I'll revert the change use getTasks then as it seems getConfiguredTasks is sufficient ?

I think execution of detected tasks without customization won't work if you use getConfiguredTasks() instead of getTasks()

@RomanNikitenko
Copy link
Contributor

I would like to test the changes, please give me about 1 hour for it.
thank you!

@Wicked7000
Copy link
Contributor Author

Okay that's fine then let me know if you have any issues!

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to run:

  • detected npm task via keybindings
  • custom configured task

It works well for me!

I tested the case when we have two tasks with the same label in different tasks.json files (so - different sources).
For example, you have tasks Run tests for different projects.
The current behavior: first found task is run.
I think we could to display both tasks in menu to provide ability to select a task for running.
But looks like the current behavior is the same as VS Code behavior. So up to you.

thank you for giving me time for testing!

@akosyakov
Copy link
Member

@RomanNikitenko please merge it tomorrow, if no one objects by that time

@paul-marechal
Copy link
Member

@akosyakov should the prompt to select a task when several have the same name be done in a follow up PR?

@akosyakov
Copy link
Member

akosyakov commented Aug 20, 2019

@RomanNikitenko @marechal-p It should behave as in VS Code. Since it is a first-time contribution from @Wicked7000 maybe we accept this PR, do an issue and work on it separately? It also works nice already for standard cases.

@RomanNikitenko RomanNikitenko merged commit f46ebf4 into eclipse-theia:master Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings tasks issues related to the task system vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants