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.

Sign up for GitHub

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

Fix '@' appearing in console #1092

Merged
merged 2 commits into from
Nov 13, 2019
Merged

Fix '@' appearing in console #1092

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d9155a
Fix '@' appearing in console
Nov 4, 2019
da19d18
Name param
Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/PowerShellEditorServices/Services/PowerShellContext/Console/ConsoleProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ internal static ConsoleKeyInfo SafeReadKey(bool intercept, CancellationToken can
}
catch (OperationCanceledException)
{
return default(ConsoleKeyInfo);
return new ConsoleKeyInfo(' ', ConsoleKey.DownArrow, shift: false, alt: false, control: false);
Copy link
Member

Choose a reason for hiding this comment

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

Why ConsoleKey.DownArrow instead of ConsoleKey.Spacebar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't add any output to the buffer, and seemed to work without consequences when I tried it, which seems logical since inputting DownArrow in PSRL would do nothing if you're at the last prompt already.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: name the ' ' parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it trigger custom key handlers? Assuming it does, is there some character we could send that isn't typically possible to receive as input that we could specifically handle on the PSRL side?

Copy link
Contributor Author

@rjmholt rjmholt Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Valid values are here: https://docs.microsoft.com/en-us/dotnet/api/system.consolekey?view=netstandard-2.0

Could possibly use a numeric value that isn't in the enum, but that could be fragile.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

For some more options, we could do something similar to how I'm currently getting around lack of SHIFT + ENTER support (e.g. sending an emoji as raw input)

For instance, if you send 0x2665 to xterm directly as a raw text sequence it comes across as this:

KeyChar Key Modifiers
------- --- ---------
      ♥  18         0

Note that 18 is VK_MENU aka ALT. Not 100% sure why it comes across that way, the raw input is just \u2665. (Apparently it comes through as a ALT + NumPad unicode sequence).

It is still possible to conflict with an existing key bind that way, but significantly less likely.

Copy link
Member

Choose a reason for hiding this comment

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

This does mean we'll need to create another contract with PSRL to throw this character out if it is seen. That makes me uneasy... Seems very hacky.

@daxian-dbw would you mind giving your point of view?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This does mean we'll need to create another contract with PSRL to throw this character out if it is seen. That makes me uneasy... Seems very hacky.

Is it really all that more hacky than sending a valid key that we hope no one uses? To be clear, I'm not saying it isn't hacky. Everything around our ReadKey is incredibly hacky and will continue to be until the day ReadKeyAsync is properly implemented in corefx (a man can dream right?). This variant of hacky is a lot less likely to spawn other issues, definitely open to better ideas though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other possibility I had was an unmodified Space. That one seems safe, but it does add a space to the console.

The only actual safe thing to do is to redefine our contract with PSRL so that we can say "we didn't get anything at all". Currently that @ character represents PSRL not understanding the key press.

Anyway, I'm not proposing that this change be permanent. Just something that works better for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, I'm not proposing that this change be permanent. Just something that works better for now.

Yeah fair enough. Probably fine for preview.

}
}
}
Expand Down