-
Notifications
You must be signed in to change notification settings - Fork 78
[io] support rgb colours for ansi escape codes #2287
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Code Review
The pull request introduces RGB color support for ANSI escape codes, which is a valuable addition. The AnsiRgbCode class and rgb utility function are well-documented. The example and tests demonstrate the new functionality effectively. However, there are a few areas for improvement regarding robustness, correctness, and adherence to Dart's type system. Specifically, the _gradient function needs to handle single-character strings gracefully, the AnsiRgbCode class has a redundant and problematic field override, and the wrapWith function should prevent combining standard and RGB color codes of the same type. There are also minor issues with a typo in a test description and an error message.
| void _gradient(String text, bool forScript) { | ||
| final length = text.length; | ||
| final buffer = StringBuffer(); | ||
| for (var i = 0; i < length; i++) { | ||
| final ratio = i / (length - 1); | ||
| int red, green, blue; | ||
| if (ratio < .5) { | ||
| red = ((1 - (ratio * 2)) * 255).round(); | ||
| green = (ratio * 2 * 255).round(); | ||
| blue = 0; | ||
| } else { | ||
| red = 0; | ||
| green = ((1 - ((ratio - .5) * 2)) * 255).round(); | ||
| blue = (((ratio - .5) * 2) * 255).round(); | ||
| } | ||
| buffer.write(rgb(red, green, blue).wrap(text[i], forScript: forScript)); | ||
| } | ||
| print(buffer.toString()); |
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.
The _gradient function can produce NaN for ratio when text contains a single character (e.g., length is 1), leading to a runtime error due to division by zero. This can be fixed by handling the single-character case explicitly to prevent this issue, improving the robustness of the function.
| void _gradient(String text, bool forScript) { | |
| final length = text.length; | |
| final buffer = StringBuffer(); | |
| for (var i = 0; i < length; i++) { | |
| final ratio = i / (length - 1); | |
| int red, green, blue; | |
| if (ratio < .5) { | |
| red = ((1 - (ratio * 2)) * 255).round(); | |
| green = (ratio * 2 * 255).round(); | |
| blue = 0; | |
| } else { | |
| red = 0; | |
| green = ((1 - ((ratio - .5) * 2)) * 255).round(); | |
| blue = (((ratio - .5) * 2) * 255).round(); | |
| } | |
| buffer.write(rgb(red, green, blue).wrap(text[i], forScript: forScript)); | |
| } | |
| print(buffer.toString()); | |
| void _gradient(String text, bool forScript) { | |
| final length = text.length; | |
| if (length == 0) return; | |
| final buffer = StringBuffer(); | |
| for (var i = 0; i < length; i++) { | |
| final ratio = length == 1 ? 0.5 : i / (length - 1); | |
| int red, green, blue; | |
| if (ratio < .5) { | |
| red = ((1 - (ratio * 2)) * 255).round(); | |
| green = (ratio * 2 * 255).round(); | |
| blue = 0; | |
| } else { | |
| red = 0; | |
| green = ((1 - ((ratio - .5) * 2)) * 255).round(); | |
| blue = (((ratio - .5) * 2) * 255).round(); | |
| } | |
| buffer.write(rgb(red, green, blue).wrap(text[i], forScript: forScript)); | |
| } | |
| print(buffer.toString()); | |
| } |
| if (code is AnsiRgbCode) { | ||
| ansiRgbCode = code; | ||
| } else if (code.code != -1) { | ||
| numericCodes.add(code.code); |
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.
The wrapWith function currently allows both a standard foreground/background AnsiCode and an AnsiRgbCode of the same type to be present in the codes iterable. This can lead to ambiguous or incorrect ANSI escape sequences (e.g., ESC[34;38;2;255;0;0m). The foreground and background counters should also account for AnsiRgbCode instances to ensure only one color type is applied per foreground/background. This improves correctness and maintainability.
if (code is AnsiRgbCode) {
if (code.type == AnsiCodeType.foreground) {
foreground++;
} else if (code.type == AnsiCodeType.background) {
background++;
}
ansiRgbCode = code;
} else if (code.code != -1) {
numericCodes.add(code.code);
}
This PR adds RGB colour support for ANSI escape codes.
I wasn't sure how to approach this, so any suggestion is welcome.
Also for the changelog, I have no clue if it's the right way to do it considering
1.1.0never got released.Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.