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.

Sign up for GitHub

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

Jump to bottom

Add 'tasks.onDidStartTask' for Plug-in API #3826

Merged
merged 1 commit into from
Jan 3, 2019
Merged

Add 'tasks.onDidStartTask' for Plug-in API #3826

merged 1 commit into from
Jan 3, 2019

Conversation

RomanNikitenko
Copy link
Contributor

@RomanNikitenko RomanNikitenko commented Dec 13, 2018 •
edited
Loading

Add:

  • 'tasks.onDidStartTask' for Plug-in API
  • ability to terminate task from plugin side

Example:

let currentTaskExecution: theia.TaskExecution;

theia.tasks.onDidStartTask(event => {
    currentTaskExecution = event.execution;
});

theia.commands.registerCommand({
    id: 'terminateTask',
    label: 'Terminate task'
}, (...args: any[]) => {
    currentTaskExecution.terminate();
});

Video: https://youtu.be/AMfN-Pdiww0

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

@RomanNikitenko RomanNikitenko added the plug-in system issues related to the plug-in system label Dec 13, 2018
benoitf
benoitf reviewed Dec 14, 2018
View reviewed changes
packages/plugin-ext/src/main/browser/tasks-main.ts Outdated
this.taskWatcher = container.get(TaskWatcher);
this.taskService = container.get(TaskService);

this.workspaceService.roots.then(async roots => {
Copy link
Contributor

Choose a reason for hiding this comment

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

hello, where is async part in this method ? (as there is async keyword)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benoitf fixed, thank you!

packages/plugin-ext/src/plugin/tasks/tasks.ts Outdated
} from '../../api/plugin-api';
import * as theia from '@theia/plugin';
import * as Converter from '../type-converters';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say to use lowercase there

import * as converter from ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benoitf fixed

benoitf
benoitf reviewed Dec 14, 2018
View reviewed changes
packages/task/src/browser/task-service.ts
@@ -233,4 +233,16 @@ export class TaskService implements TaskConfigurationClient {
protected getContext(): string | undefined {
return this.workspaceRootUri;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@simark @akosyakov notifying you as there are changes not only in plugin-ext packages but also in packages/task

Copy link
Member

Choose a reason for hiding this comment

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

ok with me

simark
simark reviewed Dec 14, 2018
View reviewed changes
Copy link
Contributor

@simark simark left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM for the task part. One little comment about the ChangeLog.

CHANGELOG.md Outdated
@@ -6,6 +6,7 @@
- [core] Add a way to prevent application exit from extensions.
- [core] Prevent application exit if some editors are dirty.
- [languages] Add a preference for every language contribution to be able to trace the communication client <-> server
- [plug-in] added `tasks.onDidStartTask` Plug-in API
Copy link
Contributor

Choose a reason for hiding this comment

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

added -> Add (capital letter and present tense)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simark fixed, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simark I fixed your comment and updated (rebased) my branch, can I merge it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. Based on #3897, it seems like you may have to go back to what you had originally :). We'll see what we decide in the end.

Copy link
Contributor Author

@RomanNikitenko RomanNikitenko Dec 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@simark I changed to original condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simark could you review - can I merge my PR?

@RomanNikitenko RomanNikitenko force-pushed the startTaskEvent branch 2 times, most recently from 0ecf27f to 4c7efa6 Compare December 26, 2018 09:24
@simark
Copy link
Contributor

simark commented Dec 26, 2018

That's fine with me. Just wondering, are there some tests that ensure the plugin API (typing and behavior) doesn't change?

And just a note, the behavior of the "shell" task is going to change soon-ish (I've been slowly working on that for a while). Those tasks are supposed to be executed through a shell, so that you can do things like echo foo && echo bar, and they aren't right now. The goal is to be compatible with VSCode tasks (as described here) as much as possible.

@RomanNikitenko
Add 'tasks.onDidStartTask' for Plug-in API
81d4712 
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
@RomanNikitenko
Copy link
Contributor Author

That's fine with me. Just wondering, are there some tests that ensure the plugin API (typing and behavior) doesn't change?

I added in current PR ability to track on the plugin side case when a task has started. I added also a couple of tests to be sure we correctly convert task.

And just a note, the behavior of the "shell" task is going to change soon-ish (I've been slowly working on that for a while). Those tasks are supposed to be executed through a shell, so that you can do things like echo foo && echo bar, and they aren't right now. The goal is to be compatible with VSCode tasks (as described here) as much as possible.

Cool!
I think we can adapt appropriate parts of code when the specified work is finished.

@RomanNikitenko RomanNikitenko mentioned this pull request Dec 27, 2018
Closed
@RomanNikitenko RomanNikitenko merged commit a2119bc into master Jan 3, 2019
@RomanNikitenko RomanNikitenko deleted the startTaskEvent branch January 3, 2019 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoitf benoitf benoitf approved these changes

@simark simark simark approved these changes

@evidolob evidolob Awaiting requested review from evidolob

@akosyakov akosyakov Awaiting requested review from akosyakov

Assignees
No one assigned
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RomanNikitenko @simark @benoitf @akosyakov