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

Add support for application keypad mode (DECKPAM) #16506

Closed
j4james opened this issue Dec 30, 2023 · 0 comments · Fixed by #16511
Closed

Add support for application keypad mode (DECKPAM) #16506

j4james opened this issue Dec 30, 2023 · 0 comments · Fixed by #16511
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase

Comments

@j4james
Copy link
Collaborator

j4james commented Dec 30, 2023

Description of the new feature/enhancement

Looking through the old issues, I see I already noted back in #159 that DECKPAM didn't appear to be working. I assumed at the time that it was a configuration issue, or possibly a minor bug in the implementation, but I've only realised now that this mode doesn't do anything at all.

We do already parse the DECKPAM and DECKPNM escape sequences, and I even recently added support for DECNKM, which is an alias of that functionality, but the mode doesn't actually have any effect. It switches between two key mapping tables in the TerminalInput class, but they're essentially identical.

This mode is useful because it provides a way for applications to distinguish the keypad keys from the top row numeric keys and arithmetic keys, as well as distinguishing the two Return keys.

Proposed technical implementation details (optional)

If we just want a simple solution, we can probably just update the existing mapping table, although it might be a little more complicated than that to handle the modifiers.

My preferred approach, though, is essentially a rewrite of the TerminalInput class, which is something I've been working on for a while. In addition to the application keypad support, it also prepares the way for future VT enhancements, and fixes a bunch of other keyboard issue.

The way it works is there's a single keyboard map, that takes a virtual key code combined with Ctrl, Alt, and Shift modifier bits as the key, and the appropiate VT sequence as the value. This map is initially built at startup, and then regenerated whenever a keyboard mode is changed.

That takes care of the "functional" keys (cursor keys, editing keys, function keys, and things like BkSp, Tab, and Return). But if the key-modifier combination is not in that map, I do the following:

  1. If the active keyboard layout has given us a UnicodeChar value, that takes priority. Although that can often still need to be "extended" with a Ctrl modifier that converts the value into a C0 control, and/or an Alt modifier that add an ESC prefix.

  2. If there isn't a UnicodeChar value, we need to calculate what value the would have been without any Ctrl or Alt modifiers applied (you can do this with the ToUnicodeEx API). And once we have that value, we can then manually apply the Ctrl and Alt modifiers as described above.

There are a whole lot of edge cases that also need to be dealt with, but that's the main gist of my proposal.

@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Dec 30, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Dec 30, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Dec 31, 2023
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Help Wanted We encourage anyone to jump in on these. Area-VT Virtual Terminal sequence support and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 3, 2024
DHowett pushed a commit that referenced this issue Jan 30, 2024
The primary reason for this refactoring was to simplify the management
of VT input sequences that vary depending on modes, adding support for
the missing application keypad sequences, and preparing the way for
future extensions like `S8C1T`.

However, it also includes fixes for a number of keyboard related bugs,
including a variety of missing or incorrect mappings for the `Ctrl` and
`Ctrl`+`Alt` key combinations, 

## References and Relevant Issues

This PR also includes a fix for #10308, which was previously closed as a
duplicate of #10551. I don't think those bugs were related, though, and
although they're both supposed to be fixed in Windows 11, this PR fixes
the issue in Windows 10.

## Detailed Description of the Pull Request / Additional comments

The way the input now works, there's a single keyboard map that takes a
virtual key code combined with `Ctrl`, `Alt`, and `Shift` modifier bits
as the lookup key, and the expected VT input sequence as the value. This
map is initially constructed at startup, and then regenerated whenever a
keyboard mode is changed.

This map takes care of the cursor keys, editing keys, function keys, and
keys like `BkSp` and `Return` which can be affected by mode changes. The
remaining "graphic" key combinations are determined manually at the time
of input.

The order of precedence looks like this:

1. If the virtual key is `0` or `VK_PACKET`, it's considered to be a
   synthesized keyboard event, and the `UnicodeChar` value is used
   exactly as given.

2. If it's a numeric keypad key, and `Alt` is pressed (but not `Ctrl`),
   then it's assumedly part of an Alt-Numpad composition, so the key
   press is ignored (the generated character will be transmitted when
   the `Alt` is released).

3. If the virtual key combined with modifier bits is found in the key
   map described above, then the matched escape sequence will be used
   used as the output.

4. If a `UnicodeChar` value has been provided, that will be used as the
   output, but possibly with additional Ctrl and Alt modifiers applied:

   a. If it's an `AltGr` key, and we've got either two `Ctrl` keys
      pressed or a left `Ctrl` key that is distinctly separate from a
      right `Alt` key, then we will try and convert the character into
      a C0 control code.

   b. If an `Alt` key is pressed (or in the case of an `AltGr` value,
      both `Alt` keys are pressed), then we will convert it into an
      Alt-key sequence by prefixing the character with an `ESC`.

5. If we don't have a `UnicodeChar`, we'll use the `ToUnicodeEx` API to
   check whether the current keyboard state reflects a dead key, and if
   so, return nothing.

6. Otherwise we'll make another `ToUnicodeEx` call but with any `Ctrl`
   and `Alt` modifiers removed from the state to determine the base key
   value. Once we have that, we can apply the modifiers ourself.

   a. If the `Ctrl` key is pressed, we'll try and convert the base value
      into a C0 control code. But if we can't do that, we'll try again
      with the virtual key code (if it's alphanumeric) as a fallback.

   b. If the `Alt` key is pressed, we'll convert the base value (or
      control code value) into an Alt-key sequence by prefixing it with
      an `ESC`.

For step 4-a, we determine whether the left `Ctrl` key is distinctly
separate from the right `Alt` key by recording the time that those keys
are pressed, and checking for a time gap greater than 50ms. This is
necessary to distinguish between the user pressing `Ctrl`+`AltGr`, or
just pressing `AltGr` alone, which triggers a fake `Ctrl` key press at
the same time.

## Validation Steps Performed

I created a test script to automate key presses in the terminal window
for every relevant key, along with every Ctrl/Alt/Shift modifier, and
every relevant mode combination. I then compared the generated input
sequences with XTerm and a DEC VT240 terminal. The idea wasn't to match
either of them exactly, but to make sure the places where we differed
were intentional and reasonable.

This mostly dealt with the US keyboard layout. Comparing international
layouts wasn't really feasible because DEC, Linux, and Windows keyboard
assignments tend to be quite different. However, I've manually tested a
number of different layouts, and tried to make sure that they were all
working in a reasonable manner.

In terms of unit testing, I haven't done much more than patching the
ones that already existed to get them to pass. They're honestly not
great tests, because they aren't generating events in the form that
you'd expect for a genuine key press, and that can significantly affect
the results, but I can't think of an easy way to improve them.

## PR Checklist
- [x] Closes #16506
- [x] Closes #16508
- [x] Closes #16509
- [x] Closes #16510
- [x] Closes #3483
- [x] Closes #11194
- [x] Closes #11700
- [x] Closes #12555
- [x] Closes #13319
- [x] Closes #15367
- [x] Closes #16173
- [x] Tests added/passed
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jan 30, 2024
DHowett pushed a commit that referenced this issue Feb 6, 2024
DECKPAM originally tracked in #16506.
Support was added in #16511.
But turns out people didn't expect the Terminal to actually be like,
compliant: #16654

This closes #16654 while we think over in #16672 how we want to solve
this
DHowett pushed a commit that referenced this issue Feb 21, 2024
DECKPAM originally tracked in #16506.
Support was added in #16511.
But turns out people didn't expect the Terminal to actually be like,
compliant: #16654

This closes #16654 while we think over in #16672 how we want to solve
this

(cherry picked from commit 71efdcb)
Service-Card-Id: 91781164
Service-Version: 1.20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants