Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235

lhecker · 2019-08-04T12:59:55Z

Summary of the Pull Request

This moves the detection of AltGr keypresses in front of the shortcut handling.
This allows one to have Ctrl+Alt shortcuts, while simultaneously being able to use the AltGr key for special characters.

References

This fixes a regression in v0.3.2142.0, which automatically creates Ctrl+Alt+[0-9] shortcuts for switching between tabs. These conflicted with the detection of the AltGr key. in SendKeyEvent().
Refixes #521.

PR Checklist

Closes AltGR SEQUENCES DO NOT WORK -- Can't type any AltGr combinations in Windows Terminal #521, Closes Bug Report (default key mapping is not good if need use location setup which need AltGr to get some ascii-7 chars) #2254
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

This PR has the flaw that it doesn't allow one to define AltGr shortcuts anymore.
Considering that it's probably more important to get a fix out of the door quickly I personally see this as a minor flaw though. VS Code has the same "flaw" for instance and doesn't allow AltGr combinations either.
We could fix it more correctly later on, by modifying the IKeyBindings interface and KeyChord class.

Validation Steps Performed

Enter AltGr-characters on my german keyboard and ensuring they appear in my shell.
Ensure that Ctrl+Alt+[0-9] shortcuts simultaneously work.

lhecker · 2019-08-04T13:14:58Z

I've based this PR on a08666b btw, which is the common ancestor between master and release-0.3.
It should be possible to merge this PR into both branches without rebasing or cherry picking, etc.

DHowett-MSFT

I think I’m comfortable with this. @lhecker again, thanks so much for being our AltGr expert. 😄

DHowett-MSFT · 2019-08-04T14:47:40Z

src/cascadia/TerminalControl/TermControl.cpp

+        // provided with an appropriate unicode character.
+        if (modifiers.IsAltGrPressed())
+        {
+            _SendKeyEvent();


I’m not sure why we need to send an empty key event here.

The key event handler must call _terminal->ClearSelection(), because the char handler doesn't.
Additionally _terminal->SetCursorVisible(true); must be called on a key press as the comment at line 683 states.

DHowett-MSFT · 2019-08-04T14:49:10Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -661,6 +664,31 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
        e.Handled(handled);
    }

+    void TermControl::_SendKeyEvent()


Feel a bit weird about a void method wrapping a bool one and discarding the result

DHowett-MSFT · 2019-08-04T14:50:08Z

src/cascadia/TerminalControl/TermControl.cpp

+        _SendKeyEvent(0, {});
+    }
+
+    bool TermControl::_SendKeyEvent(WORD vkey, ControlKeyStates modifiers)


nit: add a doc comment!

lhecker · 2019-08-04T15:56:02Z

@DHowett-MSFT No problem. 😅
I guess I just like "hecking" fixes together if it's broken. *ba dum tss* 😄

I've amended this PR with comments for the two new functions. I've also tried to give them slightly more obvious names (although I failed at it I guess lol).
I'm not entirely sure how to beautify the code any further.
There's probably a reason for it, but IMHO the architecture between TerminalControl::TermControl, Core::Terminal and Console::VirtualTerminal feels kinda... redundant, as all 3 of them fiddle around with vkeys and modifiers. I believe merging these classes and resplitting the responsibilities might yield subjectively "better" code in the long run.

E.g. my thoughts regarding character handling...

You could for instance abstract AltGr, shortcut, etc. handling into one class each and concate them together to a stream of key/character event processors which implement the full, final keyboard handling.
TermControl would also implement the common interface and be the final (self-referential) receiver in the chain.
That way you do away with the current, loopy design, of TermControl handling a bit of the keys, pushing it down to Terminal, which implements a bit of the handling, which then pushes it down to VirtualTerminal which implements a bit of the handling and finally calls TermControl again through a std::function callback, which then stringifies the character/escape-sequence array and then calls TermControl again through a std::function callback which then sends it to the shell.
It's very "loopy". 😄

zadjii-msft

I have a pair of nits, but nothing worth blocking over. If someone else says this is fine and we should ship it, I'm not going to block this.

Overall great work with the fix here.

src/cascadia/TerminalControl/TermControl.cpp

zadjii-msft · 2019-08-05T14:23:37Z

src/cascadia/TerminalControl/TermControl.cpp

+    //   the character handler counts on us though to:
+    // - Clears the current selection.
+    // - Makes the cursor briefly visible during typing.
+    void TermControl::_HandleVoidKeyEvent()


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.

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?)

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?

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?

This moves the detection of AltGr keypresses in front of the shortcut handling. This allows one to have Ctrl+Alt shortcuts, while simultaneously being able to use the AltGr key for special characters.

carlos-zamora · 2019-08-05T15:51:58Z

src/cascadia/TerminalControl/TermControl.cpp

                modifiers.IsCtrlPressed(),
                modifiers.IsAltPressed(),
                modifiers.IsShiftPressed(),
-                vkey);
-            handled = bindings.TryKeyChord(chord);
+                vkey,


Suggested change

vkey,

vkey

I don't think we need that extra , here

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.

I like this.

carlos-zamora · 2019-08-05T15:57:17Z

src/cascadia/TerminalControl/TermControl.cpp

+        // event and let the WM_CHAR handler kick in, which will be
+        // provided with an appropriate unicode character.
+        //
+        // GH#2235: Make sure to handle AltGr before trying keybindings,


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.

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.

carlos-zamora · 2019-08-05T16:00:05Z

src/cascadia/TerminalControl/TermControl.cpp

+    //   the character handler counts on us though to:
+    // - Clears the current selection.
+    // - Makes the cursor briefly visible during typing.
+    void TermControl::_HandleVoidKeyEvent()


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?

zadjii-msft

We discussed this in a meeting today and we're all fine shipping this in the state it is. The naming thing was a nit anyways and this is real goodness we'd like to get out.

zadjii-msft · 2019-08-05T21:59:00Z

@DHowett-MSFT do the thing

This moves the detection of AltGr keypresses in front of the shortcut handling. This allows one to have Ctrl+Alt shortcuts, while simultaneously being able to use the AltGr key for special characters. (cherry picked from commit 4529e46)

ghost · 2019-08-06T18:37:05Z

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

Handy links:

henrik-jensen mentioned this pull request Aug 4, 2019

AltGR SEQUENCES DO NOT WORK -- Can't type any AltGr combinations in Windows Terminal #521

Closed

henrik-jensen mentioned this pull request Aug 4, 2019

Change some default keybindings #2014

Merged

4 tasks

DHowett-MSFT approved these changes Aug 4, 2019

View reviewed changes

DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Aug 4, 2019

zadjii-msft reviewed Aug 5, 2019

View reviewed changes

zadjii-msft mentioned this pull request Aug 5, 2019

Bug Report (default key mapping is not good if need use location setup which need AltGr to get ascii-7 chars) #2253

Closed

kshji mentioned this pull request Aug 5, 2019

Bug Report (default key mapping is not good if need use location setup which need AltGr to get some ascii-7 chars) #2254

Closed

Fixed Ctrl+Alt shortcuts conflicting with AltGr

faccc1c

This moves the detection of AltGr keypresses in front of the shortcut handling. This allows one to have Ctrl+Alt shortcuts, while simultaneously being able to use the AltGr key for special characters.

carlos-zamora reviewed Aug 5, 2019

View reviewed changes

zadjii-msft approved these changes Aug 5, 2019

View reviewed changes

zadjii-msft merged commit 4529e46 into microsoft:master Aug 5, 2019

ghost removed the Needs-Second It's a PR that needs another sign-off label Aug 5, 2019

ellie-idb mentioned this pull request Aug 15, 2019

Avoid passing commands through with the shift modifier #2031

Closed

2 tasks

lhecker deleted the fix-ctrl-alt-shortcuts branch September 21, 2019 12:03

lhecker mentioned this pull request Jun 8, 2021

keybindings of dead keys using AltGr+X #10218

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235

Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235

lhecker commented Aug 4, 2019 •

edited by DHowett-MSFT

Loading

lhecker commented Aug 4, 2019

DHowett-MSFT left a comment

DHowett-MSFT Aug 4, 2019

lhecker Aug 4, 2019

DHowett-MSFT Aug 4, 2019

DHowett-MSFT Aug 4, 2019

lhecker commented Aug 4, 2019 •

edited

Loading

zadjii-msft left a comment

zadjii-msft Aug 5, 2019

lhecker Aug 5, 2019 •

edited

Loading

carlos-zamora Aug 5, 2019

lhecker Aug 5, 2019

carlos-zamora Aug 5, 2019

lhecker Aug 5, 2019

DHowett-MSFT Aug 5, 2019

carlos-zamora Aug 5, 2019

zadjii-msft Aug 5, 2019

carlos-zamora Aug 5, 2019

zadjii-msft left a comment

zadjii-msft commented Aug 5, 2019

ghost commented Aug 6, 2019

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

Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235

Fixed Ctrl+Alt shortcuts conflicting with AltGr #2235

Conversation

lhecker commented Aug 4, 2019 • edited by DHowett-MSFT Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

lhecker commented Aug 4, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Aug 4, 2019 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft commented Aug 5, 2019

ghost commented Aug 6, 2019

lhecker commented Aug 4, 2019 •

edited by DHowett-MSFT

Loading

lhecker commented Aug 4, 2019 •

edited

Loading

lhecker Aug 5, 2019 •

edited

Loading