-
Notifications
You must be signed in to change notification settings - Fork 231
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
Use a provided shell integration script directly #2156
Conversation
This was prompted by a) being annoyed at having to keep doing this and b) the changes in microsoft/vscode@d60e424 being much more difficult to incorporate except by doing it this way! /cc @Tyriar |
src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs
Outdated
Show resolved
Hide resolved
f85e2f8
to
6342ce8
Compare
22b95ae
to
fd1bb23
Compare
_shellIntegrationEnabled = true; | ||
await EnableShellIntegrationAsync(cancellationToken).ConfigureAwait(false); | ||
// TODO: Make the __psEditorServices prefix shared (it's used elsewhere too). | ||
string setupShellIntegration = $$""" |
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 like C# 11's triple-quote double-dollar thing. Very useful.
da3894a
to
400dce5
Compare
// Invoke the PSConsoleHostReadLine wrapper. We don't write the output because it | ||
// returns the command line (user input) which would then be duplicate noise. Fortunately | ||
// it writes the shell integration sequences directly using [Console]::Write. | ||
InvokePSCommand( | ||
new PSCommand().AddScript("PSConsoleHostReadLine"), | ||
new PowerShellExecutionOptions { ThrowOnError = false, WriteOutputToHost = false }, | ||
cancellationToken); |
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.
@Tyriar just a heads-up here that your PSConsoleHostReadLine
wrapper must continue to write those escape sequences directly, as we're intentionally not emitting the output (like the returned value) to the host. Write-Host
might work (though you don't use it), but we'd need to test.
} | ||
|
||
InvokePSCommand( | ||
new PSCommand().AddScript(input, useLocalScope: false), |
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.
No real change, it's the default.
400dce5
to
6d6bc34
Compare
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.
Lookin' good, just two fixes real quick
src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs
Outdated
Show resolved
Hide resolved
Instead of having to maintain an edited copy (which was really annoying) I stubbed out `PSConsoleHostReadLine` to do what's expected. So now we can just use the existing shell integration script directly! Since we can't reliably find the script using `code --locate-shell-integration-path pwsh` we now rely on it being sent by the client on initialization. Its presence implies the feature is on. This is pretty VS Code specific, but not necessarily so. Apply suggestions from code review Thanks Patrick! Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>
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.
LGTM!
9a9f893
to
f5bc51d
Compare
Instead of having to maintain an edited copy (which was really annoying) I stubbed out
PSConsoleHostReadLine
to do what's expected. So now we can just use the existing shell integration script directly!Since we can't reliably find the script using
code --locate-shell-integration-path pwsh
we now rely on it being sent by the client on initialization. Its presence implies the feature is on.This is pretty VS Code specific, but not necessarily so.