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

Move cmd start escape to end of prompt #210443

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Conversation

grgar
Copy link
Contributor

@grgar grgar commented Apr 16, 2024

#185907 moves the cmd start escape to the end of the mode prompt, but since the mode prompt ‘appears to the left of the regular prompt’ that seems too early: declaring that the start of the cmd causes the rest of the prompt to be part of the command in the recent commands list.

Screenshot 2024-04-16 at 8 24 56 AM

Moves __vsc_fish_cmd_start to the end of fish_prompt so the prompt is not part of the command. Tested that the prompt no longer appears in the recent commands and running the command from the recent list no longer tries to execute the prompt as the command.

Since the mode prompt 'appears to the left of the regular prompt', declaring that the start of the cmd causes the rest of the prompt to be part of the command.
@grgar
Copy link
Contributor Author

grgar commented Apr 16, 2024

Seems to be a more recent issue than #185907, implying #209946 is what actually broke it? Appears that before #209946, escape E was successfully specifying the command, but now C is before E the E no longer takes effect. However disregarding the change to E, #185907 still needs fixing in my opinion to place C in the right place. @Tyriar what testing steps took place for #209946 that I can use to reproduce what swapping the ordering of C and E did (when fish_mode_prompt is defined)?

@Tyriar Tyriar added this to the April 2024 milestone Apr 16, 2024
@Tyriar Tyriar self-assigned this Apr 16, 2024
@Tyriar
Copy link
Member

Tyriar commented Apr 17, 2024

@grgar the swapping of C and E makes the full command line available to the upcoming shellIntegration extension API. You can test it by uncommenting this part and then inspect the event object in devtools:

https://github.com/grgars/vscode/blob/25a72f2d208e1fa5408606693308e581f9bc6be5/src/vs/workbench/api/common/extHostTerminalShellIntegration.ts#L61-L73

It should show the correct command line with a confidence of 2.

I didn't do much testing outside of powershell and I tried just now to add a fish mode prompt but I don't think I could get it working. So if you can verify that your proposal works then we're good to merge 👌

@Tyriar Tyriar enabled auto-merge April 17, 2024 15:21
@Tyriar Tyriar merged commit 475b574 into microsoft:main Apr 17, 2024
6 checks passed
cpendery pushed a commit to cpendery/vscode that referenced this pull request Apr 17, 2024
@grgar
Copy link
Contributor Author

grgar commented Apr 17, 2024

Thanks @Tyriar, that was indeed very convenient test code!
Used that code to test before and after, now works with a fish_mode_prompt (as did trying recent commands previously):

Screenshot 2024-04-17 at 10 44 51 PM Screenshot 2024-04-17 at 10 42 05 PM

Much appreciated showing me these additional test steps and thanks for merging.

@grgar grgar deleted the dev/fish-cmd-start branch April 17, 2024 21:47
@Tyriar
Copy link
Member

Tyriar commented Apr 17, 2024

@grgar awesome! Thanks a lot for the contribution

@microsoft microsoft locked and limited conversation to collaborators Jun 5, 2024
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.

4 participants