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

pwsh shell integration issues on Windows #155315

Closed
pfitzseb opened this issue Jul 15, 2022 · 14 comments
Closed

pwsh shell integration issues on Windows #155315

pfitzseb opened this issue Jul 15, 2022 · 14 comments
Assignees
Labels
under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@pfitzseb
Copy link
Contributor

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version: 1.69.1 and 1.70.0-insider (1cd90cc)
  • OS Version: Windows_NT x64 10.0.22000

Steps to Reproduce:

  1. Start pwsh and type a few commands
  2. Note that
  • the Copy Output and Copy Command menu entries are missing in some cases
  • gutter decorations sometimes appear offset by a line until the next command is entered
  • Copy Output may miss the last line of a multi-line output
  • Copy Output for multi-line inputs may include part of the command (see last recording)

As far as I can tell these issues are specific to Windows; shell integration works great on Linux with multiple shells. I ran into this because the Julia extension also injects the relevant CSI633 codes for the integrated REPL, but the VS Code UI is broken on Windows (which may entirely be due to Julia doing weird full-prompt refreshes on Windows or something like that, but could also be due to conhost or something).

1-69-1.webm
insiders.webm
multi-line.webm
@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

I ran into this because the Julia extension also injects the relevant CSI633 codes for the integrated REPL

Oh, I had no idea about this. We haven't formalized the sequences yet so I wouldn't have recommended adopting them but kind of cool to see you did. Windows needs some special handling due to conpty so I'm not surprised it doesn't work. There are also some cases where it won't work until we get fixes from conpty (microsoft/terminal#8698).

How does the Julia shell work exactly? Are you launching pwsh (as in the gifs above) or is it a julia repl? And if it's the former, why not call our shell-integration.ps1?

(I know nothing about Julia 😅)

@Tyriar Tyriar added this to the July 2022 milestone Jul 18, 2022
@Tyriar Tyriar added the under-discussion Issue is under discussion for relevance, priority, approach label Jul 18, 2022
@pfitzseb
Copy link
Contributor Author

Oh wow, I'm pretty sure I'm running into that order-of-operations issue too, judging by the Terminal: Toggle Escape Sequence Logging output.

How does the Julia shell work exactly? Are you launching pwsh (as in the gifs above) or is it a julia repl? And if it's the former, why not call our shell-integration.ps1?

It's a Julia REPL; we're not going through any shell at all. I'm basically injecting the appropriate codes as prompt prefixes/suffixes and \e633;C\a before evaluation starts. This is what it looks like currently:
Peek 2022-07-18 20-04.webm

There's basically only the issue that Julia doesn't have the notion of a secondary prompt, so multi-line inputs directly into the REPL don't work properly. For inline evaluation (which also produces those result decorations in the editor) it works fine of course, because I can control what exactly is echo'ed into the terminal. I tried fixing that with \e633;E;$(currentcommand)\a, but that also doesn't seem to handle line-breaks properly.
Then again, the zsh integration doesn't do linebreaks well either:
Peek 2022-07-18 20-10.webm

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

Very cool 👍

I tried fixing that with \e633;E;$(currentcommand)\a, but that also doesn't seem to handle line-breaks properly.

Adding this will fix a bunch of problems you're seeing, currently we do some pretty terrible encoding of the message to support some special characters:

private _deserializeMessage(message: string): string {
return message
.replace(/<LF>/g, '\n')
.replace(/<CL>/g, ';')
.replace(/<ST>/g, '\x07');
}

This is one of the things that is expected to change when we formalize it.

You will also want to make sure you send \e633;P;IsWindows=True\a when the backend (not the client if connected remotely), this will activate a bunch of workarounds specifically for conpty.

Then again, the zsh integration doesn't do linebreaks well either:

This looks like a bug, we did just start sending the full command line via 633 E in zsh and bash as well which should fix this, but I'd still like to fix it if we don't get the command line sequence.

builtin printf "\033]633;E;$__vsc_current_command\007"


I'll try remember to ping you when we tweak the protocol, if you're on Insiders you'll probably notice before it hits stable though.

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

Also I'm guessing run recent command doesn't work for Julia, created #155530 as a follow up there

