From 571988bf99b8788ae6ddacb7ffdddf42f6e5d6da Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Wed, 21 Aug 2024 11:57:27 -0500 Subject: [PATCH 1/4] Clean up Kitty color protocol by using a union Non-exhaustive enums should be avoided, use a union to make the code cleaner and safer. --- src/terminal/kitty/color.zig | 71 +++++++--------- src/terminal/osc.zig | 32 +++---- src/termio/stream_handler.zig | 153 +++++++++++++++++----------------- 3 files changed, 118 insertions(+), 138 deletions(-) diff --git a/src/terminal/kitty/color.zig b/src/terminal/kitty/color.zig index e896ae0917..6b797e7330 100644 --- a/src/terminal/kitty/color.zig +++ b/src/terminal/kitty/color.zig @@ -18,27 +18,33 @@ pub const OSC = struct { terminator: Terminator = .st, }; -pub const Kind = enum(u9) { - // Make sure that this stays in sync with the highest numbered enum - // value. - pub const max: u9 = 263; +pub const Special = enum { + foreground, + background, + selection_foreground, + selection_background, + cursor, + cursor_text, + visual_bell, + second_transparent_background, +}; + +pub const Kind = union(enum) { + pub const max: usize = std.math.maxInt(u8) + @typeInfo(Special).Enum.fields.len; - // These _must_ start at 256 since enum values 0-255 are reserved - // for the palette. - foreground = 256, - background = 257, - selection_foreground = 258, - selection_background = 259, - cursor = 260, - cursor_text = 261, - visual_bell = 262, - second_transparent_background = 263, - _, + palette: u8, + special: Special, - /// Return the palette index that this kind is representing - /// or null if its a special color. - pub fn palette(self: Kind) ?u8 { - return std.math.cast(u8, @intFromEnum(self)) orelse null; + pub fn parse(key: []const u8) ?Kind { + return kind: { + const s = std.meta.stringToEnum(Special, key) orelse { + const p = std.fmt.parseUnsigned(u8, key, 10) catch { + break :kind null; + }; + break :kind Kind{ .palette = p }; + }; + break :kind Kind{ .special = s }; + }; } pub fn format( @@ -52,41 +58,24 @@ pub const Kind = enum(u9) { // Format as a number if its a palette color otherwise // format as a string. - if (self.palette()) |idx| { - try writer.print("{}", .{idx}); + if (self == .palette) { + try writer.print("{d}", .{self.palette}); } else { - try writer.print("{s}", .{@tagName(self)}); + try writer.print("{s}", .{@tagName(self.special)}); } } }; -test "OSC: kitty color protocol kind" { - const info = @typeInfo(Kind); - - try std.testing.expectEqual(false, info.Enum.is_exhaustive); - - var min: usize = std.math.maxInt(info.Enum.tag_type); - var max: usize = 0; - - inline for (info.Enum.fields) |field| { - if (field.value > max) max = field.value; - if (field.value < min) min = field.value; - } - - try std.testing.expect(min >= 256); - try std.testing.expect(max == Kind.max); -} - test "OSC: kitty color protocol kind string" { const testing = std.testing; var buf: [256]u8 = undefined; { - const actual = try std.fmt.bufPrint(&buf, "{}", .{Kind.foreground}); + const actual = try std.fmt.bufPrint(&buf, "{}", .{Kind{ .special = .foreground }}); try testing.expectEqualStrings("foreground", actual); } { - const actual = try std.fmt.bufPrint(&buf, "{}", .{@as(Kind, @enumFromInt(42))}); + const actual = try std.fmt.bufPrint(&buf, "{}", .{Kind{ .palette = 42 }}); try testing.expectEqualStrings("42", actual); } } diff --git a/src/terminal/osc.zig b/src/terminal/osc.zig index 16f9ad7a01..2d7cd3c6e4 100644 --- a/src/terminal/osc.zig +++ b/src/terminal/osc.zig @@ -1006,19 +1006,10 @@ pub const Parser = struct { return; } - // For our key, we first try to parse it as a special key. If that - // doesn't work then we try to parse it as a number for a palette. - const key: kitty.color.Kind = std.meta.stringToEnum( - kitty.color.Kind, - self.temp_state.key, - ) orelse @enumFromInt(std.fmt.parseUnsigned( - u8, - self.temp_state.key, - 10, - ) catch { + const key = kitty.color.Kind.parse(self.temp_state.key) orelse { log.warn("unknown key in kitty color protocol: {s}", .{self.temp_state.key}); return; - }); + }; const value = value: { if (self.buf_start == self.buf_idx) break :value ""; @@ -1633,6 +1624,7 @@ test "OSC: hyperlink end" { test "OSC: kitty color protocol" { const testing = std.testing; + const Kind = kitty.color.Kind; var p: Parser = .{ .alloc = testing.allocator }; defer p.deinit(); @@ -1646,12 +1638,12 @@ test "OSC: kitty color protocol" { { const item = cmd.kitty_color_protocol.list.items[0]; try testing.expect(item == .query); - try testing.expectEqual(kitty.color.Kind.foreground, item.query); + try testing.expectEqual(Kind{ .special = .foreground }, item.query); } { const item = cmd.kitty_color_protocol.list.items[1]; try testing.expect(item == .set); - try testing.expectEqual(kitty.color.Kind.background, item.set.key); + try testing.expectEqual(Kind{ .special = .background }, item.set.key); try testing.expectEqual(@as(u8, 0xf0), item.set.color.r); try testing.expectEqual(@as(u8, 0xf8), item.set.color.g); try testing.expectEqual(@as(u8, 0xff), item.set.color.b); @@ -1659,7 +1651,7 @@ test "OSC: kitty color protocol" { { const item = cmd.kitty_color_protocol.list.items[2]; try testing.expect(item == .set); - try testing.expectEqual(kitty.color.Kind.cursor, item.set.key); + try testing.expectEqual(Kind{ .special = .cursor }, item.set.key); try testing.expectEqual(@as(u8, 0xf0), item.set.color.r); try testing.expectEqual(@as(u8, 0xf8), item.set.color.g); try testing.expectEqual(@as(u8, 0xff), item.set.color.b); @@ -1667,22 +1659,22 @@ test "OSC: kitty color protocol" { { const item = cmd.kitty_color_protocol.list.items[3]; try testing.expect(item == .reset); - try testing.expectEqual(kitty.color.Kind.cursor_text, item.reset); + try testing.expectEqual(Kind{ .special = .cursor_text }, item.reset); } { const item = cmd.kitty_color_protocol.list.items[4]; try testing.expect(item == .reset); - try testing.expectEqual(kitty.color.Kind.visual_bell, item.reset); + try testing.expectEqual(Kind{ .special = .visual_bell }, item.reset); } { const item = cmd.kitty_color_protocol.list.items[5]; try testing.expect(item == .query); - try testing.expectEqual(kitty.color.Kind.selection_background, item.query); + try testing.expectEqual(Kind{ .special = .selection_background }, item.query); } { const item = cmd.kitty_color_protocol.list.items[6]; try testing.expect(item == .set); - try testing.expectEqual(kitty.color.Kind.selection_background, item.set.key); + try testing.expectEqual(Kind{ .special = .selection_background }, item.set.key); try testing.expectEqual(@as(u8, 0xaa), item.set.color.r); try testing.expectEqual(@as(u8, 0xbb), item.set.color.g); try testing.expectEqual(@as(u8, 0xcc), item.set.color.b); @@ -1690,12 +1682,12 @@ test "OSC: kitty color protocol" { { const item = cmd.kitty_color_protocol.list.items[7]; try testing.expect(item == .query); - try testing.expectEqual(@as(kitty.color.Kind, @enumFromInt(2)), item.query); + try testing.expectEqual(Kind{ .palette = 2 }, item.query); } { const item = cmd.kitty_color_protocol.list.items[8]; try testing.expect(item == .set); - try testing.expectEqual(@as(kitty.color.Kind, @enumFromInt(3)), item.set.key); + try testing.expectEqual(Kind{ .palette = 3 }, item.set.key); try testing.expectEqual(@as(u8, 0xff), item.set.color.r); try testing.expectEqual(@as(u8, 0xff), item.set.color.g); try testing.expectEqual(@as(u8, 0xff), item.set.color.b); diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index 5aa3dfd75c..c2c12ffa85 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -1319,17 +1319,18 @@ pub const StreamHandler = struct { switch (item) { .query => |key| { const color: terminal.color.RGB = switch (key) { - .foreground => self.foreground_color, - .background => self.background_color, - .cursor => self.cursor_color, - else => if (key.palette()) |idx| - self.terminal.color_palette.colors[idx] - else { - log.warn("ignoring unsupported kitty color protocol key: {}", .{key}); - continue; + .palette => |palette| self.terminal.color_palette.colors[palette], + .special => |special| switch (special) { + .foreground => self.foreground_color, + .background => self.background_color, + .cursor => self.cursor_color, + else => { + log.warn("ignoring unsupported kitty color protocol key: {}", .{key}); + continue; + }, }, } orelse { - log.warn("no color configured for: {s}", .{@tagName(key)}); + log.warn("no color configured for {}", .{key}); continue; }; @@ -1339,85 +1340,83 @@ pub const StreamHandler = struct { ); }, .set => |v| switch (v.key) { - .foreground => { - self.foreground_color = v.color; - - // See messageWriter which has similar logic and - // explains why we may have to do this. - self.rendererMessageWriter(.{ - .foreground_color = v.color, - }); - }, - .background => { - self.background_color = v.color; - - // See messageWriter which has similar logic and - // explains why we may have to do this. - self.rendererMessageWriter(.{ - .background_color = v.color, - }); + .palette => |palette| { + self.terminal.flags.dirty.palette = true; + self.terminal.color_palette.colors[palette] = v.color; + self.terminal.color_palette.mask.unset(palette); }, - .cursor => { - self.cursor_color = v.color; + .special => |special| { + const msg: renderer.Message = switch (special) { + .foreground => msg: { + self.foreground_color = v.color; + break :msg .{ + .foreground_color = v.color, + }; + }, + .background => msg: { + self.background_color = v.color; + break :msg .{ + .background_color = v.color, + }; + }, + .cursor => msg: { + self.cursor_color = v.color; + break :msg .{ + .cursor_color = v.color, + }; + }, + else => { + log.warn( + "ignoring unsupported kitty color protocol key: {}", + .{v.key}, + ); + continue; + }, + }; // See messageWriter which has similar logic and // explains why we may have to do this. - self.rendererMessageWriter(.{ - .cursor_color = v.color, - }); - }, - - else => if (v.key.palette()) |i| { - self.terminal.flags.dirty.palette = true; - self.terminal.color_palette.colors[i] = v.color; - self.terminal.color_palette.mask.unset(i); - } else { - log.warn( - "ignoring unsupported kitty color protocol key: {}", - .{v.key}, - ); - continue; + self.rendererMessageWriter(msg); }, }, .reset => |key| switch (key) { - .foreground => { - self.foreground_color = self.default_foreground_color; - - // See messageWriter which has similar logic and - // explains why we may have to do this. - self.rendererMessageWriter(.{ - .foreground_color = self.default_foreground_color, - }); - }, - .background => { - self.background_color = self.default_background_color; - - // See messageWriter which has similar logic and - // explains why we may have to do this. - self.rendererMessageWriter(.{ - .background_color = self.default_background_color, - }); + .palette => |palette| { + self.terminal.flags.dirty.palette = true; + self.terminal.color_palette.colors[palette] = self.terminal.default_palette[palette]; + self.terminal.color_palette.mask.unset(palette); }, - .cursor => { - self.cursor_color = self.default_cursor_color; + .special => |special| { + const msg: renderer.Message = switch (special) { + .foreground => msg: { + self.foreground_color = self.default_foreground_color; + break :msg .{ + .foreground_color = self.default_foreground_color, + }; + }, + .background => msg: { + self.background_color = self.default_background_color; + break :msg .{ + .background_color = self.default_background_color, + }; + }, + .cursor => msg: { + self.cursor_color = self.default_cursor_color; + break :msg .{ + .cursor_color = self.default_cursor_color, + }; + }, + else => { + log.warn( + "ignoring unsupported kitty color protocol key: {}", + .{key}, + ); + continue; + }, + }; // See messageWriter which has similar logic and // explains why we may have to do this. - self.rendererMessageWriter(.{ - .cursor_color = self.default_cursor_color, - }); - }, - - else => if (key.palette()) |i| { - self.terminal.flags.dirty.palette = true; - self.terminal.color_palette.colors[i] = self.terminal.default_palette[i]; - self.terminal.color_palette.mask.unset(i); - } else { - log.warn( - "ignoring unsupported kitty color protocol key: {}", - .{key}, - ); - continue; + self.rendererMessageWriter(msg); }, }, } From 54e2ea05a51203f28ecc9af63434218b294266a2 Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Wed, 21 Aug 2024 12:37:18 -0500 Subject: [PATCH 2/4] Use `switch` and not `if` to format Kind --- src/terminal/kitty/color.zig | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/terminal/kitty/color.zig b/src/terminal/kitty/color.zig index 6b797e7330..91dc3dadf5 100644 --- a/src/terminal/kitty/color.zig +++ b/src/terminal/kitty/color.zig @@ -56,12 +56,9 @@ pub const Kind = union(enum) { _ = layout; _ = opts; - // Format as a number if its a palette color otherwise - // format as a string. - if (self == .palette) { - try writer.print("{d}", .{self.palette}); - } else { - try writer.print("{s}", .{@tagName(self.special)}); + switch (self) { + .palette => |p| try writer.print("{d}", .{p}), + .special => |s| try writer.print("{s}", .{@tagName(s)}), } } }; From b8d4969feef148801081071322463e38103b85bb Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Wed, 21 Aug 2024 15:28:00 -0500 Subject: [PATCH 3/4] Make the `Kind` parsing simpler --- src/terminal/kitty/color.zig | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/terminal/kitty/color.zig b/src/terminal/kitty/color.zig index 91dc3dadf5..6c95d79c7d 100644 --- a/src/terminal/kitty/color.zig +++ b/src/terminal/kitty/color.zig @@ -36,15 +36,8 @@ pub const Kind = union(enum) { special: Special, pub fn parse(key: []const u8) ?Kind { - return kind: { - const s = std.meta.stringToEnum(Special, key) orelse { - const p = std.fmt.parseUnsigned(u8, key, 10) catch { - break :kind null; - }; - break :kind Kind{ .palette = p }; - }; - break :kind Kind{ .special = s }; - }; + if (std.meta.stringToEnum(Special, key)) |s| return Kind{ .special = s }; + return Kind{ .palette = std.fmt.parseUnsigned(u8, key, 10) catch return null }; } pub fn format( From 8e2d63b6fa5e3d7a14713d72d11ee74b8041ff36 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 24 Aug 2024 19:55:01 -0700 Subject: [PATCH 4/4] small stylistic changes --- src/terminal/kitty/color.zig | 4 ++-- src/termio/stream_handler.zig | 26 ++++++++------------------ 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/terminal/kitty/color.zig b/src/terminal/kitty/color.zig index 6c95d79c7d..a631bc6a5d 100644 --- a/src/terminal/kitty/color.zig +++ b/src/terminal/kitty/color.zig @@ -36,8 +36,8 @@ pub const Kind = union(enum) { special: Special, pub fn parse(key: []const u8) ?Kind { - if (std.meta.stringToEnum(Special, key)) |s| return Kind{ .special = s }; - return Kind{ .palette = std.fmt.parseUnsigned(u8, key, 10) catch return null }; + if (std.meta.stringToEnum(Special, key)) |s| return .{ .special = s }; + return .{ .palette = std.fmt.parseUnsigned(u8, key, 10) catch return null }; } pub fn format( diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index c2c12ffa85..83e40d4d11 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -1345,25 +1345,20 @@ pub const StreamHandler = struct { self.terminal.color_palette.colors[palette] = v.color; self.terminal.color_palette.mask.unset(palette); }, + .special => |special| { const msg: renderer.Message = switch (special) { .foreground => msg: { self.foreground_color = v.color; - break :msg .{ - .foreground_color = v.color, - }; + break :msg .{ .foreground_color = v.color }; }, .background => msg: { self.background_color = v.color; - break :msg .{ - .background_color = v.color, - }; + break :msg .{ .background_color = v.color }; }, .cursor => msg: { self.cursor_color = v.color; - break :msg .{ - .cursor_color = v.color, - }; + break :msg .{ .cursor_color = v.color }; }, else => { log.warn( @@ -1385,25 +1380,20 @@ pub const StreamHandler = struct { self.terminal.color_palette.colors[palette] = self.terminal.default_palette[palette]; self.terminal.color_palette.mask.unset(palette); }, + .special => |special| { const msg: renderer.Message = switch (special) { .foreground => msg: { self.foreground_color = self.default_foreground_color; - break :msg .{ - .foreground_color = self.default_foreground_color, - }; + break :msg .{ .foreground_color = self.default_foreground_color }; }, .background => msg: { self.background_color = self.default_background_color; - break :msg .{ - .background_color = self.default_background_color, - }; + break :msg .{ .background_color = self.default_background_color }; }, .cursor => msg: { self.cursor_color = self.default_cursor_color; - break :msg .{ - .cursor_color = self.default_cursor_color, - }; + break :msg .{ .cursor_color = self.default_cursor_color }; }, else => { log.warn(