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

Mark ESC as handled so that it doesn't come back in CharacterHandler #1974

Merged
merged 3 commits into from
Jul 16, 2019

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jul 15, 2019

Fixes #1295.

Summary of the Pull Request

There are four places where we could possibly fix this.

  1. mark it handled manually in KeyDownHandler (like Tab)
  2. "handle it manually" in TerminalCore (like Ctrl+Space and friends)
  3. ignore it in CharacterHandler (like Bksp and Del)
  4. "handle it automatically" in TerminalCore by removing it from TerminalInput
    • i cannot figure out why it's in there -- it's in the keypad numeric and keypad application mappings, but it doesn't actually get remapped?

{ VK_ESCAPE, L"\x1b" },

{ VK_ESCAPE, L"\x1b" },

I just chose one. Why not here? We should document what each of those does and why they are they way they are.

PR Checklist

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jul 15, 2019

const bool manuallyHandled = ch != UNICODE_NULL;
const bool translated = _terminalInput->HandleKey(&keyEv);
return translated && manuallyHandled;

Follow up: why do we need to track manuallyHandled at all in TerminalCore's input translator? It was handled if and only if TerminalInput said it was; if we ever second-guess it, we risk double-sending something.

I guess we want to mark "manually handled" things that TerminalInput won't handle? But then we just won't send the character at all. Hmm.

@DHowett-MSFT
Copy link
Contributor

I see. It's because when we have a character-producing key, it comes in as UNICODE_NULL because we synthesized the key event out of thin air oh my word

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.

Seems fine to me.

@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Jul 16, 2019
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

OK.

@DHowett-MSFT
Copy link
Contributor

gfdi github's merge conflict resolver changed the line endings

@DHowett-MSFT DHowett-MSFT merged commit a0782bf into master Jul 16, 2019
@DHowett-MSFT DHowett-MSFT deleted the dev/duhowett/escesc branch July 16, 2019 20:56
DHowett-MSFT pushed a commit that referenced this pull request Jul 17, 2019
@ghost
Copy link

ghost commented Aug 3, 2019

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Aug 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<ESC> comes through as <ESC><ESC>
4 participants