Use public InternalHost from origin runspace #874

SeeminglyScience · 2019-03-05T04:26:22Z

ConsoleHost from powershell.exe/pwsh still exists within the runspace created at process start. This change grabs the public reference to that host while initializing EditorServicesHost. We can then leverage that host so we can have a much closer to "default" experience.

Resolves #865, #384, PowerShell/vscode-powershell#140, PowerShell/vscode-powershell#1403

This adds support for the following host members:

$Host.UI

WriteProgress (including Write-Progress)

$Host.UI.RawUI

CursorSize (still doesn't work in xterm.js though)
WindowTitle
MaxPhysicalWindowSize
MaxWindowSize
ReadKey
GetBufferContents
ScrollBufferContents
SetBufferContents

TODO

Test RawUI members
Maybe write sync verison of ReadKey
~~Maybe avoid TerminalPSHost* breaking changes (constructors)~~ I can't really think of any scenarios where folks would be creating their own TerminalPSHost* instances
~~Consider thread safety for progress record and ReadKey buffers~~ Not sure it's necessary, open to suggestions though.

`ConsoleHost` from `powershell.exe`/`pwsh` still exists within the runspace created at process start. This change grabs the public reference to that host while initializing EditorServicesHost. We can then leverage that host so we can have a much closer to "default" experience. This change adds support for the following host members ## $Host.UI - WriteProgress (including `Write-Progress`) ## $Host.UI.RawUI - CursorSize (still doesn't work in xterm.js though) - WindowTitle - MaxPhysicalWindowSize - MaxWindowSize - ReadKey - GetBufferContents - ScrollBufferContents - SetBufferContents ## TODO [ ] Test RawUI members [ ] Maybe write sync verison of ReadKey [ ] Maybe avoid TerminalPSHost* breaking changes (constructors)

src/PowerShellEditorServices/PowerShellEditorServices.csproj

- Add the XML documentation comments to ConsoleProxy because it's more likely to be used than the interface itself. - Add a synchronous implementation of ReadKey to ConsoleProxy and use it in RawUI.ReadKey - Fix Ctrl + C not returning as input in VSCode's terminal - Use the exception message from ConsoleHost when ReadKeyOptions do not include either IncludeKeyUp nor IncludeKeyDown

TylerLeonhardt

Looking good!

src/PowerShellEditorServices.Host/EditorServicesHost.cs

src/PowerShellEditorServices/Console/WindowsConsoleOperations.cs

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs

src/PowerShellEditorServices/Session/Host/EditorServicesPSHostUserInterface.cs

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs

TylerLeonhardt · 2019-03-10T22:11:08Z

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs

@@ -184,11 +145,87 @@ public override Size MaxWindowSize
        /// <returns>A KeyInfo struct with details about the current keypress.</returns>
        public override KeyInfo ReadKey(ReadKeyOptions options)


@SteveL-MSFT @daxian-dbw would you mind taking a look at this ReadKey implementation if you have a sec? This will be the implementation for $Host.UI.RawUI.ReadKey() in the Integrated Console.

I think it looks ok.

TylerLeonhardt

LGTM I'd like to do 2 things before this goes in:

test on non-Windows
ask the community for complex Write-Progress's

potatoqualitee · 2019-03-11T01:44:35Z

Excellent!! Thank you so very very much. @okallstad has some advanced Write-Progress bars in his module, psInlineProgress

TylerLeonhardt · 2019-03-11T01:54:03Z

@potatoqualitee I think they might go by a different name on GitHub? Looks like no one was tagged.

potatoqualitee · 2019-03-11T02:11:26Z

unfortunately, i dont know his github address :/ hopefully he'll notice the twitter ping

wsmelton · 2019-03-11T02:16:45Z

@gravejester has gravejester/psInlineProgress

potatoqualitee · 2019-03-11T02:20:38Z

yay that is him 🎉

SeeminglyScience · 2019-03-11T02:51:12Z

@potatoqualitee Doesn't look like that uses Write-Progress. Actually that one probably already works without this PR.

potatoqualitee · 2019-03-11T02:56:23Z

daw sorry, i didnt check the code, i was just a lil familiar with his project. my own write-progresses are pretty straightforward

TylerLeonhardt · 2019-03-11T05:18:53Z

https://twitter.com/daoist/status/1104918585401622528 from Twitter

daxian-dbw

No much experience in this area, so my questions are mainly for my own education ;)

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs

daxian-dbw · 2019-03-11T17:42:33Z

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs

+                    info.Character,
+                    info.ControlKeyState,
+                    keyDown: false);
+            }


KeyUp is requested, but why are you returning a KeyDown, especially that this KeyDown has already been returned previously?

re: emulating ConsoleHost behavior

daxian-dbw · 2019-03-11T17:45:14Z

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs

+                var result = new KeyInfo((int)key.Key, key.KeyChar, states, isDown);
+                if (isDown)
+                {
+                    this.lastKeyDown = result;


The returned value from this local function is returned immediately at all call sites of this local function, which means the cached KeyDown returned below in if (includeUp && this.lastKeyDown != null) has already been returned previously.

@daxian-dbw

The returned value from this local function is returned immediately at all call sites of this local function, which means the cached KeyDown returned below in if (includeUp && this.lastKeyDown != null) has already been returned previously.

Yeah, that's an attempt match the behavior of ConsoleHost without dipping into native API's.

Here's an example to demonstrate what I'm trying emulate.

while ($true) { $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyDown') $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyUp') }

With that running outside of PSES, every one key press will return two KeyInfo objects. One for down, and one for up. Example output:

VirtualKeyCode Character ControlKeyState KeyDown -------------- --------- --------------- ------- 70 f NumLockOn True 70 f NumLockOn False

Now obviously ConsoleHost does this a lot better using native API's directly, but this route saves a lot of complexity. The only difference in behavior is when holding down a key with both IncludeKeyUp and IncludeKeyDown set. In that scenario ConsoleHost will continuously return key down KeyInfo objects until they key is released where we will continuously return both key down and key up objects. That difference is negated a little bit though. Even ConsoleHost act the same under winpty as it doesn't translate input one to one.

Yeah on non-consolehost I think it typically only supports KeyDown events and not KeyUp at all.

If I tried that while loop on macOS, I only get KeyDown events.

Just to clarify because the terms are confusing, when I sayConsoleHost I'm referring to Microsoft.PowerShell.ConsoleHost (the PSHost started by powershell.exe/pwsh) not conhost.exe

ah sorry. Yeah I meant non-conhost sorry.

Gotcha! Thanks for the clarification!

TylerLeonhardt · 2019-03-12T00:39:27Z

I think I'm good with this. Tomorrow I'll test on non-Windows while we wait for more feedback.

TylerLeonhardt · 2019-03-12T00:42:50Z

https://twitter.com/mrhvid/status/1105230923887464450 more from Twitter

daxian-dbw · 2019-03-12T01:36:00Z

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs

+                    // Caller doesn't want CtrlC so throw a PipelineStoppedException to emulate
+                    // a real stop.  This will not show an exception to a script based caller and it
+                    // will avoid having to return something like default(KeyInfo).
+                    throw new PipelineStoppedException();


Caller doesn't want CtrlC so throw a PipelineStoppedException to emulate a real stop.

@SeeminglyScience I'm a little confused about the comment on this throw statement. Can you please explain a bit?

Sure, so at that point we've received the input Ctrl + C. If the caller did not include AllowCtrlC in the options, then we should treat it like a break event instead and terminate the pipeline.

Emulate is probably not really the right word there, I chose it because we aren't going through PowerShell.Stop or Pipeline.Stop like a normal break event would.

Another route would be to not set Console.TreatControlCAsInput, and allow our break handler to call PowerShell.Stop like normal. This actually would be more similar to what ConsoleHost does as it will actually return default(KeyInfo), but triggers a pipeline stop.

