Skip to content

Commit

Permalink
Delegate all character input to the character event handler (#4192)
Browse files Browse the repository at this point in the history
My basic idea was that `WM_CHAR` is just the better `WM_KEYDOWN`.
The latter fails to properly support common dead key sequences like in
#3516.

As such I added some logic to `Terminal::SendKeyEvent` to make it return
false if the pressed key represents a printable character.
This causes us to receive a character event with a (hopefully) correctly
composed code unit, which then gets sent to `Terminal::SendCharEvent`.
`Terminal::SendCharEvent` in turn had to be modified to support
potentially pressed modifier keys, since `Terminal::SendKeyEvent` isn't
doing that for us anymore.
Lastly `TerminalInput` had to be modified heavily to support character
events with modifier key states. In order to do so I merged its
`HandleKey` and `HandleChar` methods into a single one, that now handles
both cases.
Since key events will now contain character data and character events
key codes the decision logic in `TerminalInput::HandleKey` had to be
rewritten.

## PR Checklist
* [x] CLA signed
* [x] Tests added/passed
* [x] I've discussed this with core contributors already.

## Validation Steps Performed

* See #3516.
* I don't have any keyboard that generates surrogate characters. Due to
  this I modified `TermControl::_SendPastedTextToConnection` to send the
  data to `_terminal->SendCharEvent()` instead. I then pasted the test
  string ""𐐌𐐜𐐬" and ensured that the new `TerminalInput::_SendChar`
  method still correctly assembles surrogate pairs.

Closes #3516
Closes #3554 (obsoleted by this PR)
Potentially impacts #391, which sounds like a duplicate of #3516
  • Loading branch information
lhecker authored Apr 7, 2020
1 parent 52d6c03 commit a9c9714
Show file tree
Hide file tree
Showing 12 changed files with 436 additions and 339 deletions.
18 changes: 12 additions & 6 deletions src/cascadia/PublicTerminalCore/HwndTerminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -427,10 +427,8 @@ const wchar_t* _stdcall TerminalGetSelection(void* terminal)
return returnText.release();
}

void _stdcall TerminalSendKeyEvent(void* terminal, WPARAM wParam)
static ControlKeyStates getControlKeyState() noexcept
{
const auto publicTerminal = static_cast<const HwndTerminal*>(terminal);
const auto scanCode = MapVirtualKeyW((UINT)wParam, MAPVK_VK_TO_VSC);
struct KeyModifier
{
int vkey;
Expand Down Expand Up @@ -458,18 +456,26 @@ void _stdcall TerminalSendKeyEvent(void* terminal, WPARAM wParam)
}
}

publicTerminal->_terminal->SendKeyEvent((WORD)wParam, (WORD)scanCode, flags);
return flags;
}

void _stdcall TerminalSendKeyEvent(void* terminal, WORD vkey, WORD scanCode)
{
const auto publicTerminal = static_cast<const HwndTerminal*>(terminal);
const auto flags = getControlKeyState();
publicTerminal->_terminal->SendKeyEvent(vkey, scanCode, flags);
}

void _stdcall TerminalSendCharEvent(void* terminal, wchar_t ch)
void _stdcall TerminalSendCharEvent(void* terminal, wchar_t ch, WORD scanCode)
{
if (ch == '\t')
{
return;
}

const auto publicTerminal = static_cast<const HwndTerminal*>(terminal);
publicTerminal->_terminal->SendCharEvent(ch);
const auto flags = getControlKeyState();
publicTerminal->_terminal->SendCharEvent(ch, scanCode, flags);
}

void _stdcall DestroyTerminal(void* terminal)
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/PublicTerminalCore/HwndTerminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ __declspec(dllexport) bool _stdcall TerminalIsSelectionActive(void* terminal);
__declspec(dllexport) void _stdcall DestroyTerminal(void* terminal);
__declspec(dllexport) void _stdcall TerminalSetTheme(void* terminal, TerminalTheme theme, LPCWSTR fontFamily, short fontSize, int newDpi);
__declspec(dllexport) void _stdcall TerminalRegisterWriteCallback(void* terminal, const void __stdcall callback(wchar_t*));
__declspec(dllexport) void _stdcall TerminalSendKeyEvent(void* terminal, WPARAM wParam);
__declspec(dllexport) void _stdcall TerminalSendCharEvent(void* terminal, wchar_t ch);
__declspec(dllexport) void _stdcall TerminalSendKeyEvent(void* terminal, WORD vkey, WORD scanCode);
__declspec(dllexport) void _stdcall TerminalSendCharEvent(void* terminal, wchar_t ch, WORD scanCode);
__declspec(dllexport) void _stdcall TerminalBlinkCursor(void* terminal);
__declspec(dllexport) void _stdcall TerminalSetCursorVisible(void* terminal, const bool visible);
};
Expand Down Expand Up @@ -82,8 +82,8 @@ struct HwndTerminal : ::Microsoft::Console::Types::IControlAccessibilityInfo
friend void _stdcall TerminalClearSelection(void* terminal);
friend const wchar_t* _stdcall TerminalGetSelection(void* terminal);
friend bool _stdcall TerminalIsSelectionActive(void* terminal);
friend void _stdcall TerminalSendKeyEvent(void* terminal, WPARAM wParam);
friend void _stdcall TerminalSendCharEvent(void* terminal, wchar_t ch);
friend void _stdcall TerminalSendKeyEvent(void* terminal, WORD vkey, WORD scanCode);
friend void _stdcall TerminalSendCharEvent(void* terminal, wchar_t ch, WORD scanCode);
friend void _stdcall TerminalSetTheme(void* terminal, TerminalTheme theme, LPCWSTR fontFamily, short fontSize, int newDpi);
friend void _stdcall TerminalBlinkCursor(void* terminal);
friend void _stdcall TerminalSetCursorVisible(void* terminal, const bool visible);
Expand Down
5 changes: 3 additions & 2 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -650,8 +650,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
}

const auto ch = e.Character();

const bool handled = _terminal->SendCharEvent(ch);
const auto scanCode = gsl::narrow_cast<WORD>(e.KeyStatus().ScanCode);
const auto modifiers = _GetPressedModifierKeys();
const bool handled = _terminal->SendCharEvent(ch, scanCode, modifiers);
e.Handled(handled);
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/ITerminalInput.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace Microsoft::Terminal::Core

virtual bool SendKeyEvent(const WORD vkey, const WORD scanCode, const ControlKeyStates states) = 0;
virtual bool SendMouseEvent(const COORD viewportPos, const unsigned int uiButton, const ControlKeyStates states, const short wheelDelta) = 0;
virtual bool SendCharEvent(const wchar_t ch) = 0;
virtual bool SendCharEvent(const wchar_t ch, const WORD scanCode, const ControlKeyStates states) = 0;

// void SendMouseEvent(uint row, uint col, KeyModifiers modifiers);
[[nodiscard]] virtual HRESULT UserResize(const COORD size) noexcept = 0;
Expand Down
165 changes: 122 additions & 43 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -387,65 +387,57 @@ bool Terminal::IsTrackingMouseInput() const noexcept
}

