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

Pass the scancode in our tunneled DirectKey event #7298

Merged
2 commits merged into from
Aug 14, 2020
Merged

Pass the scancode in our tunneled DirectKey event #7298

2 commits merged into from
Aug 14, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Aug 14, 2020 •
edited
Loading

#7145 introduced a check so that we wouldn't dispatch keys unless they
actually had a scancode. Our synthetic events actually didn't have
scancodes. Not because they couldn't--just because they didn't.

Fixes #7297

@DHowett
Pass the scancode in the our tunneled DirectKey event
59fd7cf 
actually had a scancode. Our synthetic events actually _didn't_ have
scancodes. Not because they couldn't--just because they didn't.

Fixes #7297.
@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Aug 14, 2020
zadjii-msft
zadjii-msft approved these changes Aug 14, 2020
View reviewed changes
src/cascadia/TerminalApp/IDirectKeyListener.idl
@@ -8,7 +8,7 @@ namespace TerminalApp
// If you update this one, please update the one in TerminalControl\TermControl.idl
// If you change this interface, please update the guid.
// If you press F7 or Alt and get a runtime error, go make sure both copies are the same.
[uuid("339e1a87-5315-4da6-96f0-565549b6472b")] interface IDirectKeyListener {
Boolean OnDirectKeyEvent(UInt32 vkey, Boolean down);
[uuid("0ddf4edc-3fda-4dee-97ca-a417ee3dd510")] interface IDirectKeyListener {
Copy link
Member

Choose a reason for hiding this comment

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

why bump the uuid?

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're supposed to do this on every change to an existing interface, technically. The interface is really supposed to unique the vtable layout that allows for a conformant class to be called by an unknown caller -- the chief feature of COM 😄

Also because it says it in the rules on line 9

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not applicable here but it's good to get in the habit. If somebody had taken a dependency on this interface solely by layout and old UUID, new consumers are no longer compatible with it. If we didn't change the uuid, consumers of the new layout would report as compatible but get the parameter layout wrong (unpacking the first 8 bits of the Boolean as the scancode (!!))

Copy link
Member

Choose a reason for hiding this comment

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

that's what I thought, but I'm glad to have this typed out for posterity 😄

@lhecker
Copy link
Member

lhecker commented Aug 14, 2020

Aww ****. 🙈 Thanks for fixing this!

lhecker
lhecker approved these changes Aug 14, 2020
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

LGTM. Again thanks for fixing this! It's nice to see that another use of MapVirtualKeyW is gone. 🙂

src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
@DHowett DHowett changed the title Pass the scancode in the our tunneled DirectKey event Pass the scancode in our tunneled DirectKey event Aug 14, 2020
@DHowett
Copy link
Member Author

DHowett commented Aug 14, 2020

@msftbot merge this in 2 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 14, 2020
@ghost
Copy link

ghost commented Aug 14, 2020

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Fri, 14 Aug 2020 23:43:59 GMT, which is in 2 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit aecd99e into master Aug 14, 2020
@ghost ghost deleted the dev/duhowett/tunnel_f7 branch August 14, 2020 23:44
DHowett added a commit that referenced this pull request Aug 24, 2020
@DHowett
Pass the scancode in our tunneled DirectKey event (#7298)
1da0389 
#7145 introduced a check so that we wouldn't dispatch keys unless they
actually had a scancode. Our synthetic events actually _didn't_ have
scancodes. Not because they couldn't--just because they didn't.

Fixes #7297

(cherry picked from commit aecd99e)
@ghost
Copy link

ghost commented Aug 26, 2020

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

Handy links:

@ghost ghost mentioned this pull request Aug 26, 2020
Closed
@ghost
Copy link

ghost commented Aug 26, 2020

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@miniksa miniksa miniksa approved these changes

@lhecker lhecker lhecker approved these changes

Assignees
No one assigned
Labels
Area-Input Related to input processing (key presses, mouse, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

F7 tunneling broken
4 participants
@DHowett @lhecker @miniksa @zadjii-msft