8000 Enable simpler ANSI formatting via `$PSStyle` and support suppressing ANSI output by SteveL-MSFT · Pull Request #13758 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Enable simpler ANSI formatting via $PSStyle and support suppressing ANSI output #13758

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

Jump to bottom
Merged
merged 39 commits into from
Dec 11, 2020
Merged

Enable simpler ANSI formatting via $PSStyle and support suppressing ANSI output #13758

merged 39 commits into from
Dec 11, 2020

Conversation

SteveL-MSFT
Copy link
Member
@SteveL-MSFT SteveL-MSFT commented Oct 7, 2020 •
edited
Loading

PR Summary

See #13071 for details.

  • Expose $PSStyle automatic variable to get or set ANSI escape sequences associated with text decoration
  • Formatters for $PSStyle to help users see what the output would look like using one of the colors/attributes. Note that current design choice is to automatically show nested types in $PSStyle as I expect users dumping that variable are interested in all the choices.
  • Update formatting engine to support stripping ANSI escape sequences if not supported or explicitly disabled
  • To preserve existing behavior, redirected output (from pwsh) is always plaintext

After this is merged:

  • Remove __SuppressAnsiExscapeSequences code as it's no longer needed
  • Update existing ErrorRecord and Get-Error formatting to use $PSStyle

PR Context

Fix #13071

PR Checklist

@ghost ghost assigned adityapatwardhan Oct 7, 2020
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 11, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 24, 2020
@SteveL-MSFT SteveL-MSFT force-pushed the rendering branch 6 times, most recently from 57589ea to da999d9 Compare October 25, 2020 23:16
@SteveL-MSFT
Copy link
Member Author

Still need to add tests

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Oct 29, 2020
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 8, 2020
@ghost ghost added the Stale label Nov 23, 2020
@ghost
Copy link
ghost commented Nov 23, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

powercode
powercode reviewed Nov 23, 2020
View reviewed changes
@ghost ghost removed Stale Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Nov 23, 2020
@SteveL-MSFT
Copy link
Member Author

Still need to have WriteVerbose, etc... use the $PSStyle settings before taking this out of draft

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Nov 30, 2020
powercode
powercode approved these changes Dec 1, 2020
View reviewed changes
@powercode
Copy link
Collaborator

I really like this PR! It will be super useful to me!

@SteveL-MSFT SteveL-MSFT marked this pull request as ready for review December 1, 2020 14:14
daxian-dbw
daxian-dbw reviewed Dec 1, 2020
View reviewed changes
Copy link
Member
@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

Publish my comments so far, will continue to review.

daxian-dbw
daxian-dbw reviewed Dec 2, 2020
View reviewed changes
@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Dec 11, 2020
@iSazonov
Copy link
Collaborator

So

