-
Notifications
You must be signed in to change notification settings - Fork 29
Fix unexpected blocking for pwsh command activation #952
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
Conversation
| "enabledApiProposals": [ | ||
| "terminalShellEnv" | ||
| "terminalShellEnv", | ||
| "terminalDataWriteEvent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need an engine version update if the proposed API is not in stable, and a distro change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe terminalDataWriteEventis pretty old proposed api.
Also updated the distro for env extension to enable this, and updated vscode repo with the new hash: microsoft/vscode#273148.
I'd want to pin vscode version to one from this morning to make sure we have microsoft/vscode#272369 in though --> done see commit below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep it's old
src/features/terminal/utils.ts
Outdated
|
|
||
| const config = getConfiguration('terminal.integrated'); | ||
| const timeoutValue = config.get<number | undefined>('shellIntegration.timeout'); | ||
| const timeoutMs = timeoutValue === undefined || -1 ? 5000 : timeoutValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make it more consistent with getShellIntegrationTimeout by checking terminal.integrated.shellIntegration.enabled (mostly will match siInjectionEnabled) and the window's remoteAuthority for isRemote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good,, I think the remoteAuthority is from proposed api though: https://github.com/microsoft/vscode/blob/c23478af52b09c278e21e8396a0e120eb134ae3a/src/vscode-dts/vscode.proposed.resolvers.d.ts#L472
Would env.remoteName be good enough? 28f5261
| // Condition 3: Detect prompt patterns in terminal output | ||
| new Promise<boolean>((resolve) => { | ||
| let dataSoFar = ''; | ||
| const debounced = createSimpleDebounce(50, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be disposable, created #954
Co-authored-by: Daniel Imms <[email protected]>
This reverts commit 73f3f3c.
Resolves: microsoft/vscode#273017