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

Fix dwControlKeyState always including ENHANCED_KEY #16335

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 17, 2023

Since all VT parameters are treated to be at least 1 (and 1 if they're
absent or 0), modifierParam > 0 was always true. This meant that
ENHANCED_KEY was always being set. It's unclear why ENHANCED_KEY
was used there, but it's likely not needed in general.

Closes #16266

Validation Steps Performed

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Input Related to input processing (key presses, mouse, etc.) Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Impact-Correctness It be wrong. labels Nov 17, 2023
@DHowett
Copy link
Member

DHowett commented Nov 17, 2023

/cc @j4james for this one 😄

@j4james
Copy link
Collaborator

j4james commented Nov 17, 2023

I hate this. If it were up to me I would reject this PR.

@lhecker
Copy link
Member Author

lhecker commented Nov 17, 2023

I removed value_or_enum now.

@lhecker lhecker changed the title Fix bad VTParameter enum conversion Fix dwControlKeyState always including ENHANCED_KEY Nov 18, 2023
@lhecker lhecker force-pushed the dev/lhecker/16266-bad-enum branch from 240bfb0 to 9c50a03 Compare November 21, 2023 15:39
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Huh. This is curious to me, because that line of code dates back to like, 2018:
https://github.com/microsoft/terminal/blame/2e26c3e0c958f174d838049ce43cbb7dc52eb7e8/src/terminal/parser/InputStateMachineEngine.cpp#L733

So, I wouldn't really expect it to be a recent regression... But if it works, it works.

@zadjii-msft
Copy link
Member

(is the current description up-to-date? I think that confused me here)

@j4james
Copy link
Collaborator

j4james commented Nov 25, 2023

Huh. This is curious to me, because that line of code dates back to like, 2018

FYI, the reason why it wasn't a problem initially is because that state wasn't actually used anywhere until the change made here:

https://github.com/microsoft/terminal/pull/5021/files#diff-7b75d912f8a48047ff7f06ea09f9a6f945efc3ecfc194e6aa4d44bc58cd8b779L605-R608

@lhecker lhecker merged commit be9fc20 into main Nov 30, 2023
@lhecker lhecker deleted the dev/lhecker/16266-bad-enum branch November 30, 2023 14:55
DHowett pushed a commit that referenced this pull request Dec 4, 2023
Since all VT parameters are treated to be at least 1 (and 1 if they're
absent or 0), `modifierParam > 0` was always true. This meant that
`ENHANCED_KEY` was always being set. It's unclear why `ENHANCED_KEY`
was used there, but it's likely not needed in general.

Closes #16266

## Validation Steps Performed
* Can't test this unless we fix the win32 input mode issue #16343 ❌

(cherry picked from commit be9fc20)
Service-Card-Id: 91159301
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Automerge-Not-Compatible Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty
Projects
Development

Successfully merging this pull request may close these issues.

dwControlKeyState behaviour changed between Windows 10/Windows 11
4 participants