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

add warning when shell integration fails and create a terminal #152266

Merged
merged 14 commits into from Jun 21, 2022

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Jun 15, 2022

This PR fixes #151934

@meganrogge meganrogge marked this pull request as draft June 15, 2022 22:55
@meganrogge meganrogge self-assigned this Jun 15, 2022
@meganrogge meganrogge added this to the June 2022 milestone Jun 15, 2022
@meganrogge meganrogge marked this pull request as ready for review June 16, 2022 18:29
@meganrogge meganrogge requested a review from Tyriar June 16, 2022 18:29
meganrogge and others added 3 commits June 16, 2022 16:08
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
@meganrogge
Copy link
Contributor Author

ideally, the single tab would have the action too, but not possible ATM. worth doing @Tyriar ?

@Tyriar
Copy link
Member

Tyriar commented Jun 17, 2022

@meganrogge I think that's tracked elsewhere, fine to leave that out

@Tyriar
Copy link
Member

Tyriar commented Jun 17, 2022

When I add exit 100 to the bash shell integration script the terminal is created and closes, relaunching without injection doesn't seem to happen for me?

@meganrogge
Copy link
Contributor Author

think I see what's wrong. fixing

@meganrogge
Copy link
Contributor Author

meganrogge commented Jun 17, 2022

I was only accounting for the case when createProcess returns an error. Now also handle when the process exits during launch or is killed by the process when shell integration injection is used

@meganrogge meganrogge requested a review from Tyriar June 17, 2022 18:35
@meganrogge
Copy link
Contributor Author

these tests are failing consistently will investigate on Monday

@Tyriar
Copy link
Member

Tyriar commented Jun 17, 2022

I think the warning on the tab and hover looks great. Seeing exit code undefined when using exit 100 at the end of the script though:

Screen Shot 2022-06-17 at 4 04 21 pm

Could be related to the test failure:

  1) vscode API - terminal
       Terminal
         Extension pty terminals
           exitStatus.code should be set to the exit code (non-zero)

meganrogge and others added 2 commits June 20, 2022 15:13
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
@Tyriar
Copy link
Member

Tyriar commented Jun 21, 2022

Tweaked the message:
Screen Shot 2022-06-21 at 11 37 49 am

@Tyriar Tyriar merged commit b64ba3d into main Jun 21, 2022
@Tyriar Tyriar deleted the merogge/shell-int-failure-relaunch branch June 21, 2022 19:12
@github-actions github-actions bot locked and limited conversation to collaborators Aug 5, 2022
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.

Detect shells that fail to launch when shell integration injection fails
2 participants