Conversation
Remove an `is AnsiRgbCode` type check and allow for general `AnsiCode` values to consist of multiple codes.
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Unused Dependencies ✔️
For details on how to fix these, see dependency_validator. This check can be disabled by tagging the PR with
Coverage
|
| File | Coverage |
|---|---|
| pkgs/io/example/example.dart | 💔 Not covered |
| pkgs/io/lib/src/ansi_code.dart | 💔 82 % ⬇️ 3 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
This check can be disabled by tagging the PR with skip-changelog-check.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for 24-bit RGB colors for ANSI escape codes, which is a great enhancement. The implementation includes a new AnsiRgbCode class, a convenient rgb factory function, and updates to wrapWith to handle composite codes. The accompanying tests and example are helpful.
I've found a bug in the new example code that can lead to a runtime error, and I have a couple of suggestions for improving the code's maintainability in ansi_code.dart. Overall, this is a solid contribution.
| final length = text.length; | ||
| final buffer = StringBuffer(); | ||
| for (var i = 0; i < length; i++) { | ||
| final ratio = i / (length - 1); |
There was a problem hiding this comment.
There's a potential division by zero here if text.length is 1. length - 1 would be 0, and i / (length - 1) would result in NaN. This leads to a runtime error when rgb() is called with NaN values because they cannot be assigned to int. You should handle this edge case.
| final ratio = i / (length - 1); | |
| final ratio = length > 1 ? i / (length - 1) : 0.0; |
There was a problem hiding this comment.
Or start with if (length == 0) return '';.
| @override | ||
| String get escape => '$_ansiEscapeLiteral[${codes.join(';')}m'; | ||
|
|
||
| @override | ||
| String get escapeForScript => '$_ansiEscapeForScript[${codes.join(';')}m'; | ||
| } |
There was a problem hiding this comment.
While these overrides are necessary with the current base class implementation, consider modifying the escape and escapeForScript getters in the AnsiCode base class to use codes.join(';'). This would make them work for both single and composite codes, and you could then remove these overrides, reducing code duplication and simplifying AnsiRgbCode.
Example change in AnsiCode:
// In AnsiCode
String get escape => '$_ansiEscapeLiteral[${codes.join(';')}m';
String get escapeForScript => '$_ansiEscapeForScript[${codes.join(';')}m';| break; | ||
| } | ||
| } | ||
| // myCodes.sort((a, b) => a.code.compareTo(b.code)); |
There was a problem hiding this comment.
This commented-out line appears to be a remnant from a previous implementation. It should be removed to improve code clarity.
| AnsiCodeType type = AnsiCodeType.foreground, | ||
| }) { | ||
| if (red < 0 || red > 255) { | ||
| throw ArgumentError.value(red, 'red', 'Must be between 0 and 255.'); |
There was a problem hiding this comment.
It's relevant, so throw RangeError.range(red, 0, 255, 'red');.
And even use the helper function:
RangeError.checkValueInInterval(red, 0, 255, 'red');
RangeError.checkValueInInterval(green, 0, 255, 'green');
RangeError.checkValueInInterval(blue, 0, 255, 'blue');It's not important which Error subclass is used, the specialized ones are mainly there for easy reuse and consistent error messages. But that's reason enough to use it here too.
|
It might make sense to build this on top of #2297 |
| String toString() => '$name ${type._name} ($code)'; | ||
| } | ||
|
|
||
| /// An ANSI escape code for RGB colours. |
There was a problem hiding this comment.
Do we need this class, or it being public.
Could all AnsiCode objects have a codes list, and then AnsiCode.rgb just fills it in.
class AnsiCode {
/// The numeric value associated with this code.
///
/// `-1` if this code is a composite code with multiple integer values.
/// See [codes].
final int code;
/// The numeric values associated with this code.
///
/// A composite code may have more than one integer value, in which case the
/// [code] property will be `-1`.
Iterable<int> get codes => _codes ??= [code];
AnsiCode._rgb(int red, int green, int blue, AnsiCodeType type) :
code = -1,
_codes = List.unmodifiable([
type == AnsiCodeType.background ? 48 : 38,
2,
RangeError.checkValueInInterval(red, 0, 255, 'red'),
RangeError.checkValueInInterval(green, 0, 255, 'green'),
RangeError.checkValueInInterval(blue, 0, 255, 'blue'),
]);Does codes even need to be public?
Did code? Having code now sometimes being -1 and invalid is a breaking change if anyone uses it, but why would anyone use it?
(Maybe we should make a breaking change to make the class more opaque, then it's easier to extend it.)
| /// Use [rgb] to create an instance of this class. | ||
| /// | ||
| /// [See also](https://en.wikipedia.org/wiki/ANSI_escape_code#24-bit) | ||
| class AnsiRgbCode extends AnsiCode { |
There was a problem hiding this comment.
How well-supported is RGB colors in ANSI escapes these days?
I can see xterm and rxvt seem to supports all colors, so does the Windows terminal.
So probably fairly widely supported.
There was a problem hiding this comment.
Yes, I think it has wide enough support to be worth landing.
Will take another pass at this after we land the other changes. The sorting detail may need to be revisited but I think it may be OK to skip sorting and update test expectations. |
3 participants
Add an
RgbAnsiCodeclass which hold the red, green, and blue channelsas separate fields.
Add a
codesfield toAnsiCodeand update the comment for thecodefield indicating that some ansi codes with more than one integer code
will return
-1for thecodefield. All uses should go throughcodes.