// Method Description:
// - Send this particular key event to the terminal. The terminal will translate
// the key and the modifiers pressed into the appropriate VT sequence for that
// key chord. If we do translate the key, we'll return true. In that case, the
// event should NOT be processed any further. If we return false, the event
// was NOT translated, and we should instead use the event to try and get the
// real character out of the event.
// - Send this particular (non-character) key event to the terminal.
// - The terminal will translate the key and the modifiers pressed into the
// appropriate VT sequence for that key chord. If we do translate the key,
// we'll return true. In that case, the event should NOT be processed any further.
// - Character events (e.g. WM_CHAR) are generally the best way to properly receive
// keyboard input on Windows though, as the OS is suited best at handling the
// translation of the current keyboard layout, dead keys, etc.
// As a result of this false is returned for all key events that contain characters.
// SendCharEvent may then be called with the data obtained from a character event.
// - As a special case we'll always handle VK_TAB key events.
// This must be done due to TermControl::_KeyDownHandler (one of the callers)
// always marking tab key events as handled, causing no character event to be raised.
// Arguments:
// - vkey: The vkey of the key pressed.
// - vkey: The vkey of the last pressed key.
// - scanCode: The scan code of the last pressed key.
// - states: The Microsoft::Terminal::Core::ControlKeyStates representing the modifier key states.
// Return Value:
// - true if we translated the key event, and it should not be processed any further.
// - false if we did not translate the key, and it should be processed into a character.
bool Terminal::SendKeyEvent(const WORD vkey, const WORD scanCode, const ControlKeyStates states)
{
TrySnapOnInput();
_StoreKeyEvent(vkey, scanCode);

const auto isAltOnlyPressed = states.IsAltPressed() && !states.IsCtrlPressed();

// Alt key sequences _require_ the char to be in the keyevent. If alt is
// pressed, manually get the character that's being typed, and put it in the
// KeyEvent.
// DON'T manually handle Alt+Space - the system will use this to bring up
// the system menu for restore, min/maximize, size, move, close
wchar_t ch = UNICODE_NULL;
if (states.IsAltPressed() && vkey != VK_SPACE)
// the system menu for restore, min/maximize, size, move, close.
// (This doesn't apply to Ctrl+Alt+Space.)
if (isAltOnlyPressed && vkey == VK_SPACE)
{
ch = _CharacterFromKeyEvent(vkey, scanCode, states);
return false;
}

if (states.IsCtrlPressed())
{
switch (vkey)
{
case 0x48:
// Manually handle Ctrl+H. Ctrl+H should be handled as Backspace. To do this
// correctly, the keyEvents's char needs to be set to Backspace.
// 0x48 is the VKEY for 'H', which isn't named
ch = UNICODE_BACKSPACE;
break;
case VK_SPACE:
// Manually handle Ctrl+Space here. The terminalInput translator requires
// the char to be set to Space for space handling to work correctly.
ch = UNICODE_SPACE;
break;
}
}
const auto ch = _CharacterFromKeyEvent(vkey, scanCode, states);

// Manually handle Escape here. If we let it fall through, it'll come
// back up through the character handler. It's registered as a translation
// in TerminalInput, so we'll let TerminalInput control it.
if (vkey == VK_ESCAPE)
// Delegate it to the character event handler if this key event can be
// mapped to one (see method description above). For Alt+key combinations
// we'll not receive another character event for some reason though.
// -> Don't delegate the event if this is a Alt+key combination.
//
// As a special case we'll furthermore always handle VK_TAB
// key events here instead of in Terminal::SendCharEvent.
// See the method description for more information.
if (!isAltOnlyPressed && vkey != VK_TAB && ch != UNICODE_NULL)
{
ch = UNICODE_ESC;
return false;
}

const bool manuallyHandled = ch != UNICODE_NULL;

KeyEvent keyEv{ true, 0, vkey, scanCode, ch, states.Value() };
const bool translated = _terminalInput->HandleKey(&keyEv);

return translated && manuallyHandled;
return _terminalInput->HandleKey(&keyEv);
}

// Method Description:
Expand Down Expand Up @@ -474,9 +466,39 @@ bool Terminal::SendMouseEvent(const COORD viewportPos, const unsigned int uiButt
return _terminalInput->HandleMouse(viewportPos, uiButton, GET_KEYSTATE_WPARAM(states.Value()), wheelDelta);
}

bool Terminal::SendCharEvent(const wchar_t ch)
// Method Description:
// - Send this particular character to the terminal.
// - This method is the counterpart to SendKeyEvent and behaves almost identical.
// The difference is the focus on sending characters to the terminal,
// whereas SendKeyEvent handles the sending of keys like the arrow keys.
// Arguments:
// - ch: The UTF-16 code unit to be sent.
// - scanCode: The scan code of the last pressed key. Can be left 0.
// - states: The Microsoft::Terminal::Core::ControlKeyStates representing the modifier key states.
// Return Value:
// - true if we translated the character event, and it should not be processed any further.
// - false otherwise.
bool Terminal::SendCharEvent(const wchar_t ch, const WORD scanCode, const ControlKeyStates states)
{
return _terminalInput->HandleChar(ch);
// DON'T manually handle Alt+Space - the system will use this to bring up
// the system menu for restore, min/maximize, size, move, close.
if (ch == L' ' && states.IsAltPressed() && !states.IsCtrlPressed())
{
return false;
}

auto vkey = _TakeVirtualKeyFromLastKeyEvent(scanCode);
if (vkey == 0 && scanCode != 0)
{
vkey = _VirtualKeyFromScanCode(scanCode);
}
if (vkey == 0)
{
vkey = _VirtualKeyFromCharacter(ch);
}

KeyEvent keyEv{ true, 0, vkey, scanCode, ch, states.Value() };
return _terminalInput->HandleKey(&keyEv);
}

