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

Re-workaround uncancellable Console.ReadKey #1751

Merged
merged 7 commits into from
Apr 12, 2022
Merged

Conversation

andyleejordan
Copy link
Member

First pulled out the old native UnixConsoleEcho and I think finished #1745, then removed all the ReadKeyAsync code that was unreferenced. Currently testing using the KeyAvailable workaround on Windows, and testing the buffered ReadKey workaround on macOS. They're both broken in different ways 🤷

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM, this is just removing dead code and stripping out UnixConsoleEcho yeah?

@andyleejordan
Copy link
Member Author

LGTM, this is just removing dead code and stripping out UnixConsoleEcho yeah?

So far, yeah.

@SeeminglyScience
Copy link
Collaborator

Oh right WIP OOPS 😁

This "rewrite" uses the `console.sendText()` API in VS Code to respond
to a notification and send a fake key press, allowing us to "cancel"
`ReadKey` by getting it to return. Since we know we requested the
cancellation, we just drop the fake key press. This means we no longer
have to poll on macOS/Linux, and no longer have the ghost "double
key-press" issue on Windows. This fixes a myriad of problems and opens
up a lot of simplification.
@andyleejordan andyleejordan changed the title WIP: Re-workaround uncancellable Console.ReadKey Re-workaround uncancellable Console.ReadKey Apr 7, 2022
@andyleejordan andyleejordan marked this pull request as ready for review April 7, 2022 18:56
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM! Just a couple of small things.

Well done, this is super exciting ❤️

Just going to approve, if you have auto merge on nbd

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM!

@andyleejordan andyleejordan enabled auto-merge April 12, 2022 20:11
@andyleejordan
Copy link
Member Author

Yay! Thanks @SeeminglyScience. Merging.

@andyleejordan andyleejordan merged commit 312051b into master Apr 12, 2022
@andyleejordan andyleejordan deleted the andschwa/readkey branch April 12, 2022 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants