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 DECARM (Auto Repeat Mode) #13981

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Sep 13, 2022

This PR adds support for the DECARM (Auto Repeat Mode) sequence, which
controls whether a keypress automatically repeats if you keep it held
down for long enough.

Note that this won't fully work in Windows Terminal until issue #8440 is
resolved.

Every time we receive a KeyDown event, we record the virtual key code
to track that as the last key pressed. If we receive a KeyUp event the
matches that last key code, we reset that field. Then if the Auto Repeat
Mode is reset, and we receive a KeyDown event that matches the last
key code, we simply ignore it.

Validation Steps Performed

I've manually tested the DECARM functionality in Vttest and confirmed
that it's working as expected. Although note that in Windows Terminal
this only applies to non-alphanumeric keys for now (e.g. Tab, BackSpace,
arrow keys, etc.)

I've also added a basic unit test that verifies that repeated key
presses are suppressed when the DECARM mode is disabled.

Closes #13919

@ghost ghost added Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Sep 13, 2022
@j4james j4james marked this pull request as ready for review September 13, 2022 18:17
@lhecker lhecker added the Needs-Second It's a PR that needs another sign-off label Oct 10, 2022
til::enumset<Mode> _inputMode{ Mode::Ansi };
WORD _lastVirtualKeyCode{ 0 };

til::enumset<Mode> _inputMode{ Mode::Ansi, Mode::AutoRepeat };
Copy link
Member

Choose a reason for hiding this comment

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

Is this another instance where Terminal can receive a reset/state toggle for DECARM and end up in a torn state ala #13968?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. In the DECAC1 case we needed Terminal to have the mode permanently enabled, but that's not the case here. We're always passing through the input modes, so Terminal should always be in sync with conhost.

The other thing is that Terminal would assumedly be using Win32InputMode most of the time, so the auto repeat option would typically not have any effect on that side.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

This is clever, and i like that it only applies in ENABLE_VIRTUAL_TERMINAL_INPUT mode 😄

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 10, 2022
@ghost
Copy link

ghost commented Oct 10, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@carlos-zamora carlos-zamora merged commit cacf668 into microsoft:main Oct 10, 2022
@j4james j4james deleted the feature-decarm branch October 11, 2022 08:58
ghost pushed a commit that referenced this pull request Oct 14, 2022
The way `DECARM` was initially implemented, we checked for repeated key
presses by matching the last recorded virtual key code, and used a 0 key
code to indicate that no key was pressed. This caused the VT query
responses to fail, because they generated key events with a 0 key code,
and that would end up being detected as a repeated key that should be
suppressed.

This PR fixes that issue by using a `std::optional` to track the last
key code, so if no key has been pressed we can represent that with
`std::nullopt`, and there's no way that can be confused with a genuine
key press.

The `DECARM` mode was introduced in PR #13981.

## Validation Steps Performed
I've manually tested in Vttest to confirm that the query reports are now
working again, even when `DECARM` is disabled. I've also checked that
`DECARM` itself it still working as expected.

Closes #14208
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

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 AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for DECARM (Auto Repeat Mode)
4 participants