[Leonard Hecker (lhecker)]

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.

[Leonard Hecker (lhecker)]

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

[Dustin L. Howett (MSFT) (DHowett-MSFT)]

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

[Michael Niksa (miniksa)]

I think Mike Griese (@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.

[Michael Niksa (miniksa)]

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

[Leonard Hecker (lhecker)]

Michael Niksa (@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. 😅

[Yves Lange (yveslange)]

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.

[Matthieu S. (utybo)]

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.

[Pekka Heikura (pekkah)]

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

[Michael Niksa (miniksa)]

Michael Niksa (@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 Mike Griese (@zadjii-msft) finds disagreeable later when he gets back.

[Leonard Hecker (lhecker)]

Michael Niksa (@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? 🙂

[Michael Niksa (miniksa)]

Leonard Hecker (@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.

[Dustin L. Howett (MSFT) (DHowett-MSFT)]

Thanks so much. This is great.

[Patrik Svensson (patriksvensson)]

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