The issue I have with this is that if RawUI.ReadKey is in an assignment statement, the pipeline stop won't actually happen until the next sequence point. PowerShell doesn't check if the current command is stopping between the RHS being evaluated and it's result being set to the LHS. So the LHS will actually be set to default(KeyInfo) instead of remaining unchanged. That doesn't happen if we throw instead.

Another route would be to not set Console.TreatControlCAsInput

Thanks! My confusion was mainly around this case. It makes sense now.

So the LHS will actually be set to default(KeyInfo) instead of remaining unchanged.

Why does this matter if the pipeline stop will kick in at the next sequence point? Even if it's set to default(KeyInfo), the script execution gets killed and that assignment shouldn't have any real effect anymore, right?

the script execution gets killed and that assignment shouldn't have any real effect anymore, right?

There's a few scenarios where it can linger:

LHS is a global scoped variable (or script scoped variable in a module)

LHS is a property or index expression

Current command is a prompt evaluation (or is otherwise running within the top level scope of it's SessionState)

If the assignment is in a try with a finally block (though it will only linger within the finally)

Can you please provide a repro of the problems? For example, LHS is a global scoped variable and it gets assigned with default(KeyInfo)

I tried with this in pwsh console

function Test{ $global:abc = $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyDown') } Test <ctrl+c>

It seems ReadKey will get either the LeftCtrlPressed or c, but not default(KeyInfo).

Here's the easiest way:

$key = $null # Drain "Enter" key up from accepting prompt. Maybe comment # this line for non-Windows. $null = $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyUp') $key = $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyUp')

Then press Ctrl + C and $key will hold:

VirtualKeyCode Character ControlKeyState KeyDown -------------- --------- --------------- ------- 0 0 False

OK, I can repro it now after removing PSReadline.
Interesting that if I change IncludeKeyUp to IncludeKeyDown, it doesn't repro anymore

$null = $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyDown') $key = $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyDown') VirtualKeyCode Character ControlKeyState KeyDown -------------- --------- --------------- ------- 17 LeftCtrlPressed, NumLockOn True

BTW, my comment is not blocking on this PR. I'm just curious :)

OK, I can repro it now after removing PSReadline.

Huh that's surprising... mine was with PSRL as well. Though I'm on 2.0 beta 3 (or more likely one of the appveyor builds...)

Interesting that if I change IncludeKeyUp to IncludeKeyDown, it doesn't repro anymore

Wellll sorta. IncludeKeyDown makes it a lot harder to reproduce because it can trigger on a single press of Ctrl. So by the time you type C, you're actually already back to a prompt. But if you do something like this, it'll still show up

$key = $null while ($true) { $key = $Host.UI.RawUI.ReadKey('NoEcho, IncludeKeyDown') }

That will continously ReadKey until it registers Ctrl + C and a pipeline stop is triggered.

BTW, my comment is not blocking on this PR. I'm just curious :)

No worries, I could talk about it forever 🙂

Huh that's surprising... mine was with PSRL as well. Though I'm on 2.0 beta 3 (or more likely one of the appveyor builds...)

Ha, I'm using 6.2.0-rc.1.

IncludeKeyDown makes it a lot harder to reproduce because it can trigger on a single press of Ctrl. So by the time you type C, you're actually already back to a prompt.

That makes perfect sense!

TylerLeonhardt · 2019-03-13T01:25:45Z

src/PowerShellEditorServices/Session/Host/EditorServicesPSHostUserInterface.cs

@@ -32,6 +32,7 @@ public abstract class EditorServicesPSHostUserInterface :
    {
        #region Private Fields

+        private readonly HashSet<ProgressKey> currentProgressMessages = new HashSet<ProgressKey>();


thread safety pls 😄 ConcurrentBag

Changed to using ConcurrentDictionary<,>, but haven't tested it yet so hold off on merge pls

TylerLeonhardt

LGTM just one nit

TylerLeonhardt · 2019-03-13T04:39:06Z

src/PowerShellEditorServices/Session/Host/EditorServicesPSHostUserInterface.cs

+            }
+            else
+            {
+                this.currentProgressMessages.TryAdd(new ProgressKey(sourceId, record), null);


nit: add a comment as to why you're adding null.

potatoqualitee · 2019-03-19T14:06:18Z

🎉

SeeminglyScience requested review from TylerLeonhardt, rkeithhill and rjmholt March 5, 2019 04:26

TylerLeonhardt reviewed Mar 5, 2019

View reviewed changes

src/PowerShellEditorServices/PowerShellEditorServices.csproj Show resolved Hide resolved

SeeminglyScience added 2 commits March 7, 2019 17:47

Pass $Host in the start up script

ee1636e

SeeminglyScience changed the title ~~WIP: Use public InternalHost from origin runspace~~ Use public InternalHost from origin runspace Mar 7, 2019

TylerLeonhardt reviewed Mar 7, 2019

View reviewed changes

TylerLeonhardt reviewed Mar 8, 2019

View reviewed changes

src/PowerShellEditorServices/Session/Host/EditorServicesPSHostUserInterface.cs Outdated Show resolved Hide resolved

TylerLeonhardt added the Triage label Mar 8, 2019

Address feedback

696bd81

TylerLeonhardt reviewed Mar 10, 2019

View reviewed changes

src/PowerShellEditorServices/Session/Host/TerminalPSHostRawUserInterface.cs Outdated Show resolved Hide resolved

TylerLeonhardt reviewed Mar 10, 2019

View reviewed changes

TylerLeonhardt approved these changes Mar 10, 2019

View reviewed changes

daxian-dbw reviewed Mar 11, 2019

View reviewed changes

Address feedback and also add doc comments

acf0090

daxian-dbw reviewed Mar 12, 2019

View reviewed changes

TylerLeonhardt reviewed Mar 13, 2019

View reviewed changes

rjmholt approved these changes Mar 13, 2019

View reviewed changes

Make progress cache thread safe

871ff55

TylerLeonhardt approved these changes Mar 13, 2019

View reviewed changes

Added comment about null

b1f4781

TylerLeonhardt merged commit d156a73 into PowerShell:master Mar 22, 2019

TylerLeonhardt removed the Triage label Mar 26, 2019

SeeminglyScience mentioned this pull request Nov 10, 2021

Add support for Write-Progress with integrated console #384

Closed

		@@ -184,11 +145,87 @@ public override Size MaxWindowSize
		/// <returns>A KeyInfo struct with details about the current keypress.</returns>
		public override KeyInfo ReadKey(ReadKeyOptions options)

Use public InternalHost from origin runspace #874

Use public InternalHost from origin runspace #874

Conversation

SeeminglyScience commented Mar 5, 2019 • edited Loading

$Host.UI

$Host.UI.RawUI

TODO

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

potatoqualitee commented Mar 11, 2019

TylerLeonhardt commented Mar 11, 2019

potatoqualitee commented Mar 11, 2019

wsmelton commented Mar 11, 2019

potatoqualitee commented Mar 11, 2019

SeeminglyScience commented Mar 11, 2019

potatoqualitee commented Mar 11, 2019

TylerLeonhardt commented Mar 11, 2019

daxian-dbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Mar 12, 2019

TylerLeonhardt commented Mar 12, 2019

Choose a reason for hiding this comment

SeeminglyScience Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

SeeminglyScience Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potatoqualitee commented Mar 19, 2019

SeeminglyScience commented Mar 5, 2019 •

edited

Loading

SeeminglyScience Mar 11, 2019 •

edited

Loading

SeeminglyScience Mar 12, 2019 •

edited

Loading

SeeminglyScience Mar 12, 2019 •

edited

Loading

SeeminglyScience Mar 12, 2019 •

edited

Loading

daxian-dbw Mar 12, 2019 •

edited

Loading

SeeminglyScience Mar 12, 2019 •

edited

Loading