Skip to content

Commit

Permalink
Fix SGR direct-color parsing issue (#4216)
Browse files Browse the repository at this point in the history 
Fix for a little parsing issue I took note of
[here](#2125 (comment)).

The disparity in behavior between `ghostty@main` and `xterm` can be seen
with this reproduction script:
```sh
printf "\e[0m\nForeground:\n";
printf "\e[38:2:0:255:0mGreen\n";
printf "\e[38;2;0;255;0mGreen\n";
printf "\e[38:2:0:255:0:255mMagenta\n";
printf "\e[38;2;0;255;0;255mGreen\n";

printf "\e[0m\nBackground:\n";
printf "\e[48:2:0:255:0mGreen\n";
printf "\e[48;2;0;255;0mGreen\n";
printf "\e[48:2:0:255:0:255mMagenta\n";
printf "\e[48;2;0;255;0;255mGreen\n";

printf "\e[0m\nUnderline:\n";
printf "\e[58:2:0:255:0m\e[4mGreen\n";
printf "\e[58;2;0;255;0m\e[4mGreen\n";
printf "\e[58:2:0:255:0:255m\e[4mMagenta\n";
printf "\e[58;2;0;255;0;255m\e[4mGreen\n";

printf "\e[0m\n";
```

### Outputs:
|`xterm`|`ghostty@main`|this PR|
|-|-|-|
|<img width="85" alt="image"
src="https://github.com/user-attachments/assets/a0aacff2-2103-4fff-9160-5e663d8a70a2"
/>|<img width="110" alt="image"
src="https://github.com/user-attachments/assets/0ad12e67-3f2c-46f3-b0ee-9230032d188a"
/>|<img width="110" alt="image"
src="https://github.com/user-attachments/assets/7477e3cf-7d27-419e-986b-8df581e52398"
/>|
  • Loading branch information
@mitchellh
mitchellh authored Dec 31, 2024
2 parents 47cf5cb + 4543cde commit df0620a
Showing 1 changed file with 155 additions and 64 deletions.
219 changes: 155 additions & 64 deletions src/terminal/sgr.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,26 +189,39 @@ pub const Parser = struct {
.@"8_fg" = @enumFromInt(slice[0] - 30),
},

38 => if (slice.len >= 5 and slice[1] == 2) {
self.idx += 4;

// In the 6-len form, ignore the 3rd param.
const rgb = slice[2..5];

// We use @truncate because the value should be 0 to 255. If
// it isn't, the behavior is undefined so we just... truncate it.
return Attribute{
.direct_color_fg = .{
.r = @truncate(rgb[0]),
.g = @truncate(rgb[1]),
.b = @truncate(rgb[2]),
},
};
} else if (slice.len >= 3 and slice[1] == 5) {
self.idx += 2;
return Attribute{
.@"256_fg" = @truncate(slice[2]),
};
38 => if (slice.len >= 2) switch (slice[1]) {
// `2` indicates direct-color (r, g, b).
// We need at least 3 more params for this to make sense.
2 => if (slice.len >= 5) {
self.idx += 4;
// When a colon separator is used, there may or may not be
// a color space identifier as the third param, which we
// need to ignore (it has no standardized behavior).
const rgb = if (slice.len == 5 or !self.colon)
slice[2..5]
else rgb: {
self.idx += 1;
break :rgb slice[3..6];
};

// We use @truncate because the value should be 0 to 255. If
// it isn't, the behavior is undefined so we just... truncate it.
return Attribute{
.direct_color_fg = .{
.r = @truncate(rgb[0]),
.g = @truncate(rgb[1]),
.b = @truncate(rgb[2]),
},
};
},
// `5` indicates indexed color.
5 => if (slice.len >= 3) {
self.idx += 2;
return Attribute{
.@"256_fg" = @truncate(slice[2]),
};
},
else => {},
},

39 => return Attribute{ .reset_fg = {} },
Expand All @@ -217,57 +230,79 @@ pub const Parser = struct {
.@"8_bg" = @enumFromInt(slice[0] - 40),
},

48 => if (slice.len >= 5 and slice[1] == 2) {
self.idx += 4;

// We only support the 5-len form.
const rgb = slice[2..5];

// We use @truncate because the value should be 0 to 255. If
// it isn't, the behavior is undefined so we just... truncate it.
return Attribute{
.direct_color_bg = .{
.r = @truncate(rgb[0]),
.g = @truncate(rgb[1]),
.b = @truncate(rgb[2]),
},
};
} else if (slice.len >= 3 and slice[1] == 5) {
self.idx += 2;
return Attribute{
.@"256_bg" = @truncate(slice[2]),
};
48 => if (slice.len >= 2) switch (slice[1]) {
// `2` indicates direct-color (r, g, b).
// We need at least 3 more params for this to make sense.
2 => if (slice.len >= 5) {
self.idx += 4;
// When a colon separator is used, there may or may not be
// a color space identifier as the third param, which we
// need to ignore (it has no standardized behavior).
const rgb = if (slice.len == 5 or !self.colon)
slice[2..5]
else rgb: {
self.idx += 1;
break :rgb slice[3..6];
};

// We use @truncate because the value should be 0 to 255. If
// it isn't, the behavior is undefined so we just... truncate it.
return Attribute{
.direct_color_bg = .{
.r = @truncate(rgb[0]),
.g = @truncate(rgb[1]),
.b = @truncate(rgb[2]),
},
};
},
// `5` indicates indexed color.
5 => if (slice.len >= 3) {
self.idx += 2;
return Attribute{
.@"256_bg" = @truncate(slice[2]),
};
},
else => {},
},

49 => return Attribute{ .reset_bg = {} },

53 => return Attribute{ .overline = {} },
55 => return Attribute{ .reset_overline = {} },

58 => if (slice.len >= 5 and slice[1] == 2) {
self.idx += 4;

// In the 6-len form, ignore the 3rd param. Otherwise, use it.
const rgb = if (slice.len == 5) slice[2..5] else rgb: {
// Consume one more element
self.idx += 1;
break :rgb slice[3..6];
};

// We use @truncate because the value should be 0 to 255. If
// it isn't, the behavior is undefined so we just... truncate it.
return Attribute{
.underline_color = .{
.r = @truncate(rgb[0]),
.g = @truncate(rgb[1]),
.b = @truncate(rgb[2]),
},
};
} else if (slice.len >= 3 and slice[1] == 5) {
self.idx += 2;
return Attribute{
.@"256_underline_color" = @truncate(slice[2]),
};
58 => if (slice.len >= 2) switch (slice[1]) {
// `2` indicates direct-color (r, g, b).
// We need at least 3 more params for this to make sense.
2 => if (slice.len >= 5) {
self.idx += 4;
// When a colon separator is used, there may or may not be
// a color space identifier as the third param, which we
// need to ignore (it has no standardized behavior).
const rgb = if (slice.len == 5 or !self.colon)
slice[2..5]
else rgb: {
self.idx += 1;
break :rgb slice[3..6];
};

// We use @truncate because the value should be 0 to 255. If
// it isn't, the behavior is undefined so we just... truncate it.
return Attribute{
.underline_color = .{
.r = @truncate(rgb[0]),
.g = @truncate(rgb[1]),
.b = @truncate(rgb[2]),
},
};
},
// `5` indicates indexed color.
5 => if (slice.len >= 3) {
self.idx += 2;
return Attribute{
.@"256_underline_color" = @truncate(slice[2]),
};
},
else => {},
},

59 => return Attribute{ .reset_underline_color = {} },
Expand Down Expand Up @@ -566,3 +601,59 @@ test "sgr: direct color bg missing color" {
var p: Parser = .{ .params = &[_]u16{ 48, 5 }, .colon = false };
while (p.next()) |_| {}
}

test "sgr: direct fg/bg/underline ignore optional color space" {
// These behaviors have been verified against xterm.

// Colon version should skip the optional color space identifier
{
// 3 8 : 2 : Pi : Pr : Pg : Pb
const v = testParseColon(&[_]u16{ 38, 2, 0, 1, 2, 3, 4 });
try testing.expect(v == .direct_color_fg);
try testing.expectEqual(@as(u8, 1), v.direct_color_fg.r);
try testing.expectEqual(@as(u8, 2), v.direct_color_fg.g);
try testing.expectEqual(@as(u8, 3), v.direct_color_fg.b);
}
{
// 4 8 : 2 : Pi : Pr : Pg : Pb
const v = testParseColon(&[_]u16{ 48, 2, 0, 1, 2, 3, 4 });
try testing.expect(v == .direct_color_bg);
try testing.expectEqual(@as(u8, 1), v.direct_color_bg.r);
try testing.expectEqual(@as(u8, 2), v.direct_color_bg.g);
try testing.expectEqual(@as(u8, 3), v.direct_color_bg.b);
}
{
// 5 8 : 2 : Pi : Pr : Pg : Pb
const v = testParseColon(&[_]u16{ 58, 2, 0, 1, 2, 3, 4 });
try testing.expect(v == .underline_color);
try testing.expectEqual(@as(u8, 1), v.underline_color.r);
try testing.expectEqual(@as(u8, 2), v.underline_color.g);
try testing.expectEqual(@as(u8, 3), v.underline_color.b);
}

// Semicolon version should not parse optional color space identifier
{
// 3 8 ; 2 ; Pr ; Pg ; Pb
const v = testParse(&[_]u16{ 38, 2, 0, 1, 2, 3, 4 });
try testing.expect(v == .direct_color_fg);
try testing.expectEqual(@as(u8, 0), v.direct_color_fg.r);
try testing.expectEqual(@as(u8, 1), v.direct_color_fg.g);
try testing.expectEqual(@as(u8, 2), v.direct_color_fg.b);
}
{
// 4 8 ; 2 ; Pr ; Pg ; Pb
const v = testParse(&[_]u16{ 48, 2, 0, 1, 2, 3, 4 });
try testing.expect(v == .direct_color_bg);
try testing.expectEqual(@as(u8, 0), v.direct_color_bg.r);
try testing.expectEqual(@as(u8, 1), v.direct_color_bg.g);
try testing.expectEqual(@as(u8, 2), v.direct_color_bg.b);
}
{
// 5 8 ; 2 ; Pr ; Pg ; Pb
const v = testParse(&[_]u16{ 58, 2, 0, 1, 2, 3, 4 });
try testing.expect(v == .underline_color);
try testing.expectEqual(@as(u8, 0), v.underline_color.r);
try testing.expectEqual(@as(u8, 1), v.underline_color.g);
try testing.expectEqual(@as(u8, 2), v.underline_color.b);
}
}

0 comments on commit df0620a

Please sign in to comment.