fix(kittygfx): don't respond to transmit commands with no i or I (#2920)

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.

This is a band-aid fix for #2197, and is actually more or less the fix
suggested in #2195, but it's worth having until the architecture of the
kitty image storage is reworked properly. I included multiple comments
as reminders that the underlying issue needs to be fixed.

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);
+    }
+}

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,

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