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

[Unix] Console.ReadKey rewrite #72193

Merged
merged 42 commits into from
Aug 1, 2022
Merged

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jul 14, 2022

I believe that the most important aspect of this change is testing and ensuring that the new implementation is better than the old one.
To make Console.ReadKey testable, I’ve moved the parsing logic to a new method (and type) and made all the environment-specific variables like Terminfo parameters of it. Pseudocode:

ConsoleKeyInfo Parse(char[] input, Terminfo db, out int consumedChars, [more args])

This required breaking TermInfo.cs into multiple files but allowed for testing (and debugging!) the parser on any OS. I’ve not made these types public, I am just referencing them as links in the test project. It’s not perfect, but I think it’s the best solution without introducing a new public API.

The next step I took was writing a small command line app that records all the necessary information (Terminfo, VERASE, Encoding, $TERM) and asks the user to press a LOT of key combinations, records the data returned by read() sys-call and outputs it to source code file that can be included in System.Console test project.

I’ve recorded the testing data for:

  • xterm (TERM=xterm)
  • GNOME Terminal (TERM= xterm-256color)
  • Linux Console (TERM=linux)
  • PuTTy (TERM=xterm,putty,linux)
  • Windows Terminal connected via SSH to Linux VM (TERM=xterm-256color)
  • rxvt-unicode (TERM=rxvt-unicode-256color)
  • tmux (TERM=screen)
  • And multiple PuTTy config switches that override Terminfo settings (TERM says map X to Y, but PuTTY can use Z to express X)

And implemented a new parser that uses Terminfo to map every escape sequence, but when there is no mapping (a very common example is well known keys with modifiers), fallback to mappings defined by various Terminals and/or standards.

When I run the new tests against the old implementation I can observe 987 failures. I've kept the old code and I can introduce a flag to switch to the old implementation.

fixes #802
fixes #25735
fixes #44621
fixes #45597
fixes #60101 (VT sequences are not going to miss [ anymore)
fixes #63387
fixes #63644

Limitations:

  • For Ctrl+(D1, D3, D8, D9 and D0) we can’t detect the modifier (Control) because every Unix Terminal maps it to just plain number. Example: Ctrl+1 maps to single ascii character representing digit one, so does pressing ‘1’ and ‘NumPad1’.
  • We can't distinguish Cltrl+D2 vs Ctrl+Space as they both produce null ascii character. Currently Ctrl+Space is mapped to Ctrl+D2 but I am not sure whether I made a good decision here.
  • Clr+H can’t be mapped to key=H and modifiers=Control as every Terminal maps it to Ctrl+Bacspace. Same goes for Ctrl+I (Tab), Ctrl+J (Ctrl+Enter with new line character), Ctrl+M (Enter) with carriage return character.
  • Shift+letter and CapsLock+letter is always mapped to just uppercase letter, so it’s impossible to detect whether shift was pressed or not. The new implementation is backward compatible with the old one and recognizes both scenarios as Shift+letter (so CapLock+letter=Shift+letter).
  • We can't make assumptions about keyboard layout nor read it and all Shift+Dx combinations result in a single ascii character read by the read() sys-call. Shift+Dx keys can't be mapped to key and modifier, just to resulting character. Example: Shift+D1 (‘!’ on most keyboards) is mapped to char=! with no key and no modifier.
  • We can't distinguish OemX and Divide (Numeric Keypad on Windows), as they are both mapped to / character. They are both mapped to ConsoleKey.Divide
  • We can't distinguish OemPlus and Add (Numeric Keypad on Windows), as they are both mapped to + character. They are both mapped to ConsoleKey.Add.
  • We can't distinguish OemMinus and Subtract (Numeric Keypad on Windows), as they are both mapped to - character. They are both mapped to ConsoleKey.Subtract
  • On Unix Num5 is mapped to “Begin” but ConsoleKey does not define such value, so it’s mapped to ConsoleKey.NoName.

adamsitnik and others added 30 commits June 28, 2022 19:56
…ed to reference all sys-calls used for reading files
… bytes which represent single UTF8 character by design
… between Enter (\r) and Ctrl+Enter (\n) like we do on Windows
remove UXTerm as its the same as Term
@omajid
Copy link
Member

omajid commented Jul 20, 2022

I wonder whether @omajid you might be able to look in the absence of @tmds? given .NET 7 timeline

I can take a look, but I am nowhere near as familiar with the underlying design, code and the issues that might be in play. I certainly can't fill in Tom's shoes.

src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
']' => (ConsoleKey.F8, ConsoleModifiers.Control | ConsoleModifiers.Shift),
'^' => (ConsoleKey.F9, ConsoleModifiers.Control | ConsoleModifiers.Shift),
'_' => (ConsoleKey.F10, ConsoleModifiers.Control | ConsoleModifiers.Shift),
'`' => (ConsoleKey.F11, ConsoleModifiers.Control | ConsoleModifiers.Shift),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'`' => (ConsoleKey.F11, ConsoleModifiers.Control | ConsoleModifiers.Shift),
''' => (ConsoleKey.F11, ConsoleModifiers.Control | ConsoleModifiers.Shift),

This one looks to me as ' on https://vt100.net/docs/vt510-rm/chapter6.html.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, but PuTTy uses '`' for it and it's the only Terminal that supports SCO emulation that I could find and test

https://serverfault.com/questions/429901/how-to-change-the-terminal-to-sco-compliant-in-ubuntu

Copy link
Member

@Jozkee Jozkee left a comment

Choose a reason for hiding this comment

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

LGTM

src/libraries/System.Console/tests/KeyParserTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Console/tests/KeyParserTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
Comment on lines +69 to +70
// Is it a three character sequence? (examples: '^[[H' (Home), '^[OP' (F1))
if (input[1] == 'O' || char.IsAsciiLetter(input[2]) || input.Length == MinimalSequenceLength)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Is it a three character sequence? (examples: '^[[H' (Home), '^[OP' (F1))
if (input[1] == 'O' || char.IsAsciiLetter(input[2]) || input.Length == MinimalSequenceLength)
// Is it a three character sequence? (examples: '^[[H' (Home), '^[OP' (F1))
if (char.IsAsciiLetter(input[2]) || input.Length == MinimalSequenceLength)

src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
Comment on lines +245 to +246
// based on https://en.wikipedia.org/wiki/ANSI_escape_code#Fe_Escape_sequences
static ConsoleKey MapEscapeSequenceNumber(byte number)
Copy link
Member

Choose a reason for hiding this comment

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

I looked at the wikpedia link and I still don't see the relation with the 1 to 34 numbers used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

You need to scroll down quite a bit:

image

src/libraries/System.Console/src/System/IO/KeyParser.cs Outdated Show resolved Hide resolved
@adamsitnik
Copy link
Member Author

@Jozkee big thanks for the review! I know it was not an easy one ;)

@adamsitnik adamsitnik merged commit f1de614 into dotnet:main Aug 1, 2022
@MichalStrehovsky
Copy link
Member

@adamsitnik your changes to the TermInfo.cs tests undid the changes done in #73104. typeof(Foobar).Assembly.GetType is not trim safe.

We now have a blocking-clean-CI issue on it in #73212. Could you please have a look?

@adamsitnik
Copy link
Member Author

@MichalStrehovsky sure thing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.