-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Reset output attributes if column had ESC char when using Format-Table; Replace "..." with unicode ellipsis
#8326
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.
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
Conversation
src/System.Management.Automation/FormatAndOutput/common/TableWriter.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/FormatAndOutput/common/TableWriter.cs
Outdated
Show resolved
Hide resolved
4ad1d2b to
b0a3c14
Compare
1f2d115 to
f3b7b71
Compare
Format-TableFormat-Table
f3b7b71 to
ac2034e
Compare
...stem.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/FormatAndOutput/common/TableWriter.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/FormatAndOutput/common/Utilities/MshObjectUtil.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/commands/PushRunspaceCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Format-List.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Table.Tests.ps1
Outdated
Show resolved
Hide resolved
|
@SteveL-MSFT Please look CodeFactor issues. |
Format-TableFormat-Table; Replace "..." with unicode ellipsis
|
@SteveL-MSFT After thinking about this change, I think that the original issue leads us away. It comes from that we must take care of escape sequences. But why should we do this if we have not done it before? I think this is a misconception that comes from the fact that Microsoft now invests a lot in ConTTY. I believe that this investments is aimed, among other things, at ensuring compatibility with Unix vty and existing software. And this does not mean that modern software should work according to protocols of 40 years old. |
|
@iSazonov I believe VT100 is already the de facto and de jure standard (at least in terms of the console). Cloud Shell and the integrated terminal in VSCode are javascript based and support parsing VT100 escape codes already. I would agree that we haven't thought through how this will work with something like Even if we support a PowerShell dialect for the equivalent of VT100 escape sequences, we would still need to handle VT100 escape sequences that could be emitted by any tool that runs in the console. |
I wrote about this above: I believe that this investments is aimed, among other things, at ensuring compatibility with Unix vty and existing software. And this does not mean that modern software should work according to protocols of 40 years old. I consider VT100 as a low-level assembler. It is not user-friendly. Nobody uses Intel assembler - developers prefer absractions of high level languages. Why? Because they need to accomplish their goal quickly and simply. I’m sure that PowerShell should also provide sufficient abstractions so that users don’t think about VT100 escape sequences. PowerShell style is using |
|
Continuing to think this over, I recall our implementation of the Markdown. It is tied to vty. Again this is too much of a limitation. Write-Host -HTML "HTMLstring" -Markdown "markdownstring" -SimpleString "simplestring"
"string" | Format-Table [-HTML | -Markdown]where default parameter is defined in current console type. |
Unfortunately, you're only half right on this point. We invested in ConPTY as a way of ensuring compatibility with existing Windows software. We realized early on in Windows 10 that everybody standardized on VT, and doubled down on supporting it. ConPTY is an attempt to:
For the issue at hand, it seems like table formatting should be entirely Host-driven. In the case of the console, only the console host knows how many cells each character takes up, which escape sequences should count against the table cell width (for colors, none; for cursor movement, who knows?), and how to properly terminate a dangling sequence. Unfortunately, that may not be how PowerShell was designed. |
|
@DHowett-MSFT Thanks for feedback! All you are talking about is a low level of work with a console. This is a standard because we can hardly think of something simpler for this low level. The issue is about high level - why we are starting to expose the low level to PowerShell language level and user level: (1) why PowerShell users should thinking about escapes and use its? why the users should remember the escapes? (2) Do we know high level languages which explicitly exposes the escapes to users rejecting absractions?
PowerShell is designed to expose users high level and magic things. Low level should stay on low level. In the case an implementing of rendering is the low level, high level is PowerShell language abstractions (like I assume that this problem comes from the fact that PowerShell mainly works with a simple console. In fact, it should work with any console and any remote protocol (as I mentioned above). |
|
@iSazonov I think it's fine to think of VT100 as an implementation detail and not something we want to expose to users although they can freely use it. We should have something higher level to make it easier for non-developers to leverage a subset of VT100 for formatting. Perhaps something like "`e{green}" but this needs a RFC and some thought. I don't think providing a higher level abstraction precludes the changes in this PR. |
It's low level again. Currently we have Markdown implementation based solely on escapes. Why? If we would have a universal api we could not use the escapes and get rendering on any "console".
We can look at it from the other side. Nothing prevents users from using escapes now. Users can even make their own cmdlet to display tables. But why should we complicate our rendering? I am increasingly confident that this is not justified.
Yeah, what I'm trying to say here is that the following is better: echo [html]"<a><bold>Hello world!<\a>"that should works with any target (VT, ghost, web...) |
|
@iSazonov I think you should open a new issue to discuss that. It seem you are proposing we should prevent people from embedding VT100 escape sequences which seems too strict to me. |
Will do.
I propose to keep our engine and formatting implementation-detail-independent and move rendering implementation to conhost. |
|
@iSazonov are you ok with taking this change? We can continue the higher level discussion separate from this. |
|
@SteveL-MSFT Sorry that I still have not opened a new discussion. Will do. As for this PR, as I said above, I do not agree that this should be because we will have to parse and process each line before outputting it (not partially like in this PR) that unnecessarily complicates the output formatting system and complicates the addition of support for other consoles. I propose to postpone it as it is not a security change. I delegate this to your team. |
...stem.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs
Show resolved
Hide resolved
| </resheader> | ||
| <data name="Ellipsize" xml:space="preserve"> | ||
| <value>{0}...{1}</value> | ||
| <value>{0}\u2026{1}</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this should have been … rather than \u2026; see #9586.
change ellipsis when truncating to single unicode character
reset console output if previous column contains ESC
update existing format-table tests
PR Summary
If content included a VT100 ESC sequence (like changing color), this affected all output after that cell in the table. Fix is to detect that a cell contained ESC and reset the console after it. Also, change the 3 character ellipsis
...to use the single unicode character…so that more text is available.Fix #7767
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests