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

Support shell integration in oh-my-zsh and powerlevel10k #146587

Closed
Igorgro opened this issue Apr 1, 2022 · 12 comments · Fixed by #152995
Closed

Support shell integration in oh-my-zsh and powerlevel10k #146587

Igorgro opened this issue Apr 1, 2022 · 12 comments · Fixed by #152995
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan terminal-shell-integration Shell integration infrastructure, command decorations, etc. terminal-shell-zsh An issue in the terminal specific to zsh, including shell integration
Milestone

Comments

@Igorgro
Copy link

Igorgro commented Apr 1, 2022

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

  • VS Code Version: 1.66
  • OS Version: ArchLinux

Shell integration doesn't work with oh-my-zsh and powerlevel10k. The indicator is only shown for the first line and the disappear. Another problem is the warning from powerlevel10k about instant prompt (see attached screenshot)
Steps to Reproduce:

  1. Install zsh
  2. Install oh-my-zsh and powerlevel10k theme
  3. Run vscode and start new zsh terminal
  4. Try to run some commands

Screenshot:

image

@juan-calle
Copy link

Having the same issue.

@felipecrs
Copy link
Contributor

Refs romkatv/powerlevel10k#1827

Out of curiosity, P10K explicitly disables the integration (as it does not work anyway and yet makes the prompt slower).

So, if someone tries to debug this situation, make sure to comment out the following line before testing:

https://github.com/romkatv/powerlevel10k/blob/6b128d48d675509666ff222eb08922cc6a7b6753/internal/p10k.zsh#L9245

P10k is extensively used (has almost 30k stars), so it would be very nice if VS Code can manage to make the Shell Integration work with it.

@Tyriar
Copy link
Member

Tyriar commented Jun 10, 2022

We'd like to support it, the summary of the discussion/situation around this was:

  • We're using custom sequences to control everything and extend the shell integration. Since we launch the process we can guarantee we're in VS Code (except for edge cases).
  • VS Code's shell integration will eventually be enabled by default, it works by wrapping your PS1, PROMPT_COMMAND and eventually bash-preexec.
  • In order to have the most impact, we aim to support most shells so more users get it by default, with a manual setup option for complex shells and sub shells. This will come in the form of essentially running the script manually in your rc file and disabling the automatic injection.
  • Powerlevel10k is complex, it handles shell integration used by other terminals manually. I don't blame @romkatv for not being keen on our solution if it means he needs to implement a VS Code-specific solution. After a bunch of back and forth I recommended we all take a step back, disable it explicitly for now and let us stabilize more so we don't rush into something we will regret.

