From c5a3c7e2e5b4e39ce59c14cb35c55be971058575 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 18 Aug 2026 10:15:38 -0700 Subject: [PATCH] terminal/kitty: constrain placement offsets to cell bounds Clamp X and Y offsets when a placement is created and normalize them again against current cell geometry when sizing and rendering. The protocol requires offsets to remain within the first cell and not enlarge explicit c/r rectangles: https://sw.kovidgoyal.net/kitty/graphics-protocol/#controlling-displayed-image-layout Previously, explicit placements were sized to the full c/r rectangle before the offset was applied. This extended their far edge into neighboring cells and let unbounded offsets reach renderer geometry. --- src/renderer/image.zig | 5 +- src/terminal/kitty/graphics_exec.zig | 60 +++++++++++--- src/terminal/kitty/graphics_storage.zig | 106 +++++++++++++++++++++--- 3 files changed, 148 insertions(+), 23 deletions(-) diff --git a/src/renderer/image.zig b/src/renderer/image.zig index 65a11bd70..8aa755e0b 100644 --- a/src/renderer/image.zig +++ b/src/renderer/image.zig @@ -438,6 +438,7 @@ pub const State = struct { // Calculate the dimensions of our image, taking in to // account the rows / columns specified by the placement. const dest_size = p.pixelSize(image.*, t); + const cell_offset = p.cellOffset(t); const source = p.sourceRect(image.*); @@ -453,8 +454,8 @@ pub const State = struct { .z = p.z, .width = dest_size.width, .height = dest_size.height, - .cell_offset_x = p.x_offset, - .cell_offset_y = p.y_offset, + .cell_offset_x = cell_offset.x, + .cell_offset_y = cell_offset.y, .source_x = source.x, .source_y = source.y, .source_width = source.width, diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index c0cd36549..ad314a4d7 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -259,17 +259,25 @@ fn display( }; // Add the placement - const p: ImageStorage.Placement = .{ - .location = location, - .x_offset = d.x_offset, - .y_offset = d.y_offset, - .source_x = d.x, - .source_y = d.y, - .source_width = d.width, - .source_height = d.height, - .columns = d.columns, - .rows = d.rows, - .z = d.z, + const p: ImageStorage.Placement = placement: { + var p: ImageStorage.Placement = .{ + .location = location, + .x_offset = d.x_offset, + .y_offset = d.y_offset, + .source_x = d.x, + .source_y = d.y, + .source_width = d.width, + .source_height = d.height, + .columns = d.columns, + .rows = d.rows, + .z = d.z, + }; + + const cell_offset = p.cellOffset(terminal); + if (terminal.width_px / terminal.cols > 0) p.x_offset = cell_offset.x; + if (terminal.height_px / terminal.rows > 0) p.y_offset = cell_offset.y; + + break :placement p; }; storage.addPlacement( io, @@ -916,6 +924,36 @@ test "kittygfx delete then retransmit same id gets fresh generation" { try testing.expect(gen2 > gen_delete); } +test "kittygfx display clamps cell offsets" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + + var t = try Terminal.init(io, alloc, .{ .rows = 5, .cols = 5 }); + defer t.deinit(alloc); + t.width_px = 50; // 10 px per col + t.height_px = 100; // 20 px per row + + const cmd = try command.Parser.parseString( + alloc, + "a=T,t=d,f=24,i=1,s=1,v=1,c=2,r=1,X=99,Y=99,C=1;AAAA", + ); + defer cmd.deinit(alloc); + + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + + const storage = &t.screens.active.kitty_images; + var it = storage.placements.iterator(); + const placement = it.next().?.value_ptr; + try testing.expectEqual(@as(u32, 9), placement.x_offset); + try testing.expectEqual(@as(u32, 19), placement.y_offset); + + const actual = placement.pixelSize(storage.imageById(1).?, &t); + try testing.expectEqual(@as(u32, 11), actual.width); + try testing.expectEqual(@as(u32, 1), actual.height); +} + test "kittygfx placement bounds cursor movement for untrusted dimensions" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/kitty/graphics_storage.zig b/src/terminal/kitty/graphics_storage.zig index 65259c8ed..5170a90b2 100644 --- a/src/terminal/kitty/graphics_storage.zig +++ b/src/terminal/kitty/graphics_storage.zig @@ -921,6 +921,31 @@ pub const ImageStorage = struct { }; } + /// Returns the placement offset clamped within the first cell. Pixel + /// geometry may temporarily be unavailable, in which case there is no + /// valid offset within the cell. + pub fn cellOffset( + self: Placement, + t: *const terminal.Terminal, + ) struct { + x: u32, + y: u32, + } { + const cell_width: u32 = t.width_px / t.cols; + const cell_height: u32 = t.height_px / t.rows; + + return .{ + .x = if (cell_width > 0) + @min(self.x_offset, cell_width - 1) + else + 0, + .y = if (cell_height > 0) + @min(self.y_offset, cell_height - 1) + else + 0, + }; + } + /// Returns the size of this placement's image in pixels, /// taking into account the source rectangle, specified /// rows/columns, and aspect ratio. @@ -951,6 +976,7 @@ pub const ImageStorage = struct { // count and the height by the row count, because it should be. const cell_width: u32 = t.width_px / t.cols; const cell_height: u32 = t.height_px / t.rows; + const cell_offset = self.cellOffset(t); // If we have a specified cols AND rows then we calculate // the width and height from them directly, we don't need @@ -960,8 +986,8 @@ pub const ImageStorage = struct { const calc_height = saturatingMul(cell_height, self.rows); return .{ - .width = calc_width, - .height = calc_height, + .width = calc_width -| cell_offset.x, + .height = calc_height -| cell_offset.y, }; } @@ -971,7 +997,10 @@ pub const ImageStorage = struct { // If only the columns were specified, we determine // the height of the image based on the aspect ratio. if (self.columns > 0) { - const calc_width = saturatingMul(cell_width, self.columns); + const calc_width = saturatingMul( + cell_width, + self.columns, + ) -| cell_offset.x; const calc_height = scaleDimension(calc_width, height, width); return .{ @@ -983,7 +1012,10 @@ pub const ImageStorage = struct { // Otherwise, only the rows were specified, so we // determine the width based on the aspect ratio. { - const calc_height = saturatingMul(cell_height, self.rows); + const calc_height = saturatingMul( + cell_height, + self.rows, + ) -| cell_offset.y; const calc_width = scaleDimension(calc_height, width, height); return .{ @@ -1011,15 +1043,16 @@ pub const ImageStorage = struct { // Otherwise we calculate the pixel size, divide by // cell size, and round up to the nearest integer. const calc_size = self.pixelSize(image, t); + const cell_offset = self.cellOffset(t); return .{ .cols = std.math.divCeil( u32, - calc_size.width +| self.x_offset, + calc_size.width +| cell_offset.x, t.width_px / t.cols, ) catch 0, .rows = std.math.divCeil( u32, - calc_size.height +| self.y_offset, + calc_size.height +| cell_offset.y, t.height_px / t.rows, ) catch 0, }; @@ -1760,6 +1793,59 @@ test "storage: delete images by range 4" { try testing.expectEqual(tracked + 2, t.screens.active.pages.countTrackedPins()); } +test "storage: cell offsets stay within explicit destination rectangle" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + + var t = try terminal.Terminal.init(io, alloc, .{ .cols = 5, .rows = 5 }); + defer t.deinit(alloc); + t.width_px = 50; // 10 px per col + t.height_px = 100; // 20 px per row + + // Explicit columns and rows describe the far cell boundaries. Offsets + // move the near edge inward without moving those far edges. + const placement: ImageStorage.Placement = .{ + .location = .{ .virtual = {} }, + .x_offset = 3, + .y_offset = 4, + .columns = 2, + .rows = 1, + }; + const actual = placement.pixelSize(.{ .width = 4, .height = 3 }, &t); + try testing.expectEqual(@as(u32, 17), actual.width); + try testing.expectEqual(@as(u32, 16), actual.height); + try testing.expectEqual(@as(u32, 2), placement.gridSize(.{ .width = 4, .height = 3 }, &t).cols); + try testing.expectEqual(@as(u32, 1), placement.gridSize(.{ .width = 4, .height = 3 }, &t).rows); +} + +test "storage: cell offsets clamp to cell bounds" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + + var t = try terminal.Terminal.init(io, alloc, .{ .cols = 5, .rows = 5 }); + defer t.deinit(alloc); + t.width_px = 50; // 10 px per col + t.height_px = 100; // 20 px per row + + const placement: ImageStorage.Placement = .{ + .location = .{ .virtual = {} }, + .x_offset = std.math.maxInt(u32), + .y_offset = std.math.maxInt(u32), + .columns = 1, + .rows = 1, + }; + const offset = placement.cellOffset(&t); + try testing.expectEqual(@as(u32, 9), offset.x); + try testing.expectEqual(@as(u32, 19), offset.y); + + // Even hostile offsets leave one pixel inside the requested cell. + const actual = placement.pixelSize(.{ .width = 1, .height = 1 }, &t); + try testing.expectEqual(@as(u32, 1), actual.width); + try testing.expectEqual(@as(u32, 1), actual.height); +} + test "storage: aspect ratio calculation when only columns or rows specified" { const testing = std.testing; const alloc = testing.allocator; @@ -1898,8 +1984,8 @@ test "storage: placement geometry handles untrusted dimensions" { try testing.expectEqual(max, actual.height); } - // Pixel offsets are protocol-controlled too. Include them without - // allowing the grid-size numerator to wrap. + // Pixel offsets are protocol-controlled too. Clamp them to the cell + // before including them in grid geometry. t.width_px = 2; t.height_px = 2; { @@ -1909,8 +1995,8 @@ test "storage: placement geometry handles untrusted dimensions" { .y_offset = max, }; const actual = placement.gridSize(.{ .width = 1, .height = 1 }, &t); - try testing.expectEqual(max, actual.cols); - try testing.expectEqual(max, actual.rows); + try testing.expectEqual(@as(u32, 1), actual.cols); + try testing.expectEqual(@as(u32, 1), actual.rows); } const pin = try trackPin(&t, .{ .x = 0, .y = 0 });