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

[Fix] onDidEndTaskProcess event is not fired for plugins when task ends #8141

Merged
merged 1 commit into from
Jul 9, 2020
Merged

[Fix] onDidEndTaskProcess event is not fired for plugins when task ends #8141

merged 1 commit into from
Jul 9, 2020

Conversation

tal-sapan
Copy link
Contributor

Signed-off-by: Tal Sapan [email protected]

What it does

According to VSCode documentation, when a task ends, the events onDidEndTask and onDidEndTaskProcess events are fired. onDidEndTaskProcess is only fired for tasks that execute an underlying process.
However, the onDidEndTaskProcess event is not fired in Theia.
This PR fixes this issue by firing the event for all tasks of type ProcessTask.

Note that the order of firing the two events is important: onDidEndTaskProcess must be fired first because when onDidEndTask is fired, the task execution is removed from the list of currently running task executions in $onDidEndTask and not recognized in $onDidEndTaskProcess:

this.executions.delete(id);

How to test

  1. Run this plugin in theia: https://github.com/tal-sapan/logtaskendprocessevent (or use the vsix I will attach in a comment).
  2. Run any shell or process task, for example:
{
    "label": "Hello World",
    "type": "shell",
    "command": "echo hello"
}
  1. Open the LogTaskEndProcessEvent output channel and see in the output:
    Task process ended with exit code 0: Hello World

Without this PR nothing is printed to the channel output.

Review checklist

Reminder for reviewers

@tal-sapan
Copy link
Contributor Author

vsix file of the plugin used for testing (change the file extension to .vsix instead of .vsix.zip):
logtaskendprocessevent-0.0.1.vsix.zip

});

if (task && task instanceof ProcessTask && task.processType === 'process') {
if (task && task instanceof ProcessTask) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (task instanceof ProcessTask) should be enough.

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 will change it.
However, it is the same as in fireTaskCreatedEvent method. Should I change it there too?

@vince-fugnitto vince-fugnitto added the tasks issues related to the task system label Jul 6, 2020
@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Jul 7, 2020
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.

The changes look good for me.
Tested for a shell and npm tasks using provided extension and can see the log:
Task process ended with exit code 0: test task

test_event_task

@tal-sapan
Copy link
Contributor Author

Hi @RomanNikitenko,

Thanks for the review.
Would it be possible to merge the PR? Or should another person review it first?

Thanks,
Tal

@RomanNikitenko
Copy link
Contributor

@tal-sapan
Thank you for the fix!

I'm going to merge the PR tonight for case if someone wants to review/test it today.

@RomanNikitenko RomanNikitenko merged commit 37e0437 into eclipse-theia:master Jul 9, 2020
@tal-sapan tal-sapan deleted the fix_ondidendtaskprocess__fire branch July 9, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants