Use `cwd` option instead of sending a `cd`

Tyriar / vscode-terminal-here

VS Code extensions that creates an integrated terminal session at the current file's directory

https://marketplace.visualstudio.com/items?itemName=Tyriar.vscode-terminal-here

33 stars 11 forks source link

Use `cwd` option instead of sending a `cd` #18

Closed DRSDavidSoft closed 5 years ago

DRSDavidSoft commented 5 years ago

Changes

better shell detection (e.g. relative cmd.exe, powershell, gitshell and cygwin in addition to wsl shell)
change method of operation; use the cwd options instead of passing a cd

Fixes

fixes #17 – i.e. cwd does not require dos-like drive change (e.g. d:)
fixes #14 – using both the suggested clear command and cwd on windows
addresses #15 – wsl.exe and ubuntu.exe are detected (albeit they all use the default /mnt)

Testing

cmd.exe: tested and works
powershell.exe: tested and works
cygwin: tested and works
git bash: tested and works
wsl/1: not tested
wsl/2: not tested

Feedback on the last two are appreciated. :)

DRSDavidSoft commented 5 years ago

Demo

`cmd.exe`

`bash.exe`

pedrofr commented 5 years ago

I recommend using clsCommand = 'echo -en "\\033[1A\\033[K\\033[1A\\r\\033[K"' or similar in the cases where the cwd option is not available as it emulates the behavior of the option, without interfering with possible .bashrc's and other previous output.

Furthermore, I'm not sure these changes work with vscode's Remote-WSL and/or WSL 2.

Great job though! I'm using your pull request version with some ugly tweaks instead of the current release. If I had any experience with node.js I'd contribute with actual code instead of this comment...

Tyriar commented 5 years ago

Furthermore, I'm not sure these changes work with vscode's Remote-WSL and/or WSL 2.

It should work provides cwd is passed in as a URI with the right scheme for the workspace. Not sure how I lost the notification for this PR but this should be done before it gets merged in now.

DRSDavidSoft commented 5 years ago

@pedrofr The escape sequences seem like a good idea! I'm not sure that echo on Windows is capable of throwing out those to the terminal (the same as echo on Linux and macOS do) -- but since Code uses xterm.js, maybe outputting them to the console using APIs would do the same.

Regarding the recent Remote containers (and WSL/2), I'd like to say that I wrote this patch before either were introduced, expecting that it would be merged soon. So I need to test this PR just to be sure.

In any case, I'd be happy to see your tweaks, as I'm sure I can learn a thing or two from your modifications.

@Tyriar No worries since you seem to be one of the most busiest people of the Code team. Thanks for all the work you do!

I actually thought that you had a better implementation in mind instead of this PR, since the APIs have had been updated since you originally made this.

I'll be glad to see input on my work, if it's still qualified to be merged.

Tyriar commented 5 years ago

Ok, I made a pretty radical change and removed almost all the code, it should just work now in various local shells as well as remote 🎆 published v0.2.0, let me know if you hit issues.

pedrofr commented 5 years ago

In any case, I'd be happy to see your tweaks, as I'm sure I can learn a thing or two from your modifications.

@DRSDavidSoft, well... I just changed your clsCommand variable to be echo -en "\\033[1A\\033[K\\033[1A\\r\\033[K", which isn't even optimal in what it tries to achieve. I reckon it is not portable, but I almost exclusively develop under WSL1/2 with vscode, so I didn't try to fit any other setup.

Anyway, now that it's merged the extension works elegantly as it should (at least for me)! :)