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

Don't send newlines to the shell from Terminal Chat #17994

Open
wants to merge 5 commits into
base: feature/llm
Choose a base branch
from

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Oct 3, 2024

Summary of the Pull Request

When a multiline code block is clicked in Terminal chat, the first command gets run before the user presses 'Enter'. This commit fixes that by separating the code lines by the delimiter appropriate to the shell (& for cmd, ; for everything else).

Validation Steps Performed

Newlines get replaced with the appropriate delimiter

PR Checklist

This comment has been minimized.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

This is in lieu of having good bracketed paste support in conhost and powershell

src/cascadia/QueryExtension/ExtensionPalette.cpp Outdated Show resolved Hide resolved
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

I'm a little confused why we don't just remove the \r replacement entirely. \r is what the shell considers to be an "Enter" press so if we send \n delimited strings the shell will treat it as a multi-line paste. If that is correct, then the & / ; replacement seems like the wrong way to solve this.

@PankajBhojwani
Copy link
Contributor Author

I'm a little confused why we don't just remove the \r replacement entirely. \r is what the shell considers to be an "Enter" press so if we send \n delimited strings the shell will treat it as a multi-line paste. If that is correct, then the & / ; replacement seems like the wrong way to solve this.

Unfortunately the way we send the text to the shell (i.e. via a SendInput action) does not behave well with newlines, which was why this "replace newlines with \r" was here in the first place.

@DHowett
Copy link
Member

DHowett commented Oct 7, 2024

\r is what the shell considers to be an "Enter" press so if we send \n delimited strings the shell will treat it as a multi-line paste.

cmd has entered the room

@lhecker
Copy link
Member

lhecker commented Oct 7, 2024

Unfortunately the way we send the text to the shell (i.e. via a SendInput action) does not behave well with newlines, which was why this "replace newlines with \r" was here in the first place.

What was the issue with SendInput? In my testing it work just fine (outside of cmd).

cmd has entered the room

Right, that makes sense.

@PankajBhojwani
Copy link
Contributor Author

What was the issue with SendInput? In my testing it work just fine (outside of cmd).

#15788. We closed it because we stipulated there should actually be \rs along with the \ns.

@PankajBhojwani PankajBhojwani mentioned this pull request Oct 11, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants