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

Jump to bottom

Terminal reports no longer work in Vttest #14208

Closed
j4james opened this issue Oct 13, 2022 · 5 comments · Fixed by #14216
Closed

Terminal reports no longer work in Vttest #14208

j4james opened this issue Oct 13, 2022 · 5 comments · Fixed by #14216
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Oct 13, 2022

Windows Terminal version

Commit cacf668

Windows build number

10.0.19044.2006

Other Software

Vttest

Steps to reproduce

  1. Open a WSL shell in conhost or Terminal
  2. Run vttest.
  3. Select the Private Device Attributes test (menu 6.4).

Expected Behavior

The report received from the terminal should be:

<27> [ ? 1 ; 0 c

Actual Behavior

In conhost it only seems to receive the initial ESC character, while in Terminal it receives nothing at all. And it's not just DA - other reports fail in a similar way.

I think this regression was caused by the recent DECARM merge (PR #13981), but I have no idea why. I'm still investigating.

The weird thing is that it works correctly in all of my own tests and applications. There's just something about the way vttest is reading the input that is causing it to fail.
@j4james j4james added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 13, 2022
@j4james
Copy link
Collaborator Author

j4james commented Oct 13, 2022

OK it seems the problem is that vttest disables DECARM on startup, and our VT reports don't work when DECARM is disabled. The reason they don't work is because the report sequences are generated with fake keypresses using a virtual key code of zero. And the DECARM implementation uses the virtual key code to detect repeats of the same key, so all the characters in the response string appear to be coming from the same key.

We can probably fix this by checking both the virtual key code and the char data when determining whether we've got a repeated key. I'll give that a try now. Hopefully that won't have any unwanted side effects.

@j4james
Copy link
Collaborator Author

j4james commented Oct 13, 2022

It's actually less complicated that I thought. The fact that all the generated key presses appear to be the same shouldn't be a problem. They always include both a press and release event, so that wouldn't typically be recognised as a repeat. The problem is that we're using a 0 virtual key code to indicate the "clean" state (i.e. when nothing has been pressed). That shouldn't normally match anything, since it's not a valid VK value, but in this case it does.

So I guess the solution is to use another value for the clean state - maybe 0xFFFF.

@DHowett
Copy link
Member

DHowett commented Oct 13, 2022

There's always our trusty friend, std::optional<T>!

@j4james
Copy link
Collaborator Author

j4james commented Oct 13, 2022

There's always our trusty friend, std::optional<T>!

Yeah, that's a good idea. I was trying to keep things simple but that shouldn't add too much overhead, and feels a lot safer than making another assumption about an unused VK value.

@j4james j4james mentioned this issue Oct 13, 2022
Merged
@ghost ghost added the In-PR This issue has a related PR label Oct 13, 2022
@ghost ghost closed this as completed in #14216 Oct 14, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Oct 14, 2022
ghost pushed a commit that referenced this issue Oct 14, 2022
@j4james
Make sure VT reports still work when DECARM is disabled (#14216)
0022898 
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

🎉This issue was addressed in #14216, which has now been successfully released as Windows Terminal Preview v1.17.1023.:tada:

Handy links:

@j4james j4james mentioned this issue Dec 31, 2023
Closed
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make sure VT reports still work when DECARM is disabled j4james/terminal
2 participants
@DHowett @j4james