-
Notifications
You must be signed in to change notification settings - Fork 28.7k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex seems to be duplicated please extract it above with a clear name
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.