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 presentation.reveal & focus for detected tasks #7548

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Apr 11, 2020

Signed-off-by: Liang Huang lhuang4@ualberta.ca

How to test

  1. define a detected task, e.g., npm run list
"scripts": {
  "list": "sleep 1 && ls"
}
  1. make sure npm run list is not customized in tasks.json
  2. run npm run list as a detected task. Check if the terminal widget is revealed
  3. customize the npm run list in tasks.json by adding the presentation object.
  4. Re-run the customized task, and see if the presentation works as expected.

Review checklist

@elaihau elaihau force-pushed the Liang/revealDetectedTask branch from 4c62668 to c78d8b3 Compare April 11, 2020 20:12
@vince-fugnitto vince-fugnitto added the tasks issues related to the task system label Apr 13, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified the changes using the provided test case and it worked correctly for me! 👍

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.

Tested for tsc:watch detected and configured tasks
It works as expected for me!

@RomanNikitenko
Copy link
Contributor

Looks like it's working well without these changes for configured/customized configurations, because a configuration from tasks.json file comes with default values for presentation.

configured_tasks

I guess a configuration for a detected task comes without default value for presentation field.

@elaihau
Copy link
Contributor Author

elaihau commented Apr 14, 2020

I guess a configuration for a detected task comes without default value for presentation field.

yes that's the root cause of this bug.

I will take a look if I can fix it from another angle - maybe the default values should be assigned when the tasks are retrieved from the providers.

Thank you Roman !

- default values of the taskConfig.presentation object are not taken into consideration when detected tasks are started. This pull request fixes the bug.
- fixes #7547

Signed-off-by: Liang Huang <lhuang4@ualberta.ca>
@elaihau elaihau force-pushed the Liang/revealDetectedTask branch from c78d8b3 to eeb7160 Compare April 14, 2020 19:20
@elaihau elaihau merged commit c078fb8 into master Apr 14, 2020
@elaihau elaihau deleted the Liang/revealDetectedTask branch April 14, 2020 19:46
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

task.presentation does not work properly for detected tasks
3 participants