Fixed #521 - AltGr combinations not working #1436

lhecker · 2019-06-22T20:25:38Z

Summary of the Pull Request

Let's start fixing #521 and the currently broken state of AltGr combinations, as it heavily affects many non-US users of this project.

PR Checklist

Closes AltGR SEQUENCES DO NOT WORK -- Can't type any AltGr combinations in Windows Terminal #521
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: AltGR SEQUENCES DO NOT WORK -- Can't type any AltGr combinations in Windows Terminal #521

Detailed Description of the Pull Request / Additional comments

I'm about 100% sure that this PR is not going to fix #521 by itself, but it's a start. 🙂
If others wan't to contribute to this PR, please feel free to do so. I'll gladly grant push permissions to my fork.

If you want to view the diff here on GitHub I recommend enabling "Hide whitespace changes" in the diff viewer.

Validation Steps Performed

This PR fixes all immediate/obvious issues in regards to AltGr for me in all types of shells I know.
I think it's fairly easy to see why it works, since I simply delegate the handling to WM_CHAR instead.

lhecker · 2019-06-23T12:48:50Z

Alright... I've spent 5 hours on this thing today. Dammit. I hope it was worth it. 😄

This PR should now be far more robust than my basic solution in #521.
I've changed the Terminal class to actually propagate the left/right states of Alt/Ctrl separately down into the TerminalInput class. That way we can continue (or... technically start...) using keyEvent.IsAltGrPressed() in the HandleKey method.

This also has the nice side effect of this PR allowing others to continue pressing Ctrl+Alt without it being confused for an AltGr key press.
For instance if I press AltGr+Q on my german keyboard it will now insert a @ character, BUT if I press Alt+Ctrl+Q it will properly send an escape sequence instead, just like it used to.

I've also fixed the input tests I believe, which now...

Reflect the changes to AltGr being handled by WM_CHAR instead
Added the missing Ctrl+/ sequence check - without it the tests are failing on systems with non-US keyboards

DHowett-MSFT · 2019-06-23T14:10:58Z

This is exciting. Thanks so much for looking at it! We’ll probably get to reviewing this after the weekend. 😄

src/terminal/input/terminalInput.cpp

src/terminal/adapter/ut_adapter/inputTest.cpp

miniksa · 2019-06-24T20:59:50Z

src/cascadia/TerminalControl/TermControl.cpp

-    // - a KeyModifiers value with flags set for each key that's pressed.
-    Settings::KeyModifiers TermControl::_GetPressedModifierKeys() const
+    // - The combined ControlKeyState flags as a bitfield.
+    DWORD TermControl::_GetPressedModifierKeys() const


I think @zadjii-msft was actively avoiding using a loose number of flags for representing this.

Even though Settings::KeyModifiers is no longer appropriate (given it treats left/right alt/ctrl the same), maybe it's appropriate to either expand it to have a preference (if folks want it and set both alts or both ctrls if the regular alt/ctrl is chosen) or appropriate to make a different more strongly typed flag structure for this.

To be fair, I don't mind it this way.

src/cascadia/TerminalControl/TermControl.cpp

src/terminal/input/terminalInput.cpp

lhecker · 2019-06-25T20:33:06Z

@miniksa Thank you for your kind review! I've reverted most parts of the changeset and tried to be as minimalistic as possible this time. You'll find that the diff should be easier to read now.

I could optimize the code structure here and there to use e.g. switches (like I did originally), but I feel like I might not be following any plans you guys might already have in mind for this project. For now I'd like make this PR simple and leave any larger refactorings to you. 🙂

Except of course... Regarding the modifiers bitfield... I also don't think my current approach is acceptable either.
I'm slightly unsure what you mean with "expand it [the Settings::KeyModifiers structure] to have a preference" though.
What I'd like to suggest is to write a class that abstracts over the exact same bitfield used by the TerminalInput module, but provides a simplified API to query for Alt/Ctrl/AltGr states, similar to how KeyEvent does it already. That way this class would stay compatible with the DWORD based APIs. 🙂
Since the diff is relatively minimalistic such changes can be done at any point in time though IMO.
Either way is fine for me (i.e. me writing this class or not as part of this PR).

Edit: Ah you're already done reviewing. 😅

yveslange · 2019-06-27T10:18:25Z

Please, can you review this issue ? For most people in Sweden, Germany and Switzerland the terminal with WSL is unusable at the moment without this patch.

utybo · 2019-06-27T12:45:53Z

Also relatively unusable for French users, given that the issue this PR fixes prevents using all sorts of keys. #{[|\^@]} all require pressing AltGr for French AZERTY keyboards.

pekkah · 2019-06-27T13:10:09Z

Same for Finnish users. Cannot navigate paths as cannot type path separators.

miniksa · 2019-06-27T19:49:29Z

@miniksa Michael Niksa FTE Thank you for your kind review! I've reverted most parts of the changeset and tried to be as minimalistic as possible this time. You'll find that the diff should be easier to read now.

I could optimize the code structure here and there to use e.g. switches (like I did originally), but I feel like I might not be following any plans you guys might already have in mind for this project. For now I'd like make this PR simple and leave any larger refactorings to you. 🙂

Except of course... Regarding the modifiers bitfield... I also don't think my current approach is acceptable either.
I'm slightly unsure what you mean with "expand it [the Settings::KeyModifiers structure] to have a preference" though.
What I'd like to suggest is to write a class that abstracts over the exact same bitfield used by the TerminalInput module, but provides a simplified API to query for Alt/Ctrl/AltGr states, similar to how KeyEvent does it already. That way this class would stay compatible with the DWORD based APIs. 🙂
Since the diff is relatively minimalistic such changes can be done at any point in time though IMO.
Either way is fine for me (i.e. me writing this class or not as part of this PR).

Edit: Ah you're already done reviewing. 😅

If you could write a follow on issue or PR to make it into a nice class that does what you say... that would be ideal.

For everyone else, I'll try to get someone else to 2nd this and we can correct anything that @zadjii-msft finds disagreeable later when he gets back.

lhecker · 2019-06-27T20:05:29Z

@miniksa Do you believe it might be possible to get this PR through until the weekend? Coincidentally I'm going to have some free time then and could work on that. But don't worry if it's not possible. We'll find the necessary time/solution either way, right? 🙂

miniksa · 2019-06-27T21:15:40Z

@lhecker, I think we could merge it today if I can find someone with 15 minutes to read it. Let me keep trying. We have had meetings this afternoon.

DHowett-MSFT

Thanks so much. This is great.

patriksvensson · 2019-06-29T01:28:24Z

@lhecker Just noticed this was merged. Thank you so much for submitting a PR and fixing this issue!

This commit changes how TerminalControl/TerminalCore handle key events to give it better knowledge about modifier states at the lower levels. (cherry picked from commit 6775325)

This commit changes how TerminalControl/TerminalCore handle key events to give it better knowledge about modifier states at the lower levels.

lhecker mentioned this pull request Jun 22, 2019

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

Closed

lhecker changed the title ~~[WIP] Fixed #521 - AltGr combinations not working~~ Fixed #521 - AltGr combinations not working Jun 23, 2019

lhecker commented Jun 23, 2019

View reviewed changes

src/terminal/input/terminalInput.cpp Outdated Show resolved Hide resolved

DHowett-MSFT requested review from miniksa and zadjii-msft June 25, 2019 16:38

miniksa requested changes Jun 25, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 25, 2019

Fixed #521 - AltGr combinations not working

0ad7f92

miniksa approved these changes Jun 25, 2019

View reviewed changes

DHowett-MSFT approved these changes Jun 27, 2019

View reviewed changes

DHowett-MSFT merged commit 6775325 into microsoft:master Jun 27, 2019

cpbotha mentioned this pull request Jun 28, 2019

Terminal shows an empty window and then crashes #1364

Closed

lhecker deleted the fix-altgr-handling branch June 28, 2019 16:56

lhecker mentioned this pull request Jun 29, 2019

Add a ControlKeyStates wrapper class #1718

Merged

4 tasks

DHowett-MSFT mentioned this pull request Jul 2, 2019

Cherry-pick a number of fixes for a v0.2 servicing release #1782

Merged

frousselet mentioned this pull request Jul 3, 2019

Can't use Alt Gr 0x7c13/Notepads#4

Closed

lhecker restored the fix-altgr-handling branch September 19, 2019 16:54

lhecker deleted the fix-altgr-handling branch September 19, 2019 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #521 - AltGr combinations not working #1436

Fixed #521 - AltGr combinations not working #1436

lhecker commented Jun 22, 2019 •

edited

Loading

lhecker commented Jun 23, 2019 •

edited

Loading

DHowett-MSFT commented Jun 23, 2019

miniksa Jun 24, 2019

miniksa Jun 25, 2019

lhecker commented Jun 25, 2019 •

edited

Loading

yveslange commented Jun 27, 2019

utybo commented Jun 27, 2019 •

edited

Loading

pekkah commented Jun 27, 2019

miniksa commented Jun 27, 2019

lhecker commented Jun 27, 2019

miniksa commented Jun 27, 2019

DHowett-MSFT left a comment

patriksvensson commented Jun 29, 2019

Fixed #521 - AltGr combinations not working #1436

Fixed #521 - AltGr combinations not working #1436

Conversation

lhecker commented Jun 22, 2019 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

lhecker commented Jun 23, 2019 • edited Loading

DHowett-MSFT commented Jun 23, 2019

miniksa Jun 24, 2019

Choose a reason for hiding this comment

miniksa Jun 25, 2019

Choose a reason for hiding this comment

lhecker commented Jun 25, 2019 • edited Loading

yveslange commented Jun 27, 2019

utybo commented Jun 27, 2019 • edited Loading

pekkah commented Jun 27, 2019

miniksa commented Jun 27, 2019

lhecker commented Jun 27, 2019

miniksa commented Jun 27, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

patriksvensson commented Jun 29, 2019

lhecker commented Jun 22, 2019 •

edited

Loading

lhecker commented Jun 23, 2019 •

edited

Loading

lhecker commented Jun 25, 2019 •

edited

Loading

utybo commented Jun 27, 2019 •

edited

Loading