// Method Description:
Expand All @@ -490,6 +512,29 @@ WORD Terminal::_ScanCodeFromVirtualKey(const WORD vkey) noexcept
return LOWORD(MapVirtualKeyW(vkey, MAPVK_VK_TO_VSC));
}

// Method Description:
// - Returns the virtual key code for the given keyboard's scan code.
// Arguments:
// - scanCode: The keyboard's scan code.
// Return Value:
// - The virtual key code. 0 if no mapping can be found.
WORD Terminal::_VirtualKeyFromScanCode(const WORD scanCode) noexcept
{
return LOWORD(MapVirtualKeyW(scanCode, MAPVK_VSC_TO_VK));
}

// Method Description:
// - Returns any virtual key code that produces the given character.
// Arguments:
// - scanCode: The keyboard's scan code.
// Return Value:
// - The virtual key code. 0 if no mapping can be found.
WORD Terminal::_VirtualKeyFromCharacter(const wchar_t ch) noexcept
{
const auto vkey = LOWORD(VkKeyScanW(ch));
return vkey == -1 ? 0 : vkey;
}

// Method Description:
// - Translates the specified virtual key code and keyboard state to the corresponding character.
// Arguments:
Expand Down Expand Up @@ -532,6 +577,40 @@ catch (...)
return UNICODE_INVALID;
}

// Method Description:
// - It's possible for a single scan code on a keyboard to
// produce different key codes depending on the keyboard state.
// MapVirtualKeyW(scanCode, MAPVK_VSC_TO_VK) will always chose one of the
// possibilities no matter what though and thus can't be used in SendCharEvent.
// - This method stores the key code from a key event (SendKeyEvent).
// If the key event contains character data, handling of the event will be
// denied, in order to delegate the work to the character event handler.
// - The character event handler (SendCharEvent) will now pick up
// the stored key code to restore the full key event data.
// Arguments:
// - vkey: The virtual key code.
// - scanCode: The scan code.
void Terminal::_StoreKeyEvent(const WORD vkey, const WORD scanCode)
{
_lastKeyEventCodes.emplace(KeyEventCodes{ vkey, scanCode });
}

// Method Description:
// - This method acts as a counterpart to _StoreKeyEvent and extracts a stored
// key code. As a safety measure it'll ensure that the given scan code
// matches the stored scan code from the previous key event.
// - See _StoreKeyEvent for more information.
// Arguments:
// - scanCode: The scan code.
// Return Value:
// - The key code matching the given scan code. Otherwise 0.
WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept
{
const auto codes = _lastKeyEventCodes.value_or(KeyEventCodes{});
_lastKeyEventCodes.reset();
return codes.ScanCode == scanCode ? codes.VirtualKey : 0;
}

// Method Description:
// - Acquire a read lock on the terminal.
// Return Value:
Expand Down
15 changes: 14 additions & 1 deletion src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class Microsoft::Terminal::Core::Terminal final :
// These methods are defined in Terminal.cpp
bool SendKeyEvent(const WORD vkey, const WORD scanCode, const Microsoft::Terminal::Core::ControlKeyStates states) override;
bool SendMouseEvent(const COORD viewportPos, const unsigned int uiButton, const ControlKeyStates states, const short wheelDelta) override;
bool SendCharEvent(const wchar_t ch) override;
bool SendCharEvent(const wchar_t ch, const WORD scanCode, const ControlKeyStates states) override;

[[nodiscard]] HRESULT UserResize(const COORD viewportSize) noexcept override;
void UserScrollViewport(const int viewTop) override;
Expand Down Expand Up @@ -251,9 +251,22 @@ class Microsoft::Terminal::Core::Terminal final :
// underneath them, while others would prefer to anchor it in place.
// Either way, we should make this behavior controlled by a setting.

// Since virtual keys are non-zero, you assume that this field is empty/invalid if it is.
struct KeyEventCodes
{
WORD VirtualKey;
WORD ScanCode;
};
std::optional<KeyEventCodes> _lastKeyEventCodes;

static WORD _ScanCodeFromVirtualKey(const WORD vkey) noexcept;
static WORD _VirtualKeyFromScanCode(const WORD scanCode) noexcept;
static WORD _VirtualKeyFromCharacter(const wchar_t ch) noexcept;
static wchar_t _CharacterFromKeyEvent(const WORD vkey, const WORD scanCode, const ControlKeyStates states) noexcept;

