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

add shell integration script for fish #157291

Merged
merged 17 commits into from Aug 9, 2022
Merged

add shell integration script for fish #157291

merged 17 commits into from Aug 9, 2022

Conversation

zgracem
Copy link
Contributor

@zgracem zgracem commented Aug 5, 2022

Partially addresses #139400.

How to test

Add the following to the end of $__fish_config_dir/config.fish, modifying the value of VSCODE appropriately for your system:

set -l VSCODE ~/github/vscode/src
. "$VSCODE/vs/workbench/contrib/terminal/browser/media/shellIntegration.fish"

What works

What needs testing

  • code --locate-shell-integration-path fish to assist manual installation
    • I believe I added this correctly, but I don't have the ability to build and test it myself on this machine.

What doesn't work yet

What I haven't figured out

@ghost
Copy link

ghost commented Aug 5, 2022

CLA assistant check
All CLA requirements met.

@spasche
Copy link
Contributor

spasche commented Aug 6, 2022

Great work! Small suggestion: I think it would be nice if the fish script was formatted according to fish_indent rules (i.e. spaces instead of tabs).

(I noticed this when I copied the script to my dotfiles repo, which has a pre-commit hook).

@zgracem
Copy link
Contributor Author

zgracem commented Aug 8, 2022

@spasche You'll have to take that up with their "Compile & Hygiene" CI task, I'm afraid.

@spasche
Copy link
Contributor

spasche commented Aug 8, 2022

Ok, I see. Better be consistent with the local style then (and users won't need to copy the script in their dotfiles once this is merged).

For the CI hygiene copyright failure, you may want to add an exclusion for *.fish there:

'!**/*.ico',

This appears consistent with the treatment of other shell scripts.
@zgracem
Copy link
Contributor Author

zgracem commented Aug 8, 2022

@spasche Looks like that worked to appease the CI! Thanks for the tip!

@Tyriar Tyriar added this to the August 2022 milestone Aug 8, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code --locate-shell-integration-path fish to assist manual installation
I believe I added this correctly, but I don't have the ability to build and test it myself on this machine.

I'll queue up a product build to test now 👍

Automatic injection without manual installation
This might be over my head, tbh.

This is fine, I wasn't expecting you to do this as it got pretty complicated for each shell we attempted this on.

Command navigation for multi-line commands

No need to do this, I'm marking the continuation and right prompt sequences as non-final (#157571) and they may end up getting removed. If 633 E is sent they don't do anything currently.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for doing this, just a few comments

Comment on lines 41 to 43
function __vsc_cmd_finished --on-event fish_postexec
__vsc_esc D $status
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no command ran, just __vsc_esc D should run without an exit code. That will make the below be circle outlines instead of blue:

Screen Shot 2022-08-08 at 3 53 58 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to have been addressed by adding __vsc_cmd_clear hooked into the fish_cancel event.

Copy link
Contributor

@kidonng kidonng Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zgracem This doesn't seem to be completely resolved. This is zsh:

image

And this is with Fish script from this PR:

image

As you can see the fish_cancel event is only fired for Ctrl+C but not when the commandline is empty, which only triggers a fish_prompt event. My plugin added a extra check for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kidonng nice observation, do you want to send a PR since it's your solution? I can fix it up otherwise

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad to!

@Tyriar
Copy link
Member

Tyriar commented Aug 8, 2022

I looked into automatic injection, it seemed like it was going to be easy but I couldn't figure out why fish -c "command" and fish -C "command" only work in fish sub-shells. If we're able to figure out the right args for the below profile to get this echoing hello world on startup then we should be able to do it:

    "fish si": {
      "path": "/usr/local/bin/fish",
      "args": [
        "-C",
        "echo hello world"
      ]
    }

See how only "inner shell", not "first fish" appears below:

Screen Shot 2022-08-08 at 4 34 57 pm

@zgracem
Copy link
Contributor Author

zgracem commented Aug 9, 2022

@Tyriar I can't seem to reproduce the problem you're having with fish -c:

image

@Tyriar
Copy link
Member

Tyriar commented Aug 9, 2022

I can't seem to reproduce the problem you're having with fish -c:

@zgracem weird, could this be related to some config you have? Mine was empty

@Tyriar
Copy link
Member

Tyriar commented Aug 9, 2022

The CLI worked, just the file wasn't included in the product build which should be fixed with 327773c

@Tyriar
Copy link
Member

Tyriar commented Aug 9, 2022

Merged main to test the serialization change, it works 👏

Screen Shot 2022-08-09 at 2 34 45 pm

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! This should be available in Insiders the day after it's merged 🚀

I'll write up the install section on the website now

@Tyriar Tyriar merged commit a7f9339 into microsoft:main Aug 9, 2022
@zgracem zgracem deleted the fish-shell-integration branch August 10, 2022 15:12
joyceerhl pushed a commit that referenced this pull request Aug 10, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants