Fixed #3101: Alt+Arrow-Keys print extra characters

[lhecker]

Summary of the Pull Request

This PR potentially fixes #3101.

PR Checklist

• Closes Alt arrow keys print extra characters #3101.
• 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 fixes #3101 by setting flag 0 in ToUnicodeEx() even though the documentation says "If bit 0 is set, a menu is active.". I'm not 100% sure why it works, but it definitely does in this case.
I thought that bit 2, which claims that "keyboard state is not changed" would be sufficient to prevent this from happening, but it seems that's not the case.

I believe this PR should be verified by a developer at Microsoft who's familiar with the internal workings of ToUnicodeEx().
We need this function (or something similar) to translate Alt+Key combinations to proper unicode.
But at the same time it should not send us any additional IBM-style Alt Codes to our character handler if that translation fails (and ToUnicodeEx() returns 0).

Validation Steps Performed

See #3101 for more information. I ensured that Alt+Arrow-Key combinations do not print ◘☻♠♦ anymore.

[miniksa]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[miniksa]

I believe this PR should be verified by a developer at Microsoft who's familiar with the internal workings of ToUnicodeEx().
We need this function (or something similar) to translate Alt+Key combinations to proper unicode.
But at the same time it should not send us any additional IBM-style Alt Codes to our character handler if that translation fails (and ToUnicodeEx() returns 0).

OK. I went to read the source code for the input code deep behind ToUnicodeEx(). It turns out that we're running afoul of the code that attempts to translate Alt+NumPad sequences into another symbol/character.

The arrow keys in the cluster of four have the same scan codes from the keyboard as the arrow keys on the numpad 2, 4, 6, 8. When the 0 bit is unset in the wFlags field, it attempts to map them according to as if they came in through the numpad.

If we look at https://en.wikipedia.org/wiki/Code_page_437... we can see that the symbols on 2, 4, 6, and 8 respectively were ☻♦♠◘. So it's faithfully mapping them as if you entered them on the numpad.

When the 0 bit is set in wFlags, the numpad mapping code is skipped.

So committing this fix will fix the extra characters coming out... but it will probably also end up breaking things like numpad Alt+1+3+7 becomes ë

[lhecker]

Thank you so much for looking into this!
I believe the documentation around this should be a bit extended on MSDN. Unfortunately I didn't find out whether they can be edited on GitHub as well.

And just in case you do have the time: Should we keep setting the 2nd bit in wFlags? What does it do? I wasn't able to see any difference with it set and without it.

I don't think the missing input of Alt codes will be large issue for now though.
While probably some developers might be used to using these, just as many might want to use Alt+Arrow combinations.

On the other hand the fact that we're probably not handling cases where ToUnicodeEx returns -1 correctly might be much more serious (dead keys are quite important in some languages).
Furthermore I recently read that Windows can deliver UTF-16 surrogates as two separate key events, which AFAICS means we're sending each surrogate separately as UTF-8 to the shell as well (which forbids surrogate byte sequences).

[lhecker]

The PR is generally ready to be merged from my side.
But in case you don't merge it until tomorrow I'll try to play around with this part of the code to try and see whether we're doing it correctly in case of dead keys and if there's possibly a more robust solution.

[miniksa]

The 2nd bit is about dead keys.

If it is off, then when you start a dead key combination, some state about the dead key is persisted in the keyboard layout state until the next call.

If it is on, then it will skip the persistence phase as it processes the potential dead key.

(That's my 2 minute read of it... it's massively complex. If you need further than that, let me know.)

[lhecker]

@miniksa I had a bit time just now and debugged this part of the code.

• I still couldn't find any difference between the 2nd being set and not set. Let's leave it set, because ToUnicodeEx does a bit less work that way, which is... good I guess?
• Alt^ (a dead key) causes ToUnicodeEx to return 2 and a buffer containing ^^. Since it returns 2 we ignore this. If you press Alt^ again, ToUnicodeEx returns -1 and ^, which we don't ignore. Afterwards this cycle begins anew. I don't think this is particularly wanted by the users. If you hold Alt and press a dead key it's supposed to instantly generate an escape code right?

To fix the latter tangential issue, I'd like to propose that we simply return the first character of the output buffer, if the return value of ToUnicodeEx is >1. What do you think? (I'd open a seperate PR for this.)

[miniksa]

@miniksa Michael Niksa FTE I had a bit time just now and debugged this part of the code.

• I still couldn't find any difference between the 2nd being set and not set. Let's leave it set, because ToUnicodeEx does a bit less work that way, which is... good I guess?

Yeah, sure. Until proven otherwise, I'm fine to leave the other bit on.

• Alt^ (a dead key) causes ToUnicodeEx to return 2 and a buffer containing ^^. Since it returns 2 we ignore this. If you press Alt^ again, ToUnicodeEx returns -1 and ^, which we don't ignore. Afterwards this cycle begins anew. I don't think this is particularly wanted by the users. If you hold Alt and press a dead key it's supposed to instantly generate an escape code right?

Sorry, on which keyboard layout are we talking? My understanding of "dead keys" was like the US-International keyboard where you press/release ' then press/release e to get é. I think pressAltGr + press e then release both chorded on European keyboards is supposed to fire immediately... is that also called a "dead key" combo?

This is also the behavior/return codes that I would expect to change with and without the 2 bit set in the flags field. Does it not change?

From what I'm reading internally...
-1 return code is analogous to WM_DEADCHAR. And https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-deadchar says "typically is used by applications to give the user feedback about each key pressed. For example, an application can display the accent in the current character position without moving the caret." So I don't think it should actually be inserted for a -1 reply.

As long as both of them yield the same result as typing into something like notepad.exe, then I'm happy.

To fix the latter tangential issue, I'd like to propose that we simply return the first character of the output buffer, if the return value of ToUnicodeEx is >1. What do you think? (I'd open a seperate PR for this.)

That sounds reasonable based on the analysis above.

[daveshih01]

Sounds like this PR does what it set out to do. Are there any other questions?

[miniksa]

Sounds like this PR does what it set out to do. Are there any other questions?

Sorry, I felt like the discussion with @lhecker wasn't 100% complete. But re-reviewing it, I think I just need to file a follow on task for the >1 thing and then get this merged.

[lhecker]

And I forgot about this PR. 😅
Can you file that task for me? As I said I‘ve been fairly packed with tasks for a while now.
I‘m sorry for that.

[miniksa]

And I forgot about this PR. 😅
Can you file that task for me? As I said I‘ve been fairly packed with tasks for a while now.
I‘m sorry for that.

No problem, thank you for your efforts anyway. I did file #3554 just now to follow on from this.

[zadjii-msft]

Reviewing the rest of the discussion, this seems fine by me. Thanks!

[ghost]

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

Handy links:

• Release Notes
• Store Download