PS>"$($PSStyle.Formatting.Error)custom error message"`

will output red colored string.

In time of evaluating the string we already know values of all constants - SupportsVirtualTerminal, isHost, Console.IsOutputRedirected. If so why do we need the filtering at all if we can return $null at the evaluating time if we do not need the ANSI value?

@SteveL-MSFT
Copy link
Member Author

@iSazonov Error is currently a special case due to the custom formatting that happens. After this gets merged, I'll update the error formatting to use $PSStyle.Formatting.Error.

daxian-dbw
daxian-dbw reviewed Dec 11, 2020
View reviewed changes
src/System.Management.Automation/engine/Utils.cs
if (!InternalTestHooks.BypassOutputRedirectionCheck &&
((PSStyle.Instance.OutputRendering == OutputRendering.PlainText) ||
(formatStyle == FormatStyle.Error && Console.IsErrorRedirected) ||
(formatStyle != FormatStyle.Error && Console.IsOutputRedirected)))
Copy link
Member
@daxian-dbw daxian-dbw Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@SteveL-MSFT, this method currently directly check on Console.IsErrorRedirected and Console.IsOutputRedirected. I think we shouldn't directly check on those in PowerShell engine, but should instead have separate parameters for the caller to provide that information. The engine should not assume there is a console available, and it's up to the host implementation to pass in whether there is error/output redirection when calling this method.

I've put this comment in the tracking issue #14387, so you can fix it in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

#14397

@iSazonov
Copy link
Collaborator
iSazonov commented Dec 11, 2020 •
edited
Loading

@SteveL-MSFT I say that $PSStyle could not be a const and be evaluated - as result we have no need to filter out escapes because $PSStyle would return $null if user turns off coloring or terminal does not support escapes and so on.

@daxian-dbw
Copy link
Member

In time of evaluating the string we already know values of all constants - SupportsVirtualTerminal, isHost, Console.IsOutputRedirected.

@iSazonov The PowerShell engine doesn't know if the output/error is being reidrected. It cannot depedn on Console.IsOutputRedirected because it may be hosted in a Windows GUI applicatioin which doesn't have a console at all.
The information about whether the output/error is redirected or whether ANSI sequences should be stripped off because the process's output/error is redirected should be decided only by the host implementation.

Take the host implementation of the PowerShell sub kernel (jupyter kernel) in .NET Interactive as an instance, it's a console application, and the process's output/error streams are always redirected to send to the frontend -- jupyter notebook, which is capable of handling ANSI sequences. So, in this case, even thought Console.IsOutputRedirected and Console.IsErrorRedirected return true, the ANSI sequences shouldn't be stripped off.

To summarize, the host implementation has the final call on whether ANSI sequences should be stripped off or not, and that's why I opened #14387.

@iSazonov
Copy link
Collaborator
iSazonov commented Dec 11, 2020 •
edited
Loading

To summarize, the host implementation has the final call on whether ANSI sequences should be stripped off or not

Why can't PowerShell engine get this information from the host implementation?

because it may be hosted in a Windows GUI applicatioin which doesn't have a console at all.

So PowerShell engine can get get this information from the host implementation?

@daxian-dbw
Copy link
Member

I don't know how that would work and how well that would work if it works.

  • From a design point of view, I like to decouple engine and host instead of more coupling.
  • From an implementation point of view, a host may dynamically change how it want the ANSI sequences to be handled.

@daxian-dbw
Copy link
Member

@iSazonov I will merge this PR to make the cut of 7.2.0-preview.2 release build. Feel free to comment in #14387 about how you think this feature can be improved.

@daxian-dbw daxian-dbw merged commit a99ea2a into PowerShell:master Dec 11, 2020
@daxian-dbw daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Dec 11, 2020
@daxian-dbw daxian-dbw added this to the 7.2.0-preview.2 milestone Dec 11, 2020
@SteveL-MSFT SteveL-MSFT mentioned this pull request Dec 11, 2020
Merged
14 tasks
@SteveL-MSFT SteveL-MSFT deleted the rendering branch December 11, 2020 19:20
This was referenced Dec 14, 2020
Closed
Closed
@rjmholt rjmholt added CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log and removed CL-Engine Indicates that a PR should be marked as an engine change in the Change Log labels Dec 15, 2020
@ghost
Copy link
ghost commented Dec 15, 2020

🎉v7.2.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Dec 15, 2020
Closed
@rjmholt
Copy link
Collaborator
rjmholt commented Dec 16, 2020

These changes don't seem to work in the Integrated Console: #14435

This was referenced Jan 27, 2021
Closed
Closed
@iSazonov iSazonov mentioned this pull request Oct 28, 2021
Merged
22 tasks
@iSazonov iSazonov mentioned this pull request Jan 31, 2025
Closed
5 tasks
@kborowinski kborowinski mentioned this pull request May 4, 2025
Merged
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@powercode powercode powercode approved these changes

@iSazonov iSazonov iSazonov left review comments

@anmenaga anmenaga Awaiting requested review from anmenaga

Assignees

@daxian-dbw daxian-dbw

Labels
CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log
Projects
None yet
Milestone
7.2.0-preview.2
Development

Successfully merging this pull request may close these issues.

ANSI escape handling and PowerShell
6 participants
@SteveL-MSFT @powercode @daxian-dbw @iSazonov @rjmholt @adityapatwardhan
0