microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.05k stars 29.21k forks source link

Support fish shell integration automatic injection #139400

Closed Tyriar closed 1 year ago

Tyriar commented 2 years ago

Parent issue https://github.com/microsoft/vscode/issues/133084


Update: Activate the fish shell integration by following these instructions: https://code.visualstudio.com/docs/terminal/shell-integration#_manual-installation

We're keeping this open until automatic injection is looked into.

meganrogge commented 1 year ago

I had misunderstood how this injection works because it is different from that of bash and zsh. Deleting my $XDG_DATA_DIRS in the comment above was the reason I saw it not working. I now see it working.

Screenshot 2023-06-01 at 10 21 23 AM

Tyriar commented 1 year ago

@roblourens since it's working for @meganrogge and I we'll close this and include it in the release notes. It would be good to get to the bottom of your issue, we can track that in another issue? We can have a call to investigate when you're free if that works.

roblourens commented 1 year ago

Sure, call any time

mkhl commented 1 year ago
"XDG_DATA_DIRS":"/home/roblou/snap/code-insiders/1310/.local/share:/home/roblou/snap/code-insiders/1310:/snap/code-insiders/1310/usr/share:/usr/share/xfce4:/usr/local/share:/usr/share:/var/lib/snapd/desktop:/usr/share:/snap/code-insiders/1310/usr/share/code-insiders/resources/app/out/vs/workbench/contrib/terminal/browser/media/fish_xdg_data"

@mkhl any idea what could be going wrong here wrt the XDG_DATA_DIR auto activation?

Not really, sorry. Maybe the snap packaging interferes somehow?

@roblourens From your shell session, are /snap/code-insiders/1310/usr/share/code-insiders/resources/app/out/vs/workbench/contrib/terminal/browser/media/fish_xdg_data and /snap/code-insiders/1310/usr/share/code-insiders/resources/app/out/vs/workbench/contrib/terminal/browser/media/fish_xdg_data/fish/vendor_conf.d/vscode-shell-integration.fish present?

Tyriar commented 1 year ago

We investigated, it ended up being the fish version being too old. After the update it worked great! Thanks for all the help @mkhl

mkhl commented 1 year ago

This isn't working for me in 1.79.0 (non-insider), as far as I can see the fish config is there but XDG_DATA_DIRS isn't getting set.

And looking at the code I found #184364 -- what happened?

Tyriar commented 1 year ago

@mkhl we had to disable it because it was printing an error when starting up for someone on the team: https://github.com/microsoft/vscode/issues/184191

For some reason fish_prompt was undefined at the time, it got set later, after our XDG_DATA_DIRS applied.

mkhl commented 1 year ago

Looking at the configuration files order documentation again I believe this would happen when something we need, like fish_prompt, is configured in either config.fish or in a ~/.config/fish/conf.d/*.fish file with a basename that's sorted before the one we expose.

The config.fish file is intentionally read last so people can override vendor configuration, so since we need to run after user configuration maybe providing vendor configuration is just not the way to go here… sorry 😔

Maybe instead we could invoke fish with --init-command="source vscode fish integration here"? That would ensure the integration happens after any configuration but before interactive input.

If I have the spoons and noone beats me to it I'll try to whip up a PR for that next week.

zgracem commented 1 year ago

One way to make sure something runs after the user's config.fish but before interactive input is to put the necessary code in a self-erasing function tied to the fish_prompt event, e.g.:

function init_vscode_shell_integration --on-event fish_prompt
    # Run only once
    functions --erase init_vscode_shell_integration

    # Do the magic here...
end

This can be placed in the appropriate vendor-config directory as simply (e.g.) init_vscode_shell_integration.fish with no need to worry about sorting order. The function will be defined during startup, executed just before printing the first prompt, then immediately undefined so it doesn't run twice.

meganrogge commented 1 year ago

@zgracem I tried something like that thanks to your suggestion. It's not working atm and not sure why... lmk if you see what could be wrong here

https://github.com/microsoft/vscode/compare/merogge/fish-s?expand=1

zgracem commented 1 year ago

@meganrogge I think you also need to add --on-event fish_prompt to this function:

https://github.com/microsoft/vscode/blob/843cf0a5fc3c237c4fb1ee0cf9c4a9cb145a6129/src/vs/workbench/contrib/terminal/browser/media/fish_xdg_data/fish/vendor_conf.d/shellIntegration.fish#L102-L105

alexr00 commented 1 year ago

Removing verification-needed as this was covered by a test plan item.