void _StoreKeyEvent(const WORD vkey, const WORD scanCode);
WORD _TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept;

int _VisibleStartIndex() const noexcept;
int _VisibleEndIndex() const noexcept;

Expand Down
5 changes: 3 additions & 2 deletions src/cascadia/UnitTests_TerminalCore/InputTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ namespace TerminalCoreUnitTests
// Verify that Alt+a generates a lowercase a on the input
expectedinput = L"\x1b"
"a";
VERIFY_IS_TRUE(term.SendKeyEvent(L'A', 0, ControlKeyStates::LeftAltPressed));
VERIFY_IS_TRUE(term.SendCharEvent(L'a', 0, ControlKeyStates::LeftAltPressed));

// Verify that Alt+shift+a generates a uppercase a on the input
expectedinput = L"\x1b"
"A";
VERIFY_IS_TRUE(term.SendKeyEvent(L'A', 0, ControlKeyStates::LeftAltPressed | ControlKeyStates::ShiftPressed));
VERIFY_IS_TRUE(term.SendCharEvent(L'A', 0, ControlKeyStates::LeftAltPressed | ControlKeyStates::ShiftPressed));
}

void InputTest::AltSpace()
Expand All @@ -62,5 +62,6 @@ namespace TerminalCoreUnitTests
// bring up the system menu for restore, min/maximize, size, move,
// close
VERIFY_IS_FALSE(term.SendKeyEvent(L' ', 0, ControlKeyStates::LeftAltPressed));
VERIFY_IS_FALSE(term.SendCharEvent(L' ', 0, ControlKeyStates::LeftAltPressed));
}
}
4 changes: 2 additions & 2 deletions src/cascadia/WpfTerminalControl/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,10 @@ public enum SetWindowPosFlags : uint
public static extern void DestroyTerminal(IntPtr terminal);

[DllImport("PublicTerminalCore.dll", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall)]
public static extern void TerminalSendKeyEvent(IntPtr terminal, IntPtr wParam);
public static extern void TerminalSendKeyEvent(IntPtr terminal, ushort vkey, ushort scanCode);

[DllImport("PublicTerminalCore.dll", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall)]
public static extern void TerminalSendCharEvent(IntPtr terminal, char ch);
public static extern void TerminalSendCharEvent(IntPtr terminal, char ch, ushort scanCode);

[DllImport("PublicTerminalCore.dll", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall)]
public static extern void TerminalSetTheme(IntPtr terminal, [MarshalAs(UnmanagedType.Struct)] TerminalTheme theme, string fontFamily, short fontSize, int newDpi);
Expand Down
6 changes: 4 additions & 2 deletions src/cascadia/WpfTerminalControl/TerminalContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -231,13 +231,15 @@ private IntPtr TerminalContainer_MessageHook(IntPtr hwnd, int msg, IntPtr wParam
NativeMethods.SetFocus(this.hwnd);
break;
case NativeMethods.WindowMessage.WM_KEYDOWN:
// WM_KEYDOWN lParam layout documentation: https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-keydown
NativeMethods.TerminalSetCursorVisible(this.terminal, true);
NativeMethods.TerminalClearSelection(this.terminal);
NativeMethods.TerminalSendKeyEvent(this.terminal, wParam);
NativeMethods.TerminalSendKeyEvent(this.terminal, (ushort)wParam, Marshal.ReadByte(lParam, 2));
this.blinkTimer?.Start();
break;
case NativeMethods.WindowMessage.WM_CHAR:
NativeMethods.TerminalSendCharEvent(this.terminal, (char)wParam);
// WM_CHAR lParam layout documentation: https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-char
NativeMethods.TerminalSendCharEvent(this.terminal, (char)wParam, Marshal.ReadByte(lParam, 2));
break;
case NativeMethods.WindowMessage.WM_WINDOWPOSCHANGED:
var windowpos = (NativeMethods.WINDOWPOS)Marshal.PtrToStructure(lParam, typeof(NativeMethods.WINDOWPOS));
Expand Down
Loading

1 comment on commit a9c9714

@github-actions
Copy link

Choose a reason for hiding this comment

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

New misspellings found, please review:

  • ecma
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
APPLOGIC
Impl
Spc
trackpads
unicodechar
ve
XY
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
applogic
ecma
impl
xy
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/a9c9714295cff7bf869ed74f03ebd37cef30dc80.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

Please sign in to comment.