8000 Add support for Ctrl-C to interrupt commands, and add multi-line capa… by palladia · Pull Request #613 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Add support for Ctrl-C to interrupt commands, and add multi-line capa… #613

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

Closed
wants to merge 2 commits into from

Conversation

palladia
Copy link
Contributor
@palladia palladia commented Mar 2, 2016

This update fixes two things:

  1. Add support for Ctrl-C to interrupt current command input. Similar to Windows behavior, it appends ^C to end of command line, and prints a new prompt.
  2. Add multi-line command capability for script debugger that was mistakenly left out in a previous update.

This change is Reviewable

Console.Write("[DBG] PS >> ");
if (incompleteLine)
{
Console.Write(">> ");
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be consoleReadLine.Write? So it goes through our Readline class instead of straight through to System.Console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that what we should use? It's never occurred to me that we should write through ConsoleReadLine. It is "Read"-Line after all :)

ConsoleReadLine currently has no "Write" methods at all, and we use Console.WriteLine() everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I mean this.myHost.UI.Write.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use Console.Write or Console.Out.Write all over the place. If these calls, along with Cursor, foreground/background color, etc., all should be moved to myHost.UI, then please open a separate bug.

Copy link
Member

Choose a reason for hiding this comment

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

Done, #616.

@andyleejordan
Copy link
Member

The summary message for commit 14e8dac is far longer than it should be. This template is what all Git commit messages should follow.

@andyleejordan
Copy link
Member

Your notes in the PR description are what should be the body of the commit message. Keep in mind that PRs are not permanent (they exist only on GitHub); only the Git commit messages are actually tied to the project. Be as descriptive as you can with your commit messages.

@andyleejordan
Copy link
Member

Just reiterating information from phone conversations; we have a couple things going on here:

  • We should not sleep our ReadLine thread as this breaks shell responsiveness (keys are updated only when the thread wakes up; not when the user hit a key)
  • Console.KeyAvailable appears broken in that it is only returning true when Enter was hit; at which point all the previous buffered key strokes came through
  • TreatControlCAsInput is not implemented and would be a pain to get implemented; if it were implemented, we'd have to be really careful to enable and disable it appropriately so that the handler is called appropriately during pipeline execution
  • It would not be totally odd for Esc to handle clearing multi-line input, as it already clears single-line input; I understand it is different from Windows, and that is acceptable as the alternatives are not viable

@palladia
Copy link
Contributor Author
palladia commented Mar 3, 2016

I'm not too concerned about responsiveness. I did a quick web search, and the fastest one could type is about 12 strokes a second, or about 83 milliseconds between strokes. If I change sleep to 80 milliseconds, it should be plenty to keep up with fastest typist.

As for second item, I assume a fix is forth coming. Any estimate on when they will fix that? I currently don't see this problem. Have they released the broken patch, or will wait for the new fix before releasing everything?

@palladia
Copy link
Contributor Author
palladia commented Mar 3, 2016

Since the resolution of this PR will not be immediate, I will take changes associated with multi-line out of this PR, and create a separate PR for it.

@andyleejordan
Copy link
Member

You don't see the problem because the pinned packages are before any of the changes. The fix is already up in dotnet/corefx#6619; I didn't catch it because we weren't using Console.KeyAvailable.

Thanks @palladia.

@palladia
Copy link
Contributor Author
palladia commented Mar 3, 2016

New push that includes most changes per Andy comments. Ctrl-C works great, and with 80 msec sleep, there's no apparent lag.

Will re-test after CoreFx #6619 is pulled into our build.

@andyleejordan
Copy link
Member

I'd prefer to wait until after we've merged in the new packages, so we can test before merging.

@palladia
Copy link
Contributor Author
palladia commented Mar 4, 2016
8000

No problem.

@andyleejordan
Copy link
Member

Tested on latest .NET updates... this performs very oddly.

@andyleejordan
Copy link
Member

TreatControlCAsInput has been implemented in dotnet/corefx#6669, which should be available in build 23908 (which I'll get us updated to ASAP).

@andyleejordan
Copy link
Member

Closing in favor of #717, right?

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
0