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

Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235

Merged
merged 1 commit into from
Aug 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
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
79 changes: 62 additions & 17 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -619,35 +619,41 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
}

const auto modifiers = _GetPressedModifierKeys();
const auto vkey = static_cast<WORD>(e.OriginalKey());

// AltGr key combinations don't always contain any meaningful,
// pretranslated unicode character during WM_KEYDOWN.
// E.g. on a German keyboard AltGr+Q should result in a "@" character,
// but actually results in "Q" with Alt and Ctrl modifier states.
// By returning false though, we can abort handling this WM_KEYDOWN
// event and let the WM_CHAR handler kick in, which will be
// provided with an appropriate unicode character.
lhecker marked this conversation as resolved.
Show resolved Hide resolved
//
// GH#2235: Make sure to handle AltGr before trying keybindings,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// GH#2235: Make sure to handle AltGr before trying keybindings,
// Make sure to handle AltGr before trying keybindings,

It feels a bit weird to reference this PR. Let's just keep the comment.

Copy link
Member

Choose a reason for hiding this comment

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

I usually like referencing the issue in the code near a bug fix like this. It can oftentimes provide a lot more context than just the comment in the code.

// so Ctrl+Alt keybindings won't eat an AltGr keypress.
if (modifiers.IsAltGrPressed())
{
_HandleVoidKeyEvent();
e.Handled(false);
return;
}

const auto vkey = static_cast<WORD>(e.OriginalKey());
bool handled = false;

auto bindings = _settings.KeyBindings();
if (bindings)
{
KeyChord chord(
handled = bindings.TryKeyChord({
modifiers.IsCtrlPressed(),
modifiers.IsAltPressed(),
modifiers.IsShiftPressed(),
vkey);
handled = bindings.TryKeyChord(chord);
vkey,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
vkey,
vkey

I don't think we need that extra , here

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a list initialization expression, which is kinda akin to an array and I believe this project uses trailing commas for this. Right?
Do you want me to revert this change? It's technically not needed at all for this PR. 🙂
(It only snuck in, because I structured the code entirely differently in a previous attempt for this PR and I preferred block indents over visual indents. 🙈)

P.S.: In fact modern languages often allow (or even enforce) trailing commas on multiline expressions even, because it simplifies refactoring and diffs in the future.
If you want to add a trailing argument in the future and your vkey doesn't have a trailing comma already, your diff will contain changes to two lines, even though one of them only adds a comma. This also makes git blame harder to use. Realistically it's unlikely someone is going to write a better VCS anytime soon either, that would understand this change contextually.
In Go for instance this will yield an missing ',' before newline in argument list error.
Rust and Python (PEP8) follow the same reasoning after lengthy discussions each.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this.

});
}

if (!handled)
{
_terminal->ClearSelection();
// If the terminal translated the key, mark the event as handled.
// This will prevent the system from trying to get the character out
// of it and sending us a CharacterRecieved event.
handled = _terminal->SendKeyEvent(vkey, modifiers);

if (_cursorTimer.has_value())
{
// Manually show the cursor when a key is pressed. Restarting
// the timer prevents flickering.
_terminal->SetCursorVisible(true);
_cursorTimer.value().Start();
}
handled = _TrySendKeyEvent(vkey, modifiers);
}

// Manually prevent keyboard navigation with tab. We want to send tab to
Expand All @@ -661,6 +667,45 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
e.Handled(handled);
}

// Method Description:
// - Some key events cannot be handled (e.g. AltGr combinations) and are
// delegated to the character handler. Just like with _TrySendKeyEvent(),
// the character handler counts on us though to:
// - Clears the current selection.
// - Makes the cursor briefly visible during typing.
void TermControl::_HandleVoidKeyEvent()
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think the name of this method could probably be better - the important meat of what this function is trying to accomplish isn't the sending of the void keypress, it's the dismissing of the selection and the cursor visibility.

Copy link
Member Author

@lhecker lhecker Aug 5, 2019

Choose a reason for hiding this comment

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

Do you have a suggestion for the function name? I can't come up with anything. 😕
(If we had something different to SendKeyEvent - like e.g. GenerateKeyEvent, which returns an optional input sequence - we could more comfortably abstract "reset selection and cursor" I suppose. That way you could move the "reset selection and cursor" handling to the more correct char handler, without the key event handler being responsible for this, right?)

Copy link
Member

Choose a reason for hiding this comment

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

If we're just sending two constants over to _TrySendKeyEvent(), why not get rid of the helper function overall and just add a comment where you use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact that _TrySendKeyEvent() supports a vkey == 0 seemed kinda hacky to me and I wanted to hide this hacky fact even from calls within the same class. So I created a hacky-wrapper function and prevent other methods from being "spoiled". 😄
Do you want me to inline this function?

{
_TrySendKeyEvent(0, {});
}

// Method Description:
// - Send this particular key event to the terminal.
// See Terminal::SendKeyEvent for more information.
// - Clears the current selection.
// - Makes the cursor briefly visible during typing.
// Arguments:
// - vkey: The vkey of the key pressed.
// - states: The Microsoft::Terminal::Core::ControlKeyStates representing the modifier key states.
bool TermControl::_TrySendKeyEvent(WORD vkey, const ControlKeyStates modifiers)
{
_terminal->ClearSelection();

// If the terminal translated the key, mark the event as handled.
// This will prevent the system from trying to get the character out
// of it and sending us a CharacterRecieved event.
const auto handled = vkey ? _terminal->SendKeyEvent(vkey, modifiers) : true;

if (_cursorTimer.has_value())
{
// Manually show the cursor when a key is pressed. Restarting
// the timer prevents flickering.
_terminal->SetCursorVisible(true);
_cursorTimer.value().Start();
}

return handled;
}

// Method Description:
// - handle a mouse click event. Begin selection process.
// Arguments:
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
static Windows::UI::Xaml::Thickness _ParseThicknessFromPadding(const hstring padding);

::Microsoft::Terminal::Core::ControlKeyStates _GetPressedModifierKeys() const;
void _HandleVoidKeyEvent();
bool _TrySendKeyEvent(WORD vkey, ::Microsoft::Terminal::Core::ControlKeyStates modifiers);

const COORD _GetTerminalPosition(winrt::Windows::Foundation::Point cursorPosition);
const unsigned int _NumberOfClicks(winrt::Windows::Foundation::Point clickPos, Timestamp clickTime);
Expand Down
12 changes: 0 additions & 12 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,18 +210,6 @@ bool Terminal::SendKeyEvent(const WORD vkey, const ControlKeyStates states)
_NotifyScrollEvent();
}

// AltGr key combinations don't always contain any meaningful,
// pretranslated unicode character during WM_KEYDOWN.
// E.g. on a German keyboard AltGr+Q should result in a "@" character,
// but actually results in "Q" with Alt and Ctrl modifier states.
// By returning false though, we can abort handling this WM_KEYDOWN
// event and let the WM_CHAR handler kick in, which will be
// provided with an appropriate unicode character.
if (states.IsAltGrPressed())
{
return false;
}

// 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.
Expand Down