diff --git a/src/renderer/image.zig b/src/renderer/image.zig index dc08b8432..0126d1c3a 100644 --- a/src/renderer/image.zig +++ b/src/renderer/image.zig @@ -976,7 +976,7 @@ pub const Image = union(enum) { } }; -test "kitty renderer ignores pending payloads and retains native placements" { +test "kitty renderer ignores pending payloads and removes replaced placements" { const testing = std.testing; const alloc = testing.allocator; const io = testing.io; @@ -990,6 +990,7 @@ test "kitty renderer ignores pending payloads and retains native placements" { defer state.deinit(alloc); const storage = &t.screens.active.kitty_images; + const tracked = t.screens.active.pages.countTrackedPins(); const pending = try storage.addPendingImage(io, alloc, t.screens.active, .{ .id = 1, .width = 1, @@ -1020,8 +1021,8 @@ test "kitty renderer ignores pending payloads and retains native placements" { state.images.get(.{ .kitty = 1 }).?.generation, ); - // A newer pending replacement clears the renderer placement and marks - // the copied texture data for unload without deleting the native pin. + // A newer pending replacement deletes the native placement and marks the + // copied texture data for unload. _ = try storage.addPendingImage(io, alloc, t.screens.active, .{ .id = 1, .width = 1, @@ -1030,7 +1031,8 @@ test "kitty renderer ignores pending payloads and retains native placements" { .data = .{ .pending = 4 }, }); state.kittyUpdate(alloc, &t, .{ .width = 10, .height = 10 }); - try testing.expectEqual(@as(usize, 1), storage.placements.count()); + try testing.expectEqual(@as(usize, 0), storage.placements.count()); + try testing.expectEqual(tracked, t.screens.active.pages.countTrackedPins()); try testing.expectEqual(@as(usize, 0), state.kitty_placements.items.len); try testing.expect(state.images.get(.{ .kitty = 1 }).?.image.isUnloading()); } diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index 58fa4d5c4..03a0882a6 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -636,6 +636,86 @@ test "kittygfx retransmit same id gets fresh image generation" { try testing.expectEqual(gen2, storage.generation); } +test "kittygfx retransmit same id removes existing placements" { + const testing = std.testing; + const io = testing.io; + const alloc = testing.allocator; + + var t = try Terminal.init(io, alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + const storage = &t.screens.active.kitty_images; + const tracked = t.screens.active.pages.countTrackedPins(); + + // Transmit and display an image, then add anonymous and named placements. + // Multiple anonymous a=p placements for one image are explicitly valid. + { + const cmd = try command.Parser.parseString( + alloc, + "a=T,t=d,f=24,i=1,s=1,v=2,C=1;////////", + ); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + { + const cmd = try command.Parser.parseString(alloc, "a=p,i=1,C=1"); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + { + const cmd = try command.Parser.parseString(alloc, "a=p,i=1,p=7,C=1"); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + try testing.expectEqual(@as(usize, 3), storage.placements.count()); + try testing.expectEqual( + tracked + 3, + t.screens.active.pages.countTrackedPins(), + ); + + // Retransmitting replaces the image and must delete every old placement. + // Plain a=t creates no replacement placement of its own. + { + const cmd = try command.Parser.parseString( + alloc, + "a=t,t=d,f=24,i=1,s=1,v=2;AAAAAAAA", + ); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + try testing.expectEqual(@as(usize, 0), storage.placements.count()); + try testing.expectEqual(tracked, t.screens.active.pages.countTrackedPins()); + + // a=T creates one new placement after deleting the previous image and + // placements, so repeated redraws remain bounded at one placement. + { + const cmd = try command.Parser.parseString( + alloc, + "a=T,t=d,f=24,i=1,s=1,v=2,C=1;////////", + ); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + { + const cmd = try command.Parser.parseString( + alloc, + "a=T,t=d,f=24,i=1,s=1,v=2,C=1;AAAAAAAA", + ); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + try testing.expectEqual(@as(usize, 1), storage.placements.count()); + try testing.expectEqual( + tracked + 1, + t.screens.active.pages.countTrackedPins(), + ); +} + test "kittygfx delete then retransmit same id gets fresh generation" { const testing = std.testing; const io = testing.io; diff --git a/src/terminal/kitty/graphics_storage.zig b/src/terminal/kitty/graphics_storage.zig index 29a018e24..fe97b1509 100644 --- a/src/terminal/kitty/graphics_storage.zig +++ b/src/terminal/kitty/graphics_storage.zig @@ -246,7 +246,7 @@ pub const ImageStorage = struct { // Credit an existing image's reservation before calculating the // replacement size. In particular, a completed live transmission // replacing pending snapshot metadata must be able to reuse the - // reservation without evicting its own ID and placements. + // reservation without evicting its own ID. const old_len = if (self.images.get(img.id)) |old| old.data.len() else @@ -274,16 +274,16 @@ pub const ImageStorage = struct { log.debug("addImage image={}", .{img.withoutData()}); - // Write our new image, preserving placements across replacement. - const placement_count = if (gop.found_existing) count: { + // Retransmitting a specific image ID replaces the old image and all + // of its placements, as required by the Kitty graphics protocol. + if (gop.found_existing) { + self.removePlacementsByImageId(s, img.id); self.total_bytes -= gop.value_ptr.data.len(); - const count = gop.value_ptr.metadata.placement_count; gop.value_ptr.deinit(alloc); - break :count count; - } else 0; + } gop.value_ptr.* = img; - gop.value_ptr.metadata.placement_count = placement_count; + gop.value_ptr.metadata.placement_count = 0; self.total_bytes += new_len; // Stamp the stored image with a fresh generation. This gives @@ -404,6 +404,19 @@ pub const ImageStorage = struct { self.placements.clearRetainingCapacity(); } + fn removePlacementsByImageId( + self: *ImageStorage, + s: *terminal.Screen, + image_id: u32, + ) void { + var it = self.placements.iterator(); + while (it.next()) |entry| { + if (entry.key_ptr.image_id != image_id) continue; + entry.value_ptr.deinit(s); + self.removePlacementByPtr(entry.key_ptr); + } + } + fn removePlacementByPtr(self: *ImageStorage, key: *PlacementKey) void { const img = self.images.getPtr(key.image_id).?; assert(img.metadata.placement_count > 0); @@ -2217,7 +2230,7 @@ test "storage: stale pending completion loses to delete replacement and eviction defer alloc.free(evicted_data); } -test "storage: replacement reuses pending reservation and preserves placements" { +test "storage: replacement reuses pending reservation and removes placements" { const testing = std.testing; const io = testing.io; const alloc = testing.allocator; @@ -2226,6 +2239,7 @@ test "storage: replacement reuses pending reservation and preserves placements" var s: ImageStorage = .{ .total_limit = 12 }; defer s.deinit(alloc, t.screens.active); + const tracked = t.screens.active.pages.countTrackedPins(); const pending = try s.addPendingImage(io, alloc, t.screens.active, .{ .id = 1, @@ -2251,7 +2265,12 @@ test "storage: replacement reuses pending reservation and preserves placements" }); try testing.expect(s.images.contains(1)); try testing.expect(s.images.contains(2)); - try testing.expectEqual(@as(usize, 1), s.placements.count()); + try testing.expectEqual(@as(usize, 0), s.placements.count()); + try testing.expectEqual(tracked, t.screens.active.pages.countTrackedPins()); + try testing.expectEqual( + @as(u30, 0), + s.imageById(1).?.metadata.placement_count, + ); try testing.expectEqual(@as(usize, 12), s.total_bytes); const stale = try alloc.dupe(u8, "snapshot"); @@ -2259,18 +2278,17 @@ test "storage: replacement reuses pending reservation and preserves placements" try testing.expect(!stale_completed); defer alloc.free(stale); - // Growing the replacement requires eviction, but the replacement ID and - // its placement are excluded. The other image supplies the needed bytes. + // Growing the replacement requires eviction, but the replacement ID is + // excluded. The other image supplies the needed bytes. try s.addImage(io, alloc, t.screens.active, .{ .id = 1, .data = .{ .complete = try alloc.dupe(u8, "1234567890") }, }); try testing.expect(s.images.contains(1)); try testing.expect(!s.images.contains(2)); - try testing.expectEqual(@as(usize, 1), s.placements.count()); + try testing.expectEqual(@as(usize, 0), s.placements.count()); try testing.expectEqual(@as(usize, 10), s.total_bytes); - // The placement remains a live reference across both replacements. s.delete(io, alloc, &t, .{ .id = .{ .delete = true, .image_id = 1 } }); try testing.expectEqual(@as(usize, 0), s.images.count()); try testing.expectEqual(@as(usize, 0), s.placements.count());