Skip to content

Direct Clear-Host output to terminal #10681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ilya (iSazonov) merged 1 commit into from
Oct 3, 2019
Merged

Direct Clear-Host output to terminal #10681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/System.Management.Automation/engine/InitialSessionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4078,7 +4078,9 @@ internal static string GetClearHostFunctionText()
{
// Porting note: non-Windows platforms use `clear`
return @"
& (Get-Command -CommandType Application clear | Select-Object -First 1).Definition
[Console]::Write((

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use Write-Host here instead? There may be cases where there isn't a console?

@mklement0 Michael Klement (mklement0) Oct 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steve Lee (@SteveL-MSFT): In principle, $host.UI.Write() (or Write-Host) is better than [Console]::Write(), but note that we're calling a terminal-specific utility anyway - see also: #10181 (comment)

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ilya (@iSazonov) can you validate if this works remotely?

@mklement0 Michael Klement (mklement0) Oct 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steve Lee (@SteveL-MSFT): No, it doesn't (see linked comment), but it also didn't work before. However, if we're willing to assume an xterm terminal (which sounds like a safe bet), we can fix it as follows before calling clear: if (-not $env:TERM) { $env:TERM='xterm' } - probably better to reset the variable afterwards.

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can defer the remote scenario since it doesn't work today (and I think there's a issue on that already). We can take this with the current change to address the specific problem of the VT codes emitted to the pipeline.

@mklement0 Michael Klement (mklement0) Oct 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steve Lee (@SteveL-MSFT): Couldn't find an issue relating to remoting specifically.

As it turns out, my suggested $env:TERM='xterm' workaround for remoting only works with $host.UI.Write() (Write-Host -NoNewline), not with [console]::Write(), because the escape sequences must be sent to the caller's terminal.

In light of that - if you don't want to incorporate the full workaround yet - it's better to switch to $host.UI.Write().

For the record, here's the complete workaround (from my personal notes):

To fix the remoting problem, $env:TERM can be set to xterm temporarily, under the assumption that pretty much all terminals are Xterm-compatible these days - and that it is the caller that uses such a terminal, because the escape sequences must be printed to the caller's terminal:

  function Clear-Host {
    if ($noTerm = -not $env:TERM) { $env:TERM = 'xterm' }
    $host.UI.Write((
      & (Get-Command -CommandType Application clear | Select-Object -First 1).Definition
    ))
    if ($noTerm) { $env:TERM = $null }
  }

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the suggestion to switch to $host.UI.Write(). Not so sure about the assumption that terminals are xterm compatible. The issue I was thinking about is #8606 where remoting was discussed but wasn't the focus.

@iSazonov Ilya (iSazonov) Oct 3, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look $host.UI.Write() today. I hope we could implement this step-by-step (first for local host. then remote one that would be more tricky)

@mklement0 Michael Klement (mklement0) Oct 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link, Steve Lee (@SteveL-MSFT), but resolving #8608 will not address the remoting problem; while there is a single comment - #8606 (comment) - that touches on the need to support non-console hosts, a separate issue should really be opened for that.

As for assuming xterm: Anecdotally, I'd say it's a pretty safe bet, but the better solution would be to extend the $PSSenderInfo object to include the caller's $env:TERM value, if any, and have Clear-Host consult that.

& (Get-Command -CommandType Application clear | Select-Object -First 1).Definition
))
# .Link
# https://go.microsoft.com/fwlink/?LinkID=225747
# .ExternalHelp System.Management.Automation.dll-help.xml
Expand Down
4 changes: 4 additions & 0 deletions test/powershell/Host/ConsoleHost.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ Describe "ConsoleHost unit tests" -tags "Feature" {
$Error.Clear()
}

It "Clear-Host does not injects data into PowerShell output stream" {
& { Clear-Host; 'hi' } | Should -BeExactly 'hi'
}

Context "ShellInterop" {
It "Verify Parsing Error Output Format Single Shell should throw exception" {
{ & $powershell -outp blah -comm { $input } } | Should -Throw -ErrorId "IncorrectValueForFormatParameter"
Expand Down