Skip to content

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Nov 20, 2018

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

change ellipsis when truncating to single unicode character
reset console output if previous column contains ESC
update existing format-table tests
@SteveL-MSFT SteveL-MSFT changed the title Reset output attributes if previous column had ESC char when using Format-Table Reset output attributes if column had ESC char when using Format-Table Nov 21, 2018
fix other occurrences of "..." to use "…"
@iSazonov iSazonov added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Nov 21, 2018
address Ilya's feedback
@iSazonov
Copy link
Collaborator

@SteveL-MSFT Please look CodeFactor issues.

codefactor fixes
@SteveL-MSFT SteveL-MSFT changed the title Reset output attributes if column had ESC char when using Format-Table Reset output attributes if column had ESC char when using Format-Table; Replace "..." with unicode ellipsis Nov 21, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 24, 2018

@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.
I guess this is a wrong design.
If we fill our scripts with these escape sequences, what will we do with them if we will work with other consoles like Web? Now we are condemning ourselves to the development of additional intermediate non-standard adapters. But if we are forced to create adapters for different terminals, then it is better to do this through the standard abstract API, which will eliminate the intermediate layers, which are the source of numerous superimposed errors. And current Issue and PR clearly show this.
In fact, we already have this api. It allows us to colored output both to a Windows console and to an Unix console. We should only improve this api.
This also means that we must treat escape sequences literally. If we need something like that for formatting, then we should use PowerShell standard language features which will work everywhere (with any console). We already have interpolated strings and we could enhance them to support output formatting.
Such design would allow PowerShell Core to cling to any console with minimal code addition without the imposition of transforming layers.
I think your team should discuss this thoroughly.

@SteveL-MSFT
Copy link
Member Author

@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 ConvertTo-Html, but I would imagine ideally it would be some translation of VT100 to HTML.

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.

@iSazonov
Copy link
Collaborator

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 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 guess this is a wrong design.
And I can continue that PowerShell is high level language and have many magic abstractions, for output formatting too.
Initially, PowerShell was not based on VT100. If the output should be based on VT100, then we definitely need to accept this as an RFC. And this is definitely a fundamental decision and fundamental change.

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 Write-Host -BackgroundColor Blue but not using Write-Host 0x1A .... We should consider VT100 as one of the options for implementing high-level abstractions.
We should not lower to the lower level in the PowerShell language itself. The use of abstractions allows us to be comfortable, fast and reliable with any consoles. As PowerShell engine developer we are free to use vty or Windows console api (why we'd need reject the api if it allows us to make great-look a progress bar? and it is more multi-thread friendly). We can switch free to ncurses or Web (not only HTML, we can use any api over https!). At the same time, users should use simple and powerful abstractions to easily write scripts that work on any platform and with any terminal.
Another thing that worries me is that with the allowing of escapes, we bind to the vty but will not we format the table cells on the Web? The right design would be to delegate this to the conhost implementation. Escapes is and should be implementation details. So I am sure that these escapes are not what should be in PowerShell language.

@iSazonov
Copy link
Collaborator

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.
I suppose all types of rendering should be abstracted in engine and implemented in a conhost so that the engine works consistently with any console and we could switch on the fly.
I am increasingly convinced that we should invest in an engine abstraction of a markup language. We don't even have to invent anything. This might be an RFC for a subset of HTML (to support format-table, Markdown and so on).
In this case, the output will be as simple as

Write-Host -HTML "HTMLstring" -Markdown "markdownstring" -SimpleString "simplestring"
"string" | Format-Table [-HTML | -Markdown]

where default parameter is defined in current console type.

@DHowett-MSFT
Copy link

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

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:

  • allow Windows applications to host Windows consoles, but exchange VT (and therefore be "terminal emulators")
  • allow applications using the ~73 Windows Console APIs to be run and have the effects of those APIs translated into VT.

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.

@iSazonov
Copy link
Collaborator

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

Unfortunately, that may not be how PowerShell was designed.

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 Write-Host -BackgroundColor Blue and perhaps interpalated strings as I metioned above).
Currently PowerShell conhost class has already needed abstractions to works (1) on windows (buffered console API), (2) on Unix (vty). So my thoughts is (1) use/enhance PowerShell conhost class abstractions, (2) move rendering implementations to console adapters.

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

@SteveL-MSFT
Copy link
Member Author

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

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 30, 2018

Perhaps something like "`e{green}" but this needs a RFC and some thought.

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

I don't think providing a higher level abstraction precludes the changes in this PR.

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.
I think that this change just shows that we doom ourselves to an infinite number of bugs if we don’t say now that keeping escapes out of our redering is "by-design".
If we started to implement Markdown we could do the same for other markup languages (what about HTML or YAML?). This means that we need a common power abstract API to support this uniformly. (That we already partially have)

Perhaps something like "`e{green}"

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

@SteveL-MSFT
Copy link
Member Author

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

@iSazonov
Copy link
Collaborator

I think you should open a new issue to discuss that

Will do.

It seem you are proposing we should prevent people from embedding VT100 escape sequences which seems too strict to me.

I propose to keep our engine and formatting implementation-detail-independent and move rendering implementation to conhost.

@SteveL-MSFT
Copy link
Member Author

@iSazonov are you ok with taking this change? We can continue the higher level discussion separate from this.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

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

@iSazonov iSazonov removed their assignment Dec 6, 2018
</resheader>
<data name="Ellipsize" xml:space="preserve">
<value>{0}...{1}</value>
<value>{0}\u2026{1}</value>

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 &#x2026; rather than \u2026; see #9586.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strings with Virtual Terminal (VT) / ANSI escape sequences that are truncated for display make effects linger

6 participants