@pfitzseb
Copy link
Contributor Author

pfitzseb commented Jul 18, 2022

Also I'm guessing run recent command doesn't work for Julia, created #155530 as a follow up there

Yup. It does work for the current session, of course, which is pretty cool already :)

I'll implement the command passing (with escaping) and the IsWindows code (which I'm pretty sure I tried early and which made things slightly better).

One more thing -- let me know if you want me to open a separate issue -- is that the decoration is printed multiple times when the prompt is refreshed:
Peek 2022-07-18 20-42
Here, Julia is changing the prompt color as a visual bell like thingy when hitting backspace on an empty command.

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

PRs welcome for #155530 🙂, not totally sure how the keys should work here. We could go off the terminal name, that would separate for versions in the case of "Julia REPL (v1.8.0-rc3)", so maybe there should be a new shell integration sequence to indicate the shell type? 🤔

One more issue -- let me know if you want me to open a separate issue -- is that the decoration is printed multiple times when the prompt is refreshed:

@pfitzseb that's definitely a bug, we've been trying to stamp out all possible cases where that happens but I guess the way you're sending the sequences is triggering it again. Are you by chance sending 633 D again when the prompt is reprinted? Could you create a new issue for this and include the escape sequence logs? That should be enough to repro out without needing to set up Julia.

@pfitzseb
Copy link
Contributor Author

Are you by chance sending 633 D again when the prompt is reprinted?

Yes, I'm currently unconditionally printing that as a prompt prefix. Easy fix though:
Peek 2022-07-18 20-58

Also makes the no-command case a bit nicer. Thanks for the pointer!

so maybe there should be a new shell integration sequence to indicate the shell type?

Makes sense to me! I might get around to a PR at some point.

@pfitzseb
Copy link
Contributor Author

Ok, multi-line commands now work as well:
Peek 2022-07-18 21-15

It's possible I messed the ordering up the first time around (ie sending 633 E before 633 C).

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

Yes, I'm currently unconditionally printing that as a prompt prefix.

Actually it should still print, just without an exit code. Was it not going into this route? https://github.com/julia-vscode/julia-vscode/blob/048b836faf32036f867506eab88964660cc55405/scripts/packages/VSCodeServer/src/repl.jl#L81-L83

It's possible I messed the ordering up the first time around (ie sending 633 E before 633 C).

That would make sense, 633 C overwrites the command so E must come after.

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

For the "empty command" part, you can turn on escape sequence logging and see how it works in bash/zsh for the ideal case

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

I think this issue is resolved then, there are some known issues with windows and shell integration even on pwsh, some of which we can't fix right now. Please open an issue to track any you hit and feel free to reach out again with questions/clarifications regarding this stuff.

@pfitzseb
Copy link
Contributor Author

Was it not going into this route?

It was. But that also happened for each refresh, so there would be many 633 Ds for each 633 C. With julia-vscode/julia-vscode#2960 I'm only printing one 633 D after the command output; subsequent prompt refreshes don't print that sequence anymore.

For the "empty command" part, you can turn on escape sequence logging and see how it works in bash/zsh for the ideal case

Yeah, I can fix that, but I'm not sure I want to. It's kinda nice for Ctrl-CursorUp to go to the previous meaningful command.
It's a bit inconsistent though...

I think this issue is resolved then, there are some known issues with windows and shell integration even on pwsh, some of which we can't fix right now. Please open an issue to track any you hit and feel free to reach out again with questions/clarifications regarding this stuff.

Will do, thanks a lot for your help!

@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

Yeah, I can fix that, but I'm not sure I want to. It's kinda nice for Ctrl-CursorUp to go to the previous meaningful command.
It's a bit inconsistent though...

Maybe we should refine that behavior to skip empty commands, while still respecting ctrl/cmd+shift+up to select only the previous non-empty command's output. Without the empty commands it would also copy the extra prompts when copying output or selecting via command navigation.

@Tyriar Tyriar closed this as completed Jul 18, 2022
@Tyriar
Copy link
Member

Tyriar commented Jul 18, 2022

Created #155543 for polishing empty commands

@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants