Enable PSReadline to work on NanoServer #3815
Conversation
…ont and unicode which causes PSReadline to not be usable on NanoServer docker images. Fix is to assume single byte characters if these APIs are not available.
| int charCount = 1; | ||
| try | ||
| { | ||
| charCount = NativeMethods.ToUnicode( |
There was a problem hiding this comment.
We should find a better way - this will cause an exception on every character typed, not good for performance and extremely annoying when debugging.
There is some one-time initialization code - so one less ugly option is to add a P/Invoke call there just to detect if the api is present or not, setting a flag that is checked here.
There was a problem hiding this comment.
Will make the change
There was a problem hiding this comment.
I looked into putting the check in Readline.Initialize() but since it was a different class I would have to expose two additional members. Seemed simpler to keep the state within the classes needing those apis.
| bool result = true; | ||
| try | ||
| { | ||
| result = NativeMethods.GetCurrentConsoleFontEx(consoleHandle.DangerousGetHandle(), false, ref fontInfo); |
There was a problem hiding this comment.
We should do the same here - detect once so we don't have frequent exceptions.
|
@lzybkr fixed |
| int charCount = NativeMethods.ToUnicode( | ||
| virtualKey, scanCode, state, chars, chars.Length, NativeMethods.MENU_IS_INACTIVE); | ||
| int charCount = 1; | ||
| if (_toUnicodeApiAvailable) |
There was a problem hiding this comment.
After a closer look, I don't think this is the right fix for this case - custom key bindings need a valid results, you'll just return \0.
I think we'll need the Unix variant of this code as a fallback, or potentially use it unconditionally, though that might break some more obscure key bindings.
There was a problem hiding this comment.
There's a comment in the code that GetCharFromConsoleKey() is needed to support chords like Ctrl+[. Seems like the best approach here is to defer to the Unix code if this API isn't available. I'll make that change.
|
Manually verified, you can use something like this: After the container starts in cmd, you can start powershell using |
e983193 to
3f005a5
Compare
…vailable (such as NanoServer)
3f005a5 to
f2f808f
Compare
|
@lzybkr good to go? |
| char c = ConsoleKeyChordConverter.GetCharFromConsoleKey(key.Key, | ||
| (mods & ConsoleModifiers.Shift) != 0 ? ConsoleModifiers.Shift : 0); | ||
| #if !UNIX | ||
| if (_toUnicodeApiAvailable) |
There was a problem hiding this comment.
If this api is not available, based on the comment, I would expect to see some garbage in the output of Get-PSReadlineKeyHandler -Bound, e.g. on this line:
Ctrl+] GotoBrace Go to matching brace
(or if you are using Emacs mode):
Ctrl+] CharacterSearch Read a character and move the cursor to the next occurence of that character
Ctrl+Alt+] CharacterSearchBackward Read a character and move the cursor to the previous occurence of that char...
There was a problem hiding this comment.
I can check next week when back in office where I have Docker and a Nano container, but not sure what you are proposing here when the api is not available.
There was a problem hiding this comment.
I'm proposing not degrading the default user experience if possible - which might require a few special cases. I would guess a more general fix isn't possible because this api is that general fix.
There was a problem hiding this comment.
Seems like this is a decision the Nano team should make. If they want the full experience, they need those apis on Nano. I'll certainly let them know of the limitation.
There was a problem hiding this comment.
On a Nano container that line says:
Ctrl+Oem6 GotoBrace Go to matching brace
There was a problem hiding this comment.
Indeed, not very helpful :)
There was a problem hiding this comment.
If you want, I can add specialized code so that if those apis aren't available, we don't have those bindings by default. Seems like over optimizing for what is today an edge case. Alternatively, I could have PSReadline fail to load if those apis aren't available even though most of the PSReadline capability seems to run fine on Nano. Seems like the best option is to document this and have Nano team make the call to add that api.
There was a problem hiding this comment.
A few special cases is not over optimizing and is a practical assuming those apis aren't really generally useful.
The default output of Get-PSReadlineKeyBindings should not contain Oem.
There was a problem hiding this comment.
I created #3890 to track this issue, so we can at least unblock Nano for now.
PSReadline uses two native Win32 apis to determine font and unicode information which is not available on NanoServer. Fix is to assume the input is one byte in character in size and not using a truetype font. This should work for most use cases.
Manually tested this change with our published dockerfile for NanoServer.
Fix #3463