From 2f31e1b7fa2b324ae10366e0c91f0aa0fbfcce75 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Tue, 10 Dec 2024 14:30:59 -0500 Subject: [PATCH] fix(kittygfx): don't respond to T commands with no i or I If a transmit and display command does not specify an ID or a number, then it should not be responded to. We currently automatically assign IDs in this situation, which isn't ideal since collisions can happen which shouldn't, but this at least fixes the glaring observable issue where transmit-and-display commands yield unexpected OK responses. --- src/terminal/kitty/graphics_exec.zig | 28 ++++++++++++++++++++++--- src/terminal/kitty/graphics_image.zig | 6 ++++++ src/terminal/kitty/graphics_storage.zig | 3 +++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index 42f12ea07..057f28065 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -164,9 +164,9 @@ fn transmit( // If there are more chunks expected we do not respond. if (load.more) return .{}; - // If our image has no ID or number, we don't respond at all. Conversely, - // if we have either an ID or number, we always respond. - if (load.image.id == 0 and load.image.number == 0) return .{}; + // If the loaded image was assigned its ID automatically, not based + // on a number or explicitly specified ID, then we don't respond. + if (load.image.implicit_id) return .{}; // After the image is added, set the ID in case it changed. // The resulting image number and placement ID never change. @@ -335,6 +335,10 @@ fn loadAndAddImage( if (loading.image.id == 0) { loading.image.id = storage.next_image_id; storage.next_image_id +%= 1; + + // If the image also has no number then its auto-ID is "implicit". + // See the doc comment on the Image.implicit_id field for more detail. + if (loading.image.number == 0) loading.image.implicit_id = true; } // If this is chunked, this is the beginning of a new chunked transmission. @@ -529,3 +533,21 @@ test "kittygfx test valid i32 (expect invalid image ID)" { try testing.expect(!resp.ok()); try testing.expectEqual(resp.message, "ENOENT: image not found"); } + +test "kittygfx no response with no image ID or number" { + const testing = std.testing; + const alloc = testing.allocator; + + var t = try Terminal.init(alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + + { + const cmd = try command.Parser.parseString( + alloc, + "a=t,f=24,t=d,s=1,v=2,c=10,r=1,i=0,I=0;////////", + ); + 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 931d068f9..ff498cbb8 100644 --- a/src/terminal/kitty/graphics_image.zig +++ b/src/terminal/kitty/graphics_image.zig @@ -455,6 +455,12 @@ pub const Image = struct { data: []const u8 = "", transmit_time: std.time.Instant = undefined, + /// Set this to true if this image was loaded by a command that + /// doesn't specify an ID or number, since such commands should + /// not be responded to, even though we do currently give them + /// IDs in the public range (which is bad!). + implicit_id: bool = false, + pub const Error = error{ InternalError, InvalidData, diff --git a/src/terminal/kitty/graphics_storage.zig b/src/terminal/kitty/graphics_storage.zig index ee46b2a6c..ffd3aa580 100644 --- a/src/terminal/kitty/graphics_storage.zig +++ b/src/terminal/kitty/graphics_storage.zig @@ -31,6 +31,9 @@ pub const ImageStorage = struct { /// This is the next automatically assigned image ID. We start mid-way /// through the u32 range to avoid collisions with buggy programs. + /// TODO: This isn't good enough, it's perfectly legal for programs + /// to use IDs in the latter half of the range and collisions + /// are not gracefully handled. next_image_id: u32 = 2147483647, /// This is the next automatically assigned placement ID. This is never