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

During Alt+Numpad composition, stash keys in case we bail out #17774

Merged
merged 2 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 52 additions & 28 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1543,7 +1543,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (encoding == AltNumpadEncoding::Unicode)
{
// UTF-32 -> UTF-16
if (s.accumulator <= 0xffff)
if (s.accumulator == 0)
{
// If the user pressed Alt + VK_ADD, then released Alt, they probably didn't intend to insert a numpad character at all.
// Send any accumulated key events instead.
lhecker marked this conversation as resolved.
Show resolved Hide resolved
bool h = true;
for (auto&& e : _altNumpadState.cachedKeyEvents)
{
h = h && _TrySendKeyEvent(std::get<0>(e), std::get<1>(e), std::get<2>(e), std::get<3>(e));
}
// Send the alt keyup we are currently processing
h = h && _TrySendKeyEvent(vkey, scanCode, modifiers, keyDown);
// do not accumulate into the buffer
Copy link
Member Author

Choose a reason for hiding this comment

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

oh we should probably return h somewhere...

}
else if (s.accumulator <= 0xffff)
{
buf[buf_len++] = static_cast<uint16_t>(s.accumulator);
}
Expand Down Expand Up @@ -1601,53 +1614,64 @@ namespace winrt::Microsoft::Terminal::Control::implementation
SCROLLLOCK_ON |
CAPSLOCK_ON;

if (keyDown && (modifiers.Value() & ~permittedModifiers) == 0)
if ((modifiers.Value() & ~permittedModifiers) == 0)
{
auto& s = _altNumpadState;

if (vkey == VK_ADD)
{
// Alt '+' <number> is used to input Unicode code points.
// Every time you press + it resets the entire state
// in the original OS implementation as well.
s.encoding = AltNumpadEncoding::Unicode;
s.accumulator = 0;
s.active = true;
if (keyDown)
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to move keyDown into these places so we could stash the keyUp event in case composition was active.

This makes ALTdown KP+down KP+up ALTup work.

Copy link
Member

Choose a reason for hiding this comment

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

This may be easier to read if the if (keyDown) was outside the if/else chain instead of inside it.

{
// Alt '+' <number> is used to input Unicode code points.
// Every time you press + it resets the entire state
// in the original OS implementation as well.
s.encoding = AltNumpadEncoding::Unicode;
s.accumulator = 0;
s.active = true;
}
}
else if (vkey == VK_NUMPAD0 && s.encoding == AltNumpadEncoding::OEM && s.accumulator == 0)
{
// Alt '0' <number> is used to input ANSI code points.
// Otherwise, they're OEM codepoints.
s.encoding = AltNumpadEncoding::ANSI;
s.active = true;
if (keyDown)
{
// Alt '0' <number> is used to input ANSI code points.
// Otherwise, they're OEM codepoints.
s.encoding = AltNumpadEncoding::ANSI;
s.active = true;
}
}
else
{
// Otherwise, append the pressed key to the accumulator.
const uint32_t base = s.encoding == AltNumpadEncoding::Unicode ? 16 : 10;
uint32_t add = 0xffffff;

if (vkey >= VK_NUMPAD0 && vkey <= VK_NUMPAD9)
if (keyDown)
{
add = vkey - VK_NUMPAD0;
}
else if (vkey >= 'A' && vkey <= 'F')
{
add = vkey - 'A' + 10;
}
// Otherwise, append the pressed key to the accumulator.
const uint32_t base = s.encoding == AltNumpadEncoding::Unicode ? 16 : 10;
uint32_t add = 0xffffff;

// Pressing Alt + <not a number> should not activate the Alt+Numpad input, however.
if (add < base)
{
s.accumulator = std::min(s.accumulator * base + add, 0x10FFFFu);
s.active = true;
if (vkey >= VK_NUMPAD0 && vkey <= VK_NUMPAD9)
{
add = vkey - VK_NUMPAD0;
}
else if (vkey >= 'A' && vkey <= 'F')
{
add = vkey - 'A' + 10;
}

// Pressing Alt + <not a number> should not activate the Alt+Numpad input, however.
if (add < base)
{
s.accumulator = std::min(s.accumulator * base + add, 0x10FFFFu);
s.active = true;
}
}
}

// If someone pressed Alt + <not a number>, we'll skip the early
// return and send the Alt key combination as per usual.
if (s.active)
{
// Cache it in case we have to emit it after alt is released
_altNumpadState.cachedKeyEvents.emplace_back(vkey, scanCode, modifiers, keyDown);
return true;
}

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// The state can be active, while the accumulator is 0, if the user pressed Alt+Numpad0 which enabled
// the OEM encoding mode (= active), and then pressed Numpad0 again (= accumulator is still 0).
bool active = false;
til::small_vector<std::tuple<WORD, WORD, ::Microsoft::Terminal::Core::ControlKeyStates, bool>, 4> cachedKeyEvents;
Copy link
Member

Choose a reason for hiding this comment

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

I'd introduce a struct for this to make it more descriptive.

};
AltNumpadState _altNumpadState;

Expand Down
Loading