Rewrite of ANSI code handling method #49763
Conversation
|
@danielfrankcom great work and thanks a lot for this PR! Also might you be interested in writting some unit tests for this? For me it seems like this could be unit testable - The code looks great overall, but unit tests would increase my confidence by a lot. |
| .hc-black .monaco-workbench .repl .repl-tree .monaco-tree .monaco-tree-row > .content > .output.expression .code36, .hc-black .monaco-workbench .repl .repl-tree .monaco-tree .monaco-tree-row > .content > .output.expression .code96 { color: #218D8D; } | ||
| .hc-black .monaco-workbench .repl .repl-tree .monaco-tree .monaco-tree-row > .content > .output.expression .code37, .hc-black .monaco-workbench .repl .repl-tree .monaco-tree .monaco-tree-row > .content > .output.expression .code97 { color: #707070; } | ||
| /* Regular and bright color codes are currently treated the same. */ | ||
| .monaco-workbench .repl .repl-tree .monaco-tree .monaco-tree-row > .content > .output.expression .code-fg-30, .monaco-workbench .repl .repl-tree .monaco-tree .monaco-tree-row > .content > .output.expression .code-fg-90 { color: gray; } |
There was a problem hiding this comment.
Why did you decide to add the fg to the code class names? Is that the proper name or?
There was a problem hiding this comment.
I briefly considered changing this back to the way it was for this review, but I changed them to match the pattern I used for the background colors (bg). Maybe a more explicit 'foreground'?
There was a problem hiding this comment.
Yes 'foreground' would be better
Same for 'background'.
That way it is clearer when somebody reads it a couple of months later
| this.insert(this.linkDetector.handleLinks(buffer), currentToken || tokensContainer); | ||
| buffer = ''; | ||
| } | ||
| if (char.match(/^[ABCDHIJKfhmpsu]$/)) { |
There was a problem hiding this comment.
Please add a comment explaining this regex
| } else if ((code >= 30 && code <= 37) || (code >= 90 && code <= 97)) { | ||
| styleNames.push('code-fg-' + code); | ||
| } else if (code === 39) { | ||
| styleNames = styleNames.filter(style => !style.match(/^code-fg-\d+$/)); |
There was a problem hiding this comment.
Add a comment here explaining what this does since I am not clear
|
|
||
| i = index; | ||
| } else if (ansiSequence.match(/^.*[ABCDHIJKfhmpsu]$/)) { |
There was a problem hiding this comment.
This regex seems to be duplicated please extract it above with a clear name
There was a problem hiding this comment.
Now that I think about it, we don't even need this one. I'll remove it completely.
| return; | ||
| } | ||
|
|
||
| const content: string | HTMLElement = this.linkDetector.handleLinks(stringContent); |
There was a problem hiding this comment.
No need for explicit types everywhere, ts should nicely infer the types here
|
I made the changes based on your comments and pushed. I was thinking about unit tests too, certainly seems like an area where there's a lot of room for error. I did some pretty extensive testing manually, so it shouldn't be too difficult to write some tests that do similar things. I won't be able to get to them this week, but I'll take a look on the weekend. |
|
@danielfrankcom thanks for addressing the comments. Thanks a lot for your hard work! |
|
I haven't finished the tests yet, but I've extracted the functions from the You mentioned putting them into |
|
@danielfrankcom this is great great work! Thanks a lot! |
Fixes #47457 and maintains fix from #21423
More robust and expandable way of handling ANSI escape sequences. Future sequences should be able to slot in fairly easily with the new layout, as will be demonstrated by my next pull request.