Added a -UseHostReadKey switch to instruct LegacyReadLine to use the … #1748
Conversation
|
I removed ConsoleProxy because the static constructor was making it difficult to use PSES in a web server where everything is hosted in one process. I moved the operations of ConsoleProxy directly into EditorServicesConsolePSHostUserInterface.cs since that was the only place it appears to be used, and then added -UseHostReadKey which prevents the instantiation of _consoleOperations, leaving it null. I then added null checks to all usages of _consoleOperations with fallbacks to underlyingHostUI. Most importantly for ReadKey as this was famously hardcoded to Console.ReadKey under the assumption that this library would never be used outside of a local terminal. I feel like this fits rather nicely for people like me who have taken the time to properly implement a custom PSHostRawUserInterface |
|
Reviewing your PR has me noticing that |
I have so many questions.
|
Two reasons, broken up by platform: On Windows it's so we can make On non-Windows it's due to an internal stdin lock in the BCL. If one thread is blocking on
Assuming you mean Also tbh PSRL really only works in a console based host, so using the host API is typically not very important. If the host isn't console based, it's probably better for PSRL to throw. |
I'm curious what these issues were and if they still exist. I hate to harp on it, I'm just trying to find a way to make a REPL loop work reliably in the browser in front of a net 6.0 backend. |
I'd also like to know why this doesn't work. |
| @@ -17,7 +17,7 @@ public enum ConsoleReplKind | |||
| /// <summary>Use a REPL with the legacy readline implementation. This is generally used when PSReadLine is unavailable.</summary> | |||
| LegacyReadLine = 1, | |||
| /// <summary>Use a REPL with the PSReadLine module for console interaction.</summary> | |||
| PSReadLine = 2, | |||
| PSReadLine = 2 | |||
There was a problem hiding this comment.
| PSReadLine = 2 | |
| PSReadLine = 2, |
Trailing comma is on purpose here
| @@ -88,7 +91,7 @@ public sealed class EditorServicesConfig | |||
| /// (including none to disable the integrated console). | |||
| /// </summary> | |||
| public ConsoleReplKind ConsoleRepl { get; set; } = ConsoleReplKind.None; | |||
|
|
|||
There was a problem hiding this comment.
Extra whitespace
|
Couldn't remember so I went to go test some things, and the biggest blocker I found pretty quickly is that It's technically correct, but not the behavior we need. Technically we could just ignore those but it's a lot of extra logic and I'm not really sure what it buys us. I can keep lookin' if y'all want but it's a super different API that we're going to end up just trying to get working like Really, this is why we had a different host that was not terminal focused. It was torn out recently because it wasn't referenced by anything, but that should probably just be brought back with an extra switch. Another question, you really want a custom |
|
My end game is to have xterm.js in the browser with tab completion support, so I'm pretty sure I need ReadKey. I wouldn't be opposed to my ReadKey being called after your buffering logic, I think I stand to save myself a lot of work if you guys or PSReadline was handling the buffering/cancellation/etc. PSReadline is going to take more work because they have a singleton for clipboard purposes and that breaks my ability to have multiple consoles as when new consoles get spawned they get control of the previous instance and start sharing output. In the interim if I could just LegacyReadline to honor my hosts ReadKey, I think I'd be set. |
|
So sorry @dkattan, I rewrote the readkey implementation, you'll need to redo this BUT it should be way, way easier. Thank you for the inspiration. |
|
@dkattan did you want to redo this PR or should I close it? Really sorry about the re-work I caused you, BUT hey, it's so much simpler now, no?! |
I definitely do, just been a bit busy. I'll see if I can rebase it this afternoon. |
…ReadKey provided by the host instead of Console.ReadKey
…torServicesLoader
| } | ||
| else | ||
| { | ||
| KeyInfo keyInfo = _hostInfo.PSHost.UI.RawUI.ReadKey(); |
There was a problem hiding this comment.
Silly question: Could we just make the default RawUI.ReadKey in PSES be Console.ReadKey()? I ask because I see that's how you're supplying a custom ReadKey, which is exactly what we're also trying to do.
|
@dkattan I'm totally still willing to take the change in spirit, it just needs to be re-written. Let me know if you get to it! |
…ReadKey provided by the host instead of Console.ReadKey