PowerShellEditorServices icon indicating copy to clipboard operation
PowerShellEditorServices copied to clipboard

Added a -UseHostReadKey switch to instruct LegacyReadLine to use the …

Open dkattan opened this issue 2 years ago • 11 comments

…ReadKey provided by the host instead of Console.ReadKey

dkattan avatar Mar 24 '22 20:03 dkattan

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

dkattan avatar Mar 24 '22 20:03 dkattan

Reviewing your PR has me noticing that ReadKeyAsync is implemented...yet never used. @dkattan a few issues I'm investigating seem to be caused by our override of ReadKey (for PSReadLine): https://github.com/PowerShell/vscode-powershell/issues/3881, https://github.com/PowerShell/vscode-powershell/issues/2741, and https://github.com/PowerShell/vscode-powershell/issues/3876. So...I'm very intrigued here because I'm already working in this area and trying to come up with a better solution than "gobble everything and pretend we didn't."

andyleejordan avatar Mar 25 '22 01:03 andyleejordan

Reviewing your PR has me noticing that ReadKeyAsync is implemented...yet never used. @dkattan a few issues I'm investigating seem to be caused by our override of ReadKey (for PSReadLine): PowerShell/vscode-powershell#3881, PowerShell/vscode-powershell#2741, and PowerShell/vscode-powershell#3876. So...I'm very intrigued here because I'm already working in this area and trying to come up with a better solution than "gobble everything and pretend we didn't."

I have so many questions.

  1. Why do we need that override in the first place?
  2. Why does PSReadLine use System.Console.ReadKey() instead of PSHostRawUserInterface.ReadKey?

dkattan avatar Mar 25 '22 13:03 dkattan

  1. Why do we need that override in the first place?

Two reasons, broken up by platform:

On Windows it's so we can make ReadKey cancellable, sorta. It's a hack since there isn't any real support for cancellation but we put it in another thread and store the result in a field if it's cancelled. That way it doesn't block, and when we cancel it we can reuse the result next invocation.

On non-Windows it's due to an internal stdin lock in the BCL. If one thread is blocking on Console.ReadKey and another thread calls Console.CursorTop (or similar) both threads will block until input is received. PSRL internally checks cursor position after processing events to see if it needs to redraw due to host output from an event subscriber. In practice this means that Console.ReadKey becomes synchronous and we lose the ability to cancel it.

  1. Why does PSReadLine use System.Console.RawUI.ReadKey() instead of PSHostRawUserInterface.ReadKey?

Assuming you mean System.Console.ReadKey() for the former. The latter just works quite differently depending on the version, and platform where ReadKey is pretty predictable. Every time I've tried to use RUI.ReadKey I've found some issue with it that makes it less desirable.

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.

SeeminglyScience avatar Mar 25 '22 13:03 SeeminglyScience

Every time I've tried to use RUI.ReadKey I've found some issue with it that makes it less desirable.

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.

dkattan avatar Mar 25 '22 19:03 dkattan

Every time I've tried to use RUI.ReadKey I've found some issue with it that makes it less desirable.

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.

andyleejordan avatar Apr 04 '22 22:04 andyleejordan

Couldn't remember so I went to go test some things, and the biggest blocker I found pretty quickly is that shift on it's own returns as a key press (on Windows).

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 Console.ReadKey already does.

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 ReadKey? Or do you want a custom ReadLine?

SeeminglyScience avatar Apr 04 '22 23:04 SeeminglyScience

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.

dkattan avatar Apr 05 '22 00:04 dkattan

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.

andyleejordan avatar Apr 13 '22 18:04 andyleejordan

@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?!

andyleejordan avatar May 18 '22 18:05 andyleejordan

@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.

dkattan avatar May 18 '22 18:05 dkattan

@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!

andyleejordan avatar Sep 08 '22 17:09 andyleejordan