From 761bcfad50e28dcd3b167fd2dd5b45adbab24a1f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 4 Sep 2024 10:20:32 -0700 Subject: [PATCH] kitty gfx: handle `q` with chunked transmissions properly Fixes #2190 The `q` value with chunk transmissions is a bit complicated. The initial transmission ("start" command) `q` value is used for all subsequent chunks UNLESS a subsequent chunk specifies a `q >= 1`. If a chunk specifies `q >= 1`, then that `q` value is subsequently used for all future chunks unless another chunk specifies `q >= 1`. And so on. This commit importantly also introduces the ability to test a full Kitty command from string request to structured response. This will let us prevent regressions in the future and ensure that we are correctly handling the complex underspecified behavior of Kitty Graphics. --- src/terminal/kitty/graphics_command.zig | 13 +++ src/terminal/kitty/graphics_exec.zig | 130 ++++++++++++++++++++++-- src/terminal/kitty/graphics_image.zig | 7 +- 3 files changed, 141 insertions(+), 9 deletions(-) diff --git a/src/terminal/kitty/graphics_command.zig b/src/terminal/kitty/graphics_command.zig index e165a3034..27ceb78c8 100644 --- a/src/terminal/kitty/graphics_command.zig +++ b/src/terminal/kitty/graphics_command.zig @@ -69,6 +69,14 @@ pub const Parser = struct { self.data.deinit(); } + /// Parse a complete command string. + pub fn parseString(alloc: Allocator, data: []const u8) !Command { + var parser = init(alloc); + defer parser.deinit(); + for (data) |c| try parser.feed(c); + return try parser.complete(); + } + /// Feed a single byte to the parser. /// /// The first byte to start parsing should be the byte immediately following @@ -282,6 +290,11 @@ pub const Response = struct { pub fn ok(self: Response) bool { return std.mem.eql(u8, self.message, "OK"); } + + /// Empty response + pub fn empty(self: Response) bool { + return self.id == 0 and self.image_number == 0; + } }; pub const Command = struct { diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index d8063795f..976f7936e 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -25,7 +25,7 @@ const log = std.log.scoped(.kitty_gfx); pub fn execute( alloc: Allocator, terminal: *Terminal, - cmd: *Command, + cmd: *const Command, ) ?Response { // If storage is disabled then we disable the full protocol. This means // we don't even respond to queries so the terminal completely acts as @@ -40,12 +40,36 @@ pub fn execute( cmd.control, }); + // The quiet settings used to control the response. We have to make this + // a var because in certain special cases (namely chunked transmissions) + // this can change. + var quiet = cmd.quiet; + const resp_: ?Response = switch (cmd.control) { .query => query(alloc, cmd), - .transmit, .transmit_and_display => transmit(alloc, terminal, cmd), .display => display(alloc, terminal, cmd), .delete => delete(alloc, terminal, cmd), + .transmit, .transmit_and_display => resp: { + // If we're transmitting, then our `q` setting value is complicated. + // The `q` setting inherits the value from the starting command + // unless `q` is set >= 1 on this command. If it is, then we save + // that as the new `q` setting. + const storage = &terminal.screen.kitty_images; + if (storage.loading) |loading| switch (cmd.quiet) { + // q=0 we use whatever the start command value is + .no => quiet = loading.quiet, + + // q>=1 we use the new value, but we should already be set to it + inline .ok, .failures => |tag| { + assert(quiet == tag); + loading.quiet = tag; + }, + }; + + break :resp transmit(alloc, terminal, cmd); + }, + .transmit_animation_frame, .control_animation, .compose_animation, @@ -58,8 +82,8 @@ pub fn execute( log.warn("erroneous kitty graphics response: {s}", .{resp.message}); } - return switch (cmd.quiet) { - .no => resp, + return switch (quiet) { + .no => if (resp.empty()) null else resp, .ok => if (resp.ok()) null else resp, .failures => null, }; @@ -72,7 +96,7 @@ pub fn execute( /// This command is used to attempt to load an image and respond with /// success/error but does not persist any of the command to the terminal /// state. -fn query(alloc: Allocator, cmd: *Command) Response { +fn query(alloc: Allocator, cmd: *const Command) Response { const t = cmd.control.query; // Query requires image ID. We can't actually send a response without @@ -106,7 +130,7 @@ fn query(alloc: Allocator, cmd: *Command) Response { fn transmit( alloc: Allocator, terminal: *Terminal, - cmd: *Command, + cmd: *const Command, ) Response { const t = cmd.transmission().?; var result: Response = .{ @@ -261,7 +285,7 @@ fn display( fn delete( alloc: Allocator, terminal: *Terminal, - cmd: *Command, + cmd: *const Command, ) Response { const storage = &terminal.screen.kitty_images; storage.delete(alloc, terminal, cmd.control.delete); @@ -273,7 +297,7 @@ fn delete( fn loadAndAddImage( alloc: Allocator, terminal: *Terminal, - cmd: *Command, + cmd: *const Command, ) !struct { image: Image, more: bool = false, @@ -361,3 +385,93 @@ fn encodeError(r: *Response, err: EncodeableError) void { error.DimensionsTooLarge => r.message = "EINVAL: dimensions too large", } } + +test "kittygfx more chunks with q=1" { + const testing = std.testing; + const alloc = testing.allocator; + + var t = try Terminal.init(alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + + // Initial chunk has q=1 + { + const cmd = try command.Parser.parseString( + alloc, + "a=T,f=24,t=d,i=1,s=1,v=2,c=10,r=1,m=1,q=1;////", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd); + try testing.expect(resp == null); + } + + // Subsequent chunk has no q but should respect initial + { + const cmd = try command.Parser.parseString( + alloc, + "m=0;////", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd); + try testing.expect(resp == null); + } +} + +test "kittygfx more chunks with q=0" { + const testing = std.testing; + const alloc = testing.allocator; + + var t = try Terminal.init(alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + + // Initial chunk has q=0 + { + const cmd = try command.Parser.parseString( + alloc, + "a=t,f=24,t=d,s=1,v=2,c=10,r=1,m=1,i=1,q=0;////", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd); + try testing.expect(resp == null); + } + + // Subsequent chunk has no q so should respond OK + { + const cmd = try command.Parser.parseString( + alloc, + "m=0;////", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } +} + +test "kittygfx more chunks with chunk increasing q" { + const testing = std.testing; + const alloc = testing.allocator; + + var t = try Terminal.init(alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + + // Initial chunk has q=0 + { + const cmd = try command.Parser.parseString( + alloc, + "a=t,f=24,t=d,s=1,v=2,c=10,r=1,m=1,i=1,q=0;////", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd); + try testing.expect(resp == null); + } + + // Subsequent chunk sets q=1 so should not respond + { + const cmd = try command.Parser.parseString( + alloc, + "m=0,q=1;////", + ); + defer cmd.deinit(alloc); + const resp = execute(alloc, &t, &cmd); + try testing.expect(resp == null); + } +} diff --git a/src/terminal/kitty/graphics_image.zig b/src/terminal/kitty/graphics_image.zig index 5adac9128..931d068f9 100644 --- a/src/terminal/kitty/graphics_image.zig +++ b/src/terminal/kitty/graphics_image.zig @@ -36,10 +36,14 @@ pub const LoadingImage = struct { /// so that we display the image after it is fully loaded. display: ?command.Display = null, + /// Quiet is the quiet settings for the initial load command. This is + /// used if q isn't set on subsequent chunks. + quiet: command.Command.Quiet, + /// Initialize a chunked immage from the first image transmission. /// If this is a multi-chunk image, this should only be the FIRST /// chunk. - pub fn init(alloc: Allocator, cmd: *command.Command) !LoadingImage { + pub fn init(alloc: Allocator, cmd: *const command.Command) !LoadingImage { // Build our initial image from the properties sent via the control. // These can be overwritten by the data loading process. For example, // PNG loading sets the width/height from the data. @@ -55,6 +59,7 @@ pub const LoadingImage = struct { }, .display = cmd.display(), + .quiet = cmd.quiet, }; // Special case for the direct medium, we just add the chunk directly.