We will probably at some point document the sequences (it's still getting stabilized and subject to change) or maybe support the more common sequences which would let it just work without additional changes. My main concern with supporting multiple shell integration protocols is them conflicting with each other, but that's probably the right thing to do and we could have a switch which disables the other protocol.

@Tyriar Tyriar changed the title Shell integration doesn't work with oh-my-zsh and powerlevel10k Support shell integration in oh-my-zsh and powerlevel10k Jun 10, 2022
@Tyriar Tyriar added feature-request Request for new features or functionality terminal-shell-integration Shell integration infrastructure, command decorations, etc. terminal-shell-zsh An issue in the terminal specific to zsh, including shell integration labels Jun 10, 2022
@Tyriar Tyriar added this to the Backlog milestone Jun 10, 2022
@Tyriar
Copy link
Member

Tyriar commented Jun 10, 2022

maybe support the more common sequences which would let it just work without additional changes

Haven't tested this much but this seems promising after some trivial changes:

image

Not sure yet why we're not getting the cwd sequence, I'm forcing shell integration on via export ITERM_SHELL_INTEGRATION_INSTALLED=Yes

@Tyriar
Copy link
Member

Tyriar commented Jun 10, 2022

Another problem is there doesn't seem to be a nice way to extract right prompt or continuations so this ends up happening:

Screen Shot 2022-06-10 at 1 41 39 pm

We could make some guesses by examining the content of the cells but that's not ideal. It would be a shame if we lost the fairly reliable extraction of the command since a lot of the interesting future functionality will be built on that like extension APIs and autocomplete.

@romkatv
Copy link

romkatv commented Jun 10, 2022

In general in zsh it's impossible to extract the command from the data written to the terminal. The command isn't always visible. For example, try entering a command that's taller than the height of the terminal (use ESC Enter or Alt-Enter to insert a line break). Or try running setopt singlelinezle and entering a command that's longer than the width of the terminal. If a terminal needs to know the command being executed, it'll have to provide an extension so that zsh can supply the command explicitly.

@romkatv
Copy link

romkatv commented Jun 10, 2022

I don't blame @romkatv for not being keen on our solution if it means he needs to implement a VS Code-specific solution.

I'm willing to implement a VS Code-specific solution but I would need you to provide a spec of VS Code terminal extensions and promise backward compatibility.

@Tyriar
Copy link
Member

Tyriar commented Jun 10, 2022

it'll have to provide an extension so that zsh can supply the command explicitly.

The feature that exposes the commands is not particularly discoverable but it works pretty great for the general case in all shells at the moment, I know there are edge cases when it will fall apart though. For pwsh the column and line are not reliable because of conpty so we do pass through the command line there and it works well:

image

I'm willing to implement a VS Code-specific solution but I would need you to provide a spec of VS Code terminal extensions and promise backward compatibility.

Great, I wouldn't want you to without us committing to it 👍. We're still not at the stage where reliability will become more important so we're just passively thinking about this problem atm. I'll let you know if we'd like you to do that.

For now we can just disable the parts that need the command which will still give us the markers/command navigation.


I have a couple of questions that would save me a bunch of time if you know:

  • I see the window icon/title is coming through in OSC 2 ; ... BEL and OSC 1 ; ... BEL, is there a trick to getting the cwd to be sent via something like OSC 9;9, OSC 1337 ; CurrentDir= or OSC 7 ;?
  • Is export ITERM_SHELL_INTEGRATION_INSTALLED=Yes the right way to test against?

@romkatv
Copy link

romkatv commented Jun 10, 2022

Great, I wouldn't want you to without us committing to it 👍. We're still not at the stage where reliability will become more important so we're just passively thinking about this problem atm. I'll let you know if we'd like you to do that.

Sounds good.

  • I see the window icon/title is coming through in OSC 2 ; ... BEL and OSC 1 ; ... BEL, is there a trick to getting the cwd to be sent via something like OSC 9;9, OSC 1337 ; CurrentDir= or OSC 7 ;?

Powerlevel10k doesn't set terminal title and doesn't report the current directory. These things aren't related to prompt and can be done separately. Powerlevel10k provides only prompt.

  • Is export ITERM_SHELL_INTEGRATION_INSTALLED=Yes the right way to test against?

The best way is to set POWERLEVEL9K_TERM_SHELL_INTEGRATION=true. No need to export it. ITERM_SHELL_INTEGRATION_INSTALLED=Yes is normally set by the native integration of iTerm2. It doesn't work with Powerlevel10k, so Powerlevel10k undoes that and enables its own integration.

@Tyriar
Copy link
Member

Tyriar commented Jun 23, 2022

I put a PR out which should make the upcoming release #152995. With it, shell integration should work with powerlevel10k if it emits the more common standard OSC 133 A, B, C, D sequences. This won't give all the features but it will turn on command decorations with exit status, command navigation (ctrl/cmd+up/down) and the overview ruler.

@romkatv if you could emit the OSC sequences for VS Code that would be great, checking $TERM_PROGRAM = vscode should be a reliable way of doing that. I'll let you know if we end up committing and documenting the sequences we use, in which case a command line sequence could light up more features.

@romkatv
Copy link

romkatv commented Jun 23, 2022

I put a PR out which should make the upcoming release #152995. With it, shell integration should work with powerlevel10k if it emits the more common standard OSC 133 A, B, C, D sequences.

Awesome! Have you tried it? Powerlevel10k emits OSC 133 if you set POWERLEVEL9K_TERM_SHELL_INTEGRATION=true. It's a good idea to verify that it works with POWERLEVEL9K_TRANSIENT_PROMPT=always, too.

checking $TERM_PROGRAM = vscode should be a reliable way of doing that

Powerlevel10k emits OSC 133 if users explicitly request it with POWERLEVEL9K_TERM_SHELL_INTEGRATION=true, or if they activate the built-in shell integration in their terminals. Powerlevel10k detects the latter, undoes the damage if necessary, and turns on its own OSC 133 implementation. Currently this is implemented for iTerm2 and Kitty and I could implement the same logic for vscode: If VSCODE_SHELL_INTEGRATION is set when powerlevel10k is sourced, unset it and enable OSC 133. Does this sound reasonable?

@Tyriar
Copy link
Member

Tyriar commented Jun 23, 2022

@romkatv I installed the stable build of powerlevel10k and POWERLEVEL9K_TERM_SHELL_INTEGRATION=true never seems to work, ITERM_SHELL_INTEGRATION_INSTALLED=Yes turns on OSC 133 though. POWERLEVEL9K_TRANSIENT_PROMPT=always when the ITERM_... one is set works fine.

Currently this is implemented for iTerm2 and Kitty and I could implement the same logic for vscode: If VSCODE_SHELL_INTEGRATION is set when powerlevel10k is sourced, unset it and enable OSC 133. Does this sound reasonable?

That's perfect 👍. We have started using that variable to prevent recursing or calling the scripts multiple times, ie. so it works when it's injected by VS Code and installed manually:

# Prevent the script recursing when setting up
if [ -n "$VSCODE_SHELL_INTEGRATION" ]; then
builtin return
fi

@vscodenpa vscodenpa added the unreleased Patch has not yet been released in VS Code Insiders label Jun 23, 2022
@vscodenpa vscodenpa added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jun 24, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan terminal-shell-integration Shell integration infrastructure, command decorations, etc. terminal-shell-zsh An issue in the terminal specific to zsh, including shell integration
Projects
None yet
7 participants