From afb61e1b6292f56b863ad0fe61bcdbad47392a5e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 18 Aug 2026 08:41:11 -0700 Subject: [PATCH] terminal/kitty: place cursor after tall images properly Supersedes #13886 This fixes an incompatibility between Ghostty and Kitty 0.47.1+. Previously, Ghostty handled `C=0` by moving down at most one terminal height and then clamping the horizontal destination to the screen. The limit protected against an image command requesting billions of rows, but it counted ordinary movement to the bottom as well as scrolling. Kitty defines `C=0` as leaving the cursor after the image and implements the movement as rows minus one, plus one row when the image reaches the right edge: https://sw.kovidgoyal.net/kitty/graphics-protocol/#controlling-displayed-image-layout https://github.com/kovidgoyal/kitty/blob/0ecb10d158943e971e5254c554c2a58f1fcc79fe/kitty/graphics.c#L1256-L1260 https://github.com/kovidgoyal/kitty/blob/0ecb10d158943e971e5254c554c2a58f1fcc79fe/kitty/screen.c#L1903-L1915 (Had to view Kitty source to verify what the spec meant) For example, an eight-row, full-width image in a five-row terminal needs four moves to reach the bottom and four more to scroll. The old limit allowed only five moves in total, so the next image started four rows inside the first one. Clamping the horizontal destination also left the cursor at the final column instead of wrapping to column one of the next row. Consecutive images could therefore overlap in both directions. --- src/terminal/kitty/graphics_exec.zig | 112 +++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 8 deletions(-) diff --git a/src/terminal/kitty/graphics_exec.zig b/src/terminal/kitty/graphics_exec.zig index 03a0882a6..d05ef7abc 100644 --- a/src/terminal/kitty/graphics_exec.zig +++ b/src/terminal/kitty/graphics_exec.zig @@ -273,22 +273,45 @@ fn display( .none => {}, .after => { // We use terminal.index to properly handle scroll regions. + const screen = terminal.screens.active; const size = p.gridSize(img, terminal); - // Once the requested movement leaves the screen, its exact - // position is undefined by the Kitty graphics protocol. Bound - // the work so an untrusted row count can't make us spin. + const target_x = @as(usize, pin.x) +| @as(usize, size.cols); + const wraps = target_x >= @as(usize, terminal.cols); + const requested_rows = + (@as(usize, size.rows) -| 1) +| @intFromBool(wraps); + + // The requested row count comes from the application and can + // be as large as u32. Calling terminal.index once for every + // row could therefore leave the terminal unresponsive for a + // very long time. + // + // First allow enough calls to reach the bottom of the scroll + // region. Each call after that scrolls the region by one row, + // so limit those extra calls to one screen. This follows + // Kitty's behavior while keeping the amount of work bounded. + const region = terminal.scrolling_region; + const rows_before_scroll: usize = if (screen.cursor.y >= region.top and + screen.cursor.y <= region.bottom and + screen.cursor.x >= region.left and + screen.cursor.x <= region.right) + @as(usize, region.bottom - screen.cursor.y) + else + 0; const rows_to_move: usize = @min( - @as(usize, size.rows), - @as(usize, terminal.rows), + requested_rows, + rows_before_scroll +| @as(usize, terminal.rows), ); for (0..rows_to_move) |_| terminal.index() catch |err| { log.warn("failed to move cursor: {}", .{err}); break; }; - terminal.setCursorPos( - terminal.screens.active.cursor.y, - @as(usize, pin.x) +| @as(usize, size.cols) +| 1, + // Kitty wraps once when the movement reaches the right edge; + // otherwise it leaves the cursor immediately to the right of + // the placement. + screen.cursor.pending_wrap = false; + screen.cursorHorizontalAbsolute( + if (wraps) 0 else @intCast(target_x), ); }, }, @@ -779,4 +802,77 @@ test "kittygfx placement bounds cursor movement for untrusted dimensions" { const resp = execute(io, alloc, &t, &cmd).?; try testing.expect(resp.ok()); try testing.expectEqual(@as(usize, 1), t.screens.active.kitty_images.placements.count()); + // Reaching the bottom takes four rows, then scrolling is capped to one + // screen (five rows), matching Kitty's bounded screen-scroll behavior. + try testing.expectEqual(@as(usize, 10), t.screens.active.pages.scrollbar().total); +} + +test "kittygfx placement moves cursor past a tall image" { + 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); + + // Load a one-pixel RGB image, then place it over the full width and eight + // rows. Eight rows are taller than the screen but still within Kitty's + // scroll budget. + { + const cmd = try command.Parser.parseString( + alloc, + "a=t,t=d,f=24,i=1,s=1,v=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=1,c=5,r=8", + ); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + + // A subsequent placement begins immediately after the first one instead + // of overlapping it at the old one-screen movement cap. + { + const cmd = try command.Parser.parseString( + alloc, + "a=p,i=1,p=2,C=1", + ); + defer cmd.deinit(alloc); + const resp = execute(io, alloc, &t, &cmd).?; + try testing.expect(resp.ok()); + } + + const storage = &t.screens.active.kitty_images; + const first = storage.placements.get(.{ + .image_id = 1, + .placement_id = .{ .tag = .external, .id = 1 }, + }).?; + const second = storage.placements.get(.{ + .image_id = 1, + .placement_id = .{ .tag = .external, .id = 2 }, + }).?; + const first_pin = switch (first.location) { + .pin => |pin| pin, + .virtual => unreachable, + }; + const second_pin = switch (second.location) { + .pin => |pin| pin, + .virtual => unreachable, + }; + const first_y = t.screens.active.pages.pointFromPin( + .screen, + first_pin.*, + ).?.screen.y; + const second_y = t.screens.active.pages.pointFromPin( + .screen, + second_pin.*, + ).?.screen.y; + try testing.expectEqual(first_y + 8, second_y); }