Fixed and extended Debug Console support for most ANSI SGR codes #120891
Conversation
|
|
| @@ -126,44 +131,165 @@ export function handleANSIOutput(text: string, linkDetector: LinkDetector, theme | |||
| } | |||
|
|
|||
| /** | |||
| * Calculate and set basic ANSI formatting. Supports bold, italic, underline, | |||
| * normal foreground and background colors, and bright foreground and | |||
| * Swap foregfrouned and background colors. Used for color inversion. Caller should check | |||
There was a problem hiding this comment.
Typo, should be "foreground"
There was a problem hiding this comment.
fixed this typo. I should have caught something that blatant. lol
| * None of these fonts are required and all font-family stacks will resolve to some font. | ||
| * User can change the fonts used by these classes custom css using a extension such as Customize UI/Monkey Patch | ||
| */ | ||
| .monaco-workbench .repl .repl-tree .output.expression .code-font-1 { font-family: Verdana,Arial,sans-serif; } |
There was a problem hiding this comment.
I am not a fan of hard coding all the fonts inside this CSS.
I suggest to remove the font support for now, and if there is user interest in this we can add it in the future.
Also using an extension to customize the CSS is not recommended by VS Code.
There was a problem hiding this comment.
I will just make one argument for keeping fonts. This is for the DEBUG console. The only purpose of those fonts is to give the developer an option to print out messages in DIFFERENT fonts. For me I felt that having the option to print messages in different fonts was very useful. I do multi-threaded development and I have found it very useful to have loggers from different threads (as well as async futures) log messages in different fonts to make it easier to go through logs and see what was happening.
In this context I found that it did not matter what the fonts were, it only mattered that I had some different options to use. I understand that I can use colors or italics (and I do), but I find overuse of colors can be distracting, especially with several thread's log messages interleaved and making a 'rainbow' of colors in the logs.
...And that's my argument... lol.
I knew there would be push back on the fonts, and I fully expected there was a large chance that I would have to remove the font classes. No one (typically) likes the idea of a bunch of hardcoded font stacks, but I thought in the context of this being our developer debug console, not for end user use, that there might be a chance that other developers could appreciate the usefulness of being able to use several fonts in your log files.
If that does not persuade you in any way then I will remove the classes for the font stacks. Let me know.
Thanks for letting me argue my reasoning for having them there in the first place!
There was a problem hiding this comment.
I see the use case. However I am not sure how popular it would be and I am not a fan of the approach.
So I suggest the following, just create a new PR after we merge this one with the font changes. If I see big user interest in it in the next year I will merge that one as well.
| @@ -110,29 +110,39 @@ suite('Debug - ANSI Handling', () => { | |||
| * @param element The HTML span element to look at. | |||
| * @param colorType If `foreground`, will check the element's css `color`; | |||
| * if `background`, will check the element's css `backgroundColor`. | |||
| * if `underline`, will check the elements css `textDecorationColor`. | |||
There was a problem hiding this comment.
Well the tests of course took several times longer to code than the features.. but that's always the case isn't it ;) ?
|
Thanks a lot for this PR. Good work. |
…explaination of the font support and font stacks to allow developer flexiblity in logging to the debug console.
|
I removed the comment in the code about the user changing the fonts with an extension, and added some more text about the reasoning of having the choice of different ('alternative' as the ECMA-48 spec says) fonts to give the developer additional flexibility in writing messages to the debug console. |
|
I have removed the font support as requested :) Hey, it was worth a try.. ;) lol. |
|
Thanks a lot, this looks fantastic. Merging in 👏 |
This fixes and extends the debug console's support for most ANSI SGR codes to bring it to parity (or exceed) support to all common terminals/emulators. As specified by https://en.wikipedia.org/wiki/ANSI_escape_code#SGR
This includes both the ON and OFF codes for each attributes, so standard libraries that use ANSI, such as Chalk, render correctly within the debug console.
I have extended debugANSIHandling.test.ts to completely exercise all fixed and additions so that there is complete code coverage of all added code, fixes and features.
I can include screenshots (and comparisons to most other terminals/emulators) of complete exercising of all the SGR codes if needed/requested.
(tested/compared against Windows CMD/ansi.sys, Powershell 6, Powershell 7, mintty/cygwin, bash/ubuntu)
This PR fixes #93200 (Wrong style on debug console)