From bf1ca59196d8dec6bd3953fa42eea0d544f02dcf Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Fri, 16 Jan 2026 09:28:19 -0500 Subject: [PATCH 01/37] shellcheck: move common directives to .shellcheckrc This simplifies our CI command line and makes it easier to document expected usage (in HACKING.md). There unfortunately isn't a way to set --checked-sourced or our default warning level in .shellcheckrc, and our `find` command is still a bit unwieldy, but this is still a net improvement. --- .github/workflows/test.yml | 2 -- .shellcheckrc | 8 ++++++++ HACKING.md | 22 ++++++++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 .shellcheckrc diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a9adcfbc2..fbac22a47 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -978,8 +978,6 @@ jobs: --check-sourced \ --color=always \ --severity=warning \ - --shell=bash \ - --external-sources \ $(find . \( -name "*.sh" -o -name "*.bash" \) -type f ! -path "./zig-out/*" ! -path "./macos/build/*" ! -path "./.git/*" | sort) translations: diff --git a/.shellcheckrc b/.shellcheckrc new file mode 100644 index 000000000..919cc175d --- /dev/null +++ b/.shellcheckrc @@ -0,0 +1,8 @@ +# ShellCheck +# https://github.com/koalaman/shellcheck/wiki/Directive#shellcheckrc-file + +# Allow opening any 'source'd file, even if not specified as input +external-sources=true + +# Assume bash by default +shell=bash diff --git a/HACKING.md b/HACKING.md index bde50ec99..0abb3a2d8 100644 --- a/HACKING.md +++ b/HACKING.md @@ -164,6 +164,28 @@ alejandra . Make sure your Alejandra version matches the version of Alejandra in [devShell.nix](https://github.com/ghostty-org/ghostty/blob/main/nix/devShell.nix). +### ShellCheck + +Bash scripts are checked with [ShellCheck](https://www.shellcheck.net/) in CI. + +Nix users can use the following command to run ShellCheck over all of our scripts: + +``` +nix develop -c shellcheck \ + --check-sourced \ + --severity=warning \ + $(find . \( -name "*.sh" -o -name "*.bash" \) -type f ! -path "./zig-out/*" ! -path "./macos/build/*" ! -path "./.git/*" | sort) +``` + +Non-Nix users can [install ShellCheck](https://github.com/koalaman/shellcheck#user-content-installing) and then run: + +``` +shellcheck \ + --check-sourced \ + --severity=warning \ + $(find . \( -name "*.sh" -o -name "*.bash" \) -type f ! -path "./zig-out/*" ! -path "./macos/build/*" ! -path "./.git/*" | sort) +``` + ### Updating the Zig Cache Fixed-Output Derivation Hash The Nix package depends on a [fixed-output From f1dbdc796564c0d73c97808c1bfd87ac9d29fc5e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 12:46:34 -0800 Subject: [PATCH 02/37] terminal: fix stale cursor pin usage after cursorChangePin Fixes #10282 The function `cursorChangePin` is supposed to be called anytime the cursor page pin changes, but it itself may alter the page pin if setting up the underlying managed memory forces a page size adjustment. Multiple callers to this function were erroneously reusing the old page pin value. --- src/terminal/Screen.zig | 147 ++++++++++++++++++++++++++++++++-------- 1 file changed, 117 insertions(+), 30 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 2861e02e5..fed0a8c51 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -640,9 +640,8 @@ pub fn cursorUp(self: *Screen, n: size.CellCountInt) void { defer self.assertIntegrity(); self.cursor.y -= n; // Must be set before cursorChangePin - const page_pin = self.cursor.page_pin.up(n).?; - self.cursorChangePin(page_pin); - const page_rac = page_pin.rowAndCell(); + self.cursorChangePin(self.cursor.page_pin.up(n).?); + const page_rac = self.cursor.page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; } @@ -667,9 +666,8 @@ pub fn cursorDown(self: *Screen, n: size.CellCountInt) void { // We move the offset into our page list to the next row and then // get the pointers to the row/cell and set all the cursor state up. - const page_pin = self.cursor.page_pin.down(n).?; - self.cursorChangePin(page_pin); - const page_rac = page_pin.rowAndCell(); + self.cursorChangePin(self.cursor.page_pin.down(n).?); + const page_rac = self.cursor.page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; } @@ -800,31 +798,37 @@ pub fn cursorDownScroll(self: *Screen) !void { // allocate, prune scrollback, whatever. _ = try self.pages.grow(); - // If our pin page change it means that the page that the pin - // was on was pruned. In this case, grow() moves the pin to - // the top-left of the new page. This effectively moves it by - // one already, we just need to fix up the x value. - const page_pin = if (old_pin.node == self.cursor.page_pin.node) - self.cursor.page_pin.down(1).? - else reuse: { - var pin = self.cursor.page_pin.*; - pin.x = self.cursor.x; - break :reuse pin; - }; + self.cursorChangePin(new_pin: { + // We do this all in a block here because referencing this pin + // after cursorChangePin is unsafe, and we want to keep it out + // of scope. - // These assertions help catch some pagelist math errors. Our - // x/y should be unchanged after the grow. - if (build_options.slow_runtime_safety) { - const active = self.pages.pointFromPin( - .active, - page_pin, - ).?.active; - assert(active.x == self.cursor.x); - assert(active.y == self.cursor.y); - } + // If our pin page change it means that the page that the pin + // was on was pruned. In this case, grow() moves the pin to + // the top-left of the new page. This effectively moves it by + // one already, we just need to fix up the x value. + const page_pin = if (old_pin.node == self.cursor.page_pin.node) + self.cursor.page_pin.down(1).? + else reuse: { + var pin = self.cursor.page_pin.*; + pin.x = self.cursor.x; + break :reuse pin; + }; - self.cursorChangePin(page_pin); - const page_rac = page_pin.rowAndCell(); + // These assertions help catch some pagelist math errors. Our + // x/y should be unchanged after the grow. + if (build_options.slow_runtime_safety) { + const active = self.pages.pointFromPin( + .active, + page_pin, + ).?.active; + assert(active.x == self.cursor.x); + assert(active.y == self.cursor.y); + } + + break :new_pin page_pin; + }); + const page_rac = self.cursor.page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; @@ -834,7 +838,7 @@ pub fn cursorDownScroll(self: *Screen) !void { // Clear the new row so it gets our bg color. We only do this // if we have a bg color at all. if (self.cursor.style.bg_color != .none) { - const page: *Page = &page_pin.node.data; + const page: *Page = &self.cursor.page_pin.node.data; self.clearCells( page, self.cursor.page_row, @@ -1081,6 +1085,11 @@ pub fn cursorCopy(self: *Screen, other: Cursor, opts: struct { /// page than the old AND we have a style or hyperlink set. In that case, /// we must release our old one and insert the new one, since styles are /// stored per-page. +/// +/// Note that this can change the cursor pin AGAIN if the process of +/// setting up our cursor forces a capacity adjustment of the underlying +/// cursor page, so any references to the page pin should be re-read +/// from `self.cursor.page_pin` after calling this. inline fn cursorChangePin(self: *Screen, new: Pin) void { // Moving the cursor affects text run splitting (ligatures) so // we must mark the old and new page dirty. We do this as long @@ -9016,3 +9025,81 @@ test "Screen: adjustCapacity cursor style exceeds style set capacity" { try testing.expect(s.cursor.style.default()); try testing.expectEqual(style.default_id, s.cursor.style_id); } + +test "Screen: cursorDown to page with insufficient capacity" { + // Regression test for https://github.com/ghostty-org/ghostty/issues/10282 + // + // This test exposes a use-after-realloc bug in cursorDown (and similar + // cursor movement functions). The bug pattern: + // + // 1. cursorDown creates a by-value copy of the pin via page_pin.down(n) + // 2. cursorChangePin is called, which may trigger adjustCapacity + // if the target page's style map is full + // 3. adjustCapacity frees the old page and creates a new one + // 4. The local pin copy still points to the freed page + // 5. rowAndCell() on the stale pin accesses freed memory + + const testing = std.testing; + const alloc = testing.allocator; + + // Small screen to make page boundary crossing easy to set up + var s = try init(alloc, .{ .cols = 10, .rows = 3, .max_scrollback = 1 }); + defer s.deinit(); + + // Scroll down enough to create a second page + const start_page = &s.pages.pages.last.?.data; + const rem = start_page.capacity.rows; + start_page.pauseIntegrityChecks(true); + for (0..rem) |_| try s.cursorDownOrScroll(); + start_page.pauseIntegrityChecks(false); + + // Cursor should now be on a new page + const new_page = &s.cursor.page_pin.node.data; + try testing.expect(start_page != new_page); + + // Fill new_page's style map to capacity. When we move INTO this page + // with a style set, adjustCapacity will be triggered. + { + new_page.pauseIntegrityChecks(true); + defer new_page.pauseIntegrityChecks(false); + defer new_page.assertIntegrity(); + + var n: u24 = 1; + while (new_page.styles.add( + new_page.memory, + .{ .bg_color = .{ .rgb = @bitCast(n) } }, + )) |_| n += 1 else |_| {} + } + + // Move cursor to start of active area and set a style + s.cursorAbsolute(0, 0); + try s.setAttribute(.bold); + try testing.expect(s.cursor.style.flags.bold); + try testing.expect(s.cursor.style_id != style.default_id); + + // Find the row just before the page boundary + for (0..s.pages.rows - 1) |row| { + s.cursorAbsolute(0, @intCast(row)); + const cur_node = s.cursor.page_pin.node; + if (s.cursor.page_pin.down(1)) |next_pin| { + if (next_pin.node != cur_node) { + // Cursor is at 'row', moving down crosses to new_page + try testing.expect(&next_pin.node.data == new_page); + + // This cursorDown triggers the bug: the local page_pin copy + // becomes stale after adjustCapacity, causing rowAndCell() + // to access freed memory. + s.cursorDown(1); + + // If the fix is applied, verify correct state + try testing.expect(s.cursor.y == row + 1); + try testing.expect(s.cursor.style.flags.bold); + + break; + } + } + } else { + // Didn't find boundary + try testing.expect(false); + } +} From 95a23f756d0160f814f1043b7b2ebe96f0941482 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 13 Jan 2026 13:09:41 -0800 Subject: [PATCH 03/37] terminal: more strict sizing for page capacities, max cap can fit 64-bit --- src/terminal/PageList.zig | 24 ++++++++++-------------- src/terminal/hyperlink.zig | 5 +++-- src/terminal/page.zig | 34 +++++++++++++++++++++++++++++----- src/terminal/size.zig | 27 +++++++++++++++++++++++++-- src/terminal/style.zig | 2 +- 5 files changed, 68 insertions(+), 24 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index fc680b971..4592b3db6 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1318,7 +1318,7 @@ const ReflowCursor = struct { // Grow our capacity until we can // definitely fit the extra bytes. const required = cps.len * @sizeOf(u21); - var new_grapheme_capacity: usize = cap.grapheme_bytes; + var new_grapheme_capacity: size.GraphemeBytesInt = cap.grapheme_bytes; while (new_grapheme_capacity - cap.grapheme_bytes < required) { new_grapheme_capacity *= 2; } @@ -1362,7 +1362,7 @@ const ReflowCursor = struct { } else |_| { // Grow our capacity until we can // definitely fit the extra bytes. - var new_string_capacity: usize = cap.string_bytes; + var new_string_capacity: size.StringBytesInt = cap.string_bytes; while (new_string_capacity - cap.string_bytes < additional_required_string_capacity) { new_string_capacity *= 2; } @@ -2647,16 +2647,16 @@ pub const AdjustCapacity = struct { /// Adjust the number of styles in the page. This may be /// rounded up if necessary to fit alignment requirements, /// but it will never be rounded down. - styles: ?usize = null, + styles: ?size.StyleCountInt = null, /// Adjust the number of available grapheme bytes in the page. - grapheme_bytes: ?usize = null, + grapheme_bytes: ?size.GraphemeBytesInt = null, /// Adjust the number of available hyperlink bytes in the page. - hyperlink_bytes: ?usize = null, + hyperlink_bytes: ?size.HyperlinkCountInt = null, /// Adjust the number of available string bytes in the page. - string_bytes: ?usize = null, + string_bytes: ?size.StringBytesInt = null, }; pub const AdjustCapacityError = Allocator.Error || Page.CloneFromError; @@ -2692,23 +2692,19 @@ pub fn adjustCapacity( // All ceilPowerOfTwo is unreachable because we're always same or less // bit width so maxInt is always possible. if (adjustment.styles) |v| { - comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); - const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; + const aligned = std.math.ceilPowerOfTwo(size.StyleCountInt, v) catch unreachable; cap.styles = @max(cap.styles, aligned); } if (adjustment.grapheme_bytes) |v| { - comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); - const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; + const aligned = std.math.ceilPowerOfTwo(size.GraphemeBytesInt, v) catch unreachable; cap.grapheme_bytes = @max(cap.grapheme_bytes, aligned); } if (adjustment.hyperlink_bytes) |v| { - comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); - const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; + const aligned = std.math.ceilPowerOfTwo(size.HyperlinkCountInt, v) catch unreachable; cap.hyperlink_bytes = @max(cap.hyperlink_bytes, aligned); } if (adjustment.string_bytes) |v| { - comptime assert(@bitSizeOf(@TypeOf(v)) <= @bitSizeOf(usize)); - const aligned = std.math.ceilPowerOfTwo(usize, v) catch unreachable; + const aligned = std.math.ceilPowerOfTwo(size.StringBytesInt, v) catch unreachable; cap.string_bytes = @max(cap.string_bytes, aligned); } diff --git a/src/terminal/hyperlink.zig b/src/terminal/hyperlink.zig index 975e6f30e..94f86466c 100644 --- a/src/terminal/hyperlink.zig +++ b/src/terminal/hyperlink.zig @@ -13,8 +13,9 @@ const autoHash = std.hash.autoHash; const autoHashStrat = std.hash.autoHashStrat; /// The unique identifier for a hyperlink. This is at most the number of cells -/// that can fit in a single terminal page. -pub const Id = size.CellCountInt; +/// that can fit in a single terminal page, since each cell can only contain +/// at most one hyperlink. +pub const Id = size.HyperlinkCountInt; // The mapping of cell to hyperlink. We use an offset hash map to save space // since its very unlikely a cell is a hyperlink, so its a waste to store diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 848123405..559f536f1 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -1569,7 +1569,10 @@ pub const Page = struct { const grapheme_alloc_start = alignForward(usize, styles_end, GraphemeAlloc.base_align.toByteUnits()); const grapheme_alloc_end = grapheme_alloc_start + grapheme_alloc_layout.total_size; - const grapheme_count = @divFloor(cap.grapheme_bytes, grapheme_chunk); + const grapheme_count = std.math.ceilPowerOfTwo( + usize, + @divFloor(cap.grapheme_bytes, grapheme_chunk), + ) catch unreachable; const grapheme_map_layout = GraphemeMap.layout(@intCast(grapheme_count)); const grapheme_map_start = alignForward(usize, grapheme_alloc_end, GraphemeMap.base_align.toByteUnits()); const grapheme_map_end = grapheme_map_start + grapheme_map_layout.total_size; @@ -1639,25 +1642,33 @@ pub const Size = struct { }; /// Capacity of this page. +/// +/// This capacity can be maxed out (every field max) and still fit +/// within a 64-bit memory space. If you need more than this, you will +/// need to split data across separate pages. +/// +/// For 32-bit systems, it is possible to overflow the addressable +/// space and this is something we still need to address in the future +/// likely by limiting the maximum capacity on 32-bit systems further. pub const Capacity = struct { /// Number of columns and rows we can know about. cols: size.CellCountInt, rows: size.CellCountInt, /// Number of unique styles that can be used on this page. - styles: usize = 16, + styles: size.StyleCountInt = 16, /// Number of bytes to allocate for hyperlink data. Note that the /// amount of data used for hyperlinks in total is more than this because /// hyperlinks use string data as well as a small amount of lookup metadata. /// This number is a rough approximation. - hyperlink_bytes: usize = hyperlink_bytes_default, + hyperlink_bytes: size.HyperlinkCountInt = hyperlink_bytes_default, /// Number of bytes to allocate for grapheme data. - grapheme_bytes: usize = grapheme_bytes_default, + grapheme_bytes: size.GraphemeBytesInt = grapheme_bytes_default, /// Number of bytes to allocate for strings. - string_bytes: usize = string_bytes_default, + string_bytes: size.StringBytesInt = string_bytes_default, pub const Adjustment = struct { cols: ?size.CellCountInt = null, @@ -2025,6 +2036,19 @@ pub const Cell = packed struct(u64) { // //const pages = total_size / std.heap.page_size_min; // } +test "Page.layout can take a maxed capacity" { + // Our intention is for a maxed-out capacity to always fit + // within a page layout without trigering runtime safety on any + // overflow. This simplifies some of our handling downstream of the + // call (relevant to: https://github.com/ghostty-org/ghostty/issues/10258) + var cap: Capacity = undefined; + inline for (@typeInfo(Capacity).@"struct".fields) |field| { + @field(cap, field.name) = std.math.maxInt(field.type); + } + + _ = Page.layout(cap); +} + test "Cell is zero by default" { const cell = Cell.init(0); const cell_int: u64 = @bitCast(cell); diff --git a/src/terminal/size.zig b/src/terminal/size.zig index 0dedfcc14..7be09739e 100644 --- a/src/terminal/size.zig +++ b/src/terminal/size.zig @@ -11,9 +11,32 @@ pub const max_page_size = std.math.maxInt(u32); /// derived from the maximum terminal page size. pub const OffsetInt = std.math.IntFittingRange(0, max_page_size - 1); -/// The int type that can contain the maximum number of cells in a page. -pub const CellCountInt = u16; // TODO: derive +/// Int types for maximum values of things. A lot of these sizes are +/// based on "X is enough for any reasonable use case" principles. +// The goal is that a user can have the maxInt amount of all of these +// present at one time and be able to address them in a single Page.zig. + +// Total number of cells that are possible in each dimension (row/col). +// Based on 2^16 being enough for any reasonable terminal size and allowing +// IDs to remain 16-bit. +pub const CellCountInt = u16; + +// Total number of styles and hyperlinks that are possible in a page. +// We match CellCountInt here because each cell in a single row can have at +// most one style, making it simple to split a page by splitting rows. // +// Note due to the way RefCountedSet works, we are short one value, but +// this is a theoretical limit we accept. A page with a single row max +// columns wide would be one short of having every cell have a unique style. +pub const StyleCountInt = CellCountInt; +pub const HyperlinkCountInt = CellCountInt; + +// Total number of bytes that can be taken up by grapheme data and string +// data. Both of these technically unlimited with malicious input, but +// we choose a reasonable limit of 2^32 (4GB) per. +pub const GraphemeBytesInt = u32; +pub const StringBytesInt = u32; + /// The offset from the base address of the page to the start of some data. /// This is typed for ease of use. /// diff --git a/src/terminal/style.zig b/src/terminal/style.zig index e5c47b9fe..7908beefa 100644 --- a/src/terminal/style.zig +++ b/src/terminal/style.zig @@ -11,7 +11,7 @@ const RefCountedSet = @import("ref_counted_set.zig").RefCountedSet; /// The unique identifier for a style. This is at most the number of cells /// that can fit into a terminal page. -pub const Id = size.CellCountInt; +pub const Id = size.StyleCountInt; /// The Id to use for default styling. pub const default_id: Id = 0; From 8306f96d9460a440579d002bfcea254fac5889ab Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 13 Jan 2026 13:41:11 -0800 Subject: [PATCH 04/37] terminal: PageList.increaseCapacity --- src/terminal/PageList.zig | 399 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 399 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 4592b3db6..9c63fd578 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -2736,6 +2736,110 @@ pub fn adjustCapacity( return new_node; } +/// Possible dimensions to increase capacity for. +pub const IncreaseCapacity = enum { + styles, + grapheme_bytes, + hyperlink_bytes, + string_bytes, +}; + +pub const IncreaseCapacityError = error{ + // An actual system OOM trying to allocate memory. + OutOfMemory, + + // The existing page is already at max capacity for the given + // adjustment. The caller must create a new page, remove data from + // the old page, etc. (up to the caller). + OutOfSpace, +}; + +/// Increase the capacity of the given page node in the given direction. +/// This will always allocate a new node and remove the old node, so the +/// existing node pointer will be invalid after this call. The newly created +/// node on success is returned. +/// +/// The increase amount is at the control of the PageList implementation, +/// but is guaranteed to always increase by at least one unit in the +/// given dimension. Practically, we'll always increase by much more +/// (we currently double every time) but callers shouldn't depend on that. +/// The only guarantee is some amount of growth. +/// +/// Adjustment can be null if you want to recreate, reclone the page +/// with the same capacity. This is a special case used for rehashing since +/// the logic is otherwise the same. In this case, OutOfMemory is the +/// only possible error. +pub fn increaseCapacity( + self: *PageList, + node: *List.Node, + adjustment: ?IncreaseCapacity, +) IncreaseCapacityError!*List.Node { + defer self.assertIntegrity(); + const page: *Page = &node.data; + + // Apply our adjustment + var cap = page.capacity; + if (adjustment) |v| switch (v) { + inline else => |tag| { + const field_name = @tagName(tag); + const Int = @FieldType(Capacity, field_name); + const old = @field(cap, field_name); + + // We use checked math to prevent overflow. If there is an + // overflow it means we're out of space in this dimension, + // since pages can take up to their maxInt capacity in any + // category. + const new = std.math.mul(Int, old, 2) catch |err| { + comptime assert(@TypeOf(err) == error{Overflow}); + return error.OutOfSpace; + }; + @field(cap, field_name) = new; + }, + }; + + log.info("adjusting page capacity={}", .{cap}); + + // Create our new page and clone the old page into it. + const new_node = try self.createPage(cap); + errdefer self.destroyNode(new_node); + const new_page: *Page = &new_node.data; + assert(new_page.capacity.rows >= page.capacity.rows); + assert(new_page.capacity.cols >= page.capacity.cols); + new_page.size.rows = page.size.rows; + new_page.size.cols = page.size.cols; + new_page.cloneFrom( + page, + 0, + page.size.rows, + ) catch |err| { + // cloneFrom only errors if there isn't capacity for the data + // from the source page but we're only increasing capacity so + // this should never be possible. If it happens, we should crash + // because we're in no man's land and can't safely recover. + log.err("increaseCapacity clone failed err={}", .{err}); + @panic("unexpected clone failure"); + }; + + // Must not fail after this because the operations we do after this + // can't be recovered. + errdefer comptime unreachable; + + // Fix up all our tracked pins to point to the new page. + const pin_keys = self.tracked_pins.keys(); + for (pin_keys) |p| { + if (p.node != node) continue; + p.node = new_node; + } + + // Insert this page and destroy the old page + self.pages.insertBefore(node, new_node); + self.pages.remove(node); + self.destroyNode(node); + + new_page.assertIntegrity(); + return new_node; +} + /// Create a new page node. This does not add it to the list and this /// does not do any memory size accounting with max_size/page_size. inline fn createPage( @@ -6482,6 +6586,301 @@ test "PageList adjustCapacity after col shrink" { } } +test "PageList increaseCapacity to increase styles" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 2, 2, 0); + defer s.deinit(); + + const original_styles_cap = s.pages.first.?.data.capacity.styles; + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + // Write all our data so we can assert its the same after + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(x) }, + }; + } + } + } + + // Increase our styles + _ = try s.increaseCapacity(s.pages.first.?, .styles); + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + // Verify capacity doubled + try testing.expectEqual( + original_styles_cap * 2, + page.capacity.styles, + ); + + // Verify data preserved + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + try testing.expectEqual( + @as(u21, @intCast(x)), + rac.cell.content.codepoint, + ); + } + } + } +} + +test "PageList increaseCapacity to increase graphemes" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 2, 2, 0); + defer s.deinit(); + + const original_cap = s.pages.first.?.data.capacity.grapheme_bytes; + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(x) }, + }; + } + } + } + + _ = try s.increaseCapacity(s.pages.first.?, .grapheme_bytes); + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + try testing.expectEqual(original_cap * 2, page.capacity.grapheme_bytes); + + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + try testing.expectEqual( + @as(u21, @intCast(x)), + rac.cell.content.codepoint, + ); + } + } + } +} + +test "PageList increaseCapacity to increase hyperlinks" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 2, 2, 0); + defer s.deinit(); + + const original_cap = s.pages.first.?.data.capacity.hyperlink_bytes; + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(x) }, + }; + } + } + } + + _ = try s.increaseCapacity(s.pages.first.?, .hyperlink_bytes); + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + try testing.expectEqual(original_cap * 2, page.capacity.hyperlink_bytes); + + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + try testing.expectEqual( + @as(u21, @intCast(x)), + rac.cell.content.codepoint, + ); + } + } + } +} + +test "PageList increaseCapacity to increase string_bytes" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 2, 2, 0); + defer s.deinit(); + + const original_cap = s.pages.first.?.data.capacity.string_bytes; + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(x) }, + }; + } + } + } + + _ = try s.increaseCapacity(s.pages.first.?, .string_bytes); + + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + try testing.expectEqual(original_cap * 2, page.capacity.string_bytes); + + for (0..s.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + try testing.expectEqual( + @as(u21, @intCast(x)), + rac.cell.content.codepoint, + ); + } + } + } +} + +test "PageList increaseCapacity tracked pins" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 2, 2, 0); + defer s.deinit(); + + // Create a tracked pin on the first page + const tracked = try s.trackPin(s.pin(.{ .active = .{ .x = 1, .y = 1 } }).?); + defer s.untrackPin(tracked); + + const old_node = s.pages.first.?; + try testing.expectEqual(old_node, tracked.node); + + // Increase capacity + const new_node = try s.increaseCapacity(s.pages.first.?, .styles); + + // Pin should now point to the new node + try testing.expectEqual(new_node, tracked.node); + try testing.expectEqual(@as(size.CellCountInt, 1), tracked.x); + try testing.expectEqual(@as(size.CellCountInt, 1), tracked.y); +} + +test "PageList increaseCapacity returns OutOfSpace at max capacity" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 2, 2, 0); + defer s.deinit(); + + // Set styles capacity to a value that will overflow when doubled + // We need to create a page with capacity at more than half of max + const max_styles = std.math.maxInt(size.StyleCountInt); + const half_max = max_styles / 2 + 1; + + // Adjust capacity to near-max first + _ = try s.adjustCapacity(s.pages.first.?, .{ .styles = half_max }); + + // Now increaseCapacity should fail with OutOfSpace + try testing.expectError( + error.OutOfSpace, + s.increaseCapacity(s.pages.first.?, .styles), + ); +} + +test "PageList increaseCapacity after col shrink" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 2, 0); + defer s.deinit(); + + // Shrink columns + try s.resize(.{ .cols = 5, .reflow = false }); + try testing.expectEqual(5, s.cols); + + { + const page = &s.pages.first.?.data; + try testing.expectEqual(5, page.size.cols); + try testing.expect(page.capacity.cols >= 10); + } + + // Increase capacity + _ = try s.increaseCapacity(s.pages.first.?, .styles); + + { + const page = &s.pages.first.?.data; + // size.cols should still be 5, not reverted to capacity.cols + try testing.expectEqual(5, page.size.cols); + try testing.expectEqual(5, s.cols); + } +} + +test "PageList increaseCapacity multi-page" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, null); + defer s.deinit(); + + // Grow to create a second page + const page1_node = s.pages.last.?; + page1_node.data.pauseIntegrityChecks(true); + for (0..page1_node.data.capacity.rows - page1_node.data.size.rows) |_| { + try testing.expect(try s.grow() == null); + } + page1_node.data.pauseIntegrityChecks(false); + try testing.expect(try s.grow() != null); + + // Now we have two pages + try testing.expect(s.pages.first != s.pages.last); + const page2_node = s.pages.last.?; + + const page1_styles_cap = s.pages.first.?.data.capacity.styles; + const page2_styles_cap = page2_node.data.capacity.styles; + + // Increase capacity on the first page only + _ = try s.increaseCapacity(s.pages.first.?, .styles); + + // First page capacity should be doubled + try testing.expectEqual( + page1_styles_cap * 2, + s.pages.first.?.data.capacity.styles, + ); + + // Second page should be unchanged + try testing.expectEqual( + page2_styles_cap, + s.pages.last.?.data.capacity.styles, + ); +} + test "PageList pageIterator single page" { const testing = std.testing; const alloc = testing.allocator; From 1e5973386b5eb262526b4990102500e556f6f2ea Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 13 Jan 2026 13:41:11 -0800 Subject: [PATCH 05/37] terminal: Screen.increaseCapacity --- src/terminal/Screen.zig | 312 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 309 insertions(+), 3 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index fed0a8c51..8edb72ba5 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -588,6 +588,76 @@ pub fn adjustCapacity( return new_node; } +pub fn increaseCapacity( + self: *Screen, + node: *PageList.List.Node, + adjustment: ?PageList.IncreaseCapacity, +) PageList.IncreaseCapacityError!*PageList.List.Node { + // If the page being modified isn't our cursor page then + // this is a quick operation because we have no additional + // accounting. We have to do this check here BEFORE calling + // increaseCapacity because increaseCapacity will update all + // our tracked pins (including our cursor). + if (node != self.cursor.page_pin.node) return try self.pages.increaseCapacity( + node, + adjustment, + ); + + // We're modifying the cursor page. When we increase the + // capacity below it will be short the ref count on our + // current style and hyperlink, so we need to init those. + const new_node = try self.pages.increaseCapacity(node, adjustment); + const new_page: *Page = &new_node.data; + + // Re-add the style, if the page somehow doesn't have enough + // memory to add it, we emit a warning and gracefully degrade + // to the default style for the cursor. + if (self.cursor.style_id != style.default_id) { + self.cursor.style_id = new_page.styles.add( + new_page.memory, + self.cursor.style, + ) catch |err| id: { + // TODO: Should we increase the capacity further in this case? + log.warn( + "(Screen.increaseCapacity) Failed to add cursor style back to page, err={}", + .{err}, + ); + + // Reset the cursor style. + self.cursor.style = .{}; + break :id style.default_id; + }; + } + + // Re-add the hyperlink, if the page somehow doesn't have enough + // memory to add it, we emit a warning and gracefully degrade to + // no hyperlink. + if (self.cursor.hyperlink) |link| { + // So we don't attempt to free any memory in the replaced page. + self.cursor.hyperlink_id = 0; + self.cursor.hyperlink = null; + + // Re-add + self.startHyperlinkOnce(link.*) catch |err| { + // TODO: Should we increase the capacity further in this case? + log.warn( + "(Screen.increaseCapacity) Failed to add cursor hyperlink back to page, err={}", + .{err}, + ); + }; + + // Remove our old link + link.deinit(self.alloc); + self.alloc.destroy(link); + } + + // Reload the cursor information because the pin changed. + // So our page row/cell and so on are all off. + self.cursorReload(); + + return new_node; +} + pub inline fn cursorCellRight(self: *Screen, n: size.CellCountInt) *pagepkg.Cell { assert(self.cursor.x + n < self.pages.cols); const cell: [*]pagepkg.Cell = @ptrCast(self.cursor.page_cell); @@ -3007,15 +3077,15 @@ pub fn testWriteString(self: *Screen, text: []const u8) !void { .protected = self.cursor.protected, }; - // If we have a hyperlink, add it to the cell. - if (self.cursor.hyperlink_id > 0) try self.cursorSetHyperlink(); - // If we have a ref-counted style, increase. if (self.cursor.style_id != style.default_id) { const page = self.cursor.page_pin.node.data; page.styles.use(page.memory, self.cursor.style_id); self.cursor.page_row.styled = true; } + + // If we have a hyperlink, add it to the cell. + if (self.cursor.hyperlink_id > 0) try self.cursorSetHyperlink(); }, 2 => { @@ -9103,3 +9173,239 @@ test "Screen: cursorDown to page with insufficient capacity" { try testing.expect(false); } } + +test "Screen: increaseCapacity cursor style ref count preserved" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ + .cols = 5, + .rows = 5, + .max_scrollback = 0, + }); + defer s.deinit(); + try s.setAttribute(.bold); + try s.testWriteString("1ABCD"); + + // We should have one page and it should be our cursor page + try testing.expect(s.pages.pages.first == s.pages.pages.last); + try testing.expect(s.pages.pages.first == s.cursor.page_pin.node); + + const old_style = s.cursor.style; + + { + const page = &s.pages.pages.last.?.data; + // 5 chars + cursor = 6 refs + try testing.expectEqual( + 6, + page.styles.refCount(page.memory, s.cursor.style_id), + ); + } + + // This forces the page to change via increaseCapacity. + const new_node = try s.increaseCapacity( + s.cursor.page_pin.node, + .grapheme_bytes, + ); + + // Cursor's page_pin should now point to the new node + try testing.expect(s.cursor.page_pin.node == new_node); + + // Verify cursor's page_cell and page_row are correctly reloaded from the pin + const page_rac = s.cursor.page_pin.rowAndCell(); + try testing.expect(s.cursor.page_row == page_rac.row); + try testing.expect(s.cursor.page_cell == page_rac.cell); + + // Style should be preserved + try testing.expectEqual(old_style, s.cursor.style); + try testing.expect(s.cursor.style_id != style.default_id); + + // After increaseCapacity, the 5 chars are cloned (5 refs) and + // the cursor's style is re-added (1 ref) = 6 total. + { + const page = &s.pages.pages.last.?.data; + const ref_count = page.styles.refCount(page.memory, s.cursor.style_id); + try testing.expectEqual(6, ref_count); + } +} + +test "Screen: increaseCapacity cursor hyperlink ref count preserved" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ + .cols = 5, + .rows = 5, + .max_scrollback = 0, + }); + defer s.deinit(); + try s.startHyperlink("https://example.com/", null); + try s.testWriteString("1ABCD"); + + // We should have one page and it should be our cursor page + try testing.expect(s.pages.pages.first == s.pages.pages.last); + try testing.expect(s.pages.pages.first == s.cursor.page_pin.node); + + { + const page = &s.pages.pages.last.?.data; + // Cursor has the hyperlink active = 1 count in hyperlink_set + try testing.expectEqual(1, page.hyperlink_set.count()); + try testing.expect(s.cursor.hyperlink_id != 0); + try testing.expect(s.cursor.hyperlink != null); + } + + // This forces the page to change via increaseCapacity. + _ = try s.increaseCapacity( + s.cursor.page_pin.node, + .grapheme_bytes, + ); + + // Hyperlink should be preserved with correct URI + try testing.expect(s.cursor.hyperlink != null); + try testing.expect(s.cursor.hyperlink_id != 0); + try testing.expectEqualStrings("https://example.com/", s.cursor.hyperlink.?.uri); + + // After increaseCapacity, the hyperlink is re-added to the new page. + { + const page = &s.pages.pages.last.?.data; + try testing.expectEqual(1, page.hyperlink_set.count()); + } +} + +test "Screen: increaseCapacity cursor with both style and hyperlink preserved" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ + .cols = 5, + .rows = 5, + .max_scrollback = 0, + }); + defer s.deinit(); + + // Set both a non-default style AND an active hyperlink. + // Write one character first with bold to mark the row as styled, + // then start the hyperlink and write more characters. + try s.setAttribute(.bold); + try s.startHyperlink("https://example.com/", null); + try s.testWriteString("1ABCD"); + + // We should have one page and it should be our cursor page + try testing.expect(s.pages.pages.first == s.pages.pages.last); + try testing.expect(s.pages.pages.first == s.cursor.page_pin.node); + + const old_style = s.cursor.style; + + { + const page = &s.pages.pages.last.?.data; + // 5 chars + cursor = 6 refs for bold style + try testing.expectEqual( + 6, + page.styles.refCount(page.memory, s.cursor.style_id), + ); + // Cursor has the hyperlink active = 1 count in hyperlink_set + try testing.expectEqual(1, page.hyperlink_set.count()); + try testing.expect(s.cursor.style_id != style.default_id); + try testing.expect(s.cursor.hyperlink_id != 0); + try testing.expect(s.cursor.hyperlink != null); + } + + // This forces the page to change via increaseCapacity. + _ = try s.increaseCapacity( + s.cursor.page_pin.node, + .grapheme_bytes, + ); + + // Style should be preserved + try testing.expectEqual(old_style, s.cursor.style); + try testing.expect(s.cursor.style_id != style.default_id); + + // Hyperlink should be preserved with correct URI + try testing.expect(s.cursor.hyperlink != null); + try testing.expect(s.cursor.hyperlink_id != 0); + try testing.expectEqualStrings("https://example.com/", s.cursor.hyperlink.?.uri); + + // After increaseCapacity, both style and hyperlink are re-added to the new page. + { + const page = &s.pages.pages.last.?.data; + const ref_count = page.styles.refCount(page.memory, s.cursor.style_id); + try testing.expectEqual(6, ref_count); + try testing.expectEqual(1, page.hyperlink_set.count()); + } +} + +test "Screen: increaseCapacity non-cursor page returns early" { + // Test that calling increaseCapacity on a page that is NOT the cursor's + // page properly delegates to pages.increaseCapacity without doing the + // extra cursor accounting (style/hyperlink re-adding). + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ + .cols = 80, + .rows = 24, + .max_scrollback = 10000, + }); + defer s.deinit(); + + // Set up a custom style and hyperlink on the cursor + try s.setAttribute(.bold); + try s.startHyperlink("https://example.com/", null); + try s.testWriteString("Hello"); + + // Store cursor state before growing pages + const old_style = s.cursor.style; + const old_style_id = s.cursor.style_id; + const old_hyperlink = s.cursor.hyperlink; + const old_hyperlink_id = s.cursor.hyperlink_id; + + // The cursor is on the first (and only) page + try testing.expect(s.pages.pages.first == s.pages.pages.last); + try testing.expect(s.cursor.page_pin.node == s.pages.pages.first.?); + + // Grow pages until we have multiple pages. The cursor's pin stays on + // the first page since we're just adding rows. + const first_page_node = s.pages.pages.first.?; + first_page_node.data.pauseIntegrityChecks(true); + for (0..first_page_node.data.capacity.rows - first_page_node.data.size.rows) |_| { + _ = try s.pages.grow(); + } + first_page_node.data.pauseIntegrityChecks(false); + _ = try s.pages.grow(); + + // Now we have two pages + try testing.expect(s.pages.pages.first != s.pages.pages.last); + const second_page = s.pages.pages.last.?; + + // Cursor should still be on the first page (where it was created) + try testing.expect(s.cursor.page_pin.node == s.pages.pages.first.?); + try testing.expect(s.cursor.page_pin.node != second_page); + + const second_page_styles_cap = second_page.data.capacity.styles; + const cursor_page_styles_cap = s.cursor.page_pin.node.data.capacity.styles; + + // Call increaseCapacity on the second page (NOT the cursor's page) + const new_second_page = try s.increaseCapacity(second_page, .styles); + + // The second page should have increased capacity + try testing.expectEqual( + second_page_styles_cap * 2, + new_second_page.data.capacity.styles, + ); + + // The cursor's page (first page) should be unchanged + try testing.expectEqual( + cursor_page_styles_cap, + s.cursor.page_pin.node.data.capacity.styles, + ); + + // Cursor state should be completely unchanged since we didn't touch its page + try testing.expectEqual(old_style, s.cursor.style); + try testing.expectEqual(old_style_id, s.cursor.style_id); + try testing.expectEqual(old_hyperlink, s.cursor.hyperlink); + try testing.expectEqual(old_hyperlink_id, s.cursor.hyperlink_id); + + // Verify hyperlink is still valid + try testing.expect(s.cursor.hyperlink != null); + try testing.expectEqualStrings("https://example.com/", s.cursor.hyperlink.?.uri); +} From 29d4aba03337d7e9d6a0c357969e8924240b84dd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 15 Jan 2026 09:07:44 -0800 Subject: [PATCH 06/37] terminal: Screen replace adjust with increaseCapacity --- src/terminal/Screen.zig | 101 ++++++++++++++++++++++++++++------------ src/terminal/page.zig | 2 +- 2 files changed, 72 insertions(+), 31 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 8edb72ba5..66dc37006 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1176,14 +1176,19 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void { return; } - // If we have a old style then we need to release it from the old page. + // If we have an old style then we need to release it from the old page. const old_style_: ?style.Style = if (self.cursor.style_id == style.default_id) null else self.cursor.style; if (old_style_ != null) { + // Release the style directly from the old page instead of going through + // manualStyleUpdate, because the cursor position may have already been + // updated but the pin has not, which would fail integrity checks. + const old_page: *Page = &self.cursor.page_pin.node.data; + old_page.styles.release(old_page.memory, self.cursor.style_id); self.cursor.style = .{}; - self.manualStyleUpdate() catch unreachable; // Removing a style should never fail + self.cursor.style_id = style.default_id; } // If we have a hyperlink then we need to release it from the old page. @@ -2000,7 +2005,17 @@ pub fn setAttribute(self: *Screen, attr: sgr.Attribute) !void { } /// Call this whenever you manually change the cursor style. -pub fn manualStyleUpdate(self: *Screen) !void { +/// +/// Note that this can return any PageList capacity error, because it +/// is possible for the internal pagelist to not accommodate the new style +/// at all. This WILL attempt to resize our internal pages to fit the style +/// but it is possible that it cannot be done, in which case upstream callers +/// need to split the page or do something else. +/// +/// NOTE(mitchellh): I think in the future we'll do page splitting +/// automatically here and remove this failure scenario. +pub fn manualStyleUpdate(self: *Screen) PageList.IncreaseCapacityError!void { + defer self.assertIntegrity(); var page: *Page = &self.cursor.page_pin.node.data; // std.log.warn("active styles={}", .{page.styles.count()}); @@ -2019,6 +2034,9 @@ pub fn manualStyleUpdate(self: *Screen) !void { // Clear the cursor style ID to prevent weird things from happening // if the page capacity has to be adjusted which would end up calling // manualStyleUpdate again. + // + // This also ensures that if anything fails below, we fall back to + // clearing our style. self.cursor.style_id = style.default_id; // After setting the style, we need to update our style map. @@ -2030,30 +2048,50 @@ pub fn manualStyleUpdate(self: *Screen) !void { page.memory, self.cursor.style, ) catch |err| id: { - // Our style map is full or needs to be rehashed, - // so we allocate a new page, which will rehash, - // and double the style capacity for it if it was - // full. - const node = try self.adjustCapacity( + // Our style map is full or needs to be rehashed, so we need to + // increase style capacity (or rehash). + const node = try self.increaseCapacity( self.cursor.page_pin.node, switch (err) { - error.OutOfMemory => .{ .styles = page.capacity.styles * 2 }, - error.NeedsRehash => .{}, + error.OutOfMemory => .styles, + error.NeedsRehash => null, }, ); page = &node.data; - break :id try page.styles.add( + break :id page.styles.add( page.memory, self.cursor.style, - ); + ) catch |err2| switch (err2) { + error.OutOfMemory => { + // This shouldn't happen because increaseCapacity is + // guaranteed to increase our capacity by at least one and + // we only need one space, but again, I don't want to crash + // here so let's log loudly and reset. + log.err("style addition failed after capacity increase", .{}); + return error.OutOfMemory; + }, + error.NeedsRehash => { + // This should be impossible because we rehash above + // and rehashing should never result in a duplicate. But + // we don't want to simply hard crash so log it and + // clear our style. + log.err("style rehash resulted in needs rehash", .{}); + return; + }, + }; }; + errdefer page.styles.release(page.memory, id); + self.cursor.style_id = id; - self.assertIntegrity(); } /// Append a grapheme to the given cell within the current cursor row. -pub fn appendGrapheme(self: *Screen, cell: *Cell, cp: u21) !void { +pub fn appendGrapheme( + self: *Screen, + cell: *Cell, + cp: u21, +) PageList.IncreaseCapacityError!void { defer self.cursor.page_pin.node.data.assertIntegrity(); self.cursor.page_pin.node.data.appendGrapheme( self.cursor.page_row, @@ -2073,11 +2111,9 @@ pub fn appendGrapheme(self: *Screen, cell: *Cell, cp: u21) !void { // Adjust our capacity. This will update our cursor page pin and // force us to reload. - const original_node = self.cursor.page_pin.node; - const new_bytes = original_node.data.capacity.grapheme_bytes * 2; - _ = try self.adjustCapacity( - original_node, - .{ .grapheme_bytes = new_bytes }, + _ = try self.increaseCapacity( + self.cursor.page_pin.node, + .grapheme_bytes, ); // The cell pointer is now invalid, so we need to get it from @@ -2088,17 +2124,22 @@ pub fn appendGrapheme(self: *Screen, cell: *Cell, cp: u21) !void { .gt => self.cursorCellRight(@intCast(cell_idx - self.cursor.x)), }; - try self.cursor.page_pin.node.data.appendGrapheme( + self.cursor.page_pin.node.data.appendGrapheme( self.cursor.page_row, reloaded_cell, cp, - ); + ) catch |err2| { + comptime assert(@TypeOf(err2) == error{OutOfMemory}); + // This should never happen because we just increased capacity. + // Log loudly but still return an error so we don't just + // crash. + log.err("grapheme append failed after capacity increase", .{}); + return err2; + }; }, }; } -pub const StartHyperlinkError = Allocator.Error || PageList.AdjustCapacityError; - /// Start the hyperlink state. Future cells will be marked as hyperlinks with /// this state. Note that various terminal operations may clear the hyperlink /// state, such as switching screens (alt screen). @@ -2106,7 +2147,7 @@ pub fn startHyperlink( self: *Screen, uri: []const u8, id_: ?[]const u8, -) StartHyperlinkError!void { +) PageList.IncreaseCapacityError!void { // Create our pending entry. const link: hyperlink.Hyperlink = .{ .uri = uri, @@ -2131,21 +2172,21 @@ pub fn startHyperlink( error.OutOfMemory => return error.OutOfMemory, // strings table is out of memory, adjust it up - error.StringsOutOfMemory => _ = try self.adjustCapacity( + error.StringsOutOfMemory => _ = try self.increaseCapacity( self.cursor.page_pin.node, - .{ .string_bytes = self.cursor.page_pin.node.data.capacity.string_bytes * 2 }, + .string_bytes, ), // hyperlink set is out of memory, adjust it up - error.SetOutOfMemory => _ = try self.adjustCapacity( + error.SetOutOfMemory => _ = try self.increaseCapacity( self.cursor.page_pin.node, - .{ .hyperlink_bytes = self.cursor.page_pin.node.data.capacity.hyperlink_bytes * 2 }, + .hyperlink_bytes, ), // hyperlink set is too full, rehash it - error.SetNeedsRehash => _ = try self.adjustCapacity( + error.SetNeedsRehash => _ = try self.increaseCapacity( self.cursor.page_pin.node, - .{}, + null, ), } diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 559f536f1..1150027a4 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -2038,7 +2038,7 @@ pub const Cell = packed struct(u64) { test "Page.layout can take a maxed capacity" { // Our intention is for a maxed-out capacity to always fit - // within a page layout without trigering runtime safety on any + // within a page layout without triggering runtime safety on any // overflow. This simplifies some of our handling downstream of the // call (relevant to: https://github.com/ghostty-org/ghostty/issues/10258) var cap: Capacity = undefined; From 25b7cc9f2cc28071d9d07f3a96ab86c811f1d1e1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 15 Jan 2026 10:56:32 -0800 Subject: [PATCH 07/37] terminal: hyperlink state uses increaseCapacity on screen --- src/terminal/Screen.zig | 48 ++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 66dc37006..f524bd7f4 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -2248,7 +2248,7 @@ pub fn endHyperlink(self: *Screen) void { } /// Set the current hyperlink state on the current cell. -pub fn cursorSetHyperlink(self: *Screen) !void { +pub fn cursorSetHyperlink(self: *Screen) PageList.IncreaseCapacityError!void { assert(self.cursor.hyperlink_id != 0); var page = &self.cursor.page_pin.node.data; @@ -2263,10 +2263,6 @@ pub fn cursorSetHyperlink(self: *Screen) !void { } else |err| switch (err) { // hyperlink_map is out of space, realloc the page to be larger error.HyperlinkMapOutOfMemory => { - const uri_size = if (self.cursor.hyperlink) |link| link.uri.len else 0; - - var string_bytes = page.capacity.string_bytes; - // Attempt to allocate the space that would be required to // insert a new copy of the cursor hyperlink uri in to the // string alloc, since right now adjustCapacity always just @@ -2274,29 +2270,31 @@ pub fn cursorSetHyperlink(self: *Screen) !void { // If this alloc fails then we know we also need to grow our // string bytes. // - // FIXME: This SUCKS - if (page.string_alloc.alloc( - u8, - page.memory, - uri_size, - )) |slice| { - // We don't bother freeing because we're - // about to free the entire page anyway. - _ = &slice; - } else |_| { - // We didn't have enough room, let's just double our - // string bytes until there's definitely enough room - // for our uri. - const before = string_bytes; - while (string_bytes - before < uri_size) string_bytes *= 2; + // FIXME: increaseCapacity should not do this. + while (self.cursor.hyperlink) |link| { + if (page.string_alloc.alloc( + u8, + page.memory, + link.uri.len, + )) |slice| { + // We don't bother freeing because we're + // about to free the entire page anyway. + _ = slice; + break; + } else |_| {} + + // We didn't have enough room, let's increase string bytes + const new_node = try self.increaseCapacity( + self.cursor.page_pin.node, + .string_bytes, + ); + assert(new_node == self.cursor.page_pin.node); + page = &new_node.data; } - _ = try self.adjustCapacity( + _ = try self.increaseCapacity( self.cursor.page_pin.node, - .{ - .hyperlink_bytes = page.capacity.hyperlink_bytes * 2, - .string_bytes = string_bytes, - }, + .hyperlink_bytes, ); // Retry From c8afc423082ef764c9f5e62b5269cc5dc2fcff22 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 15 Jan 2026 11:37:58 -0800 Subject: [PATCH 08/37] terminal: switch to increaseCapacity --- src/terminal/Terminal.zig | 72 ++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 43 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index d717a9724..310adaf29 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1634,54 +1634,48 @@ pub fn insertLines(self: *Terminal, count: usize) void { self.scrolling_region.left, self.scrolling_region.right + 1, ) catch |err| { - const cap = dst_p.node.data.capacity; // Adjust our page capacity to make // room for we didn't have space for - _ = self.screens.active.adjustCapacity( + _ = self.screens.active.increaseCapacity( dst_p.node, switch (err) { // Rehash the sets error.StyleSetNeedsRehash, error.HyperlinkSetNeedsRehash, - => .{}, + => null, // Increase style memory error.StyleSetOutOfMemory, - => .{ .styles = cap.styles * 2 }, + => .styles, // Increase string memory error.StringAllocOutOfMemory, - => .{ .string_bytes = cap.string_bytes * 2 }, + => .string_bytes, // Increase hyperlink memory error.HyperlinkSetOutOfMemory, error.HyperlinkMapOutOfMemory, - => .{ .hyperlink_bytes = cap.hyperlink_bytes * 2 }, + => .hyperlink_bytes, // Increase grapheme memory error.GraphemeMapOutOfMemory, error.GraphemeAllocOutOfMemory, - => .{ .grapheme_bytes = cap.grapheme_bytes * 2 }, + => .grapheme_bytes, }, ) catch |e| switch (e) { - // This shouldn't be possible because above we're only - // adjusting capacity _upwards_. So it should have all - // the existing capacity it had to fit the adjusted - // data. Panic since we don't expect this. - error.StyleSetOutOfMemory, - error.StyleSetNeedsRehash, - error.StringAllocOutOfMemory, - error.HyperlinkSetOutOfMemory, - error.HyperlinkSetNeedsRehash, - error.HyperlinkMapOutOfMemory, - error.GraphemeMapOutOfMemory, - error.GraphemeAllocOutOfMemory, - => @panic("adjustCapacity resulted in capacity errors"), - - // The system allocator is OOM. We can't currently do - // anything graceful here. We panic. + // System OOM. We have no way to recover from this + // currently. We should probably change insertLines + // to raise an error here. error.OutOfMemory, - => @panic("adjustCapacity system allocator OOM"), + => @panic("increaseCapacity system allocator OOM"), + + // The page can't accomodate the managed memory required + // for this operation. We previously just corrupted + // memory here so a crash is better. The right long + // term solution is to allocate a new page here + // move this row to the new page, and start over. + error.OutOfSpace, + => @panic("increaseCapacity OutOfSpace"), }; // Continue the loop to try handling this row again. @@ -1834,49 +1828,41 @@ pub fn deleteLines(self: *Terminal, count: usize) void { self.scrolling_region.left, self.scrolling_region.right + 1, ) catch |err| { - const cap = dst_p.node.data.capacity; // Adjust our page capacity to make // room for we didn't have space for - _ = self.screens.active.adjustCapacity( + _ = self.screens.active.increaseCapacity( dst_p.node, switch (err) { // Rehash the sets error.StyleSetNeedsRehash, error.HyperlinkSetNeedsRehash, - => .{}, + => null, // Increase style memory error.StyleSetOutOfMemory, - => .{ .styles = cap.styles * 2 }, + => .styles, // Increase string memory error.StringAllocOutOfMemory, - => .{ .string_bytes = cap.string_bytes * 2 }, + => .string_bytes, // Increase hyperlink memory error.HyperlinkSetOutOfMemory, error.HyperlinkMapOutOfMemory, - => .{ .hyperlink_bytes = cap.hyperlink_bytes * 2 }, + => .hyperlink_bytes, // Increase grapheme memory error.GraphemeMapOutOfMemory, error.GraphemeAllocOutOfMemory, - => .{ .grapheme_bytes = cap.grapheme_bytes * 2 }, + => .grapheme_bytes, }, ) catch |e| switch (e) { - // See insertLines which has the same error capture. - error.StyleSetOutOfMemory, - error.StyleSetNeedsRehash, - error.StringAllocOutOfMemory, - error.HyperlinkSetOutOfMemory, - error.HyperlinkSetNeedsRehash, - error.HyperlinkMapOutOfMemory, - error.GraphemeMapOutOfMemory, - error.GraphemeAllocOutOfMemory, - => @panic("adjustCapacity resulted in capacity errors"), - + // See insertLines error.OutOfMemory, - => @panic("adjustCapacity system allocator OOM"), + => @panic("increaseCapacity system allocator OOM"), + + error.OutOfSpace, + => @panic("increaseCapacity OutOfSpace"), }; // Continue the loop to try handling this row again. From b59ac60a8772ae1e259966efc0eb9feb5c4167cb Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 15 Jan 2026 11:40:39 -0800 Subject: [PATCH 09/37] terminal: remove Screen.adjustCapacity --- src/terminal/Screen.zig | 359 +++++++++----------------------------- src/terminal/Terminal.zig | 2 +- 2 files changed, 80 insertions(+), 281 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index f524bd7f4..1538da5da 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -517,77 +517,6 @@ pub fn clone( result.assertIntegrity(); return result; } - -/// Adjust the capacity of a page within the pagelist of this screen. -/// This handles some accounting if the page being modified is the -/// cursor page. -pub fn adjustCapacity( - self: *Screen, - node: *PageList.List.Node, - adjustment: PageList.AdjustCapacity, -) PageList.AdjustCapacityError!*PageList.List.Node { - // If the page being modified isn't our cursor page then - // this is a quick operation because we have no additional - // accounting. - if (node != self.cursor.page_pin.node) { - return try self.pages.adjustCapacity(node, adjustment); - } - - // We're modifying the cursor page. When we adjust the - // capacity below it will be short the ref count on our - // current style and hyperlink, so we need to init those. - const new_node = try self.pages.adjustCapacity(node, adjustment); - const new_page: *Page = &new_node.data; - - // Re-add the style, if the page somehow doesn't have enough - // memory to add it, we emit a warning and gracefully degrade - // to the default style for the cursor. - if (self.cursor.style_id != 0) { - self.cursor.style_id = new_page.styles.add( - new_page.memory, - self.cursor.style, - ) catch |err| id: { - // TODO: Should we increase the capacity further in this case? - log.warn( - "(Screen.adjustCapacity) Failed to add cursor style back to page, err={}", - .{err}, - ); - - // Reset the cursor style. - self.cursor.style = .{}; - break :id style.default_id; - }; - } - - // Re-add the hyperlink, if the page somehow doesn't have enough - // memory to add it, we emit a warning and gracefully degrade to - // no hyperlink. - if (self.cursor.hyperlink) |link| { - // So we don't attempt to free any memory in the replaced page. - self.cursor.hyperlink_id = 0; - self.cursor.hyperlink = null; - - // Re-add - self.startHyperlinkOnce(link.*) catch |err| { - // TODO: Should we increase the capacity further in this case? - log.warn( - "(Screen.adjustCapacity) Failed to add cursor hyperlink back to page, err={}", - .{err}, - ); - }; - - // Remove our old link - link.deinit(self.alloc); - self.alloc.destroy(link); - } - - // Reload the cursor information because the pin changed. - // So our page row/cell and so on are all off. - self.cursorReload(); - - return new_node; -} - pub fn increaseCapacity( self: *Screen, node: *PageList.List.Node, @@ -2265,7 +2194,7 @@ pub fn cursorSetHyperlink(self: *Screen) PageList.IncreaseCapacityError!void { error.HyperlinkMapOutOfMemory => { // Attempt to allocate the space that would be required to // insert a new copy of the cursor hyperlink uri in to the - // string alloc, since right now adjustCapacity always just + // string alloc, since right now increaseCapacity always just // adds an extra copy even if one already exists in the page. // If this alloc fails then we know we also need to grow our // string bytes. @@ -9005,214 +8934,6 @@ test "Screen: cursorSetHyperlink OOM + URI too large for string alloc" { try testing.expect(base_string_bytes < s.cursor.page_pin.node.data.capacity.string_bytes); } -test "Screen: adjustCapacity cursor style ref count" { - const testing = std.testing; - const alloc = testing.allocator; - - var s = try init(alloc, .{ .cols = 5, .rows = 5, .max_scrollback = 0 }); - defer s.deinit(); - - try s.setAttribute(.{ .bold = {} }); - try s.testWriteString("1ABCD"); - - { - const page = &s.pages.pages.last.?.data; - try testing.expectEqual( - 6, // All chars + cursor - page.styles.refCount(page.memory, s.cursor.style_id), - ); - } - - // This forces the page to change. - _ = try s.adjustCapacity( - s.cursor.page_pin.node, - .{ .grapheme_bytes = s.cursor.page_pin.node.data.capacity.grapheme_bytes * 2 }, - ); - - // Our ref counts should still be the same - { - const page = &s.pages.pages.last.?.data; - try testing.expectEqual( - 6, // All chars + cursor - page.styles.refCount(page.memory, s.cursor.style_id), - ); - } -} - -test "Screen: adjustCapacity cursor hyperlink exceeds string alloc size" { - const testing = std.testing; - const alloc = testing.allocator; - - var s = try init(alloc, .{ .cols = 80, .rows = 24, .max_scrollback = 0 }); - defer s.deinit(); - - // Start a hyperlink with a URI that just barely fits in the string alloc. - // This will ensure that the redundant copy added in `adjustCapacity` won't - // fit in the available string alloc space. - const uri = "a" ** (pagepkg.std_capacity.string_bytes - 8); - try s.startHyperlink(uri, null); - - // Write some characters with this so that the URI - // is copied to the new page when adjusting capacity. - try s.testWriteString("Hello"); - - // Adjust the capacity, right now this will cause a redundant copy of - // the URI to be added to the string alloc, but since there isn't room - // for this this will clear the cursor hyperlink. - _ = try s.adjustCapacity(s.cursor.page_pin.node, .{}); - - // The cursor hyperlink should have been cleared by the `adjustCapacity` - // call, because there isn't enough room to add the redundant URI string. - // - // This behavior will change, causing this test to fail, if any of these - // changes are made: - // - // - The string alloc is changed to intern strings. - // - // - The adjustCapacity function is changed to ensure the new - // capacity will fit the redundant copy of the hyperlink uri. - // - // - The cursor managed memory handling is reworked so that it - // doesn't reside in the pages anymore and doesn't need this - // accounting. - // - // In such a case, adjust this test accordingly. - try testing.expectEqual(null, s.cursor.hyperlink); - try testing.expectEqual(0, s.cursor.hyperlink_id); -} - -test "Screen: adjustCapacity cursor style exceeds style set capacity" { - const testing = std.testing; - const alloc = testing.allocator; - - var s = try init(alloc, .{ .cols = 80, .rows = 24, .max_scrollback = 1000 }); - defer s.deinit(); - - const page = &s.cursor.page_pin.node.data; - - // We add unique styles to the page until no more will fit. - fill: for (0..255) |bg| { - for (0..255) |fg| { - const st: style.Style = .{ - .bg_color = .{ .palette = @intCast(bg) }, - .fg_color = .{ .palette = @intCast(fg) }, - }; - - s.cursor.style = st; - - // Try to insert the new style, if it doesn't fit then - // we succeeded in filling the style set, so we break. - s.cursor.style_id = page.styles.add( - page.memory, - s.cursor.style, - ) catch break :fill; - - try s.testWriteString("a"); - } - } - - // Adjust the capacity, this should cause the style set to reach the - // same state it was in to begin with, since it will clone the page - // in the same order as the styles were added to begin with, meaning - // the cursor style will not be able to be added to the set, which - // should, right now, result in the cursor style being cleared. - _ = try s.adjustCapacity(s.cursor.page_pin.node, .{}); - - // The cursor style should have been cleared by the `adjustCapacity`. - // - // This behavior will change, causing this test to fail, if either - // of these changes are made: - // - // - The adjustCapacity function is changed to ensure the - // new capacity will definitely fit the cursor style. - // - // - The cursor managed memory handling is reworked so that it - // doesn't reside in the pages anymore and doesn't need this - // accounting. - // - // In such a case, adjust this test accordingly. - try testing.expect(s.cursor.style.default()); - try testing.expectEqual(style.default_id, s.cursor.style_id); -} - -test "Screen: cursorDown to page with insufficient capacity" { - // Regression test for https://github.com/ghostty-org/ghostty/issues/10282 - // - // This test exposes a use-after-realloc bug in cursorDown (and similar - // cursor movement functions). The bug pattern: - // - // 1. cursorDown creates a by-value copy of the pin via page_pin.down(n) - // 2. cursorChangePin is called, which may trigger adjustCapacity - // if the target page's style map is full - // 3. adjustCapacity frees the old page and creates a new one - // 4. The local pin copy still points to the freed page - // 5. rowAndCell() on the stale pin accesses freed memory - - const testing = std.testing; - const alloc = testing.allocator; - - // Small screen to make page boundary crossing easy to set up - var s = try init(alloc, .{ .cols = 10, .rows = 3, .max_scrollback = 1 }); - defer s.deinit(); - - // Scroll down enough to create a second page - const start_page = &s.pages.pages.last.?.data; - const rem = start_page.capacity.rows; - start_page.pauseIntegrityChecks(true); - for (0..rem) |_| try s.cursorDownOrScroll(); - start_page.pauseIntegrityChecks(false); - - // Cursor should now be on a new page - const new_page = &s.cursor.page_pin.node.data; - try testing.expect(start_page != new_page); - - // Fill new_page's style map to capacity. When we move INTO this page - // with a style set, adjustCapacity will be triggered. - { - new_page.pauseIntegrityChecks(true); - defer new_page.pauseIntegrityChecks(false); - defer new_page.assertIntegrity(); - - var n: u24 = 1; - while (new_page.styles.add( - new_page.memory, - .{ .bg_color = .{ .rgb = @bitCast(n) } }, - )) |_| n += 1 else |_| {} - } - - // Move cursor to start of active area and set a style - s.cursorAbsolute(0, 0); - try s.setAttribute(.bold); - try testing.expect(s.cursor.style.flags.bold); - try testing.expect(s.cursor.style_id != style.default_id); - - // Find the row just before the page boundary - for (0..s.pages.rows - 1) |row| { - s.cursorAbsolute(0, @intCast(row)); - const cur_node = s.cursor.page_pin.node; - if (s.cursor.page_pin.down(1)) |next_pin| { - if (next_pin.node != cur_node) { - // Cursor is at 'row', moving down crosses to new_page - try testing.expect(&next_pin.node.data == new_page); - - // This cursorDown triggers the bug: the local page_pin copy - // becomes stale after adjustCapacity, causing rowAndCell() - // to access freed memory. - s.cursorDown(1); - - // If the fix is applied, verify correct state - try testing.expect(s.cursor.y == row + 1); - try testing.expect(s.cursor.style.flags.bold); - - break; - } - } - } else { - // Didn't find boundary - try testing.expect(false); - } -} - test "Screen: increaseCapacity cursor style ref count preserved" { const testing = std.testing; const alloc = testing.allocator; @@ -9448,3 +9169,81 @@ test "Screen: increaseCapacity non-cursor page returns early" { try testing.expect(s.cursor.hyperlink != null); try testing.expectEqualStrings("https://example.com/", s.cursor.hyperlink.?.uri); } + +test "Screen: cursorDown to page with insufficient capacity" { + // Regression test for https://github.com/ghostty-org/ghostty/issues/10282 + // + // This test exposes a use-after-realloc bug in cursorDown (and similar + // cursor movement functions). The bug pattern: + // + // 1. cursorDown creates a by-value copy of the pin via page_pin.down(n) + // 2. cursorChangePin is called, which may trigger adjustCapacity + // if the target page's style map is full + // 3. adjustCapacity frees the old page and creates a new one + // 4. The local pin copy still points to the freed page + // 5. rowAndCell() on the stale pin accesses freed memory + + const testing = std.testing; + const alloc = testing.allocator; + + // Small screen to make page boundary crossing easy to set up + var s = try init(alloc, .{ .cols = 10, .rows = 3, .max_scrollback = 1 }); + defer s.deinit(); + + // Scroll down enough to create a second page + const start_page = &s.pages.pages.last.?.data; + const rem = start_page.capacity.rows; + start_page.pauseIntegrityChecks(true); + for (0..rem) |_| try s.cursorDownOrScroll(); + start_page.pauseIntegrityChecks(false); + + // Cursor should now be on a new page + const new_page = &s.cursor.page_pin.node.data; + try testing.expect(start_page != new_page); + + // Fill new_page's style map to capacity. When we move INTO this page + // with a style set, adjustCapacity will be triggered. + { + new_page.pauseIntegrityChecks(true); + defer new_page.pauseIntegrityChecks(false); + defer new_page.assertIntegrity(); + + var n: u24 = 1; + while (new_page.styles.add( + new_page.memory, + .{ .bg_color = .{ .rgb = @bitCast(n) } }, + )) |_| n += 1 else |_| {} + } + + // Move cursor to start of active area and set a style + s.cursorAbsolute(0, 0); + try s.setAttribute(.bold); + try testing.expect(s.cursor.style.flags.bold); + try testing.expect(s.cursor.style_id != style.default_id); + + // Find the row just before the page boundary + for (0..s.pages.rows - 1) |row| { + s.cursorAbsolute(0, @intCast(row)); + const cur_node = s.cursor.page_pin.node; + if (s.cursor.page_pin.down(1)) |next_pin| { + if (next_pin.node != cur_node) { + // Cursor is at 'row', moving down crosses to new_page + try testing.expect(&next_pin.node.data == new_page); + + // This cursorDown triggers the bug: the local page_pin copy + // becomes stale after adjustCapacity, causing rowAndCell() + // to access freed memory. + s.cursorDown(1); + + // If the fix is applied, verify correct state + try testing.expect(s.cursor.y == row + 1); + try testing.expect(s.cursor.style.flags.bold); + + break; + } + } + } else { + // Didn't find boundary + try testing.expect(false); + } +} diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 310adaf29..3740397d3 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1669,7 +1669,7 @@ pub fn insertLines(self: *Terminal, count: usize) void { error.OutOfMemory, => @panic("increaseCapacity system allocator OOM"), - // The page can't accomodate the managed memory required + // The page can't accommodate the managed memory required // for this operation. We previously just corrupted // memory here so a crash is better. The right long // term solution is to allocate a new page here From e70452588790f32864400709e38d6eed83966cb1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 15 Jan 2026 13:08:13 -0800 Subject: [PATCH 10/37] terminal: PageList remove adjustCapacity --- src/terminal/PageList.zig | 396 ++++++-------------------------------- 1 file changed, 63 insertions(+), 333 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 9c63fd578..bf2071497 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1300,31 +1300,30 @@ const ReflowCursor = struct { // If our page can't support an additional cell // with graphemes then we increase capacity. if (self.page.graphemeCount() >= self.page.graphemeCapacity()) { - try self.adjustCapacity(list, .{ - .grapheme_bytes = cap.grapheme_bytes * 2, - }); + try self.increaseCapacity( + list, + .grapheme_bytes, + ); } // Attempt to allocate the space that would be required // for these graphemes, and if it's not available, then - // increase capacity. - if (self.page.grapheme_alloc.alloc( - u21, - self.page.memory, - cps.len, - )) |slice| { - self.page.grapheme_alloc.free(self.page.memory, slice); - } else |_| { - // Grow our capacity until we can - // definitely fit the extra bytes. - const required = cps.len * @sizeOf(u21); - var new_grapheme_capacity: size.GraphemeBytesInt = cap.grapheme_bytes; - while (new_grapheme_capacity - cap.grapheme_bytes < required) { - new_grapheme_capacity *= 2; + // increase capacity. Keep trying until we succeed. + while (true) { + if (self.page.grapheme_alloc.alloc( + u21, + self.page.memory, + cps.len, + )) |slice| { + self.page.grapheme_alloc.free( + self.page.memory, + slice, + ); + break; + } else |_| { + // Grow our capacity until we can fit the extra bytes. + try self.increaseCapacity(list, .grapheme_bytes); } - try self.adjustCapacity(list, .{ - .grapheme_bytes = new_grapheme_capacity, - }); } // This shouldn't fail since we made sure we have space above. @@ -1339,9 +1338,7 @@ const ReflowCursor = struct { // If our page can't support an additional cell // with a hyperlink then we increase capacity. if (self.page.hyperlinkCount() >= self.page.hyperlinkCapacity()) { - try self.adjustCapacity(list, .{ - .hyperlink_bytes = cap.hyperlink_bytes * 2, - }); + try self.increaseCapacity(list, .hyperlink_bytes); } // Ensure that the string alloc has sufficient capacity @@ -1352,23 +1349,26 @@ const ReflowCursor = struct { .explicit => |v| v.len, .implicit => 0, }; - if (self.page.string_alloc.alloc( - u8, - self.page.memory, - additional_required_string_capacity, - )) |slice| { - // We have enough capacity, free the test alloc. - self.page.string_alloc.free(self.page.memory, slice); - } else |_| { - // Grow our capacity until we can - // definitely fit the extra bytes. - var new_string_capacity: size.StringBytesInt = cap.string_bytes; - while (new_string_capacity - cap.string_bytes < additional_required_string_capacity) { - new_string_capacity *= 2; + // Keep trying until we have enough capacity. + while (true) { + if (self.page.string_alloc.alloc( + u8, + self.page.memory, + additional_required_string_capacity, + )) |slice| { + // We have enough capacity, free the test alloc. + self.page.string_alloc.free( + self.page.memory, + slice, + ); + break; + } else |_| { + // Grow our capacity until we can fit the extra bytes. + try self.increaseCapacity( + list, + .string_bytes, + ); } - try self.adjustCapacity(list, .{ - .string_bytes = new_string_capacity, - }); } const dst_id = self.page.hyperlink_set.addWithIdContext( @@ -1380,18 +1380,16 @@ const ReflowCursor = struct { ) catch |err| id: { // If the add failed then either the set needs to grow // or it needs to be rehashed. Either one of those can - // be accomplished by adjusting capacity, either with + // be accomplished by increasing capacity, either with // no actual change or with an increased hyperlink cap. - try self.adjustCapacity(list, switch (err) { - error.OutOfMemory => .{ - .hyperlink_bytes = cap.hyperlink_bytes * 2, - }, - error.NeedsRehash => .{}, + try self.increaseCapacity(list, switch (err) { + error.OutOfMemory => .hyperlink_bytes, + error.NeedsRehash => null, }); // We assume this one will succeed. We dupe the link // again, and don't have to worry about the other one - // because adjusting the capacity naturally clears up + // because increasing the capacity naturally clears up // any managed memory not associated with a cell yet. break :id try self.page.hyperlink_set.addWithIdContext( self.page.memory, @@ -1424,13 +1422,11 @@ const ReflowCursor = struct { ) catch |err| id: { // If the add failed then either the set needs to grow // or it needs to be rehashed. Either one of those can - // be accomplished by adjusting capacity, either with + // be accomplished by increasing capacity, either with // no actual change or with an increased style cap. - try self.adjustCapacity(list, switch (err) { - error.OutOfMemory => .{ - .styles = cap.styles * 2, - }, - error.NeedsRehash => .{}, + try self.increaseCapacity(list, switch (err) { + error.OutOfMemory => .styles, + error.NeedsRehash => null, }); // We assume this one will succeed. @@ -1507,11 +1503,11 @@ const ReflowCursor = struct { } } - /// Adjust the capacity of the current page. - fn adjustCapacity( + /// Increase the capacity of the current page. + fn increaseCapacity( self: *ReflowCursor, list: *PageList, - adjustment: AdjustCapacity, + adjustment: ?IncreaseCapacity, ) !void { const old_x = self.x; const old_y = self.y; @@ -1522,7 +1518,7 @@ const ReflowCursor = struct { // be correct during a reflow. list.pauseIntegrityChecks(true); defer list.pauseIntegrityChecks(false); - break :node try list.adjustCapacity( + break :node try list.increaseCapacity( self.node, adjustment, ); @@ -2642,100 +2638,6 @@ pub fn grow(self: *PageList) !?*List.Node { return next_node; } -/// Adjust the capacity of the given page in the list. -pub const AdjustCapacity = struct { - /// Adjust the number of styles in the page. This may be - /// rounded up if necessary to fit alignment requirements, - /// but it will never be rounded down. - styles: ?size.StyleCountInt = null, - - /// Adjust the number of available grapheme bytes in the page. - grapheme_bytes: ?size.GraphemeBytesInt = null, - - /// Adjust the number of available hyperlink bytes in the page. - hyperlink_bytes: ?size.HyperlinkCountInt = null, - - /// Adjust the number of available string bytes in the page. - string_bytes: ?size.StringBytesInt = null, -}; - -pub const AdjustCapacityError = Allocator.Error || Page.CloneFromError; - -/// Adjust the capacity of the given page in the list. This should -/// be used in cases where OutOfMemory is returned by some operation -/// i.e to increase style counts, grapheme counts, etc. -/// -/// Adjustment works by increasing the capacity of the desired -/// dimension to a certain amount and increases the memory allocation -/// requirement for the backing memory of the page. We currently -/// never split pages or anything like that. Because increased allocation -/// has to happen outside our memory pool, its generally much slower -/// so pages should be sized to be large enough to handle all but -/// exceptional cases. -/// -/// This can currently only INCREASE capacity size. It cannot -/// decrease capacity size. This limitation is only because we haven't -/// yet needed that use case. If we ever do, this can be added. Currently -/// any requests to decrease will be ignored. -pub fn adjustCapacity( - self: *PageList, - node: *List.Node, - adjustment: AdjustCapacity, -) AdjustCapacityError!*List.Node { - defer self.assertIntegrity(); - const page: *Page = &node.data; - - // We always start with the base capacity of the existing page. This - // ensures we never shrink from what we need. - var cap = page.capacity; - - // All ceilPowerOfTwo is unreachable because we're always same or less - // bit width so maxInt is always possible. - if (adjustment.styles) |v| { - const aligned = std.math.ceilPowerOfTwo(size.StyleCountInt, v) catch unreachable; - cap.styles = @max(cap.styles, aligned); - } - if (adjustment.grapheme_bytes) |v| { - const aligned = std.math.ceilPowerOfTwo(size.GraphemeBytesInt, v) catch unreachable; - cap.grapheme_bytes = @max(cap.grapheme_bytes, aligned); - } - if (adjustment.hyperlink_bytes) |v| { - const aligned = std.math.ceilPowerOfTwo(size.HyperlinkCountInt, v) catch unreachable; - cap.hyperlink_bytes = @max(cap.hyperlink_bytes, aligned); - } - if (adjustment.string_bytes) |v| { - const aligned = std.math.ceilPowerOfTwo(size.StringBytesInt, v) catch unreachable; - cap.string_bytes = @max(cap.string_bytes, aligned); - } - - log.info("adjusting page capacity={}", .{cap}); - - // Create our new page and clone the old page into it. - const new_node = try self.createPage(cap); - errdefer self.destroyNode(new_node); - const new_page: *Page = &new_node.data; - assert(new_page.capacity.rows >= page.capacity.rows); - assert(new_page.capacity.cols >= page.capacity.cols); - new_page.size.rows = page.size.rows; - new_page.size.cols = page.size.cols; - try new_page.cloneFrom(page, 0, page.size.rows); - - // Fix up all our tracked pins to point to the new page. - const pin_keys = self.tracked_pins.keys(); - for (pin_keys) |p| { - if (p.node != node) continue; - p.node = new_node; - } - - // Insert this page and destroy the old page - self.pages.insertBefore(node, new_node); - self.pages.remove(node); - self.destroyNode(node); - - new_page.assertIntegrity(); - return new_node; -} - /// Possible dimensions to increase capacity for. pub const IncreaseCapacity = enum { styles, @@ -4991,21 +4893,14 @@ test "PageList grow prune required with a single page" { // behavior during a refactor. This is setting up a scenario that is // possible to trigger a bug (#2280). { - // Adjust our capacity until our page is larger than the standard size. + // Increase our capacity until our page is larger than the standard size. // This is important because it triggers a scenario where our calculated // minSize() which is supposed to accommodate 2 pages is no longer true. - var cap = std_capacity; while (true) { - cap.grapheme_bytes *= 2; - const layout = Page.layout(cap); + const layout = Page.layout(s.pages.first.?.data.capacity); if (layout.total_size > std_size) break; + _ = try s.increaseCapacity(s.pages.first.?, .grapheme_bytes); } - - // Adjust to that capacity. After we should still have one page. - _ = try s.adjustCapacity( - s.pages.first.?, - .{ .grapheme_bytes = cap.grapheme_bytes }, - ); try testing.expect(s.pages.first != null); try testing.expect(s.pages.first == s.pages.last); } @@ -6424,168 +6319,6 @@ test "PageList eraseRowBounded exhausts pages invalidates viewport offset cache" }, s.scrollbar()); } -test "PageList adjustCapacity to increase styles" { - const testing = std.testing; - const alloc = testing.allocator; - - var s = try init(alloc, 2, 2, 0); - defer s.deinit(); - { - try testing.expect(s.pages.first == s.pages.last); - const page = &s.pages.first.?.data; - - // Write all our data so we can assert its the same after - for (0..s.rows) |y| { - for (0..s.cols) |x| { - const rac = page.getRowAndCell(x, y); - rac.cell.* = .{ - .content_tag = .codepoint, - .content = .{ .codepoint = @intCast(x) }, - }; - } - } - } - - // Increase our styles - _ = try s.adjustCapacity( - s.pages.first.?, - .{ .styles = std_capacity.styles * 2 }, - ); - - { - try testing.expect(s.pages.first == s.pages.last); - const page = &s.pages.first.?.data; - for (0..s.rows) |y| { - for (0..s.cols) |x| { - const rac = page.getRowAndCell(x, y); - try testing.expectEqual( - @as(u21, @intCast(x)), - rac.cell.content.codepoint, - ); - } - } - } -} - -test "PageList adjustCapacity to increase graphemes" { - const testing = std.testing; - const alloc = testing.allocator; - - var s = try init(alloc, 2, 2, 0); - defer s.deinit(); - { - try testing.expect(s.pages.first == s.pages.last); - const page = &s.pages.first.?.data; - - // Write all our data so we can assert its the same after - for (0..s.rows) |y| { - for (0..s.cols) |x| { - const rac = page.getRowAndCell(x, y); - rac.cell.* = .{ - .content_tag = .codepoint, - .content = .{ .codepoint = @intCast(x) }, - }; - } - } - } - - // Increase our graphemes - _ = try s.adjustCapacity( - s.pages.first.?, - .{ .grapheme_bytes = std_capacity.grapheme_bytes * 2 }, - ); - - { - try testing.expect(s.pages.first == s.pages.last); - const page = &s.pages.first.?.data; - for (0..s.rows) |y| { - for (0..s.cols) |x| { - const rac = page.getRowAndCell(x, y); - try testing.expectEqual( - @as(u21, @intCast(x)), - rac.cell.content.codepoint, - ); - } - } - } -} - -test "PageList adjustCapacity to increase hyperlinks" { - const testing = std.testing; - const alloc = testing.allocator; - - var s = try init(alloc, 2, 2, 0); - defer s.deinit(); - { - try testing.expect(s.pages.first == s.pages.last); - const page = &s.pages.first.?.data; - - // Write all our data so we can assert its the same after - for (0..s.rows) |y| { - for (0..s.cols) |x| { - const rac = page.getRowAndCell(x, y); - rac.cell.* = .{ - .content_tag = .codepoint, - .content = .{ .codepoint = @intCast(x) }, - }; - } - } - } - - // Increase our graphemes - _ = try s.adjustCapacity( - s.pages.first.?, - .{ .hyperlink_bytes = @max(std_capacity.hyperlink_bytes * 2, 2048) }, - ); - - { - try testing.expect(s.pages.first == s.pages.last); - const page = &s.pages.first.?.data; - for (0..s.rows) |y| { - for (0..s.cols) |x| { - const rac = page.getRowAndCell(x, y); - try testing.expectEqual( - @as(u21, @intCast(x)), - rac.cell.content.codepoint, - ); - } - } - } -} - -test "PageList adjustCapacity after col shrink" { - const testing = std.testing; - const alloc = testing.allocator; - - var s = try init(alloc, 10, 2, 0); - defer s.deinit(); - - // Shrink columns - this updates size.cols but not capacity.cols - try s.resize(.{ .cols = 5, .reflow = false }); - try testing.expectEqual(5, s.cols); - - { - const page = &s.pages.first.?.data; - // capacity.cols is still 10, but size.cols should be 5 - try testing.expectEqual(5, page.size.cols); - try testing.expect(page.capacity.cols >= 10); - } - - // Now adjust capacity (e.g., to increase styles) - // This should preserve the current size.cols, not revert to capacity.cols - _ = try s.adjustCapacity( - s.pages.first.?, - .{ .styles = std_capacity.styles * 2 }, - ); - - { - const page = &s.pages.first.?.data; - // After adjustCapacity, size.cols should still be 5, not 10 - try testing.expectEqual(5, page.size.cols); - try testing.expectEqual(5, s.cols); - } -} - test "PageList increaseCapacity to increase styles" { const testing = std.testing; const alloc = testing.allocator; @@ -6799,13 +6532,12 @@ test "PageList increaseCapacity returns OutOfSpace at max capacity" { var s = try init(alloc, 2, 2, 0); defer s.deinit(); - // Set styles capacity to a value that will overflow when doubled - // We need to create a page with capacity at more than half of max + // Keep increasing styles capacity until we're at more than half of max const max_styles = std.math.maxInt(size.StyleCountInt); const half_max = max_styles / 2 + 1; - - // Adjust capacity to near-max first - _ = try s.adjustCapacity(s.pages.first.?, .{ .styles = half_max }); + while (s.pages.first.?.data.capacity.styles < half_max) { + _ = try s.increaseCapacity(s.pages.first.?, .styles); + } // Now increaseCapacity should fail with OutOfSpace try testing.expectError( @@ -11485,12 +11217,10 @@ test "PageList grow reuses non-standard page without leak" { var s = try init(alloc, 80, 24, 3 * std_size); defer s.deinit(); - // Save the first page node before adjustment - const first_before = s.pages.first.?; - - // Adjust the first page to have non-standard capacity. We use a small - // increase that makes it just slightly larger than std_size. - _ = try s.adjustCapacity(first_before, .{ .grapheme_bytes = std_size + 1 }); + // Increase the first page capacity to make it non-standard (larger than std_size). + while (s.pages.first.?.data.memory.len <= std_size) { + _ = try s.increaseCapacity(s.pages.first.?, .grapheme_bytes); + } // The first page should now have non-standard memory size. try testing.expect(s.pages.first.?.data.memory.len > std_size); From 6b2455828e1aba8898d3c11fc5b9e06f78a1b91c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 15 Jan 2026 15:26:12 -0800 Subject: [PATCH 11/37] terminal: resizeWithoutReflowGrowCols can only fail for OOM --- src/terminal/PageList.zig | 89 +++++++++++++++++++++++++++++++++------ 1 file changed, 75 insertions(+), 14 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index bf2071497..043a8f191 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1652,7 +1652,7 @@ const ReflowCursor = struct { } }; -fn resizeWithoutReflow(self: *PageList, opts: Resize) !void { +fn resizeWithoutReflow(self: *PageList, opts: Resize) Allocator.Error!void { // We only set the new min_max_size if we're not reflowing. If we are // reflowing, then resize handles this for us. const old_min_max_size = self.min_max_size; @@ -1812,26 +1812,44 @@ fn resizeWithoutReflowGrowCols( self: *PageList, cols: size.CellCountInt, chunk: PageIterator.Chunk, -) !void { +) Allocator.Error!void { assert(cols > self.cols); const page = &chunk.node.data; - const cap = try page.capacity.adjust(.{ .cols = cols }); // Update our col count const old_cols = self.cols; - self.cols = cap.cols; + self.cols = cols; errdefer self.cols = old_cols; // Unlikely fast path: we have capacity in the page. This // is only true if we resized to less cols earlier. - if (page.capacity.cols >= cap.cols) { - page.size.cols = cap.cols; + if (page.capacity.cols >= cols) { + page.size.cols = cols; return; } // Likely slow path: we don't have capacity, so we need // to allocate a page, and copy the old data into it. + // Try to fit our new column size into our existing page capacity. + // If that doesn't work then use a non-standard page with the + // given columns. + const cap = page.capacity.adjust( + .{ .cols = cols }, + ) catch |err| err: { + comptime assert(@TypeOf(err) == error{OutOfMemory}); + + // We verify all maxed out page layouts work. + var cap = page.capacity; + cap.cols = cols; + + // We're growing columns so we can only get less rows so use + // the lesser of our capacity and size so we minimize wasted + // rows. + cap.rows = @min(page.size.rows, cap.rows); + break :err cap; + }; + // On error, we need to undo all the pages we've added. const prev = chunk.node.prev; errdefer { @@ -1895,6 +1913,31 @@ fn resizeWithoutReflowGrowCols( assert(prev_page.size.rows <= prev_page.capacity.rows); } + // If we have an error, we clear the rows we just added to our prev page. + const prev_copied = copied; + errdefer if (prev_copied > 0) { + const prev_page = &prev.?.data; + const prev_size = prev_page.size.rows - prev_copied; + const prev_rows = prev_page.rows.ptr(prev_page.memory)[prev_size..prev_page.size.rows]; + for (prev_rows) |*row| prev_page.clearCells( + row, + 0, + prev_page.size.cols, + ); + prev_page.size.rows = prev_size; + }; + + // We delete any of the nodes we added. + errdefer { + var it = chunk.node.prev; + while (it) |node| { + if (node == prev) break; + it = node.prev; + self.pages.remove(node); + self.destroyNode(node); + } + } + // We need to loop because our col growth may force us // to split pages. while (copied < page.size.rows) { @@ -1907,19 +1950,33 @@ fn resizeWithoutReflowGrowCols( // Perform the copy const y_start = copied; - const y_end = copied + len; - const src_rows = page.rows.ptr(page.memory)[y_start..y_end]; + const src_rows = page.rows.ptr(page.memory)[y_start .. copied + len]; const dst_rows = new_node.data.rows.ptr(new_node.data.memory)[0..len]; for (dst_rows, src_rows) |*dst_row, *src_row| { new_node.data.size.rows += 1; - errdefer new_node.data.size.rows -= 1; - try new_node.data.cloneRowFrom( + if (new_node.data.cloneRowFrom( page, dst_row, src_row, - ); + )) |_| { + copied += 1; + } else |err| { + // I don't THINK this should be possible, because while our + // row count may diminish due to the adjustment, our + // prior capacity should have been sufficient to hold all the + // managed memory. + log.warn( + "unexpected cloneRowFrom failure during resizeWithoutReflowGrowCols: {}", + .{err}, + ); + + // We can actually safely handle this though by exiting + // this loop early and cutting our copy short. + new_node.data.size.rows -= 1; + break; + } } - copied = y_end; + const y_end = copied; // Insert our new page self.pages.insertBefore(chunk.node, new_node); @@ -1936,6 +1993,10 @@ fn resizeWithoutReflowGrowCols( } assert(copied == page.size.rows); + // Our prior errdeferes are invalid after this point so ensure + // we don't have any more errors. + errdefer comptime unreachable; + // Remove the old page. // Deallocate the old page. self.pages.remove(chunk.node); @@ -2514,7 +2575,7 @@ inline fn growRequiredForActive(self: *const PageList) bool { /// adhere to max_size. /// /// This returns the newly allocated page node if there is one. -pub fn grow(self: *PageList) !?*List.Node { +pub fn grow(self: *PageList) Allocator.Error!?*List.Node { defer self.assertIntegrity(); const last = self.pages.last.?; @@ -2533,7 +2594,7 @@ pub fn grow(self: *PageList) !?*List.Node { // Get the layout first so our failable work is done early. // We'll need this for both paths. - const cap = try std_capacity.adjust(.{ .cols = self.cols }); + const cap = initialCapacity(self.cols); // If allocation would exceed our max size, we prune the first page. // We don't need to reallocate because we can simply reuse that first From d626984418cf91910543683f64ba95d24815f4ca Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 15 Jan 2026 20:31:22 -0800 Subject: [PATCH 12/37] terminal: reflowCursor improve error handling on assumed cases --- src/terminal/PageList.zig | 233 +++++++++++++++++++++++++++++++------- 1 file changed, 190 insertions(+), 43 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 043a8f191..755e0a393 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -992,32 +992,71 @@ fn resizeCols( } else null; defer if (preserved_cursor) |c| self.untrackPin(c.tracked_pin); - const first = self.pages.first.?; - var it = self.rowIterator(.right_down, .{ .screen = .{} }, null); + // Create the first node that contains our reflow. + const first_rewritten_node = node: { + const page = &self.pages.first.?.data; + const cap = page.capacity.adjust( + .{ .cols = cols }, + ) catch |err| err: { + comptime assert(@TypeOf(err) == error{OutOfMemory}); - const dst_node = try self.createPage(try first.data.capacity.adjust(.{ .cols = cols })); - dst_node.data.size.rows = 1; + // We verify all maxed out page layouts work. + var cap = page.capacity; + cap.cols = cols; + + // We're growing columns so we can only get less rows so use + // the lesser of our capacity and size so we minimize wasted + // rows. + cap.rows = @min(page.size.rows, cap.rows); + break :err cap; + }; + + const node = try self.createPage(cap); + node.data.size.rows = 1; + break :node node; + }; + + // We need to grab our rowIterator now before we rewrite our + // linked list below. + var it = self.rowIterator( + .right_down, + .{ .screen = .{} }, + null, + ); + errdefer { + // If an error occurs, we're in a pretty disastrous broken state, + // but we should still try to clean up our leaked memory. Free + // any of the remaining orphaned pages from before. If we reflowed + // successfully this will be null. + var node_: ?*Node = if (it.chunk) |chunk| chunk.node else null; + while (node_) |node| { + node_ = node.next; + self.destroyNode(node); + } + } // Set our new page as the only page. This orphans the existing pages // in the list, but that's fine since we're gonna delete them anyway. - self.pages.first = dst_node; - self.pages.last = dst_node; + self.pages.first = first_rewritten_node; + self.pages.last = first_rewritten_node; // Reflow all our rows. { - var dst_cursor = ReflowCursor.init(dst_node); + var reflow_cursor: ReflowCursor = .init(first_rewritten_node); while (it.next()) |row| { - try dst_cursor.reflowRow(self, row); + try reflow_cursor.reflowRow(self, row); - // Once we're done reflowing a page, destroy it. + // Once we're done reflowing a page, destroy it immediately. + // This frees memory and makes it more likely in memory + // constrained environments that the next reflow will work. if (row.y == row.node.data.size.rows - 1) { self.destroyNode(row.node); } } // At the end of the reflow, setup our total row cache - // log.warn("total old={} new={}", .{ self.total_rows, dst_cursor.total_rows }); - self.total_rows = dst_cursor.total_rows; + // log.warn("total old={} new={}", .{ self.total_rows, reflow_cursor.total_rows }); + self.total_rows = reflow_cursor.total_rows; } // If our total rows is less than our active rows, we need to grow. @@ -1114,16 +1153,11 @@ const ReflowCursor = struct { const src_page: *Page = &row.node.data; const src_row = row.rowAndCell().row; const src_y = row.y; - - // Inherit increased styles or grapheme bytes from - // the src page we're reflowing from for new pages. - const cap = try src_page.capacity.adjust(.{ .cols = self.page.size.cols }); - const cells = src_row.cells.ptr(src_page.memory)[0..src_page.size.cols]; + // Calculate the columns in this row. First up we trim non-semantic + // rightmost blanks. var cols_len = src_page.size.cols; - - // If the row is wrapped, all empty cells are meaningful. if (!src_row.wrap) { while (cols_len > 0) { if (!cells[cols_len - 1].isEmpty()) break; @@ -1145,9 +1179,10 @@ const ReflowCursor = struct { // If this pin is in the blanks on the right and past the end // of the dst col width then we move it to the end of the dst // col width instead. - if (p.x >= cols_len) { - p.x = @min(p.x, cap.cols - 1 - self.x); - } + if (p.x >= cols_len) p.x = @min( + p.x, + self.page.size.cols - 1 - self.x, + ); // We increase our col len to at least include this pin. // This ensures that blank rows with pins are processed, @@ -1162,16 +1197,29 @@ const ReflowCursor = struct { // If this blank row was a wrap continuation somehow // then we won't need to write it since it should be // a part of the previously written row. - if (!src_row.wrap_continuation) { - self.new_rows += 1; - } + if (!src_row.wrap_continuation) self.new_rows += 1; return; } + // Inherit increased styles or grapheme bytes from the src page + // we're reflowing from for new pages. + const cap = src_page.capacity.adjust( + .{ .cols = self.page.size.cols }, + ) catch |err| err: { + comptime assert(@TypeOf(err) == error{OutOfMemory}); + + var cap = src_page.capacity; + cap.cols = self.page.size.cols; + // We're already a non-standard page. We don't want to + // inherit a massive set of rows, so cap it at our std size. + cap.rows = @min(src_page.size.rows, std_capacity.rows); + break :err cap; + }; + // Our row isn't blank, write any new rows we deferred. while (self.new_rows > 0) { - self.new_rows -= 1; try self.cursorScrollOrNewPage(list, cap); + self.new_rows -= 1; } self.copyRowMetadata(src_row); @@ -1326,12 +1374,30 @@ const ReflowCursor = struct { } } - // This shouldn't fail since we made sure we have space above. - try self.page.setGraphemes(self.page_row, self.page_cell, cps); + self.page.setGraphemes( + self.page_row, + self.page_cell, + cps, + ) catch |err| { + // This shouldn't fail since we made sure we have space + // above. There is no reasonable behavior we can take here + // so we have a warn level log. This is ALMOST non-recoverable, + // though we choose to recover by corrupting the cell + // to a non-grapheme codepoint. + log.err("setGraphemes failed after capacity increase err={}", .{err}); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + // Unsafe builds we throw away grapheme data! + cell.content_tag = .codepoint; + cell.content = .{ .codepoint = 0xFFFD }; + }; } // Copy hyperlink data. - if (cell.hyperlink) { + if (cell.hyperlink) hyperlink: { const src_id = src_page.lookupHyperlink(cell).?; const src_link = src_page.hyperlink_set.get(src_page.memory, src_id); @@ -1371,10 +1437,25 @@ const ReflowCursor = struct { } } + const dst_link = src_link.dupe( + src_page, + self.page, + ) catch |err| { + // This shouldn't fail since we did a capacity + // check above. + log.err("link dupe failed with capacity check err={}", .{err}); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + cell.hyperlink = false; + break :hyperlink; + }; + const dst_id = self.page.hyperlink_set.addWithIdContext( self.page.memory, - // We made sure there was enough capacity for this above. - try src_link.dupe(src_page, self.page), + dst_link, src_id, .{ .page = self.page }, ) catch |err| id: { @@ -1387,29 +1468,80 @@ const ReflowCursor = struct { error.NeedsRehash => null, }); + // We dupe the link again, and don't have to worry about + // freeing the other one because increasing the capacity + // destroyed the prior page. + const dst_link2 = src_link.dupe( + src_page, + self.page, + ) catch |err2| { + // This shouldn't fail since we did a capacity + // check above. + log.err("link dupe failed with capacity check err={}", .{err2}); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + cell.hyperlink = false; + break :hyperlink; + }; + // We assume this one will succeed. We dupe the link // again, and don't have to worry about the other one // because increasing the capacity naturally clears up // any managed memory not associated with a cell yet. - break :id try self.page.hyperlink_set.addWithIdContext( + break :id self.page.hyperlink_set.addWithIdContext( self.page.memory, - try src_link.dupe(src_page, self.page), + dst_link2, src_id, .{ .page = self.page }, - ); + ) catch |err2| { + // This shouldn't happen since we increased capacity + // above so we handle it like the other similar + // cases and log it, crash in safe builds, and + // remove the hyperlink in unsafe builds. + log.err( + "addWithIdContext failed after capacity increase err={}", + .{err2}, + ); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + dst_link2.free(self.page); + cell.hyperlink = false; + break :hyperlink; + }; } orelse src_id; - // We expect this to succeed due to the - // hyperlinkCapacity check we did before. - try self.page.setHyperlink( + // We expect this to succeed due to the hyperlinkCapacity + // check we did before. If it doesn't succeed let's + // log it, crash (in safe builds), and clear our state. + self.page.setHyperlink( self.page_row, self.page_cell, dst_id, - ); + ) catch |err| { + log.err( + "setHyperlink failed after capacity increase err={}", + .{err}, + ); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + // Unsafe builds we throw away hyperlink data! + self.page.hyperlink_set.release(self.page.memory, dst_id); + cell.hyperlink = false; + break :hyperlink; + }; } // Copy style data. - if (cell.hasStyling()) { + if (cell.hasStyling()) style: { const style = src_page.styles.get( src_page.memory, cell.style_id, @@ -1430,15 +1562,29 @@ const ReflowCursor = struct { }); // We assume this one will succeed. - break :id try self.page.styles.addWithId( + break :id self.page.styles.addWithId( self.page.memory, style, cell.style_id, - ); + ) catch |err2| { + // Should not fail since we just modified capacity + // above. Log it, crash in safe builds, clear style + // in unsafe builds. + log.err( + "addWithId failed after capacity increase err={}", + .{err2}, + ); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + cell.style_id = stylepkg.default_id; + break :style; + }; } orelse cell.style_id; self.page_row.styled = true; - self.page_cell.style_id = id; } @@ -1574,12 +1720,13 @@ const ReflowCursor = struct { self: *ReflowCursor, list: *PageList, cap: Capacity, - ) !void { + ) Allocator.Error!void { // Remember our new row count so we can restore it // after reinitializing our cursor on the new page. const new_rows = self.new_rows; const node = try list.createPage(cap); + errdefer comptime unreachable; node.data.size.rows = 1; list.pages.insertAfter(self.node, node); @@ -1594,7 +1741,7 @@ const ReflowCursor = struct { self: *ReflowCursor, list: *PageList, cap: Capacity, - ) !void { + ) Allocator.Error!void { // The functions below may overwrite self so we need to cache // our total rows. We add one because no matter what when this // returns we'll have one more row added. From 25643ec806e5be828b236b8ead5814a0fcabaff8 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 06:22:05 -0800 Subject: [PATCH 13/37] terminal: reflowRow extract writeCell --- src/terminal/PageList.zig | 733 ++++++++++++++++++++------------------ 1 file changed, 384 insertions(+), 349 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 755e0a393..0ca00f45c 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1247,355 +1247,19 @@ const ReflowCursor = struct { } } - const cell = &cells[x]; - x += 1; - - // Copy cell contents. - switch (cell.content_tag) { - .codepoint, - .codepoint_grapheme, - => switch (cell.wide) { - .narrow => self.page_cell.* = cell.*, - - .wide => if (self.page.size.cols > 1) { - if (self.x == self.page.size.cols - 1) { - // If there's a wide character in the last column of - // the reflowed page then we need to insert a spacer - // head and wrap before handling it. - self.page_cell.* = .{ - .content_tag = .codepoint, - .content = .{ .codepoint = 0 }, - .wide = .spacer_head, - }; - - // Decrement the source position so that when we - // loop we'll process this source cell again, - // since we can't copy it into a spacer head. - x -= 1; - - // Move to the next row (this sets pending wrap - // which will cause us to wrap on the next - // iteration). - self.cursorForward(); - continue; - } else { - self.page_cell.* = cell.*; - } - } else { - // Edge case, when resizing to 1 column, wide - // characters are just destroyed and replaced - // with empty narrow cells. - self.page_cell.content.codepoint = 0; - self.page_cell.wide = .narrow; - self.cursorForward(); - // Skip spacer tail so it doesn't cause a wrap. - x += 1; - continue; - }, - - .spacer_tail => if (self.page.size.cols > 1) { - self.page_cell.* = cell.*; - } else { - // Edge case, when resizing to 1 column, wide - // characters are just destroyed and replaced - // with empty narrow cells, so we should just - // discard any spacer tails. - continue; - }, - - .spacer_head => { - // Spacer heads should be ignored. If we need a - // spacer head in our reflowed page, it is added - // when processing the wide cell it belongs to. - continue; - }, - }, - - .bg_color_palette, - .bg_color_rgb, - => { - // These are guaranteed to have no style or grapheme - // data associated with them so we can fast path them. - self.page_cell.* = cell.*; - self.cursorForward(); - continue; - }, + switch (try self.writeCell( + list, + &cells[x], + src_page, + )) { + // Wrote the cell, move to the next. + .success => x += 1, + // Wrote the cell but request to skip the next so skip it. + // This is used for things like spacers. + .skip_next => x += 2, + // Didn't write the cell, repeat writing this same cell. + .repeat => {}, } - - // These will create issues by trying to clone managed memory that - // isn't set if the current dst row needs to be moved to a new page. - // They'll be fixed once we do properly copy the relevant memory. - self.page_cell.content_tag = .codepoint; - self.page_cell.hyperlink = false; - self.page_cell.style_id = stylepkg.default_id; - - // std.log.warn("\nsrc_y={} src_x={} dst_y={} dst_x={} dst_cols={} cp={X} wide={} page_cell_wide={}", .{ - // src_y, - // x, - // self.y, - // self.x, - // self.page.size.cols, - // cell.content.codepoint, - // cell.wide, - // self.page_cell.wide, - // }); - - // Copy grapheme data. - if (cell.content_tag == .codepoint_grapheme) { - // Copy the graphemes - const cps = src_page.lookupGrapheme(cell).?; - - // If our page can't support an additional cell - // with graphemes then we increase capacity. - if (self.page.graphemeCount() >= self.page.graphemeCapacity()) { - try self.increaseCapacity( - list, - .grapheme_bytes, - ); - } - - // Attempt to allocate the space that would be required - // for these graphemes, and if it's not available, then - // increase capacity. Keep trying until we succeed. - while (true) { - if (self.page.grapheme_alloc.alloc( - u21, - self.page.memory, - cps.len, - )) |slice| { - self.page.grapheme_alloc.free( - self.page.memory, - slice, - ); - break; - } else |_| { - // Grow our capacity until we can fit the extra bytes. - try self.increaseCapacity(list, .grapheme_bytes); - } - } - - self.page.setGraphemes( - self.page_row, - self.page_cell, - cps, - ) catch |err| { - // This shouldn't fail since we made sure we have space - // above. There is no reasonable behavior we can take here - // so we have a warn level log. This is ALMOST non-recoverable, - // though we choose to recover by corrupting the cell - // to a non-grapheme codepoint. - log.err("setGraphemes failed after capacity increase err={}", .{err}); - if (comptime std.debug.runtime_safety) { - // Force a crash with safe builds. - unreachable; - } - - // Unsafe builds we throw away grapheme data! - cell.content_tag = .codepoint; - cell.content = .{ .codepoint = 0xFFFD }; - }; - } - - // Copy hyperlink data. - if (cell.hyperlink) hyperlink: { - const src_id = src_page.lookupHyperlink(cell).?; - const src_link = src_page.hyperlink_set.get(src_page.memory, src_id); - - // If our page can't support an additional cell - // with a hyperlink then we increase capacity. - if (self.page.hyperlinkCount() >= self.page.hyperlinkCapacity()) { - try self.increaseCapacity(list, .hyperlink_bytes); - } - - // Ensure that the string alloc has sufficient capacity - // to dupe the link (and the ID if it's not implicit). - const additional_required_string_capacity = - src_link.uri.len + - switch (src_link.id) { - .explicit => |v| v.len, - .implicit => 0, - }; - // Keep trying until we have enough capacity. - while (true) { - if (self.page.string_alloc.alloc( - u8, - self.page.memory, - additional_required_string_capacity, - )) |slice| { - // We have enough capacity, free the test alloc. - self.page.string_alloc.free( - self.page.memory, - slice, - ); - break; - } else |_| { - // Grow our capacity until we can fit the extra bytes. - try self.increaseCapacity( - list, - .string_bytes, - ); - } - } - - const dst_link = src_link.dupe( - src_page, - self.page, - ) catch |err| { - // This shouldn't fail since we did a capacity - // check above. - log.err("link dupe failed with capacity check err={}", .{err}); - if (comptime std.debug.runtime_safety) { - // Force a crash with safe builds. - unreachable; - } - - cell.hyperlink = false; - break :hyperlink; - }; - - const dst_id = self.page.hyperlink_set.addWithIdContext( - self.page.memory, - dst_link, - src_id, - .{ .page = self.page }, - ) catch |err| id: { - // If the add failed then either the set needs to grow - // or it needs to be rehashed. Either one of those can - // be accomplished by increasing capacity, either with - // no actual change or with an increased hyperlink cap. - try self.increaseCapacity(list, switch (err) { - error.OutOfMemory => .hyperlink_bytes, - error.NeedsRehash => null, - }); - - // We dupe the link again, and don't have to worry about - // freeing the other one because increasing the capacity - // destroyed the prior page. - const dst_link2 = src_link.dupe( - src_page, - self.page, - ) catch |err2| { - // This shouldn't fail since we did a capacity - // check above. - log.err("link dupe failed with capacity check err={}", .{err2}); - if (comptime std.debug.runtime_safety) { - // Force a crash with safe builds. - unreachable; - } - - cell.hyperlink = false; - break :hyperlink; - }; - - // We assume this one will succeed. We dupe the link - // again, and don't have to worry about the other one - // because increasing the capacity naturally clears up - // any managed memory not associated with a cell yet. - break :id self.page.hyperlink_set.addWithIdContext( - self.page.memory, - dst_link2, - src_id, - .{ .page = self.page }, - ) catch |err2| { - // This shouldn't happen since we increased capacity - // above so we handle it like the other similar - // cases and log it, crash in safe builds, and - // remove the hyperlink in unsafe builds. - log.err( - "addWithIdContext failed after capacity increase err={}", - .{err2}, - ); - if (comptime std.debug.runtime_safety) { - // Force a crash with safe builds. - unreachable; - } - - dst_link2.free(self.page); - cell.hyperlink = false; - break :hyperlink; - }; - } orelse src_id; - - // We expect this to succeed due to the hyperlinkCapacity - // check we did before. If it doesn't succeed let's - // log it, crash (in safe builds), and clear our state. - self.page.setHyperlink( - self.page_row, - self.page_cell, - dst_id, - ) catch |err| { - log.err( - "setHyperlink failed after capacity increase err={}", - .{err}, - ); - if (comptime std.debug.runtime_safety) { - // Force a crash with safe builds. - unreachable; - } - - // Unsafe builds we throw away hyperlink data! - self.page.hyperlink_set.release(self.page.memory, dst_id); - cell.hyperlink = false; - break :hyperlink; - }; - } - - // Copy style data. - if (cell.hasStyling()) style: { - const style = src_page.styles.get( - src_page.memory, - cell.style_id, - ).*; - - const id = self.page.styles.addWithId( - self.page.memory, - style, - cell.style_id, - ) catch |err| id: { - // If the add failed then either the set needs to grow - // or it needs to be rehashed. Either one of those can - // be accomplished by increasing capacity, either with - // no actual change or with an increased style cap. - try self.increaseCapacity(list, switch (err) { - error.OutOfMemory => .styles, - error.NeedsRehash => null, - }); - - // We assume this one will succeed. - break :id self.page.styles.addWithId( - self.page.memory, - style, - cell.style_id, - ) catch |err2| { - // Should not fail since we just modified capacity - // above. Log it, crash in safe builds, clear style - // in unsafe builds. - log.err( - "addWithId failed after capacity increase err={}", - .{err2}, - ); - if (comptime std.debug.runtime_safety) { - // Force a crash with safe builds. - unreachable; - } - - cell.style_id = stylepkg.default_id; - break :style; - }; - } orelse cell.style_id; - - self.page_row.styled = true; - self.page_cell.style_id = id; - } - - if (comptime build_options.kitty_graphics) { - // Copy Kitty virtual placeholder status - if (cell.codepoint() == kitty.graphics.unicode.placeholder) { - self.page_row.kitty_virtual_placeholder = true; - } - } - - self.cursorForward(); } // If the source row isn't wrapped then we should scroll afterwards. @@ -1604,6 +1268,377 @@ const ReflowCursor = struct { } } + /// Write a cell. On error, this will not unwrite the cell but + /// the cell may be incomplete (but valid). For example, if the source + /// cell is styled and we failed to allocate space for styles, the + /// written cell may not be styled but it is valid. + /// + /// The key failure to recognize for callers is when we can't increase + /// capacity in our destination page. In this case, the caller may want + /// to split the page at this row, rewrite the row into a new page + /// and continue from there. + /// + /// But this function guarantees the terminal/page will be in a + /// coherent state even on error. + fn writeCell( + self: *ReflowCursor, + list: *PageList, + cell: *const pagepkg.Cell, + src_page: *const Page, + ) IncreaseCapacityError!enum { + success, + repeat, + skip_next, + } { + // Copy cell contents. + switch (cell.content_tag) { + .codepoint, + .codepoint_grapheme, + => switch (cell.wide) { + .narrow => self.page_cell.* = cell.*, + + .wide => if (self.page.size.cols > 1) { + if (self.x == self.page.size.cols - 1) { + // If there's a wide character in the last column of + // the reflowed page then we need to insert a spacer + // head and wrap before handling it. + self.page_cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = 0 }, + .wide = .spacer_head, + }; + + // Move to the next row (this sets pending wrap + // which will cause us to wrap on the next + // iteration). + self.cursorForward(); + + // Decrement the source position so that when we + // loop we'll process this source cell again, + // since we can't copy it into a spacer head. + return .repeat; + } else { + self.page_cell.* = cell.*; + } + } else { + // Edge case, when resizing to 1 column, wide + // characters are just destroyed and replaced + // with empty narrow cells. + self.page_cell.content.codepoint = 0; + self.page_cell.wide = .narrow; + self.cursorForward(); + + // Skip spacer tail so it doesn't cause a wrap. + return .skip_next; + }, + + .spacer_tail => if (self.page.size.cols > 1) { + self.page_cell.* = cell.*; + } else { + // Edge case, when resizing to 1 column, wide + // characters are just destroyed and replaced + // with empty narrow cells, so we should just + // discard any spacer tails. + return .success; + }, + + .spacer_head => { + // Spacer heads should be ignored. If we need a + // spacer head in our reflowed page, it is added + // when processing the wide cell it belongs to. + return .success; + }, + }, + + .bg_color_palette, + .bg_color_rgb, + => { + // These are guaranteed to have no style or grapheme + // data associated with them so we can fast path them. + self.page_cell.* = cell.*; + self.cursorForward(); + return .success; + }, + } + + // These will create issues by trying to clone managed memory that + // isn't set if the current dst row needs to be moved to a new page. + // They'll be fixed once we do properly copy the relevant memory. + self.page_cell.content_tag = .codepoint; + self.page_cell.hyperlink = false; + self.page_cell.style_id = stylepkg.default_id; + + // std.log.warn("\nsrc_y={} src_x={} dst_y={} dst_x={} dst_cols={} cp={X} wide={} page_cell_wide={}", .{ + // src_y, + // x, + // self.y, + // self.x, + // self.page.size.cols, + // cell.content.codepoint, + // cell.wide, + // self.page_cell.wide, + // }); + + if (comptime build_options.kitty_graphics) { + // Copy Kitty virtual placeholder status + if (cell.codepoint() == kitty.graphics.unicode.placeholder) { + self.page_row.kitty_virtual_placeholder = true; + } + } + + // Copy grapheme data. + if (cell.content_tag == .codepoint_grapheme) { + // Copy the graphemes + const cps = src_page.lookupGrapheme(cell).?; + + // If our page can't support an additional cell + // with graphemes then we increase capacity. + if (self.page.graphemeCount() >= self.page.graphemeCapacity()) { + try self.increaseCapacity( + list, + .grapheme_bytes, + ); + } + + // Attempt to allocate the space that would be required + // for these graphemes, and if it's not available, then + // increase capacity. Keep trying until we succeed. + while (true) { + if (self.page.grapheme_alloc.alloc( + u21, + self.page.memory, + cps.len, + )) |slice| { + self.page.grapheme_alloc.free( + self.page.memory, + slice, + ); + break; + } else |_| { + // Grow our capacity until we can fit the extra bytes. + try self.increaseCapacity(list, .grapheme_bytes); + } + } + + self.page.setGraphemes( + self.page_row, + self.page_cell, + cps, + ) catch |err| { + // This shouldn't fail since we made sure we have space + // above. There is no reasonable behavior we can take here + // so we have a warn level log. This is ALMOST non-recoverable, + // though we choose to recover by corrupting the cell + // to a non-grapheme codepoint. + log.err("setGraphemes failed after capacity increase err={}", .{err}); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + // Unsafe builds we throw away grapheme data! + cell.content_tag = .codepoint; + cell.content = .{ .codepoint = 0xFFFD }; + }; + } + + // Copy hyperlink data. + if (cell.hyperlink) hyperlink: { + const src_id = src_page.lookupHyperlink(cell).?; + const src_link = src_page.hyperlink_set.get(src_page.memory, src_id); + + // If our page can't support an additional cell + // with a hyperlink then we increase capacity. + if (self.page.hyperlinkCount() >= self.page.hyperlinkCapacity()) { + try self.increaseCapacity(list, .hyperlink_bytes); + } + + // Ensure that the string alloc has sufficient capacity + // to dupe the link (and the ID if it's not implicit). + const additional_required_string_capacity = + src_link.uri.len + + switch (src_link.id) { + .explicit => |v| v.len, + .implicit => 0, + }; + // Keep trying until we have enough capacity. + while (true) { + if (self.page.string_alloc.alloc( + u8, + self.page.memory, + additional_required_string_capacity, + )) |slice| { + // We have enough capacity, free the test alloc. + self.page.string_alloc.free( + self.page.memory, + slice, + ); + break; + } else |_| { + // Grow our capacity until we can fit the extra bytes. + try self.increaseCapacity( + list, + .string_bytes, + ); + } + } + + const dst_link = src_link.dupe( + src_page, + self.page, + ) catch |err| { + // This shouldn't fail since we did a capacity + // check above. + log.err("link dupe failed with capacity check err={}", .{err}); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + break :hyperlink; + }; + + const dst_id = self.page.hyperlink_set.addWithIdContext( + self.page.memory, + dst_link, + src_id, + .{ .page = self.page }, + ) catch |err| id: { + // Always free our original link in case the increaseCap + // call fails so we aren't leaking memory. + dst_link.free(self.page); + + // If the add failed then either the set needs to grow + // or it needs to be rehashed. Either one of those can + // be accomplished by increasing capacity, either with + // no actual change or with an increased hyperlink cap. + try self.increaseCapacity(list, switch (err) { + error.OutOfMemory => .hyperlink_bytes, + error.NeedsRehash => null, + }); + + // We need to recreate the link into the new page. + const dst_link2 = src_link.dupe( + src_page, + self.page, + ) catch |err2| { + // This shouldn't fail since we did a capacity + // check above. + log.err("link dupe failed with capacity check err={}", .{err2}); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + cell.hyperlink = false; + break :hyperlink; + }; + + // We assume this one will succeed. We dupe the link + // again, and don't have to worry about the other one + // because increasing the capacity naturally clears up + // any managed memory not associated with a cell yet. + break :id self.page.hyperlink_set.addWithIdContext( + self.page.memory, + dst_link2, + src_id, + .{ .page = self.page }, + ) catch |err2| { + // This shouldn't happen since we increased capacity + // above so we handle it like the other similar + // cases and log it, crash in safe builds, and + // remove the hyperlink in unsafe builds. + log.err( + "addWithIdContext failed after capacity increase err={}", + .{err2}, + ); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + dst_link2.free(self.page); + cell.hyperlink = false; + break :hyperlink; + }; + } orelse src_id; + + // We expect this to succeed due to the hyperlinkCapacity + // check we did before. If it doesn't succeed let's + // log it, crash (in safe builds), and clear our state. + self.page.setHyperlink( + self.page_row, + self.page_cell, + dst_id, + ) catch |err| { + log.err( + "setHyperlink failed after capacity increase err={}", + .{err}, + ); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + // Unsafe builds we throw away hyperlink data! + self.page.hyperlink_set.release(self.page.memory, dst_id); + cell.hyperlink = false; + break :hyperlink; + }; + } + + // Copy style data. + if (cell.hasStyling()) style: { + const style = src_page.styles.get( + src_page.memory, + cell.style_id, + ).*; + + const id = self.page.styles.addWithId( + self.page.memory, + style, + cell.style_id, + ) catch |err| id: { + // If the add failed then either the set needs to grow + // or it needs to be rehashed. Either one of those can + // be accomplished by increasing capacity, either with + // no actual change or with an increased style cap. + try self.increaseCapacity(list, switch (err) { + error.OutOfMemory => .styles, + error.NeedsRehash => null, + }); + + // We assume this one will succeed. + break :id self.page.styles.addWithId( + self.page.memory, + style, + cell.style_id, + ) catch |err2| { + // Should not fail since we just modified capacity + // above. Log it, crash in safe builds, clear style + // in unsafe builds. + log.err( + "addWithId failed after capacity increase err={}", + .{err2}, + ); + if (comptime std.debug.runtime_safety) { + // Force a crash with safe builds. + unreachable; + } + + cell.style_id = stylepkg.default_id; + break :style; + }; + } orelse cell.style_id; + + self.page_row.styled = true; + self.page_cell.style_id = id; + } + + self.cursorForward(); + return .success; + } + /// Create a new page in the provided list with the provided /// capacity then clone the row currently being worked on to /// it and delete it from the old page. Places cursor in the @@ -1654,7 +1689,7 @@ const ReflowCursor = struct { self: *ReflowCursor, list: *PageList, adjustment: ?IncreaseCapacity, - ) !void { + ) IncreaseCapacityError!void { const old_x = self.x; const old_y = self.y; const old_total_rows = self.total_rows; From 97621ece935228efe82bfb451d3be75051e12cf1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 07:21:17 -0800 Subject: [PATCH 14/37] terminal: handle reflowRow OutOfSpace by no-op --- src/terminal/PageList.zig | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 0ca00f45c..2045ccc01 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -874,7 +874,7 @@ pub const Resize = struct { /// Resize /// TODO: docs -pub fn resize(self: *PageList, opts: Resize) !void { +pub fn resize(self: *PageList, opts: Resize) Allocator.Error!void { defer self.assertIntegrity(); if (comptime std.debug.runtime_safety) { @@ -944,7 +944,7 @@ fn resizeCols( self: *PageList, cols: size.CellCountInt, cursor: ?Resize.Cursor, -) !void { +) Allocator.Error!void { assert(cols != self.cols); // Update our cols. We have to do this early because grow() that we @@ -1149,7 +1149,7 @@ const ReflowCursor = struct { self: *ReflowCursor, list: *PageList, row: Pin, - ) !void { + ) Allocator.Error!void { const src_page: *Page = &row.node.data; const src_row = row.rowAndCell().row; const src_y = row.y; @@ -1247,11 +1247,11 @@ const ReflowCursor = struct { } } - switch (try self.writeCell( + if (self.writeCell( list, &cells[x], src_page, - )) { + )) |result| switch (result) { // Wrote the cell, move to the next. .success => x += 1, // Wrote the cell but request to skip the next so skip it. @@ -1259,6 +1259,24 @@ const ReflowCursor = struct { .skip_next => x += 2, // Didn't write the cell, repeat writing this same cell. .repeat => {}, + } else |err| switch (err) { + // System out of memory, we can't fix this. + error.OutOfMemory => return error.OutOfMemory, + + // We reached the capacity of a single page and can't + // add any more of some type of managed memory. + error.OutOfSpace => { + log.warn("OutOfSpace during reflow at src_y={} src_x={} dst_y={} dst_x={} cp={X}", .{ + src_y, + x, + self.y, + self.x, + cells[x].content.codepoint, + }); + + // TODO: Split the page + x += 1; + }, } } From 42321cc7d59f3c3f02c14e225c0d374c8a002339 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 07:24:37 -0800 Subject: [PATCH 15/37] terminal: write to the proper cell --- src/terminal/PageList.zig | 160 ++++++++++++++++++++------------------ 1 file changed, 85 insertions(+), 75 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 2045ccc01..84518e8b7 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1308,84 +1308,98 @@ const ReflowCursor = struct { repeat, skip_next, } { - // Copy cell contents. - switch (cell.content_tag) { - .codepoint, - .codepoint_grapheme, - => switch (cell.wide) { - .narrow => self.page_cell.* = cell.*, + // Initialize self.page_cell with basic, unmanaged memory contents. + { + // This must not fail because we want to make sure we atomically + // setup our page cell to be valid. + errdefer comptime unreachable; - .wide => if (self.page.size.cols > 1) { - if (self.x == self.page.size.cols - 1) { - // If there's a wide character in the last column of - // the reflowed page then we need to insert a spacer - // head and wrap before handling it. - self.page_cell.* = .{ - .content_tag = .codepoint, - .content = .{ .codepoint = 0 }, - .wide = .spacer_head, - }; + // Copy cell contents. + switch (cell.content_tag) { + .codepoint, + .codepoint_grapheme, + => switch (cell.wide) { + .narrow => self.page_cell.* = cell.*, - // Move to the next row (this sets pending wrap - // which will cause us to wrap on the next - // iteration). + .wide => if (self.page.size.cols > 1) { + if (self.x == self.page.size.cols - 1) { + // If there's a wide character in the last column of + // the reflowed page then we need to insert a spacer + // head and wrap before handling it. + self.page_cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = 0 }, + .wide = .spacer_head, + }; + + // Move to the next row (this sets pending wrap + // which will cause us to wrap on the next + // iteration). + self.cursorForward(); + + // Decrement the source position so that when we + // loop we'll process this source cell again, + // since we can't copy it into a spacer head. + return .repeat; + } else { + self.page_cell.* = cell.*; + } + } else { + // Edge case, when resizing to 1 column, wide + // characters are just destroyed and replaced + // with empty narrow cells. + self.page_cell.content.codepoint = 0; + self.page_cell.wide = .narrow; self.cursorForward(); - // Decrement the source position so that when we - // loop we'll process this source cell again, - // since we can't copy it into a spacer head. - return .repeat; - } else { + // Skip spacer tail so it doesn't cause a wrap. + return .skip_next; + }, + + .spacer_tail => if (self.page.size.cols > 1) { self.page_cell.* = cell.*; - } - } else { - // Edge case, when resizing to 1 column, wide - // characters are just destroyed and replaced - // with empty narrow cells. - self.page_cell.content.codepoint = 0; - self.page_cell.wide = .narrow; - self.cursorForward(); + } else { + // Edge case, when resizing to 1 column, wide + // characters are just destroyed and replaced + // with empty narrow cells, so we should just + // discard any spacer tails. + return .success; + }, - // Skip spacer tail so it doesn't cause a wrap. - return .skip_next; + .spacer_head => { + // Spacer heads should be ignored. If we need a + // spacer head in our reflowed page, it is added + // when processing the wide cell it belongs to. + return .success; + }, }, - .spacer_tail => if (self.page.size.cols > 1) { + .bg_color_palette, + .bg_color_rgb, + => { + // These are guaranteed to have no style or grapheme + // data associated with them so we can fast path them. self.page_cell.* = cell.*; - } else { - // Edge case, when resizing to 1 column, wide - // characters are just destroyed and replaced - // with empty narrow cells, so we should just - // discard any spacer tails. + self.cursorForward(); return .success; }, + } - .spacer_head => { - // Spacer heads should be ignored. If we need a - // spacer head in our reflowed page, it is added - // when processing the wide cell it belongs to. - return .success; - }, - }, + // These will create issues by trying to clone managed memory that + // isn't set if the current dst row needs to be moved to a new page. + // They'll be fixed once we do properly copy the relevant memory. + self.page_cell.content_tag = .codepoint; + self.page_cell.hyperlink = false; + self.page_cell.style_id = stylepkg.default_id; - .bg_color_palette, - .bg_color_rgb, - => { - // These are guaranteed to have no style or grapheme - // data associated with them so we can fast path them. - self.page_cell.* = cell.*; - self.cursorForward(); - return .success; - }, + if (comptime build_options.kitty_graphics) { + // Copy Kitty virtual placeholder status + if (cell.codepoint() == kitty.graphics.unicode.placeholder) { + self.page_row.kitty_virtual_placeholder = true; + } + } } - // These will create issues by trying to clone managed memory that - // isn't set if the current dst row needs to be moved to a new page. - // They'll be fixed once we do properly copy the relevant memory. - self.page_cell.content_tag = .codepoint; - self.page_cell.hyperlink = false; - self.page_cell.style_id = stylepkg.default_id; - // std.log.warn("\nsrc_y={} src_x={} dst_y={} dst_x={} dst_cols={} cp={X} wide={} page_cell_wide={}", .{ // src_y, // x, @@ -1397,12 +1411,10 @@ const ReflowCursor = struct { // self.page_cell.wide, // }); - if (comptime build_options.kitty_graphics) { - // Copy Kitty virtual placeholder status - if (cell.codepoint() == kitty.graphics.unicode.placeholder) { - self.page_row.kitty_virtual_placeholder = true; - } - } + // From this point on we're moving on to failable, managed memory. + // If we reach an error, we do the minimal cleanup necessary to + // not leave dangling memory but otherwise we gracefully degrade + // into some functional but not strictly correct cell. // Copy grapheme data. if (cell.content_tag == .codepoint_grapheme) { @@ -1455,8 +1467,8 @@ const ReflowCursor = struct { } // Unsafe builds we throw away grapheme data! - cell.content_tag = .codepoint; - cell.content = .{ .codepoint = 0xFFFD }; + self.page_cell.content_tag = .codepoint; + self.page_cell.content = .{ .codepoint = 0xFFFD }; }; } @@ -1548,7 +1560,6 @@ const ReflowCursor = struct { unreachable; } - cell.hyperlink = false; break :hyperlink; }; @@ -1576,7 +1587,6 @@ const ReflowCursor = struct { } dst_link2.free(self.page); - cell.hyperlink = false; break :hyperlink; }; } orelse src_id; @@ -1600,7 +1610,7 @@ const ReflowCursor = struct { // Unsafe builds we throw away hyperlink data! self.page.hyperlink_set.release(self.page.memory, dst_id); - cell.hyperlink = false; + self.page_cell.hyperlink = false; break :hyperlink; }; } @@ -1644,7 +1654,7 @@ const ReflowCursor = struct { unreachable; } - cell.style_id = stylepkg.default_id; + self.page_cell.style_id = stylepkg.default_id; break :style; }; } orelse cell.style_id; From c9d15949d85564aa47763de2da341ae6654f6e90 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 08:22:53 -0800 Subject: [PATCH 16/37] terminal: on reflow OutOfSpace, move last row to new page and try again --- src/terminal/PageList.zig | 60 +++++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 84518e8b7..710c0aaa7 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1264,18 +1264,22 @@ const ReflowCursor = struct { error.OutOfMemory => return error.OutOfMemory, // We reached the capacity of a single page and can't - // add any more of some type of managed memory. - error.OutOfSpace => { - log.warn("OutOfSpace during reflow at src_y={} src_x={} dst_y={} dst_x={} cp={X}", .{ - src_y, - x, - self.y, - self.x, - cells[x].content.codepoint, - }); - - // TODO: Split the page + // add any more of some type of managed memory. When this + // happens we split out the current row we're working on + // into a new page and continue from there. + error.OutOfSpace => if (self.y == 0) { + // If we're already on the first-row, we can't split + // any further, so we just ignore bad cells and take + // corrupted (but valid) cell contents. + log.warn("reflowRow OutOfSpace on first row, discarding cell managed memory", .{}); x += 1; + self.cursorForward(); + } else { + // Move our last row to a new page. + try self.moveLastRowToNewPage(list, cap); + + // Do NOT increment x so that we retry writing + // the same existing cell. }, } } @@ -1683,7 +1687,7 @@ const ReflowCursor = struct { self: *ReflowCursor, list: *PageList, cap: Capacity, - ) !void { + ) Allocator.Error!void { assert(self.y == self.page.size.rows - 1); assert(!self.pending_wrap); @@ -1693,15 +1697,41 @@ const ReflowCursor = struct { const old_x = self.x; try self.cursorNewPage(list, cap); + assert(self.node != old_node); + assert(self.y == 0); // Restore the x position of the cursor. self.cursorAbsolute(old_x, 0); - // We expect to have enough capacity to clone the row. - try self.page.cloneRowFrom(old_page, self.page_row, old_row); + // Copy our old data. This should NOT fail because we have the + // capacity of the old page which already fits the data we requested. + self.page.cloneRowFrom( + old_page, + self.page_row, + old_row, + ) catch |err| { + log.err( + "error cloning single row for moveLastRowToNewPage err={}", + .{err}, + ); + @panic("unexpected copy row failure"); + }; + + // Move any tracked pins from that last row into this new node. + { + const pin_keys = list.tracked_pins.keys(); + for (pin_keys) |p| { + if (&p.node.data != old_page or + p.y != old_page.size.rows - 1) continue; + + p.node = self.node; + p.y = self.y; + // p.x remains the same since we're copying the row as-is + } + } // Clear the row from the old page and truncate it. - old_page.clearCells(old_row, 0, self.page.size.cols); + old_page.clearCells(old_row, 0, old_page.size.cols); old_page.size.rows -= 1; // If that was the last row in that page From ec0a1500984403eefffbbf7528cf22230b08cfb1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 09:12:41 -0800 Subject: [PATCH 17/37] terminal: moveLastRowToNewPage needs to fix up total_rows --- src/terminal/PageList.zig | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 710c0aaa7..69edcafb1 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1696,10 +1696,18 @@ const ReflowCursor = struct { const old_row = self.page_row; const old_x = self.x; + // Our total row count never changes, because we're removing one + // row from the last page and moving it into a new page. + const old_total_rows = self.total_rows; + defer self.total_rows = old_total_rows; + try self.cursorNewPage(list, cap); assert(self.node != old_node); assert(self.y == 0); + // We have no cleanup for our old state from here on out. No failures! + errdefer comptime unreachable; + // Restore the x position of the cursor. self.cursorAbsolute(old_x, 0); @@ -1752,7 +1760,7 @@ const ReflowCursor = struct { const old_y = self.y; const old_total_rows = self.total_rows; - self.* = .init(node: { + const node = node: { // Pause integrity checks because the total row count won't // be correct during a reflow. list.pauseIntegrityChecks(true); @@ -1761,8 +1769,12 @@ const ReflowCursor = struct { self.node, adjustment, ); - }); + }; + // We must not fail after this, we've modified our self.node + // and we need to fix it up. + errdefer comptime unreachable; + self.* = .init(node); self.cursorAbsolute(old_x, old_y); self.total_rows = old_total_rows; } @@ -1824,7 +1836,6 @@ const ReflowCursor = struct { list.pages.insertAfter(self.node, node); self.* = .init(node); - self.new_rows = new_rows; } From f89b6433c2470f363609ffbf3fbc4fbd2dd99c58 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 16 Jan 2026 16:26:54 -0500 Subject: [PATCH 18/37] osc: add failing test for osc 133 parsing trailing ; This actually causes a crash lol, bad indexing of a slice with `1..0` because it's `key.len + 1 ..` and the length is `0`. --- src/terminal/osc/parsers/semantic_prompt.zig | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/terminal/osc/parsers/semantic_prompt.zig b/src/terminal/osc/parsers/semantic_prompt.zig index 510fe3447..811b6d055 100644 --- a/src/terminal/osc/parsers/semantic_prompt.zig +++ b/src/terminal/osc/parsers/semantic_prompt.zig @@ -348,6 +348,18 @@ test "OSC 133: prompt_start with special_key empty" { try testing.expect(cmd.prompt_start.special_key == false); } +test "OSC 133: prompt_start with trailing ;" { + const testing = std.testing; + + var p: Parser = .init(null); + + const input = "133;A;"; + for (input) |ch| p.next(ch); + + const cmd = p.end(null).?.*; + try testing.expect(cmd == .prompt_start); +} + test "OSC 133: prompt_start with click_events true" { const testing = std.testing; From 4e5c1dcdc17d67757d32faeb2bffe0e27376db00 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 16 Jan 2026 16:29:39 -0500 Subject: [PATCH 19/37] osc: fix bad indexing for empty kv in semantic prompt --- src/terminal/osc/parsers/semantic_prompt.zig | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/terminal/osc/parsers/semantic_prompt.zig b/src/terminal/osc/parsers/semantic_prompt.zig index 811b6d055..ac7298267 100644 --- a/src/terminal/osc/parsers/semantic_prompt.zig +++ b/src/terminal/osc/parsers/semantic_prompt.zig @@ -186,6 +186,15 @@ const SemanticPromptKVIterator = struct { break :kv kv; }; + // If we have an empty item, we return an empty key and value. + // + // This allows for trailing semicolons, but also lets us parse + // (or rather, ignore) empty fields; for example `a=b;;e=f`. + if (kv.len < 1) return .{ + .key = kv, + .value = kv, + }; + const key = key: { const index = std.mem.indexOfScalar(u8, kv, '=') orelse break :key kv; kv[index] = 0; From 85a3d623b2f3452a49eb634c10e46ba6746d0cd6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 13:42:34 -0800 Subject: [PATCH 20/37] terminal: increaseCapacity should reach maxInt before overflow --- src/terminal/PageList.zig | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 69edcafb1..b027f54d4 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3003,8 +3003,16 @@ pub fn increaseCapacity( // overflow it means we're out of space in this dimension, // since pages can take up to their maxInt capacity in any // category. - const new = std.math.mul(Int, old, 2) catch |err| { + const new = std.math.mul( + Int, + old, + 2, + ) catch |err| overflow: { comptime assert(@TypeOf(err) == error{Overflow}); + // Our final doubling would overflow since maxInt is + // 2^N - 1 for an unsignged int of N bits. So, if we overflow + // and we haven't used all the bits, use all the bits. + if (old < std.math.maxInt(Int)) break :overflow std.math.maxInt(Int); return error.OutOfSpace; }; @field(cap, field_name) = new; @@ -6844,18 +6852,19 @@ test "PageList increaseCapacity returns OutOfSpace at max capacity" { var s = try init(alloc, 2, 2, 0); defer s.deinit(); - // Keep increasing styles capacity until we're at more than half of max + // Keep increasing styles capacity until we get OutOfSpace const max_styles = std.math.maxInt(size.StyleCountInt); - const half_max = max_styles / 2 + 1; - while (s.pages.first.?.data.capacity.styles < half_max) { - _ = try s.increaseCapacity(s.pages.first.?, .styles); + while (true) { + _ = s.increaseCapacity( + s.pages.first.?, + .styles, + ) catch |err| { + // Before OutOfSpace, we should have reached maxInt + try testing.expectEqual(error.OutOfSpace, err); + try testing.expectEqual(max_styles, s.pages.first.?.data.capacity.styles); + break; + }; } - - // Now increaseCapacity should fail with OutOfSpace - try testing.expectError( - error.OutOfSpace, - s.increaseCapacity(s.pages.first.?, .styles), - ); } test "PageList increaseCapacity after col shrink" { From 69066200ef24985f4f5214f0adc38ce6c40cb975 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 16 Jan 2026 16:58:58 -0500 Subject: [PATCH 21/37] fix: handle double tmux control mode exit command --- src/termio/stream_handler.zig | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index c647e3ba2..082a0fa10 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -398,11 +398,16 @@ pub const StreamHandler = struct { break :tmux; }, - .exit => if (self.tmux_viewer) |viewer| { - // Free our viewer state - viewer.deinit(); - self.alloc.destroy(viewer); - self.tmux_viewer = null; + .exit => { + // Free our viewer state if we have one + if (self.tmux_viewer) |viewer| { + viewer.deinit(); + self.alloc.destroy(viewer); + self.tmux_viewer = null; + } + + // And always break since we assert below + // that we're not handling an exit command. break :tmux; }, From 442a395850f30a2b6fc455414359368dfbefb9fc Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 14:24:02 -0800 Subject: [PATCH 22/37] terminal: ensure our std_capacity fits within max page size --- src/terminal/PageList.zig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index b027f54d4..d8354916b 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -292,7 +292,8 @@ fn initialCapacity(cols: size.CellCountInt) Capacity { comptime { var cap = std_capacity; cap.cols = std.math.maxInt(size.CellCountInt); - _ = Page.layout(cap); + const layout = Page.layout(cap); + assert(layout.total_size <= size.max_page_size); } if (std_capacity.adjust( From df35363b15ef1c1284ab0ad3615e43fec4777316 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 14:35:14 -0800 Subject: [PATCH 23/37] terminal: more robust handling of max_page_size not 100% --- src/terminal/PageList.zig | 19 ++++++++++++++++++- src/terminal/page.zig | 2 ++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index d8354916b..3e6a39a3f 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -414,6 +414,10 @@ fn initPages( const pooled = layout.total_size <= std_size; const page_alloc = pool.pages.arena.child_allocator; + // Guaranteed by comptime checks in initialCapacity but + // redundant here for safety. + assert(layout.total_size <= size.max_page_size); + var rem = rows; while (rem > 0) { const node = try pool.nodes.create(); @@ -2091,7 +2095,7 @@ fn resizeWithoutReflowGrowCols( ) catch |err| err: { comptime assert(@TypeOf(err) == error{OutOfMemory}); - // We verify all maxed out page layouts work. + // We verify all maxed out page layouts don't overflow, var cap = page.capacity; cap.cols = cols; @@ -3017,6 +3021,14 @@ pub fn increaseCapacity( return error.OutOfSpace; }; @field(cap, field_name) = new; + + // If our capacity exceeds the maximum page size, treat it + // as an OutOfSpace because things like page splitting will + // help. + const layout = Page.layout(cap); + if (layout.total_size > size.max_page_size) { + return error.OutOfSpace; + } }, }; @@ -3091,6 +3103,11 @@ inline fn createPageExt( const pooled = layout.total_size <= std_size; const page_alloc = pool.pages.arena.child_allocator; + // It would be better to encode this into the Zig error handling + // system but that is a big undertaking and we only have a few + // centralized call sites so it is handled on its own currently. + assert(layout.total_size <= size.max_page_size); + // Our page buffer comes from our standard memory pool if it // is within our standard size since this is what the pool // dispenses. Otherwise, we use the heap allocator to allocate. diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 1150027a4..075f03b57 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -2046,6 +2046,8 @@ test "Page.layout can take a maxed capacity" { @field(cap, field.name) = std.math.maxInt(field.type); } + // Note that a max capacity will exceed our max_page_size so we + // can't init a page with it, but it should layout. _ = Page.layout(cap); } From 464c31328e6ced1c8bbcafe42aac67097d7c505e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 16 Jan 2026 20:43:42 -0800 Subject: [PATCH 24/37] terminal: grow prune check should not prune if required for active Fixes #10352 The bug was that non-standard pages would mix the old `growRequiredForActive` check and make our active area insufficient in the PageList. But, since scrollbars now require we have a cached `total_rows` that our safety checks always verify, we can remove the old linked list traversal and switch to some simple math in general across all page sizes. --- src/terminal/PageList.zig | 104 ++++++++++++++++++++++++++++++-------- 1 file changed, 82 insertions(+), 22 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 3e6a39a3f..c5897b7e7 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -2812,18 +2812,6 @@ pub fn maxSize(self: *const PageList) usize { return @max(self.explicit_max_size, self.min_max_size); } -/// Returns true if we need to grow into our active area. -inline fn growRequiredForActive(self: *const PageList) bool { - var rows: usize = 0; - var page = self.pages.last; - while (page) |p| : (page = p.prev) { - rows += p.data.size.rows; - if (rows >= self.rows) return false; - } - - return true; -} - /// Grow the active area by exactly one row. /// /// This may allocate, but also may not if our current page has more @@ -2864,16 +2852,22 @@ pub fn grow(self: *PageList) Allocator.Error!?*List.Node { self.pages.first != self.pages.last and self.page_size + PagePool.item_size > self.maxSize()) prune: { - // If we need to add more memory to ensure our active area is - // satisfied then we do not prune. - if (self.growRequiredForActive()) break :prune; - const first = self.pages.popFirst().?; assert(first != last); // Decrease our total row count from the pruned page self.total_rows -= first.data.size.rows; + // If our total row count is now less than our required + // rows then we can't prune. The "+ 1" is because we'll add one + // more row below. + if (self.total_rows + 1 < self.rows) { + self.pages.prepend(first); + assert(self.pages.first == first); + self.total_rows += first.data.size.rows; + break :prune; + } + // If we have a pin viewport cache then we need to update it. if (self.viewport == .pin) viewport: { if (self.viewport_pin_row_offset) |*v| { @@ -2902,12 +2896,8 @@ pub fn grow(self: *PageList) Allocator.Error!?*List.Node { } self.viewport_pin.garbage = false; - // If our first node has non-standard memory size, we can't reuse - // it. This is because our initBuf below would change the underlying - // memory length which would break our memory free outside the pool. - // It is easiest in this case to prune the node. + // Non-standard pages can't be reused, just destroy them. if (first.data.memory.len > std_size) { - // Node is already removed so we can just destroy it. self.destroyNode(first); break :prune; } @@ -11585,7 +11575,7 @@ test "PageList grow reuses non-standard page without leak" { try testing.expect(s.pages.first.?.data.memory.len > std_size); // Verify we have enough rows for active area (so prune path isn't skipped) - try testing.expect(!s.growRequiredForActive()); + try testing.expect(s.totalRows() >= s.rows); // Verify last page is full (so grow will need to allocate/reuse) try testing.expect(s.pages.last.?.data.size.rows == s.pages.last.?.data.capacity.rows); @@ -11619,3 +11609,73 @@ test "PageList grow reuses non-standard page without leak" { try testing.expectEqual(0, tracked_pin.y); try testing.expect(tracked_pin.garbage); } + +test "PageList grow non-standard page prune protection" { + const testing = std.testing; + const alloc = testing.allocator; + + // This test specifically verifies the fix for the bug where pruning a + // non-standard page would cause totalRows() < self.rows. + // + // Bug trigger conditions (all must be true simultaneously): + // 1. first page is non-standard (memory.len > std_size) + // 2. page_size + PagePool.item_size > maxSize() (triggers prune consideration) + // 3. pages.first != pages.last (have multiple pages) + // 4. total_rows >= self.rows (have enough rows for active area) + // 5. total_rows - first.size.rows + 1 < self.rows (prune would lose too many) + + // This is kind of magic and likely depends on std_size. + const rows_count = 600; + var s = try init(alloc, 80, rows_count, std_size); + defer s.deinit(); + + // Make the first page non-standard + while (s.pages.first.?.data.memory.len <= std_size) { + _ = try s.increaseCapacity( + s.pages.first.?, + .grapheme_bytes, + ); + } + try testing.expect(s.pages.first.?.data.memory.len > std_size); + + const first_page_node = s.pages.first.?; + const first_page_cap = first_page_node.data.capacity.rows; + + // Fill first page to capacity + while (first_page_node.data.size.rows < first_page_cap) _ = try s.grow(); + + // Grow until we have a second page (first page fills up first) + var second_node: ?*List.Node = null; + while (s.pages.first == s.pages.last) second_node = try s.grow(); + try testing.expect(s.pages.first != s.pages.last); + + // Fill the second page to capacity so that the next grow() triggers prune + const last_node = s.pages.last.?; + const second_cap = last_node.data.capacity.rows; + while (last_node.data.size.rows < second_cap) _ = try s.grow(); + + // Now the last page is full. The next grow must either: + // 1. Prune the first page and reuse it, OR + // 2. Allocate a new page + const total = s.totalRows(); + const would_remain = total - first_page_cap + 1; + + // Verify the bug condition is present: pruning first page would leave < rows + try testing.expect(would_remain < s.rows); + + // Verify prune path conditions are met + try testing.expect(s.pages.first != s.pages.last); + try testing.expect(s.page_size + PagePool.item_size > s.maxSize()); + try testing.expect(s.totalRows() >= s.rows); + + // Verify last page is at capacity (so grow must prune or allocate new) + try testing.expectEqual(second_cap, last_node.data.size.rows); + + // The next grow should trigger prune consideration. + // Without the fix, this would destroy the non-standard first page, + // leaving only second_cap + 1 rows, which is < self.rows. + _ = try s.grow(); + + // Verify the invariant holds - the fix prevents the destructive prune + try testing.expect(s.totalRows() >= s.rows); +} From 73a8d64b8a0410f3f57225bae52eeb8c7f0f0bbb Mon Sep 17 00:00:00 2001 From: mitchellh <1299+mitchellh@users.noreply.github.com> Date: Sun, 18 Jan 2026 00:16:59 +0000 Subject: [PATCH 25/37] deps: Update iTerm2 color schemes --- build.zig.zon | 4 ++-- build.zig.zon.json | 6 +++--- build.zig.zon.nix | 6 +++--- build.zig.zon.txt | 2 +- flatpak/zig-packages.json | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/build.zig.zon b/build.zig.zon index c3e2de9f8..d5c06259a 100644 --- a/build.zig.zon +++ b/build.zig.zon @@ -116,8 +116,8 @@ // Other .apple_sdk = .{ .path = "./pkg/apple-sdk" }, .iterm2_themes = .{ - .url = "https://deps.files.ghostty.org/ghostty-themes-release-20251229-150532-f279991.tgz", - .hash = "N-V-__8AAIdIAwAO4ro1DOaG7QTFq3ewrTQIViIKJ3lKY6lV", + .url = "https://deps.files.ghostty.org/ghostty-themes-release-20260112-150707-28c8f5b.tgz", + .hash = "N-V-__8AAIdIAwDt5PxH-cwCxEcTfw4jBV8sR6fZ_XLh-cR7", .lazy = true, }, }, diff --git a/build.zig.zon.json b/build.zig.zon.json index 022a7401e..b12216bd9 100644 --- a/build.zig.zon.json +++ b/build.zig.zon.json @@ -54,10 +54,10 @@ "url": "https://github.com/ocornut/imgui/archive/refs/tags/v1.92.5-docking.tar.gz", "hash": "sha256-yBbCDox18+Fa6Gc1DnmSVQLRpqhZOLsac7iSfl8x+cs=" }, - "N-V-__8AAIdIAwAO4ro1DOaG7QTFq3ewrTQIViIKJ3lKY6lV": { + "N-V-__8AAIdIAwDt5PxH-cwCxEcTfw4jBV8sR6fZ_XLh-cR7": { "name": "iterm2_themes", - "url": "https://deps.files.ghostty.org/ghostty-themes-release-20251229-150532-f279991.tgz", - "hash": "sha256-bWKQxRggz/ZLr6w0Zt/hTnnAAb13VQWV70ScCsNFIZk=" + "url": "https://deps.files.ghostty.org/ghostty-themes-release-20260112-150707-28c8f5b.tgz", + "hash": "sha256-NIqF12KqXhIrP+LyBtg6WtkHxNUdWOyziAdq8S45RrU=" }, "N-V-__8AAIC5lwAVPJJzxnCAahSvZTIlG-HhtOvnM1uh-66x": { "name": "jetbrains_mono", diff --git a/build.zig.zon.nix b/build.zig.zon.nix index d845ea10e..430619e74 100644 --- a/build.zig.zon.nix +++ b/build.zig.zon.nix @@ -171,11 +171,11 @@ in }; } { - name = "N-V-__8AAIdIAwAO4ro1DOaG7QTFq3ewrTQIViIKJ3lKY6lV"; + name = "N-V-__8AAIdIAwDt5PxH-cwCxEcTfw4jBV8sR6fZ_XLh-cR7"; path = fetchZigArtifact { name = "iterm2_themes"; - url = "https://deps.files.ghostty.org/ghostty-themes-release-20251229-150532-f279991.tgz"; - hash = "sha256-bWKQxRggz/ZLr6w0Zt/hTnnAAb13VQWV70ScCsNFIZk="; + url = "https://deps.files.ghostty.org/ghostty-themes-release-20260112-150707-28c8f5b.tgz"; + hash = "sha256-NIqF12KqXhIrP+LyBtg6WtkHxNUdWOyziAdq8S45RrU="; }; } { diff --git a/build.zig.zon.txt b/build.zig.zon.txt index 0eeb7c5f1..72597a650 100644 --- a/build.zig.zon.txt +++ b/build.zig.zon.txt @@ -6,7 +6,7 @@ https://deps.files.ghostty.org/breakpad-b99f444ba5f6b98cac261cbb391d8766b34a5918 https://deps.files.ghostty.org/fontconfig-2.14.2.tar.gz https://deps.files.ghostty.org/freetype-1220b81f6ecfb3fd222f76cf9106fecfa6554ab07ec7fdc4124b9bb063ae2adf969d.tar.gz https://deps.files.ghostty.org/gettext-0.24.tar.gz -https://deps.files.ghostty.org/ghostty-themes-release-20251229-150532-f279991.tgz +https://deps.files.ghostty.org/ghostty-themes-release-20260112-150707-28c8f5b.tgz https://deps.files.ghostty.org/glslang-12201278a1a05c0ce0b6eb6026c65cd3e9247aa041b1c260324bf29cee559dd23ba1.tar.gz https://deps.files.ghostty.org/gobject-2025-11-08-23-1.tar.zst https://deps.files.ghostty.org/gtk4-layer-shell-1.1.0.tar.gz diff --git a/flatpak/zig-packages.json b/flatpak/zig-packages.json index 7749eb67e..3e2b1e26d 100644 --- a/flatpak/zig-packages.json +++ b/flatpak/zig-packages.json @@ -67,9 +67,9 @@ }, { "type": "archive", - "url": "https://deps.files.ghostty.org/ghostty-themes-release-20251229-150532-f279991.tgz", - "dest": "vendor/p/N-V-__8AAIdIAwAO4ro1DOaG7QTFq3ewrTQIViIKJ3lKY6lV", - "sha256": "6d6290c51820cff64bafac3466dfe14e79c001bd77550595ef449c0ac3452199" + "url": "https://deps.files.ghostty.org/ghostty-themes-release-20260112-150707-28c8f5b.tgz", + "dest": "vendor/p/N-V-__8AAIdIAwDt5PxH-cwCxEcTfw4jBV8sR6fZ_XLh-cR7", + "sha256": "348a85d762aa5e122b3fe2f206d83a5ad907c4d51d58ecb388076af12e3946b5" }, { "type": "archive", From 5423d64c6aba7c5e160a076c808cb72b39a092e3 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Sat, 17 Jan 2026 20:13:05 -0500 Subject: [PATCH 26/37] ssh-cache: use AtomicFile to write the cache file We previously wrote our new cache file into a temporary directory and the (atomically) renamed it to the canonical cache file path. This rename operation unfortunately only works when both files are on the same file system, and that's not always the case (e.g. when $TMPDIR is on its own file system). Instead, we can use Zig's AtomicFile to safely perform this operation inside of the cache directory. There's a new risk of a crash leaving the temporary file around in this directory (and not getting cleaned up like $TMPDIR-based files), but the probability is low and those files will only be readable by the creating user (mode 0o600). There's a new test cash that verifies the expected AtomicFile clean up behavior. I also switched the file-oriented tests to use testing.tmpDir rather than using our application-level TempDir type. --- src/cli/ssh-cache/DiskCache.zig | 75 +++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/src/cli/ssh-cache/DiskCache.zig b/src/cli/ssh-cache/DiskCache.zig index 62620ecb0..6214d0429 100644 --- a/src/cli/ssh-cache/DiskCache.zig +++ b/src/cli/ssh-cache/DiskCache.zig @@ -9,7 +9,6 @@ const assert = @import("../../quirks.zig").inlineAssert; const Allocator = std.mem.Allocator; const internal_os = @import("../../os/main.zig"); const xdg = internal_os.xdg; -const TempDir = internal_os.TempDir; const Entry = @import("Entry.zig"); // 512KB - sufficient for approximately 10k entries @@ -125,7 +124,7 @@ pub fn add( break :update .updated; }; - try self.writeCacheFile(alloc, entries, null); + try self.writeCacheFile(entries, null); return result; } @@ -166,7 +165,7 @@ pub fn remove( alloc.free(kv.value.terminfo_version); } - try self.writeCacheFile(alloc, entries, null); + try self.writeCacheFile(entries, null); } /// Check if a hostname exists in the cache. @@ -209,32 +208,30 @@ fn fixupPermissions(file: std.fs.File) (std.fs.File.StatError || std.fs.File.Chm fn writeCacheFile( self: DiskCache, - alloc: Allocator, entries: std.StringHashMap(Entry), expire_days: ?u32, ) !void { - var td: TempDir = try .init(); - defer td.deinit(); + const cache_dir = std.fs.path.dirname(self.path) orelse return error.InvalidCachePath; + const cache_basename = std.fs.path.basename(self.path); - const tmp_file = try td.dir.createFile("ssh-cache", .{ .mode = 0o600 }); - defer tmp_file.close(); - const tmp_path = try td.dir.realpathAlloc(alloc, "ssh-cache"); - defer alloc.free(tmp_path); + var dir = try std.fs.cwd().openDir(cache_dir, .{}); + defer dir.close(); var buf: [1024]u8 = undefined; - var writer = tmp_file.writer(&buf); + var atomic_file = try dir.atomicFile(cache_basename, .{ + .mode = 0o600, + .write_buffer = &buf, + }); + defer atomic_file.deinit(); + var iter = entries.iterator(); while (iter.next()) |kv| { // Only write non-expired entries if (kv.value_ptr.isExpired(expire_days)) continue; - try kv.value_ptr.format(&writer.interface); + try kv.value_ptr.format(&atomic_file.file_writer.interface); } - // Don't forget to flush!! - try writer.interface.flush(); - - // Atomic replace - try std.fs.renameAbsolute(tmp_path, self.path); + try atomic_file.finish(); } /// List all entries in the cache. @@ -382,16 +379,16 @@ test "disk cache clear" { const alloc = testing.allocator; // Create our path - var td: TempDir = try .init(); - defer td.deinit(); + var tmp = testing.tmpDir(.{}); + defer tmp.cleanup(); var buf: [4096]u8 = undefined; { - var file = try td.dir.createFile("cache", .{}); + var file = try tmp.dir.createFile("cache", .{}); defer file.close(); var file_writer = file.writer(&buf); try file_writer.interface.writeAll("HELLO!"); } - const path = try td.dir.realpathAlloc(alloc, "cache"); + const path = try tmp.dir.realpathAlloc(alloc, "cache"); defer alloc.free(path); // Setup our cache @@ -401,7 +398,7 @@ test "disk cache clear" { // Verify the file is gone try testing.expectError( error.FileNotFound, - td.dir.openFile("cache", .{}), + tmp.dir.openFile("cache", .{}), ); } @@ -410,18 +407,18 @@ test "disk cache operations" { const alloc = testing.allocator; // Create our path - var td: TempDir = try .init(); - defer td.deinit(); + var tmp = testing.tmpDir(.{}); + defer tmp.cleanup(); var buf: [4096]u8 = undefined; { - var file = try td.dir.createFile("cache", .{}); + var file = try tmp.dir.createFile("cache", .{}); defer file.close(); var file_writer = file.writer(&buf); const writer = &file_writer.interface; try writer.writeAll("HELLO!"); try writer.flush(); } - const path = try td.dir.realpathAlloc(alloc, "cache"); + const path = try tmp.dir.realpathAlloc(alloc, "cache"); defer alloc.free(path); // Setup our cache @@ -453,6 +450,32 @@ test "disk cache operations" { ); } +test "disk cache cleans up temp files" { + const testing = std.testing; + const alloc = testing.allocator; + + var tmp = testing.tmpDir(.{ .iterate = true }); + defer tmp.cleanup(); + + const tmp_path = try tmp.dir.realpathAlloc(alloc, "."); + defer alloc.free(tmp_path); + const cache_path = try std.fs.path.join(alloc, &.{ tmp_path, "cache" }); + defer alloc.free(cache_path); + + const cache: DiskCache = .{ .path = cache_path }; + try testing.expectEqual(AddResult.added, try cache.add(alloc, "example.com")); + try testing.expectEqual(AddResult.added, try cache.add(alloc, "example.org")); + + // Verify only the cache file exists and no temp files left behind + var count: usize = 0; + var iter = tmp.dir.iterate(); + while (try iter.next()) |entry| { + count += 1; + try testing.expectEqualStrings("cache", entry.name); + } + try testing.expectEqual(1, count); +} + test isValidHost { const testing = std.testing; From 3ee30058ab8eeeefa9b6503693abd0f3e1328ae7 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 18 Jan 2026 14:43:19 -0800 Subject: [PATCH 27/37] terminal: increaseCapacity should preserve dirty flag This never caused any known issues, but it's a bug! `increaseCapacity` should produce a node with identical contents, just more capacity. We were forgetting to copy over the dirty flag. I looked back at `adjustCapacity` and it also didn't preserve the dirty flag so presumably downstream consumers have been handling this case manually. But, I think semantically it makes sense for `increaseCapacity` to preserve the dirty flag. This bug was found by AI (while I was doing another task). I fixed it and wrote the test by hand though. --- src/terminal/PageList.zig | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index c5897b7e7..ea6168caa 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3045,6 +3045,9 @@ pub fn increaseCapacity( @panic("unexpected clone failure"); }; + // Preserve page-level dirty flag (cloneFrom only copies row data) + new_page.dirty = page.dirty; + // Must not fail after this because the operations we do after this // can't be recovered. errdefer comptime unreachable; @@ -6942,6 +6945,37 @@ test "PageList increaseCapacity multi-page" { ); } +test "PageList increaseCapacity preserves dirty flag" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 2, 4, 0); + defer s.deinit(); + + // Set page dirty flag and mark some rows as dirty + const page = &s.pages.first.?.data; + page.dirty = true; + + const rows = page.rows.ptr(page.memory); + rows[0].dirty = true; + rows[1].dirty = false; + rows[2].dirty = true; + rows[3].dirty = false; + + // Increase capacity + const new_node = try s.increaseCapacity(s.pages.first.?, .styles); + + // The page dirty flag should be preserved + try testing.expect(new_node.data.dirty); + + // Row dirty flags should be preserved + const new_rows = new_node.data.rows.ptr(new_node.data.memory); + try testing.expect(new_rows[0].dirty); + try testing.expect(!new_rows[1].dirty); + try testing.expect(new_rows[2].dirty); + try testing.expect(!new_rows[3].dirty); +} + test "PageList pageIterator single page" { const testing = std.testing; const alloc = testing.allocator; From 1daba40247432a20f14de3fe99ede98bad199b66 Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Mon, 19 Jan 2026 10:53:54 -0600 Subject: [PATCH 28/37] osc 133: handle bare keys with no '=' Fixes #10379 --- src/terminal/osc/parsers/semantic_prompt.zig | 114 ++++++++++++++----- 1 file changed, 85 insertions(+), 29 deletions(-) diff --git a/src/terminal/osc/parsers/semantic_prompt.zig b/src/terminal/osc/parsers/semantic_prompt.zig index ac7298267..652fe34da 100644 --- a/src/terminal/osc/parsers/semantic_prompt.zig +++ b/src/terminal/osc/parsers/semantic_prompt.zig @@ -33,64 +33,69 @@ pub fn parse(parser: *Parser, _: ?u8) ?*Command { return null; }; while (it.next()) |kv| { - if (std.mem.eql(u8, kv.key, "aid")) { + const key = kv.key orelse continue; + if (std.mem.eql(u8, key, "aid")) { parser.command.prompt_start.aid = kv.value; - } else if (std.mem.eql(u8, kv.key, "redraw")) redraw: { + } else if (std.mem.eql(u8, key, "redraw")) redraw: { // https://sw.kovidgoyal.net/kitty/shell-integration/#notes-for-shell-developers // Kitty supports a "redraw" option for prompt_start. I can't find // this documented anywhere but can see in the code that this is used // by shell environments to tell the terminal that the shell will NOT // redraw the prompt so we should attempt to resize it. parser.command.prompt_start.redraw = (value: { - if (kv.value.len != 1) break :value null; - switch (kv.value[0]) { + const value = kv.value orelse break :value null; + if (value.len != 1) break :value null; + switch (value[0]) { '0' => break :value false, '1' => break :value true, else => break :value null, } }) orelse { - log.info("OSC 133 A: invalid redraw value: {s}", .{kv.value}); + log.info("OSC 133 A: invalid redraw value: {?s}", .{kv.value}); break :redraw; }; - } else if (std.mem.eql(u8, kv.key, "special_key")) redraw: { + } else if (std.mem.eql(u8, key, "special_key")) redraw: { // https://sw.kovidgoyal.net/kitty/shell-integration/#notes-for-shell-developers parser.command.prompt_start.special_key = (value: { - if (kv.value.len != 1) break :value null; - switch (kv.value[0]) { + const value = kv.value orelse break :value null; + if (value.len != 1) break :value null; + switch (value[0]) { '0' => break :value false, '1' => break :value true, else => break :value null, } }) orelse { - log.info("OSC 133 A invalid special_key value: {s}", .{kv.value}); + log.info("OSC 133 A invalid special_key value: {?s}", .{kv.value}); break :redraw; }; - } else if (std.mem.eql(u8, kv.key, "click_events")) redraw: { + } else if (std.mem.eql(u8, key, "click_events")) redraw: { // https://sw.kovidgoyal.net/kitty/shell-integration/#notes-for-shell-developers parser.command.prompt_start.click_events = (value: { - if (kv.value.len != 1) break :value null; - switch (kv.value[0]) { + const value = kv.value orelse break :value null; + if (value.len != 1) break :value null; + switch (value[0]) { '0' => break :value false, '1' => break :value true, else => break :value null, } }) orelse { - log.info("OSC 133 A invalid click_events value: {s}", .{kv.value}); + log.info("OSC 133 A invalid click_events value: {?s}", .{kv.value}); break :redraw; }; - } else if (std.mem.eql(u8, kv.key, "k")) k: { + } else if (std.mem.eql(u8, key, "k")) k: { // The "k" marks the kind of prompt, or "primary" if we don't know. // This can be used to distinguish between the first (initial) prompt, // a continuation, etc. - if (kv.value.len != 1) break :k; - parser.command.prompt_start.kind = switch (kv.value[0]) { + const value = kv.value orelse break :k; + if (value.len != 1) break :k; + parser.command.prompt_start.kind = switch (value[0]) { 'c' => .continuation, 's' => .secondary, 'r' => .right, 'i' => .primary, else => .primary, }; - } else log.info("OSC 133 A: unknown semantic prompt option: {s}", .{kv.key}); + } else log.info("OSC 133 A: unknown semantic prompt option: {?s}", .{kv.key}); } }, 'B' => prompt_end: { @@ -105,7 +110,7 @@ pub fn parse(parser: *Parser, _: ?u8) ?*Command { return null; }; while (it.next()) |kv| { - log.info("OSC 133 B: unknown semantic prompt option: {s}", .{kv.key}); + log.info("OSC 133 B: unknown semantic prompt option: {?s}", .{kv.key}); } }, 'C' => end_of_input: { @@ -122,12 +127,13 @@ pub fn parse(parser: *Parser, _: ?u8) ?*Command { return null; }; while (it.next()) |kv| { - if (std.mem.eql(u8, kv.key, "cmdline")) { - parser.command.end_of_input.cmdline = string_encoding.printfQDecode(kv.value) catch null; - } else if (std.mem.eql(u8, kv.key, "cmdline_url")) { - parser.command.end_of_input.cmdline = string_encoding.urlPercentDecode(kv.value) catch null; + const key = kv.key orelse continue; + if (std.mem.eql(u8, key, "cmdline")) { + parser.command.end_of_input.cmdline = if (kv.value) |value| string_encoding.printfQDecode(value) catch null else null; + } else if (std.mem.eql(u8, key, "cmdline_url")) { + parser.command.end_of_input.cmdline = if (kv.value) |value| string_encoding.urlPercentDecode(value) catch null else null; } else { - log.info("OSC 133 C: unknown semantic prompt option: {s}", .{kv.key}); + log.info("OSC 133 C: unknown semantic prompt option: {s}", .{key}); } } }, @@ -159,8 +165,8 @@ const SemanticPromptKVIterator = struct { string: []u8, pub const SemanticPromptKV = struct { - key: [:0]u8, - value: [:0]u8, + key: ?[:0]u8, + value: ?[:0]u8, }; pub fn init(writer: *std.Io.Writer) std.Io.Writer.Error!SemanticPromptKVIterator { @@ -186,17 +192,24 @@ const SemanticPromptKVIterator = struct { break :kv kv; }; - // If we have an empty item, we return an empty key and value. + // If we have an empty item, we return a null key and value. // // This allows for trailing semicolons, but also lets us parse // (or rather, ignore) empty fields; for example `a=b;;e=f`. if (kv.len < 1) return .{ - .key = kv, - .value = kv, + .key = null, + .value = null, }; const key = key: { - const index = std.mem.indexOfScalar(u8, kv, '=') orelse break :key kv; + const index = std.mem.indexOfScalar(u8, kv, '=') orelse { + // If there is no '=' return entire `kv` string as the key and + // a null value. + return .{ + .key = kv, + .value = null, + }; + }; kv[index] = 0; const key = kv[0..index :0]; break :key key; @@ -408,6 +421,36 @@ test "OSC 133: prompt_start with click_events empty" { try testing.expect(cmd.prompt_start.click_events == false); } +test "OSC 133: prompt_start with click_events bare key" { + const testing = std.testing; + + var p: Parser = .init(null); + + const input = "133;A;click_events"; + for (input) |ch| p.next(ch); + + const cmd = p.end(null).?.*; + try testing.expect(cmd == .prompt_start); + try testing.expect(cmd.prompt_start.click_events == false); +} + +test "OSC 133: prompt_start with invalid bare key" { + const testing = std.testing; + + var p: Parser = .init(null); + + const input = "133;A;barekey"; + for (input) |ch| p.next(ch); + + const cmd = p.end(null).?.*; + try testing.expect(cmd == .prompt_start); + try testing.expect(cmd.prompt_start.aid == null); + try testing.expectEqual(.primary, cmd.prompt_start.kind); + try testing.expect(cmd.prompt_start.redraw == true); + try testing.expect(cmd.prompt_start.special_key == false); + try testing.expect(cmd.prompt_start.click_events == false); +} + test "OSC 133: end_of_command no exit code" { const testing = std.testing; @@ -713,3 +756,16 @@ test "OSC 133: end_of_input with cmdline_url 9" { try testing.expect(cmd == .end_of_input); try testing.expect(cmd.end_of_input.cmdline == null); } + +test "OSC 133: end_of_input with bare key" { + const testing = std.testing; + + var p: Parser = .init(null); + + const input = "133;C;cmdline_url"; + for (input) |ch| p.next(ch); + + const cmd = p.end(null).?.*; + try testing.expect(cmd == .end_of_input); + try testing.expect(cmd.end_of_input.cmdline == null); +} From d67dd08a21bdacb1cca0fcb73cdff392083ca8b6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 19 Jan 2026 07:09:02 -0800 Subject: [PATCH 29/37] terminal: remap tracked pins in backfill path during resize Fixes #10369 When `resizeWithoutReflowGrowCols` copies rows to a previous page with spare capacity, tracked pins pointing to those rows were not being remapped. This left pins pointing to the original page which was subsequently destroyed. The fix adds pin remapping for rows copied to the previous page, matching the existing remapping logic for rows copied to new pages. I also added new integrity checks to verify that our tracked pins are always valid at points where internal operations complete. Thanks to @grishy for finding this! --- src/terminal/PageList.zig | 106 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 102 insertions(+), 4 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index ea6168caa..b4f51a7c9 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -485,10 +485,11 @@ pub inline fn pauseIntegrityChecks(self: *PageList, pause: bool) void { } const IntegrityError = error{ + PageSerialInvalid, TotalRowsMismatch, + TrackedPinInvalid, ViewportPinOffsetMismatch, ViewportPinInsufficientRows, - PageSerialInvalid, }; /// Verify the integrity of the PageList. This is expensive and should @@ -529,6 +530,11 @@ fn verifyIntegrity(self: *const PageList) IntegrityError!void { return IntegrityError.TotalRowsMismatch; } + // Verify that all our tracked pins point to valid pages. + for (self.tracked_pins.keys()) |p| { + if (!self.pinIsValid(p.*)) return error.TrackedPinInvalid; + } + if (self.viewport == .pin) { // Verify that our viewport pin row offset is correct. const actual_offset: usize = offset: { @@ -750,8 +756,8 @@ pub fn clone( ); errdefer pool.deinit(); - // Our viewport pin is always undefined since our viewport in a clones - // goes back to the top + // Create our viewport. In a clone, the viewport always goes + // to the top. const viewport_pin = try pool.pins.create(); var tracked_pins: PinSet = .{}; errdefer tracked_pins.deinit(pool.alloc); @@ -817,6 +823,10 @@ pub fn clone( } } + // Initialize our viewport pin to point to the first cloned page + // so it points to valid memory. + viewport_pin.* = .{ .node = page_list.first.? }; + var result: PageList = .{ .pool = pool, .pages = page_list, @@ -1259,9 +1269,26 @@ const ReflowCursor = struct { )) |result| switch (result) { // Wrote the cell, move to the next. .success => x += 1, + // Wrote the cell but request to skip the next so skip it. // This is used for things like spacers. - .skip_next => x += 2, + .skip_next => { + // Remap any tracked pins at the skipped position (x+1) + // since we won't process that cell in the loop. + const pin_keys = list.tracked_pins.keys(); + for (pin_keys) |p| { + if (&p.node.data != src_page or + p.y != src_y or + p.x != x + 1) continue; + + p.node = self.node; + p.x = self.x; + p.y = self.y; + } + + x += 2; + }, + // Didn't write the cell, repeat writing this same cell. .repeat => {}, } else |err| switch (err) { @@ -2167,6 +2194,14 @@ fn resizeWithoutReflowGrowCols( assert(copied == len); assert(prev_page.size.rows <= prev_page.capacity.rows); + + // Remap any tracked pins that pointed to rows we just copied to prev. + const pin_keys = self.tracked_pins.keys(); + for (pin_keys) |p| { + if (p.node != chunk.node or p.y >= len) continue; + p.node = prev_node; + p.y += prev_page.size.rows - len; + } } // If we have an error, we clear the rows we just added to our prev page. @@ -11713,3 +11748,66 @@ test "PageList grow non-standard page prune protection" { // Verify the invariant holds - the fix prevents the destructive prune try testing.expect(s.totalRows() >= s.rows); } + +test "PageList resize (no reflow) more cols remaps pins in backfill path" { + // Regression test: when resizeWithoutReflowGrowCols copies rows to a previous + // page with spare capacity, tracked pins in those rows must be remapped. + // Without the fix, pins become dangling pointers when the original page is destroyed. + const testing = std.testing; + const alloc = testing.allocator; + + const cols: size.CellCountInt = 5; + const cap = try std_capacity.adjust(.{ .cols = cols }); + var s = try init(alloc, cols, cap.rows, null); + defer s.deinit(); + + // Grow until we have two pages. + while (s.pages.first == s.pages.last) { + _ = try s.grow(); + } + const first_page = s.pages.first.?; + const second_page = s.pages.last.?; + try testing.expect(first_page != second_page); + + // Trim a history row so the first page has spare capacity. + // This triggers the backfill path in resizeWithoutReflowGrowCols. + s.eraseRows(.{ .history = .{} }, .{ .history = .{ .y = 0 } }); + try testing.expect(first_page.data.size.rows < first_page.data.capacity.rows); + + // Ensure the resize takes the slow path (new capacity > current capacity). + const new_cols: size.CellCountInt = cols + 1; + const adjusted = try second_page.data.capacity.adjust(.{ .cols = new_cols }); + try testing.expect(second_page.data.capacity.cols < adjusted.cols); + + // Track a pin in row 0 of the second page. This row will be copied + // to the first page during backfill and the pin must be remapped. + const tracked = try s.trackPin(.{ .node = second_page, .x = 0, .y = 0 }); + defer s.untrackPin(tracked); + + // Write a marker character to the tracked cell so we can verify + // the pin points to the correct cell after resize. + const marker: u21 = 'X'; + tracked.rowAndCell().cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = marker }, + }; + + try s.resize(.{ .cols = new_cols, .reflow = false }); + + // Verify the pin points to a valid node still in the page list. + var found = false; + var it = s.pages.first; + while (it) |node| : (it = node.next) { + if (node == tracked.node) { + found = true; + break; + } + } + try testing.expect(found); + try testing.expect(tracked.y < tracked.node.data.size.rows); + + // Verify the pin still points to the cell with our marker content. + const cell = tracked.rowAndCell().cell; + try testing.expectEqual(.codepoint, cell.content_tag); + try testing.expectEqual(marker, cell.content.codepoint); +} From 93436217c8315d660214ef105b7eb59e92095342 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 18 Jan 2026 07:21:41 -0800 Subject: [PATCH 30/37] terminal: page.exactRowCapacity --- src/terminal/bitmap_allocator.zig | 50 +++ src/terminal/page.zig | 628 +++++++++++++++++++++++++++++- src/terminal/ref_counted_set.zig | 17 +- 3 files changed, 688 insertions(+), 7 deletions(-) diff --git a/src/terminal/bitmap_allocator.zig b/src/terminal/bitmap_allocator.zig index 258d73071..23a5048e1 100644 --- a/src/terminal/bitmap_allocator.zig +++ b/src/terminal/bitmap_allocator.zig @@ -63,6 +63,14 @@ pub fn BitmapAllocator(comptime chunk_size: comptime_int) type { }; } + /// Returns the number of bytes required to allocate n elements of + /// type T. This accounts for the chunk size alignment used by the + /// bitmap allocator. + pub fn bytesRequired(comptime T: type, n: usize) usize { + const byte_count = @sizeOf(T) * n; + return alignForward(usize, byte_count, chunk_size); + } + /// Allocate n elements of type T. This will return error.OutOfMemory /// if there isn't enough space in the backing buffer. /// @@ -955,3 +963,45 @@ test "BitmapAllocator alloc and free two 1.5 bitmaps offset 0.75" { bm.bitmap.ptr(buf)[0..4], ); } + +test "BitmapAllocator bytesRequired" { + const testing = std.testing; + + // Chunk size of 16 bytes (like grapheme_chunk in page.zig) + { + const Alloc = BitmapAllocator(16); + + // Single byte rounds up to chunk size + try testing.expectEqual(16, Alloc.bytesRequired(u8, 1)); + try testing.expectEqual(16, Alloc.bytesRequired(u8, 16)); + try testing.expectEqual(32, Alloc.bytesRequired(u8, 17)); + + // u21 (4 bytes each) + try testing.expectEqual(16, Alloc.bytesRequired(u21, 1)); // 4 bytes -> 16 + try testing.expectEqual(16, Alloc.bytesRequired(u21, 4)); // 16 bytes -> 16 + try testing.expectEqual(32, Alloc.bytesRequired(u21, 5)); // 20 bytes -> 32 + try testing.expectEqual(32, Alloc.bytesRequired(u21, 6)); // 24 bytes -> 32 + } + + // Chunk size of 4 bytes + { + const Alloc = BitmapAllocator(4); + + try testing.expectEqual(4, Alloc.bytesRequired(u8, 1)); + try testing.expectEqual(4, Alloc.bytesRequired(u8, 4)); + try testing.expectEqual(8, Alloc.bytesRequired(u8, 5)); + + // u32 (4 bytes each) - exactly one chunk per element + try testing.expectEqual(4, Alloc.bytesRequired(u32, 1)); + try testing.expectEqual(8, Alloc.bytesRequired(u32, 2)); + } + + // Chunk size of 32 bytes (like string_chunk in page.zig) + { + const Alloc = BitmapAllocator(32); + + try testing.expectEqual(32, Alloc.bytesRequired(u8, 1)); + try testing.expectEqual(32, Alloc.bytesRequired(u8, 32)); + try testing.expectEqual(64, Alloc.bytesRequired(u8, 33)); + } +} diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 075f03b57..6a5958681 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -633,6 +633,114 @@ pub const Page = struct { HyperlinkError || GraphemeError; + /// Compute the exact capacity required to store a range of rows from + /// this page. + /// + /// The returned capacity will have the same number of columns as this + /// page and the number of rows equal to the range given. The returned + /// capacity is by definition strictly less than or equal to this + /// page's capacity, so the layout is guaranteed to succeed. + /// + /// Preconditions: + /// - Range must be at least 1 row + /// - Start and end must be valid for this page + pub fn exactRowCapacity( + self: *const Page, + y_start: usize, + y_end: usize, + ) Capacity { + assert(y_start < y_end); + assert(y_end <= self.size.rows); + + // Track unique IDs using a bitset. Both style IDs and hyperlink IDs + // are CellCountInt (u16), so we reuse this set for both to save + // stack memory (~8KB instead of ~16KB). + const CellCountSet = std.StaticBitSet(std.math.maxInt(size.CellCountInt) + 1); + comptime assert(size.StyleCountInt == size.CellCountInt); + comptime assert(size.HyperlinkCountInt == size.CellCountInt); + + // Accumulators + var id_set: CellCountSet = .initEmpty(); + var grapheme_bytes: usize = 0; + var string_bytes: usize = 0; + + // First pass: count styles and grapheme bytes + const rows = self.rows.ptr(self.memory)[y_start..y_end]; + for (rows) |*row| { + const cells = row.cells.ptr(self.memory)[0..self.size.cols]; + for (cells) |*cell| { + if (cell.style_id != stylepkg.default_id) { + id_set.set(cell.style_id); + } + + if (cell.hasGrapheme()) { + if (self.lookupGrapheme(cell)) |cps| { + grapheme_bytes += GraphemeAlloc.bytesRequired(u21, cps.len); + } + } + } + } + const styles_cap = StyleSet.capacityForCount(id_set.count()); + + // Second pass: count hyperlinks and string bytes + // We count both unique hyperlinks (for hyperlink_set) and total + // hyperlink cells (for hyperlink_map capacity). + id_set = .initEmpty(); + var hyperlink_cells: usize = 0; + for (rows) |*row| { + const cells = row.cells.ptr(self.memory)[0..self.size.cols]; + for (cells) |*cell| { + if (cell.hyperlink) { + hyperlink_cells += 1; + if (self.lookupHyperlink(cell)) |id| { + // Only count each unique hyperlink once for set sizing + if (!id_set.isSet(id)) { + id_set.set(id); + + // Get the hyperlink entry to compute string bytes + const entry = self.hyperlink_set.get(self.memory, id); + string_bytes += StringAlloc.bytesRequired(u8, entry.uri.len); + + switch (entry.id) { + .implicit => {}, + .explicit => |slice| { + string_bytes += StringAlloc.bytesRequired(u8, slice.len); + }, + } + } + } + } + } + } + + // The hyperlink_map capacity in layout() is computed as: + // hyperlink_count * hyperlink_cell_multiplier (rounded to power of 2) + // We need enough hyperlink_bytes so that when layout() computes + // the map capacity, it can accommodate all hyperlink cells. This + // is unit tested. + const hyperlink_cap = cap: { + const hyperlink_count = id_set.count(); + const hyperlink_set_cap = hyperlink.Set.capacityForCount(hyperlink_count); + const hyperlink_map_min = std.math.divCeil( + usize, + hyperlink_cells, + hyperlink_cell_multiplier, + ) catch 0; + break :cap @max(hyperlink_set_cap, hyperlink_map_min); + }; + + // All the intCasts below are safe because we should have a + // capacity strictly less than or equal to this page's capacity. + return .{ + .cols = self.size.cols, + .rows = @intCast(y_end - y_start), + .styles = @intCast(styles_cap), + .grapheme_bytes = @intCast(grapheme_bytes), + .hyperlink_bytes = @intCast(hyperlink_cap * @sizeOf(hyperlink.Set.Item)), + .string_bytes = @intCast(string_bytes), + }; + } + /// Clone the contents of another page into this page. The capacities /// can be different, but the size of the other page must fit into /// this page. @@ -1569,10 +1677,13 @@ pub const Page = struct { const grapheme_alloc_start = alignForward(usize, styles_end, GraphemeAlloc.base_align.toByteUnits()); const grapheme_alloc_end = grapheme_alloc_start + grapheme_alloc_layout.total_size; - const grapheme_count = std.math.ceilPowerOfTwo( - usize, - @divFloor(cap.grapheme_bytes, grapheme_chunk), - ) catch unreachable; + const grapheme_count: usize = count: { + if (cap.grapheme_bytes == 0) break :count 0; + // Use divCeil to match GraphemeAlloc.layout() which uses alignForward, + // ensuring grapheme_map has capacity when grapheme_alloc has chunks. + const base = std.math.divCeil(usize, cap.grapheme_bytes, grapheme_chunk) catch unreachable; + break :count std.math.ceilPowerOfTwo(usize, base) catch unreachable; + }; const grapheme_map_layout = GraphemeMap.layout(@intCast(grapheme_count)); const grapheme_map_start = alignForward(usize, grapheme_alloc_end, GraphemeMap.base_align.toByteUnits()); const grapheme_map_end = grapheme_map_start + grapheme_map_layout.total_size; @@ -3217,3 +3328,512 @@ test "Page verifyIntegrity zero cols" { page.verifyIntegrity(testing.allocator), ); } + +test "Page exactRowCapacity empty rows" { + var page = try Page.init(.{ + .cols = 10, + .rows = 10, + .styles = 8, + .hyperlink_bytes = 32 * @sizeOf(hyperlink.Set.Item), + .string_bytes = 512, + }); + defer page.deinit(); + + // Empty page: all capacity fields should be 0 (except cols/rows) + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(10, cap.cols); + try testing.expectEqual(5, cap.rows); + try testing.expectEqual(0, cap.styles); + try testing.expectEqual(0, cap.grapheme_bytes); + try testing.expectEqual(0, cap.hyperlink_bytes); + try testing.expectEqual(0, cap.string_bytes); +} + +test "Page exactRowCapacity styles" { + var page = try Page.init(.{ + .cols = 10, + .rows = 10, + .styles = 8, + }); + defer page.deinit(); + + // No styles: capacity should be 0 + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(0, cap.styles); + } + + // Add one style to a cell + const style1_id = try page.styles.add(page.memory, .{ .flags = .{ .bold = true } }); + { + const rac = page.getRowAndCell(0, 0); + rac.row.styled = true; + rac.cell.style_id = style1_id; + } + + // One unique style - capacity accounts for load factor + const cap_one_style = page.exactRowCapacity(0, 5); + { + try testing.expectEqual(StyleSet.capacityForCount(1), cap_one_style.styles); + } + + // Add same style to another cell (duplicate) - capacity unchanged + { + const rac = page.getRowAndCell(1, 0); + rac.cell.style_id = style1_id; + } + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(cap_one_style.styles, cap.styles); + } + + // Add a different style + const style2_id = try page.styles.add(page.memory, .{ .flags = .{ .italic = true } }); + { + const rac = page.getRowAndCell(2, 0); + rac.cell.style_id = style2_id; + } + + // Two unique styles - capacity accounts for load factor + const cap_two_styles = page.exactRowCapacity(0, 5); + { + try testing.expectEqual(StyleSet.capacityForCount(2), cap_two_styles.styles); + try testing.expect(cap_two_styles.styles > cap_one_style.styles); + } + + // Style outside the row range should not be counted + { + const rac = page.getRowAndCell(0, 7); + rac.row.styled = true; + rac.cell.style_id = try page.styles.add(page.memory, .{ .flags = .{ .underline = .single } }); + } + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(cap_two_styles.styles, cap.styles); + } + + // Full range includes the new style + { + const cap = page.exactRowCapacity(0, 10); + try testing.expectEqual(StyleSet.capacityForCount(3), cap.styles); + } + + // Verify clone works with exact capacity and produces same result + { + const cap = page.exactRowCapacity(0, 5); + var cloned = try Page.init(cap); + defer cloned.deinit(); + for (0..5) |y| { + const src_row = &page.rows.ptr(page.memory)[y]; + const dst_row = &cloned.rows.ptr(cloned.memory)[y]; + try cloned.cloneRowFrom(&page, dst_row, src_row); + } + const cloned_cap = cloned.exactRowCapacity(0, 5); + try testing.expectEqual(cap, cloned_cap); + } +} + +test "Page exactRowCapacity single style clone" { + // Regression test: verify a single style can be cloned with exact capacity. + // This tests that capacityForCount properly accounts for ID 0 being reserved. + var page = try Page.init(.{ + .cols = 10, + .rows = 2, + .styles = 8, + }); + defer page.deinit(); + + // Add exactly one style to row 0 + const style_id = try page.styles.add(page.memory, .{ .flags = .{ .bold = true } }); + { + const rac = page.getRowAndCell(0, 0); + rac.row.styled = true; + rac.cell.style_id = style_id; + } + + // exactRowCapacity for just row 0 should give capacity for 1 style + const cap = page.exactRowCapacity(0, 1); + try testing.expectEqual(StyleSet.capacityForCount(1), cap.styles); + + // Create a new page with exact capacity and clone + var cloned = try Page.init(cap); + defer cloned.deinit(); + + const src_row = &page.rows.ptr(page.memory)[0]; + const dst_row = &cloned.rows.ptr(cloned.memory)[0]; + + // This must not fail with StyleSetOutOfMemory + try cloned.cloneRowFrom(&page, dst_row, src_row); + + // Verify the style was cloned correctly + const cloned_cell = &cloned.rows.ptr(cloned.memory)[0].cells.ptr(cloned.memory)[0]; + try testing.expect(cloned_cell.style_id != stylepkg.default_id); +} + +test "Page exactRowCapacity styles max single row" { + var page = try Page.init(.{ + .cols = std.math.maxInt(size.CellCountInt), + .rows = 1, + .styles = std.math.maxInt(size.StyleCountInt), + }); + defer page.deinit(); + + // Style our first row + const row = &page.rows.ptr(page.memory)[0]; + row.styled = true; + + // Fill cells with styles until we get OOM, but limit to a reasonable count + // to avoid overflow when computing capacityForCount near maxInt + const cells = row.cells.ptr(page.memory)[0..page.size.cols]; + var count: usize = 0; + const max_count: usize = 1000; // Limit to avoid overflow in capacity calculation + for (cells, 0..) |*cell, i| { + if (count >= max_count) break; + const style_id = page.styles.add(page.memory, .{ + .fg_color = .{ .rgb = .{ + .r = @intCast(i & 0xFF), + .g = @intCast((i >> 8) & 0xFF), + .b = 0, + } }, + }) catch break; + cell.style_id = style_id; + count += 1; + } + + // Verify we added a meaningful number of styles + try testing.expect(count > 0); + + // Capacity should be at least count (adjusted for load factor) + const cap = page.exactRowCapacity(0, 1); + try testing.expectEqual(StyleSet.capacityForCount(count), cap.styles); +} + +test "Page exactRowCapacity grapheme_bytes" { + var page = try Page.init(.{ + .cols = 10, + .rows = 10, + .styles = 8, + }); + defer page.deinit(); + + // No graphemes: capacity should be 0 + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(0, cap.grapheme_bytes); + } + + // Add one grapheme (1 codepoint) to a cell - rounds up to grapheme_chunk + { + const rac = page.getRowAndCell(0, 0); + rac.cell.* = .init('a'); + try page.appendGrapheme(rac.row, rac.cell, 0x0301); // combining acute accent + } + { + const cap = page.exactRowCapacity(0, 5); + // 1 codepoint = 4 bytes, rounds up to grapheme_chunk (16) + try testing.expectEqual(grapheme_chunk, cap.grapheme_bytes); + } + + // Add another grapheme to a different cell - should sum + { + const rac = page.getRowAndCell(1, 0); + rac.cell.* = .init('e'); + try page.appendGrapheme(rac.row, rac.cell, 0x0300); // combining grave accent + } + { + const cap = page.exactRowCapacity(0, 5); + // 2 graphemes, each 1 codepoint = 2 * grapheme_chunk + try testing.expectEqual(grapheme_chunk * 2, cap.grapheme_bytes); + } + + // Add a larger grapheme (multiple codepoints) that fits in one chunk + { + const rac = page.getRowAndCell(2, 0); + rac.cell.* = .init('o'); + try page.appendGrapheme(rac.row, rac.cell, 0x0301); + try page.appendGrapheme(rac.row, rac.cell, 0x0302); + try page.appendGrapheme(rac.row, rac.cell, 0x0303); + } + { + const cap = page.exactRowCapacity(0, 5); + // First two cells: 2 * grapheme_chunk + // Third cell: 3 codepoints = 12 bytes, rounds up to grapheme_chunk + try testing.expectEqual(grapheme_chunk * 3, cap.grapheme_bytes); + } + + // Grapheme outside the row range should not be counted + { + const rac = page.getRowAndCell(0, 7); + rac.cell.* = .init('x'); + try page.appendGrapheme(rac.row, rac.cell, 0x0304); + } + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(grapheme_chunk * 3, cap.grapheme_bytes); + } + + // Full range includes the new grapheme + { + const cap = page.exactRowCapacity(0, 10); + try testing.expectEqual(grapheme_chunk * 4, cap.grapheme_bytes); + } + + // Verify clone works with exact capacity and produces same result + { + const cap = page.exactRowCapacity(0, 5); + var cloned = try Page.init(cap); + defer cloned.deinit(); + for (0..5) |y| { + const src_row = &page.rows.ptr(page.memory)[y]; + const dst_row = &cloned.rows.ptr(cloned.memory)[y]; + try cloned.cloneRowFrom(&page, dst_row, src_row); + } + const cloned_cap = cloned.exactRowCapacity(0, 5); + try testing.expectEqual(cap, cloned_cap); + } +} + +test "Page exactRowCapacity grapheme_bytes larger than chunk" { + var page = try Page.init(.{ + .cols = 10, + .rows = 10, + .styles = 8, + }); + defer page.deinit(); + + // Add a grapheme larger than one chunk (grapheme_chunk_len = 4 codepoints) + const rac = page.getRowAndCell(0, 0); + rac.cell.* = .init('a'); + + // Add 6 codepoints - requires 2 chunks (6 * 4 = 24 bytes, rounds up to 32) + for (0..6) |i| { + try page.appendGrapheme(rac.row, rac.cell, @intCast(0x0300 + i)); + } + + const cap = page.exactRowCapacity(0, 1); + // 6 codepoints = 24 bytes, alignForward(24, 16) = 32 + try testing.expectEqual(32, cap.grapheme_bytes); + + // Verify clone works with exact capacity and produces same result + var cloned = try Page.init(cap); + defer cloned.deinit(); + const src_row = &page.rows.ptr(page.memory)[0]; + const dst_row = &cloned.rows.ptr(cloned.memory)[0]; + try cloned.cloneRowFrom(&page, dst_row, src_row); + const cloned_cap = cloned.exactRowCapacity(0, 1); + try testing.expectEqual(cap, cloned_cap); +} + +test "Page exactRowCapacity hyperlinks" { + var page = try Page.init(.{ + .cols = 10, + .rows = 10, + .styles = 8, + .hyperlink_bytes = 32 * @sizeOf(hyperlink.Set.Item), + .string_bytes = 512, + }); + defer page.deinit(); + + // No hyperlinks: capacity should be 0 + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(0, cap.hyperlink_bytes); + try testing.expectEqual(0, cap.string_bytes); + } + + // Add one hyperlink with implicit ID + const uri1 = "https://example.com"; + const id1 = blk: { + const rac = page.getRowAndCell(0, 0); + + // Create and add hyperlink entry + const id = try page.insertHyperlink(.{ + .id = .{ .implicit = 1 }, + .uri = uri1, + }); + try page.setHyperlink(rac.row, rac.cell, id); + break :blk id; + }; + // 1 hyperlink - capacity accounts for load factor + const cap_one_link = page.exactRowCapacity(0, 5); + { + try testing.expectEqual(hyperlink.Set.capacityForCount(1) * @sizeOf(hyperlink.Set.Item), cap_one_link.hyperlink_bytes); + // URI "https://example.com" = 19 bytes, rounds up to string_chunk (32) + try testing.expectEqual(string_chunk, cap_one_link.string_bytes); + } + + // Add same hyperlink to another cell (duplicate ID) - capacity unchanged + { + const rac = page.getRowAndCell(1, 0); + + // Use the same hyperlink ID for another cell + page.hyperlink_set.use(page.memory, id1); + try page.setHyperlink(rac.row, rac.cell, id1); + } + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(cap_one_link.hyperlink_bytes, cap.hyperlink_bytes); + try testing.expectEqual(cap_one_link.string_bytes, cap.string_bytes); + } + + // Add a different hyperlink with explicit ID + const uri2 = "https://other.example.org/path"; + const explicit_id = "my-link-id"; + { + const rac = page.getRowAndCell(2, 0); + + const id = try page.insertHyperlink(.{ + .id = .{ .explicit = explicit_id }, + .uri = uri2, + }); + try page.setHyperlink(rac.row, rac.cell, id); + } + // 2 hyperlinks - capacity accounts for load factor + const cap_two_links = page.exactRowCapacity(0, 5); + { + try testing.expectEqual(hyperlink.Set.capacityForCount(2) * @sizeOf(hyperlink.Set.Item), cap_two_links.hyperlink_bytes); + // First URI: 19 bytes -> 32, Second URI: 30 bytes -> 32, Explicit ID: 10 bytes -> 32 + try testing.expectEqual(string_chunk * 3, cap_two_links.string_bytes); + } + + // Hyperlink outside the row range should not be counted + { + const rac = page.getRowAndCell(0, 7); // row 7 is outside range [0, 5) + + const id = try page.insertHyperlink(.{ + .id = .{ .implicit = 99 }, + .uri = "https://outside.example.com", + }); + try page.setHyperlink(rac.row, rac.cell, id); + } + { + const cap = page.exactRowCapacity(0, 5); + try testing.expectEqual(cap_two_links.hyperlink_bytes, cap.hyperlink_bytes); + try testing.expectEqual(cap_two_links.string_bytes, cap.string_bytes); + } + + // Full range includes the new hyperlink + { + const cap = page.exactRowCapacity(0, 10); + try testing.expectEqual(hyperlink.Set.capacityForCount(3) * @sizeOf(hyperlink.Set.Item), cap.hyperlink_bytes); + // Third URI: 27 bytes -> 32 + try testing.expectEqual(string_chunk * 4, cap.string_bytes); + } + + // Verify clone works with exact capacity and produces same result + { + const cap = page.exactRowCapacity(0, 5); + var cloned = try Page.init(cap); + defer cloned.deinit(); + for (0..5) |y| { + const src_row = &page.rows.ptr(page.memory)[y]; + const dst_row = &cloned.rows.ptr(cloned.memory)[y]; + try cloned.cloneRowFrom(&page, dst_row, src_row); + } + const cloned_cap = cloned.exactRowCapacity(0, 5); + try testing.expectEqual(cap, cloned_cap); + } +} + +test "Page exactRowCapacity single hyperlink clone" { + // Regression test: verify a single hyperlink can be cloned with exact capacity. + // This tests that capacityForCount properly accounts for ID 0 being reserved. + var page = try Page.init(.{ + .cols = 10, + .rows = 2, + .styles = 8, + .hyperlink_bytes = 32 * @sizeOf(hyperlink.Set.Item), + .string_bytes = 512, + }); + defer page.deinit(); + + // Add exactly one hyperlink to row 0 + const uri = "https://example.com"; + const id = blk: { + const rac = page.getRowAndCell(0, 0); + const link_id = try page.insertHyperlink(.{ + .id = .{ .implicit = 1 }, + .uri = uri, + }); + try page.setHyperlink(rac.row, rac.cell, link_id); + break :blk link_id; + }; + _ = id; + + // exactRowCapacity for just row 0 should give capacity for 1 hyperlink + const cap = page.exactRowCapacity(0, 1); + try testing.expectEqual(hyperlink.Set.capacityForCount(1) * @sizeOf(hyperlink.Set.Item), cap.hyperlink_bytes); + + // Create a new page with exact capacity and clone + var cloned = try Page.init(cap); + defer cloned.deinit(); + + const src_row = &page.rows.ptr(page.memory)[0]; + const dst_row = &cloned.rows.ptr(cloned.memory)[0]; + + // This must not fail with HyperlinkSetOutOfMemory + try cloned.cloneRowFrom(&page, dst_row, src_row); + + // Verify the hyperlink was cloned correctly + const cloned_cell = &cloned.rows.ptr(cloned.memory)[0].cells.ptr(cloned.memory)[0]; + try testing.expect(cloned_cell.hyperlink); +} + +test "Page exactRowCapacity hyperlink map capacity for many cells" { + // A single hyperlink spanning many cells requires hyperlink_map capacity + // based on cell count, not unique hyperlink count. + const cols = 50; + var page = try Page.init(.{ + .cols = cols, + .rows = 2, + .styles = 8, + .hyperlink_bytes = 32 * @sizeOf(hyperlink.Set.Item), + .string_bytes = 512, + }); + defer page.deinit(); + + // Add one hyperlink spanning all 50 columns in row 0 + const uri = "https://example.com"; + const id = blk: { + const rac = page.getRowAndCell(0, 0); + const link_id = try page.insertHyperlink(.{ + .id = .{ .implicit = 1 }, + .uri = uri, + }); + try page.setHyperlink(rac.row, rac.cell, link_id); + break :blk link_id; + }; + + // Apply same hyperlink to remaining cells in row 0 + for (1..cols) |x| { + const rac = page.getRowAndCell(@intCast(x), 0); + page.hyperlink_set.use(page.memory, id); + try page.setHyperlink(rac.row, rac.cell, id); + } + + // exactRowCapacity must account for 50 hyperlink cells, not just 1 unique hyperlink + const cap = page.exactRowCapacity(0, 1); + + // The hyperlink_bytes must be large enough that layout() computes sufficient + // hyperlink_map capacity. With hyperlink_cell_multiplier=16, we need at least + // ceil(50/16) = 4 hyperlink entries worth of bytes for the map. + const min_for_map = std.math.divCeil(usize, cols, hyperlink_cell_multiplier) catch 0; + const min_hyperlink_bytes = min_for_map * @sizeOf(hyperlink.Set.Item); + try testing.expect(cap.hyperlink_bytes >= min_hyperlink_bytes); + + // Create a new page with exact capacity and clone - must not fail + var cloned = try Page.init(cap); + defer cloned.deinit(); + + const src_row = &page.rows.ptr(page.memory)[0]; + const dst_row = &cloned.rows.ptr(cloned.memory)[0]; + + // This must not fail with HyperlinkMapOutOfMemory + try cloned.cloneRowFrom(&page, dst_row, src_row); + + // Verify all hyperlinks were cloned correctly + for (0..cols) |x| { + const cloned_cell = &cloned.rows.ptr(cloned.memory)[0].cells.ptr(cloned.memory)[x]; + try testing.expect(cloned_cell.hyperlink); + } +} diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index 883dd2f0d..8040039ae 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -64,6 +64,20 @@ pub fn RefCountedSet( @alignOf(Id), )); + /// This is the max load until the set returns OutOfMemory and + /// requires more capacity. + /// + /// Experimentally, this load factor works quite well. + pub const load_factor = 0.8125; + + /// Returns the minimum capacity needed to store `n` items, + /// accounting for the load factor and the reserved ID 0. + pub fn capacityForCount(n: usize) usize { + if (n == 0) return 0; + // +1 because ID 0 is reserved, so we need at least n+1 slots. + return @intFromFloat(@ceil(@as(f64, @floatFromInt(n + 1)) / load_factor)); + } + /// Set item pub const Item = struct { /// The value this item represents. @@ -154,9 +168,6 @@ pub fn RefCountedSet( /// The returned layout `cap` property will be 1 more than the number /// of items that the set can actually store, since ID 0 is reserved. pub fn init(cap: usize) Layout { - // Experimentally, this load factor works quite well. - const load_factor = 0.8125; - assert(cap <= @as(usize, @intCast(std.math.maxInt(Id))) + 1); // Zero-cap set is valid, return special case From f9699eceb03c2be01f40e9b90e906540e7185192 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 18 Jan 2026 14:25:45 -0800 Subject: [PATCH 31/37] terminal: PageList.compact --- src/terminal/PageList.zig | 202 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 202 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index b4f51a7c9..90bd3b12f 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -2685,6 +2685,74 @@ pub fn scrollClear(self: *PageList) !void { for (0..non_empty) |_| _ = try self.grow(); } +/// Compact a page to use the minimum required memory for the contents +/// it stores. Returns the new node pointer if compaction occurred, or null +/// if the page was already compact or compaction would not provide meaningful +/// savings. +/// +/// The current design of PageList at the time of writing this doesn't +/// allow for smaller than `std_size` nodes so if the current node's backing +/// page is standard size or smaller, no compaction will occur. In the +/// future we should fix this up. +/// +/// If this returns OOM, the PageList is left unchanged and no dangling +/// memory references exist. It is safe to ignore the error and continue using +/// the uncompacted page. +pub fn compact(self: *PageList, node: *List.Node) Allocator.Error!?*List.Node { + defer self.assertIntegrity(); + const page: *Page = &node.data; + + // We should never have empty rows in our pagelist anyways... + assert(page.size.rows > 0); + + // We never compact standard size or smaller pages because changing + // the capacity to something smaller won't save memory. + if (page.memory.len <= std_size) return null; + + // Compute the minimum capacity required for this page's content + const req_cap = page.exactRowCapacity(0, page.size.rows); + const new_size = Page.layout(req_cap).total_size; + const old_size = page.memory.len; + if (new_size >= old_size) return null; + + // Create the new smaller page + const new_node = try self.createPage(req_cap); + errdefer self.destroyNode(new_node); + const new_page: *Page = &new_node.data; + new_page.size = page.size; + new_page.dirty = page.dirty; + new_page.cloneFrom( + page, + 0, + page.size.rows, + ) catch |err| { + // cloneFrom should not fail when compacting since req_cap is + // computed to exactly fit the source content and our expectation + // of exactRowCapacity ensures it can fit all the requested + // data. + log.err("compact clone failed err={}", .{err}); + + // In this case, let's gracefully degrade by pretending we + // didn't need to compact. + self.destroyNode(new_node); + return null; + }; + + // Fix up all tracked pins to point to the new page + const pin_keys = self.tracked_pins.keys(); + for (pin_keys) |p| { + if (p.node != node) continue; + p.node = new_node; + } + + // Insert the new page and destroy the old one + self.pages.insertBefore(node, new_node); + self.pages.remove(node); + self.destroyNode(node); + + new_page.assertIntegrity(); + return new_node; +} /// This represents the state necessary to render a scrollbar for this /// PageList. It has the total size, the offset, and the size of the viewport. pub const Scrollbar = struct { @@ -11811,3 +11879,137 @@ test "PageList resize (no reflow) more cols remaps pins in backfill path" { try testing.expectEqual(.codepoint, cell.content_tag); try testing.expectEqual(marker, cell.content.codepoint); } + +test "PageList compact std_size page returns null" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, 0); + defer s.deinit(); + + // A freshly created page should be at std_size + const node = s.pages.first.?; + try testing.expect(node.data.memory.len <= std_size); + + // compact should return null since there's nothing to compact + const result = try s.compact(node); + try testing.expectEqual(null, result); + + // Page should still be the same + try testing.expectEqual(node, s.pages.first.?); +} + +test "PageList compact oversized page" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, null); + defer s.deinit(); + + // Grow until we have multiple pages + const page1_node = s.pages.first.?; + page1_node.data.pauseIntegrityChecks(true); + for (0..page1_node.data.capacity.rows - page1_node.data.size.rows) |_| { + _ = try s.grow(); + } + page1_node.data.pauseIntegrityChecks(false); + _ = try s.grow(); + try testing.expect(s.pages.first != s.pages.last); + + var node = s.pages.first.?; + + // Write content to verify it's preserved + { + const page = &node.data; + for (0..page.size.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(x + y * s.cols) }, + }; + } + } + } + + // Create a tracked pin on this page + const tracked = try s.trackPin(.{ .node = node, .x = 5, .y = 10 }); + defer s.untrackPin(tracked); + + // Make the page oversized + while (node.data.memory.len <= std_size) { + node = try s.increaseCapacity(node, .grapheme_bytes); + } + try testing.expect(node.data.memory.len > std_size); + const oversized_len = node.data.memory.len; + const original_size = node.data.size; + const second_node = node.next.?; + + // Set dirty flag after increaseCapacity + node.data.dirty = true; + + // Compact the page + const new_node = try s.compact(node); + try testing.expect(new_node != null); + + // Verify memory is smaller + try testing.expect(new_node.?.data.memory.len < oversized_len); + + // Verify size preserved + try testing.expectEqual(original_size.rows, new_node.?.data.size.rows); + try testing.expectEqual(original_size.cols, new_node.?.data.size.cols); + + // Verify dirty flag preserved + try testing.expect(new_node.?.data.dirty); + + // Verify linked list integrity + try testing.expectEqual(new_node.?, s.pages.first.?); + try testing.expectEqual(null, new_node.?.prev); + try testing.expectEqual(second_node, new_node.?.next); + try testing.expectEqual(new_node.?, second_node.prev); + + // Verify pin updated correctly + try testing.expectEqual(new_node.?, tracked.node); + try testing.expectEqual(@as(size.CellCountInt, 5), tracked.x); + try testing.expectEqual(@as(size.CellCountInt, 10), tracked.y); + + // Verify content preserved + const page = &new_node.?.data; + for (0..page.size.rows) |y| { + for (0..s.cols) |x| { + const rac = page.getRowAndCell(x, y); + try testing.expectEqual( + @as(u21, @intCast(x + y * s.cols)), + rac.cell.content.codepoint, + ); + } + } +} + +test "PageList compact insufficient savings returns null" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, 0); + defer s.deinit(); + + var node = s.pages.first.?; + + // Make the page slightly oversized (just one increase) + // This might not provide enough savings to justify compaction + node = try s.increaseCapacity(node, .grapheme_bytes); + + // If the page is still at or below std_size, compact returns null + if (node.data.memory.len <= std_size) { + const result = try s.compact(node); + try testing.expectEqual(null, result); + } else { + // If it did grow beyond std_size, verify that compaction + // works or returns null based on savings calculation + const result = try s.compact(node); + // Either it compacted or determined insufficient savings + if (result) |new_node| { + try testing.expect(new_node.data.memory.len < node.data.memory.len); + } + } +} From 5ee56409c7bc06a7f881da1fdc18c91799a2b6d4 Mon Sep 17 00:00:00 2001 From: Tim Culverhouse Date: Mon, 19 Jan 2026 11:58:58 -0600 Subject: [PATCH 32/37] macos: support mouse buttons 8/9 (back/forward) Add support for mouse buttons 4-11 in the macOS app. Previously only left, right, and middle buttons were handled. Now otherMouseDown/Up events properly map NSEvent.buttonNumber to the corresponding Ghostty mouse button, enabling back/forward button support. Fixes: https://github.com/ghostty-org/ghostty/issues/2425 Amp-Thread-ID: https://ampcode.com/threads/T-019bd74e-6b2b-731d-b43a-ac73b3460c32 Co-authored-by: Amp --- include/ghostty.h | 8 +++++ macos/Sources/Ghostty/Ghostty.Input.swift | 35 +++++++++++++++++++ .../Surface View/SurfaceView_AppKit.swift | 8 ++--- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/include/ghostty.h b/include/ghostty.h index b884ebc08..0133fac73 100644 --- a/include/ghostty.h +++ b/include/ghostty.h @@ -66,6 +66,14 @@ typedef enum { GHOSTTY_MOUSE_LEFT, GHOSTTY_MOUSE_RIGHT, GHOSTTY_MOUSE_MIDDLE, + GHOSTTY_MOUSE_FOUR, + GHOSTTY_MOUSE_FIVE, + GHOSTTY_MOUSE_SIX, + GHOSTTY_MOUSE_SEVEN, + GHOSTTY_MOUSE_EIGHT, + GHOSTTY_MOUSE_NINE, + GHOSTTY_MOUSE_TEN, + GHOSTTY_MOUSE_ELEVEN, } ghostty_input_mouse_button_e; typedef enum { diff --git a/macos/Sources/Ghostty/Ghostty.Input.swift b/macos/Sources/Ghostty/Ghostty.Input.swift index 4b3fb9937..7b2905abb 100644 --- a/macos/Sources/Ghostty/Ghostty.Input.swift +++ b/macos/Sources/Ghostty/Ghostty.Input.swift @@ -370,6 +370,14 @@ extension Ghostty.Input { case left case right case middle + case four + case five + case six + case seven + case eight + case nine + case ten + case eleven var cMouseButton: ghostty_input_mouse_button_e { switch self { @@ -377,6 +385,33 @@ extension Ghostty.Input { case .left: GHOSTTY_MOUSE_LEFT case .right: GHOSTTY_MOUSE_RIGHT case .middle: GHOSTTY_MOUSE_MIDDLE + case .four: GHOSTTY_MOUSE_FOUR + case .five: GHOSTTY_MOUSE_FIVE + case .six: GHOSTTY_MOUSE_SIX + case .seven: GHOSTTY_MOUSE_SEVEN + case .eight: GHOSTTY_MOUSE_EIGHT + case .nine: GHOSTTY_MOUSE_NINE + case .ten: GHOSTTY_MOUSE_TEN + case .eleven: GHOSTTY_MOUSE_ELEVEN + } + } + + /// Initialize from NSEvent.buttonNumber + /// NSEvent buttonNumber: 0=left, 1=right, 2=middle, 3=back (button 8), 4=forward (button 9), etc. + init(fromNSEventButtonNumber buttonNumber: Int) { + switch buttonNumber { + case 0: self = .left + case 1: self = .right + case 2: self = .middle + case 3: self = .eight // Back button + case 4: self = .nine // Forward button + case 5: self = .six + case 6: self = .seven + case 7: self = .four + case 8: self = .five + case 9: self = .ten + case 10: self = .eleven + default: self = .unknown } } } diff --git a/macos/Sources/Ghostty/Surface View/SurfaceView_AppKit.swift b/macos/Sources/Ghostty/Surface View/SurfaceView_AppKit.swift index 80de2e823..0ddfe57b8 100644 --- a/macos/Sources/Ghostty/Surface View/SurfaceView_AppKit.swift +++ b/macos/Sources/Ghostty/Surface View/SurfaceView_AppKit.swift @@ -860,16 +860,16 @@ extension Ghostty { override func otherMouseDown(with event: NSEvent) { guard let surface = self.surface else { return } - guard event.buttonNumber == 2 else { return } let mods = Ghostty.ghosttyMods(event.modifierFlags) - ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, GHOSTTY_MOUSE_MIDDLE, mods) + let button = Ghostty.Input.MouseButton(fromNSEventButtonNumber: event.buttonNumber) + ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_PRESS, button.cMouseButton, mods) } override func otherMouseUp(with event: NSEvent) { guard let surface = self.surface else { return } - guard event.buttonNumber == 2 else { return } let mods = Ghostty.ghosttyMods(event.modifierFlags) - ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_RELEASE, GHOSTTY_MOUSE_MIDDLE, mods) + let button = Ghostty.Input.MouseButton(fromNSEventButtonNumber: event.buttonNumber) + ghostty_surface_mouse_button(surface, GHOSTTY_MOUSE_RELEASE, button.cMouseButton, mods) } From 93a070c6de47cac5d02d525e33846361b423b382 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 18 Jan 2026 06:48:49 -0800 Subject: [PATCH 33/37] terminal: PageList split operation --- src/terminal/PageList.zig | 685 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 685 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 90bd3b12f..c639f15cd 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -2753,6 +2753,98 @@ pub fn compact(self: *PageList, node: *List.Node) Allocator.Error!?*List.Node { new_page.assertIntegrity(); return new_node; } + +pub const SplitError = error{ + // Allocator OOM + OutOfMemory, + // Page can't be split further because it is already a single row. + OutOfSpace, +}; + +/// Split the given node in the PageList at the given pin. +/// +/// The row at the pin and after will be moved into a new page with +/// the same capacity as the original page. Alternatively, you can "split +/// above" by splitting the row following the desired split row. +/// +/// Since the split happens below the pin, the pin remains valid. +pub fn split( + self: *PageList, + p: Pin, +) SplitError!void { + if (build_options.slow_runtime_safety) assert(self.pinIsValid(p)); + + // Ran into a bug that I can only explain via aliasing. If a tracked + // pin is passed in, its possible Zig will alias the memory and then + // when we modify it later it updates our p here. Coyping the node + // fixes this. + const original_node = p.node; + const page: *Page = &original_node.data; + + // A page that is already 1 row can't be split. In the future we can + // theoretically maybe split by soft-wrapping multiple pages but that + // seems crazy and the rest of our PageList can't handle heterogeneously + // sized pages today. + if (page.size.rows <= 1) return error.OutOfSpace; + + // Splitting at row 0 is a no-op since there's nothing before the split point. + if (p.y == 0) return; + + // At this point we're doing actual modification so make sure + // on the return that we're good. + defer self.assertIntegrity(); + + // Create a new node with the same capacity of managed memory. + const target = try self.createPage(page.capacity); + errdefer self.destroyNode(target); + + // Determine how many rows we're copying + const y_start = p.y; + const y_end = page.size.rows; + target.data.size.rows = y_end - y_start; + assert(target.data.size.rows <= target.data.capacity.rows); + + // Copy our old data. This should NOT fail because we have the + // capacity of the old page which already fits the data we requested. + target.data.cloneFrom(page, y_start, y_end) catch |err| { + log.err( + "error cloning rows for split err={}", + .{err}, + ); + + // Rather than crash, we return an OutOfSpace to show that + // we couldn't split and let our callers gracefully handle it. + // Realistically though... this should not happen. + return error.OutOfSpace; + }; + + // From this point forward there is no going back. We have no + // error handling. It is possible but we haven't written it. + errdefer comptime unreachable; + + // Move any tracked pins from the copied rows + for (self.tracked_pins.keys()) |tracked| { + if (&tracked.node.data != page or + tracked.y < p.y) continue; + + tracked.node = target; + tracked.y -= p.y; + // p.x remains the same since we're copying the row as-is + } + + // Clear our rows + for (page.rows.ptr(page.memory)[y_start..y_end]) |*row| { + page.clearCells( + row, + 0, + page.size.cols, + ); + } + page.size.rows -= y_end - y_start; + + self.pages.insertAfter(original_node, target); +} + /// This represents the state necessary to render a scrollbar for this /// PageList. It has the total size, the offset, and the size of the viewport. pub const Scrollbar = struct { @@ -12013,3 +12105,596 @@ test "PageList compact insufficient savings returns null" { } } } + +test "PageList split at middle row" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const page = &s.pages.first.?.data; + + // Write content to rows: row 0 gets codepoint 0, row 1 gets 1, etc. + for (0..page.size.rows) |y| { + const rac = page.getRowAndCell(0, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(y) }, + }; + } + + // Split at row 5 (middle) + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 5, .x = 0 }; + try s.split(split_pin); + + // Verify two pages exist + try testing.expect(s.pages.first != null); + try testing.expect(s.pages.first.?.next != null); + + const first_page = &s.pages.first.?.data; + const second_page = &s.pages.first.?.next.?.data; + + // First page should have rows 0-4 (5 rows) + try testing.expectEqual(@as(usize, 5), first_page.size.rows); + // Second page should have rows 5-9 (5 rows) + try testing.expectEqual(@as(usize, 5), second_page.size.rows); + + // Verify content in first page is preserved (rows 0-4 have codepoints 0-4) + for (0..5) |y| { + const rac = first_page.getRowAndCell(0, y); + try testing.expectEqual(@as(u21, @intCast(y)), rac.cell.content.codepoint); + } + + // Verify content in second page (original rows 5-9, now at y=0-4) + for (0..5) |y| { + const rac = second_page.getRowAndCell(0, y); + try testing.expectEqual(@as(u21, @intCast(y + 5)), rac.cell.content.codepoint); + } +} + +test "PageList split at row 0 is no-op" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const page = &s.pages.first.?.data; + + // Write content to all rows + for (0..page.size.rows) |y| { + const rac = page.getRowAndCell(0, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(y) }, + }; + } + + // Split at row 0 should be a no-op + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 0, .x = 0 }; + try s.split(split_pin); + + // Verify only one page exists (no split occurred) + try testing.expect(s.pages.first != null); + try testing.expect(s.pages.first.?.next == null); + + // Verify all content is still in the original page + try testing.expectEqual(@as(usize, 10), page.size.rows); + for (0..10) |y| { + const rac = page.getRowAndCell(0, y); + try testing.expectEqual(@as(u21, @intCast(y)), rac.cell.content.codepoint); + } +} + +test "PageList split at last row" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const page = &s.pages.first.?.data; + + // Write content to all rows + for (0..page.size.rows) |y| { + const rac = page.getRowAndCell(0, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = @intCast(y) }, + }; + } + + // Split at last row (row 9) + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 9, .x = 0 }; + try s.split(split_pin); + + // Verify two pages exist + try testing.expect(s.pages.first != null); + try testing.expect(s.pages.first.?.next != null); + + const first_page = &s.pages.first.?.data; + const second_page = &s.pages.first.?.next.?.data; + + // First page should have 9 rows + try testing.expectEqual(@as(usize, 9), first_page.size.rows); + // Second page should have 1 row + try testing.expectEqual(@as(usize, 1), second_page.size.rows); + + // Verify content in second page (original row 9, now at y=0) + const rac = second_page.getRowAndCell(0, 0); + try testing.expectEqual(@as(u21, 9), rac.cell.content.codepoint); +} + +test "PageList split single row page returns OutOfSpace" { + const testing = std.testing; + const alloc = testing.allocator; + + // Initialize with 1 row + var s = try init(alloc, 10, 1, 0); + defer s.deinit(); + + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 0, .x = 0 }; + const result = s.split(split_pin); + + try testing.expectError(error.OutOfSpace, result); +} + +test "PageList split moves tracked pins" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + // Track a pin at row 7 + const tracked = try s.trackPin(.{ .node = s.pages.first.?, .y = 7, .x = 3 }); + defer s.untrackPin(tracked); + + // Split at row 5 + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 5, .x = 0 }; + try s.split(split_pin); + + // The tracked pin should now be in the second page + try testing.expect(tracked.node == s.pages.first.?.next.?); + // y should be adjusted: was 7, split at 5, so new y = 7 - 5 = 2 + try testing.expectEqual(@as(usize, 2), tracked.y); + // x should remain unchanged + try testing.expectEqual(@as(usize, 3), tracked.x); +} + +test "PageList split tracked pin before split point unchanged" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const original_node = s.pages.first.?; + + // Track a pin at row 2 (before the split point) + const tracked = try s.trackPin(.{ .node = original_node, .y = 2, .x = 5 }); + defer s.untrackPin(tracked); + + // Split at row 5 + const split_pin: Pin = .{ .node = original_node, .y = 5, .x = 0 }; + try s.split(split_pin); + + // The tracked pin should remain in the original page + try testing.expect(tracked.node == s.pages.first.?); + // y and x should be unchanged + try testing.expectEqual(@as(usize, 2), tracked.y); + try testing.expectEqual(@as(usize, 5), tracked.x); +} + +test "PageList split tracked pin at split point moves to new page" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const original_node = s.pages.first.?; + + // Track a pin at the exact split point (row 5) + const tracked = try s.trackPin(.{ .node = original_node, .y = 5, .x = 4 }); + defer s.untrackPin(tracked); + + // Split at row 5 + const split_pin: Pin = .{ .node = original_node, .y = 5, .x = 0 }; + try s.split(split_pin); + + // The tracked pin should be in the new page + try testing.expect(tracked.node == s.pages.first.?.next.?); + // y should be 0 since it was at the split point: 5 - 5 = 0 + try testing.expectEqual(@as(usize, 0), tracked.y); + // x should remain unchanged + try testing.expectEqual(@as(usize, 4), tracked.x); +} + +test "PageList split multiple tracked pins across regions" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const original_node = s.pages.first.?; + + // Track multiple pins in different regions + const pin_before = try s.trackPin(.{ .node = original_node, .y = 1, .x = 0 }); + defer s.untrackPin(pin_before); + const pin_at_split = try s.trackPin(.{ .node = original_node, .y = 5, .x = 2 }); + defer s.untrackPin(pin_at_split); + const pin_after1 = try s.trackPin(.{ .node = original_node, .y = 7, .x = 3 }); + defer s.untrackPin(pin_after1); + const pin_after2 = try s.trackPin(.{ .node = original_node, .y = 9, .x = 8 }); + defer s.untrackPin(pin_after2); + + // Split at row 5 + const split_pin: Pin = .{ .node = original_node, .y = 5, .x = 0 }; + try s.split(split_pin); + + const first_page = s.pages.first.?; + const second_page = first_page.next.?; + + // Pin before split point stays in original page + try testing.expect(pin_before.node == first_page); + try testing.expectEqual(@as(usize, 1), pin_before.y); + try testing.expectEqual(@as(usize, 0), pin_before.x); + + // Pin at split point moves to new page with y=0 + try testing.expect(pin_at_split.node == second_page); + try testing.expectEqual(@as(usize, 0), pin_at_split.y); + try testing.expectEqual(@as(usize, 2), pin_at_split.x); + + // Pins after split point move to new page with adjusted y + try testing.expect(pin_after1.node == second_page); + try testing.expectEqual(@as(usize, 2), pin_after1.y); // 7 - 5 = 2 + try testing.expectEqual(@as(usize, 3), pin_after1.x); + + try testing.expect(pin_after2.node == second_page); + try testing.expectEqual(@as(usize, 4), pin_after2.y); // 9 - 5 = 4 + try testing.expectEqual(@as(usize, 8), pin_after2.x); +} + +test "PageList split tracked viewport_pin in split region moves correctly" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const original_node = s.pages.first.?; + + // Set viewport_pin to row 7 (after split point) + s.viewport_pin.node = original_node; + s.viewport_pin.y = 7; + s.viewport_pin.x = 6; + + // Split at row 5 + const split_pin: Pin = .{ .node = original_node, .y = 5, .x = 0 }; + try s.split(split_pin); + + // viewport_pin should be in the new page + try testing.expect(s.viewport_pin.node == s.pages.first.?.next.?); + // y should be adjusted: 7 - 5 = 2 + try testing.expectEqual(@as(usize, 2), s.viewport_pin.y); + // x should remain unchanged + try testing.expectEqual(@as(usize, 6), s.viewport_pin.x); +} + +test "PageList split middle page preserves linked list order" { + const testing = std.testing; + const alloc = testing.allocator; + + // Create a single page with 12 rows + var s = try init(alloc, 10, 12, 0); + defer s.deinit(); + + // Split at row 4 to create: page1 (rows 0-3), page2 (rows 4-11) + const first_node = s.pages.first.?; + const split_pin1: Pin = .{ .node = first_node, .y = 4, .x = 0 }; + try s.split(split_pin1); + + // Now we have 2 pages + const page1 = s.pages.first.?; + const page2 = s.pages.first.?.next.?; + try testing.expectEqual(@as(usize, 4), page1.data.size.rows); + try testing.expectEqual(@as(usize, 8), page2.data.size.rows); + + // Split page2 at row 4 to create: page1 -> page2 (rows 0-3) -> page3 (rows 4-7) + const split_pin2: Pin = .{ .node = page2, .y = 4, .x = 0 }; + try s.split(split_pin2); + + // Now we have 3 pages + const first = s.pages.first.?; + const middle = first.next.?; + const last = middle.next.?; + + // Verify linked list order: first -> middle -> last + try testing.expectEqual(page1, first); + try testing.expectEqual(page2, middle); + try testing.expectEqual(s.pages.last.?, last); + + // Verify prev pointers + try testing.expect(first.prev == null); + try testing.expectEqual(first, middle.prev.?); + try testing.expectEqual(middle, last.prev.?); + + // Verify next pointers + try testing.expectEqual(middle, first.next.?); + try testing.expectEqual(last, middle.next.?); + try testing.expect(last.next == null); + + // Verify row counts + try testing.expectEqual(@as(usize, 4), first.data.size.rows); + try testing.expectEqual(@as(usize, 4), middle.data.size.rows); + try testing.expectEqual(@as(usize, 4), last.data.size.rows); +} + +test "PageList split last page makes new page the last" { + const testing = std.testing; + const alloc = testing.allocator; + + // Create a single page with 10 rows + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + // Split to create 2 pages first + const first_node = s.pages.first.?; + const split_pin1: Pin = .{ .node = first_node, .y = 5, .x = 0 }; + try s.split(split_pin1); + + // Now split the last page + const last_before_split = s.pages.last.?; + try testing.expectEqual(@as(usize, 5), last_before_split.data.size.rows); + + const split_pin2: Pin = .{ .node = last_before_split, .y = 2, .x = 0 }; + try s.split(split_pin2); + + // The new page should be the new last + const new_last = s.pages.last.?; + try testing.expect(new_last != last_before_split); + try testing.expectEqual(last_before_split, new_last.prev.?); + try testing.expect(new_last.next == null); + + // Verify row counts: original last has 2 rows, new last has 3 rows + try testing.expectEqual(@as(usize, 2), last_before_split.data.size.rows); + try testing.expectEqual(@as(usize, 3), new_last.data.size.rows); +} + +test "PageList split first page keeps original as first" { + const testing = std.testing; + const alloc = testing.allocator; + + // Create 2 pages by splitting + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const original_first = s.pages.first.?; + const split_pin1: Pin = .{ .node = original_first, .y = 5, .x = 0 }; + try s.split(split_pin1); + + // Get second page (created by first split) + const second_page = s.pages.first.?.next.?; + + // Now split the first page again + const split_pin2: Pin = .{ .node = s.pages.first.?, .y = 2, .x = 0 }; + try s.split(split_pin2); + + // Original first should still be first + try testing.expectEqual(original_first, s.pages.first.?); + try testing.expect(s.pages.first.?.prev == null); + + // New page should be inserted between first and second + const inserted = s.pages.first.?.next.?; + try testing.expect(inserted != second_page); + try testing.expectEqual(second_page, inserted.next.?); + + // Verify row counts: first has 2, inserted has 3, second has 5 + try testing.expectEqual(@as(usize, 2), s.pages.first.?.data.size.rows); + try testing.expectEqual(@as(usize, 3), inserted.data.size.rows); + try testing.expectEqual(@as(usize, 5), second_page.data.size.rows); +} + +test "PageList split preserves wrap flags" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const page = &s.pages.first.?.data; + + // Set wrap flags on rows that will be in the second page after split + // Row 5: wrap = true (this is the start of a wrapped line) + // Row 6: wrap_continuation = true (this continues the wrap) + // Row 7: wrap = true, wrap_continuation = true (wrapped and continues) + { + const rac5 = page.getRowAndCell(0, 5); + rac5.row.wrap = true; + + const rac6 = page.getRowAndCell(0, 6); + rac6.row.wrap_continuation = true; + + const rac7 = page.getRowAndCell(0, 7); + rac7.row.wrap = true; + rac7.row.wrap_continuation = true; + } + + // Split at row 5 + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 5, .x = 0 }; + try s.split(split_pin); + + const second_page = &s.pages.first.?.next.?.data; + + // Verify wrap flags are preserved in new page + // Original row 5 is now row 0 in second page + { + const rac0 = second_page.getRowAndCell(0, 0); + try testing.expect(rac0.row.wrap); + try testing.expect(!rac0.row.wrap_continuation); + } + + // Original row 6 is now row 1 in second page + { + const rac1 = second_page.getRowAndCell(0, 1); + try testing.expect(!rac1.row.wrap); + try testing.expect(rac1.row.wrap_continuation); + } + + // Original row 7 is now row 2 in second page + { + const rac2 = second_page.getRowAndCell(0, 2); + try testing.expect(rac2.row.wrap); + try testing.expect(rac2.row.wrap_continuation); + } +} + +test "PageList split preserves styled cells" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const page = &s.pages.first.?.data; + + // Create a style and apply it to cells in rows 5-7 (which will be in the second page) + const style: stylepkg.Style = .{ .flags = .{ .bold = true } }; + const style_id = try page.styles.add(page.memory, style); + + for (5..8) |y| { + const rac = page.getRowAndCell(0, y); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = 'S' }, + .style_id = style_id, + }; + rac.row.styled = true; + page.styles.use(page.memory, style_id); + } + // Release the extra ref from add + page.styles.release(page.memory, style_id); + + // Split at row 5 + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 5, .x = 0 }; + try s.split(split_pin); + + const first_page = &s.pages.first.?.data; + const second_page = &s.pages.first.?.next.?.data; + + // First page should have no styles (all styled rows moved to second page) + try testing.expectEqual(@as(usize, 0), first_page.styles.count()); + + // Second page should have exactly 1 style (the bold style, used by 3 cells) + try testing.expectEqual(@as(usize, 1), second_page.styles.count()); + + // Verify styled cells are preserved in new page + for (0..3) |y| { + const rac = second_page.getRowAndCell(0, y); + try testing.expectEqual(@as(u21, 'S'), rac.cell.content.codepoint); + try testing.expect(rac.cell.style_id != 0); + + const got_style = second_page.styles.get(second_page.memory, rac.cell.style_id); + try testing.expect(got_style.flags.bold); + try testing.expect(rac.row.styled); + } +} + +test "PageList split preserves grapheme clusters" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const page = &s.pages.first.?.data; + + // Add a grapheme cluster to row 6 (will be row 1 in second page after split at 5) + { + const rac = page.getRowAndCell(0, 6); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = 0x1F468 }, // Man emoji + }; + try page.setGraphemes(rac.row, rac.cell, &.{ + 0x200D, // ZWJ + 0x1F469, // Woman emoji + }); + } + + // Split at row 5 + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 5, .x = 0 }; + try s.split(split_pin); + + const first_page = &s.pages.first.?.data; + const second_page = &s.pages.first.?.next.?.data; + + // First page should have no graphemes (the grapheme row moved to second page) + try testing.expectEqual(@as(usize, 0), first_page.graphemeCount()); + + // Second page should have exactly 1 grapheme + try testing.expectEqual(@as(usize, 1), second_page.graphemeCount()); + + // Verify grapheme is preserved in new page (original row 6 is now row 1) + { + const rac = second_page.getRowAndCell(0, 1); + try testing.expectEqual(@as(u21, 0x1F468), rac.cell.content.codepoint); + try testing.expect(rac.row.grapheme); + + const cps = second_page.lookupGrapheme(rac.cell).?; + try testing.expectEqual(@as(usize, 2), cps.len); + try testing.expectEqual(@as(u21, 0x200D), cps[0]); + try testing.expectEqual(@as(u21, 0x1F469), cps[1]); + } +} + +test "PageList split preserves hyperlinks" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 10, 10, 0); + defer s.deinit(); + + const page = &s.pages.first.?.data; + + // Add a hyperlink to row 7 (will be row 2 in second page after split at 5) + const hyperlink_id = try page.insertHyperlink(.{ + .id = .{ .implicit = 0 }, + .uri = "https://example.com", + }); + { + const rac = page.getRowAndCell(0, 7); + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = 'L' }, + }; + try page.setHyperlink(rac.row, rac.cell, hyperlink_id); + } + + // Split at row 5 + const split_pin: Pin = .{ .node = s.pages.first.?, .y = 5, .x = 0 }; + try s.split(split_pin); + + const first_page = &s.pages.first.?.data; + const second_page = &s.pages.first.?.next.?.data; + + // First page should have no hyperlinks (the hyperlink row moved to second page) + try testing.expectEqual(@as(usize, 0), first_page.hyperlink_set.count()); + + // Second page should have exactly 1 hyperlink + try testing.expectEqual(@as(usize, 1), second_page.hyperlink_set.count()); + + // Verify hyperlink is preserved in new page (original row 7 is now row 2) + { + const rac = second_page.getRowAndCell(0, 2); + try testing.expectEqual(@as(u21, 'L'), rac.cell.content.codepoint); + try testing.expect(rac.cell.hyperlink); + + const link_id = second_page.lookupHyperlink(rac.cell).?; + const link = second_page.hyperlink_set.get(second_page.memory, link_id); + try testing.expectEqualStrings("https://example.com", link.uri.slice(second_page.memory)); + } +} From 4e60a850995ae7cd89faf456208b6df38eab051a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 19 Jan 2026 10:27:43 -0800 Subject: [PATCH 34/37] terminal: setAttribute failure reverts back to prior style --- src/terminal/Screen.zig | 66 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 1538da5da..d09477eda 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1789,9 +1789,22 @@ fn resizeInternal( /// Set a style attribute for the current cursor. /// -/// This can cause a page split if the current page cannot fit this style. -/// This is the only scenario an error return is possible. +/// If the style can't be set due to any internal errors (memory-related), +/// then this will revert back to the existing style and return an error. pub fn setAttribute(self: *Screen, attr: sgr.Attribute) !void { + // If we fail to set our style for any reason, we should revert + // back to the old style. If we fail to do that, we revert back to + // the default style. + const old_style = self.cursor.style; + errdefer { + self.cursor.style = old_style; + self.manualStyleUpdate() catch |err| { + log.warn("setAttribute error restoring old style after failure err={}", .{err}); + self.cursor.style = .{}; + self.cursor.style_id = style.default_id; + }; + } + switch (attr) { .unset => { self.cursor.style = .{}; @@ -1935,6 +1948,9 @@ pub fn setAttribute(self: *Screen, attr: sgr.Attribute) !void { /// Call this whenever you manually change the cursor style. /// +/// If this returns an error, the style change did not take effect and +/// the cursor style is reverted back to the default. +/// /// Note that this can return any PageList capacity error, because it /// is possible for the internal pagelist to not accommodate the new style /// at all. This WILL attempt to resize our internal pages to fit the style @@ -9247,3 +9263,49 @@ test "Screen: cursorDown to page with insufficient capacity" { try testing.expect(false); } } + +test "Screen setAttribute returns OutOfSpace at max styles" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ .cols = 10, .rows = 10, .max_scrollback = 0 }); + defer s.deinit(); + + // Increase the page's style capacity to max by repeatedly calling increaseCapacity + const max_styles = std.math.maxInt(size.CellCountInt); + while (s.cursor.page_pin.node.data.capacity.styles < max_styles) { + _ = s.pages.increaseCapacity( + s.cursor.page_pin.node, + .styles, + ) catch break; + } + + // Get the page reference after increaseCapacity + const page = &s.cursor.page_pin.node.data; + try testing.expectEqual(max_styles, page.capacity.styles); + + // Fill up all style slots + page.pauseIntegrityChecks(true); + var n: u24 = 1; + while (page.styles.add( + page.memory, + .{ .bg_color = .{ .rgb = @bitCast(n) } }, + )) |_| n += 1 else |_| {} + page.pauseIntegrityChecks(false); + + // Set a style that we can track as "prior" + // Use a style that's already in the page by reusing one we added + s.cursor.style = .{ .bg_color = .{ .rgb = @bitCast(@as(u24, 1)) } }; + const prior_id = page.styles.add(page.memory, s.cursor.style) catch unreachable; + s.cursor.style_id = prior_id; + + // Now try to set a new unique attribute that would require a new style slot + // This should fail with OutOfSpace since we're at max capacity + const result = s.setAttribute(.bold); + + // Should return OutOfSpace error + try testing.expectError(error.OutOfSpace, result); + + // The cursor style_id should remain the prior one + try testing.expectEqual(prior_id, s.cursor.style_id); +} From c412b30cb5ffc1a208d4bb8b39c967cb00a5e6b4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 19 Jan 2026 10:08:51 -0800 Subject: [PATCH 35/37] terminal: splitForCapacity, manualStyleUpdate uses this --- src/terminal/PageList.zig | 2 +- src/terminal/Screen.zig | 189 ++++++++++++++++++++++++++++++++------ 2 files changed, 163 insertions(+), 28 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index c639f15cd..41f8d6533 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -2776,7 +2776,7 @@ pub fn split( // Ran into a bug that I can only explain via aliasing. If a tracked // pin is passed in, its possible Zig will alias the memory and then - // when we modify it later it updates our p here. Coyping the node + // when we modify it later it updates our p here. Copying the node // fixes this. const original_node = p.node; const page: *Page = &original_node.data; diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index d09477eda..5ea338b70 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1791,7 +1791,10 @@ fn resizeInternal( /// /// If the style can't be set due to any internal errors (memory-related), /// then this will revert back to the existing style and return an error. -pub fn setAttribute(self: *Screen, attr: sgr.Attribute) !void { +pub fn setAttribute( + self: *Screen, + attr: sgr.Attribute, +) PageList.IncreaseCapacityError!void { // If we fail to set our style for any reason, we should revert // back to the old style. If we fail to do that, we revert back to // the default style. @@ -1995,13 +1998,22 @@ pub fn manualStyleUpdate(self: *Screen) PageList.IncreaseCapacityError!void { ) catch |err| id: { // Our style map is full or needs to be rehashed, so we need to // increase style capacity (or rehash). - const node = try self.increaseCapacity( + const node = self.increaseCapacity( self.cursor.page_pin.node, switch (err) { error.OutOfMemory => .styles, error.NeedsRehash => null, }, - ); + ) catch |increase_err| switch (increase_err) { + error.OutOfMemory => return error.OutOfMemory, + error.OutOfSpace => space: { + // Out of space, we need to split the page. Split wherever + // is using less capacity and hope that works. If it doesn't + // work, we tried. + try self.splitForCapacity(self.cursor.page_pin.*); + break :space self.cursor.page_pin.node; + }, + }; page = &node.data; break :id page.styles.add( @@ -2031,6 +2043,62 @@ pub fn manualStyleUpdate(self: *Screen) PageList.IncreaseCapacityError!void { self.cursor.style_id = id; } +/// Split at the given pin so that the pinned row moves to the page +/// with less used capacity after the split. +/// +/// The primary use case for this is to handle IncreaseCapacityError +/// OutOfSpace conditions where we need to split the page in order +/// to make room for more managed memory. +/// +/// If the caller cares about where the pin moves to, they should +/// setup a tracked pin before calling this and then check that. +/// In many calling cases, the input pin is tracked (e.g. the cursor +/// pin). +/// +/// If this returns OOM then its a system OOM. If this returns OutOfSpace +/// then it means the page can't be split further. +fn splitForCapacity( + self: *Screen, + pin: Pin, +) PageList.SplitError!void { + // Get our capacities. We include our target row because its + // capacity will be preserved. + const bytes_above = Page.layout(pin.node.data.exactRowCapacity( + 0, + pin.y + 1, + )).total_size; + const bytes_below = Page.layout(pin.node.data.exactRowCapacity( + pin.y, + pin.node.data.size.rows, + )).total_size; + + // We need to track the old cursor pin because if our split + // moves the cursor pin we need to update our accounting. + const old_cursor = self.cursor.page_pin.*; + + // If our bytes above are less than bytes below, we move the pin + // to split down one since splitting includes the pinned row in + // the new node. + try self.pages.split(if (bytes_above < bytes_below) + pin.down(1) orelse pin + else + pin); + + // Cursor didn't change nodes, we're done. + if (self.cursor.page_pin.node == old_cursor.node) return; + + // Cursor changed, we need to restore the old pin then use + // cursorChangePin to move to the new pin. The old node is guaranteed + // to still exist, just not the row. + // + // Note that page_row and all that will be invalid, it points to the + // new node, but at the time of writing this we don't need any of that + // to be right in cursorChangePin. + const new_cursor = self.cursor.page_pin.*; + self.cursor.page_pin.* = old_cursor; + self.cursorChangePin(new_cursor); +} + /// Append a grapheme to the given cell within the current cursor row. pub fn appendGrapheme( self: *Screen, @@ -9264,48 +9332,115 @@ test "Screen: cursorDown to page with insufficient capacity" { } } -test "Screen setAttribute returns OutOfSpace at max styles" { +test "Screen setAttribute increases capacity when style map is full" { + // Tests that setAttribute succeeds when the style map is full by + // increasing page capacity. When capacity is at max and increaseCapacity + // returns OutOfSpace, manualStyleUpdate will split the page instead. const testing = std.testing; const alloc = testing.allocator; - var s = try init(alloc, .{ .cols = 10, .rows = 10, .max_scrollback = 0 }); + // Use a small screen with multiple rows + var s = try init(alloc, .{ .cols = 10, .rows = 5, .max_scrollback = 10 }); defer s.deinit(); + // Write content to multiple rows + try s.testWriteString("line1\nline2\nline3\nline4\nline5"); + + // Get the page and fill its style map to capacity + const page = &s.cursor.page_pin.node.data; + const original_styles_capacity = page.capacity.styles; + + // Fill the style map to capacity + { + page.pauseIntegrityChecks(true); + defer page.pauseIntegrityChecks(false); + defer page.assertIntegrity(); + + var n: u24 = 1; + while (page.styles.add( + page.memory, + .{ .bg_color = .{ .rgb = @bitCast(n) } }, + )) |_| n += 1 else |_| {} + } + + // Now try to set a new unique attribute that would require a new style slot + // This should succeed by increasing capacity (or splitting if at max capacity) + try s.setAttribute(.bold); + + // The style should have been applied (bold flag set) + try testing.expect(s.cursor.style.flags.bold); + + // The cursor should have a valid non-default style_id + try testing.expect(s.cursor.style_id != style.default_id); + + // Either the capacity increased or the page was split/changed + const current_page = &s.cursor.page_pin.node.data; + const capacity_increased = current_page.capacity.styles > original_styles_capacity; + const page_changed = current_page != page; + try testing.expect(capacity_increased or page_changed); +} + +test "Screen setAttribute splits page on OutOfSpace at max styles" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, .{ + .cols = 10, + .rows = 10, + .max_scrollback = 0, + }); + defer s.deinit(); + + // Write content to multiple rows so we have something to split + try s.testWriteString("line1\nline2\nline3\nline4\nline5"); + + // Remember the original node + const original_node = s.cursor.page_pin.node; + // Increase the page's style capacity to max by repeatedly calling increaseCapacity + // Use Screen.increaseCapacity to properly maintain cursor state const max_styles = std.math.maxInt(size.CellCountInt); while (s.cursor.page_pin.node.data.capacity.styles < max_styles) { - _ = s.pages.increaseCapacity( + _ = s.increaseCapacity( s.cursor.page_pin.node, .styles, ) catch break; } - // Get the page reference after increaseCapacity - const page = &s.cursor.page_pin.node.data; + // Get the page reference after increaseCapacity - cursor may have moved + var page = &s.cursor.page_pin.node.data; try testing.expectEqual(max_styles, page.capacity.styles); - // Fill up all style slots - page.pauseIntegrityChecks(true); - var n: u24 = 1; - while (page.styles.add( - page.memory, - .{ .bg_color = .{ .rgb = @bitCast(n) } }, - )) |_| n += 1 else |_| {} - page.pauseIntegrityChecks(false); + // Fill the style map to capacity + { + page.pauseIntegrityChecks(true); + defer page.pauseIntegrityChecks(false); + defer page.assertIntegrity(); - // Set a style that we can track as "prior" - // Use a style that's already in the page by reusing one we added - s.cursor.style = .{ .bg_color = .{ .rgb = @bitCast(@as(u24, 1)) } }; - const prior_id = page.styles.add(page.memory, s.cursor.style) catch unreachable; - s.cursor.style_id = prior_id; + var n: u24 = 1; + while (page.styles.add( + page.memory, + .{ .bg_color = .{ .rgb = @bitCast(n) } }, + )) |_| n += 1 else |_| {} + } + + // Track the node before setAttribute + const node_before_set = s.cursor.page_pin.node; // Now try to set a new unique attribute that would require a new style slot - // This should fail with OutOfSpace since we're at max capacity - const result = s.setAttribute(.bold); + // At max capacity, increaseCapacity will return OutOfSpace, triggering page split + try s.setAttribute(.bold); - // Should return OutOfSpace error - try testing.expectError(error.OutOfSpace, result); + // The style should have been applied (bold flag set) + try testing.expect(s.cursor.style.flags.bold); - // The cursor style_id should remain the prior one - try testing.expectEqual(prior_id, s.cursor.style_id); + // The cursor should have a valid non-default style_id + try testing.expect(s.cursor.style_id != style.default_id); + + // The page should have been split + const page_was_split = s.cursor.page_pin.node != node_before_set or + node_before_set.next != null or + node_before_set.prev != null or + s.cursor.page_pin.node != original_node; + try testing.expect(page_was_split); } From a8b31ceb8489d8e26a9a62155752672dcb4dcd0a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 19 Jan 2026 12:04:34 -0800 Subject: [PATCH 36/37] terminal: restoreCursor is now longer fallible We need to have sane behavior in error handling because the running program that sends the restore cursor command has no way to realize it failed. So if our style fails to add (our only fail case) then we revert to no style. https://ampcode.com/threads/T-019bd7dc-cf0b-7439-ad2f-218b3406277a --- src/terminal/Screen.zig | 32 +++++++---- src/terminal/Terminal.zig | 99 +++++++++++++++++++++++++------- src/terminal/stream_readonly.zig | 4 +- src/termio/stream_handler.zig | 4 +- 4 files changed, 103 insertions(+), 36 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 5ea338b70..00bf4078d 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -9350,17 +9350,21 @@ test "Screen setAttribute increases capacity when style map is full" { const page = &s.cursor.page_pin.node.data; const original_styles_capacity = page.capacity.styles; - // Fill the style map to capacity + // Fill the style map to capacity using the StyleSet's layout capacity + // which accounts for the load factor { page.pauseIntegrityChecks(true); defer page.pauseIntegrityChecks(false); defer page.assertIntegrity(); - var n: u24 = 1; - while (page.styles.add( - page.memory, - .{ .bg_color = .{ .rgb = @bitCast(n) } }, - )) |_| n += 1 else |_| {} + const max_items = page.styles.layout.cap; + var n: usize = 1; + while (n < max_items) : (n += 1) { + _ = page.styles.add( + page.memory, + .{ .bg_color = .{ .rgb = @bitCast(@as(u24, @intCast(n))) } }, + ) catch break; + } } // Now try to set a new unique attribute that would require a new style slot @@ -9411,17 +9415,21 @@ test "Screen setAttribute splits page on OutOfSpace at max styles" { var page = &s.cursor.page_pin.node.data; try testing.expectEqual(max_styles, page.capacity.styles); - // Fill the style map to capacity + // Fill the style map to capacity using the StyleSet's layout capacity + // which accounts for the load factor { page.pauseIntegrityChecks(true); defer page.pauseIntegrityChecks(false); defer page.assertIntegrity(); - var n: u24 = 1; - while (page.styles.add( - page.memory, - .{ .bg_color = .{ .rgb = @bitCast(n) } }, - )) |_| n += 1 else |_| {} + const max_items = page.styles.layout.cap; + var n: usize = 1; + while (n < max_items) : (n += 1) { + _ = page.styles.add( + page.memory, + .{ .bg_color = .{ .rgb = @bitCast(@as(u24, @intCast(n))) } }, + ) catch break; + } } // Track the node before setAttribute diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 3740397d3..73571874e 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -996,7 +996,7 @@ pub fn saveCursor(self: *Terminal) void { /// /// The primary and alternate screen have distinct save state. /// If no save was done before values are reset to their initial values. -pub fn restoreCursor(self: *Terminal) !void { +pub fn restoreCursor(self: *Terminal) void { const saved: Screen.SavedCursor = self.screens.active.saved_cursor orelse .{ .x = 0, .y = 0, @@ -1008,10 +1008,17 @@ pub fn restoreCursor(self: *Terminal) !void { }; // Set the style first because it can fail - const old_style = self.screens.active.cursor.style; self.screens.active.cursor.style = saved.style; - errdefer self.screens.active.cursor.style = old_style; - try self.screens.active.manualStyleUpdate(); + self.screens.active.manualStyleUpdate() catch |err| { + // Regardless of the error here, we revert back to an unstyled + // cursor. It is more important that the restore succeeds in + // other attributes because terminals have no way to communicate + // failure back. + log.warn("restoreCursor error updating style err={}", .{err}); + const screen: *Screen = self.screens.active; + screen.cursor.style = .{}; + screen.cursor.style_id = style.default_id; + }; self.screens.active.charset = saved.charset; self.modes.set(.origin, saved.origin); @@ -2747,12 +2754,7 @@ pub fn switchScreenMode( } } else { assert(self.screens.active_key == .primary); - self.restoreCursor() catch |err| { - log.warn( - "restore cursor on switch screen failed to={} err={}", - .{ to, err }, - ); - }; + self.restoreCursor(); }, } } @@ -4807,7 +4809,7 @@ test "Terminal: horizontal tab back with cursor before left margin" { t.saveCursor(); t.modes.set(.enable_left_and_right_margin, true); t.setLeftAndRightMargin(5, 0); - try t.restoreCursor(); + t.restoreCursor(); try t.horizontalTabBack(); try t.print('X'); @@ -9873,7 +9875,7 @@ test "Terminal: saveCursor" { t.screens.active.charset.gr = .G0; try t.setAttribute(.{ .unset = {} }); t.modes.set(.origin, false); - try t.restoreCursor(); + t.restoreCursor(); try testing.expect(t.screens.active.cursor.style.flags.bold); try testing.expect(t.screens.active.charset.gr == .G3); try testing.expect(t.modes.get(.origin)); @@ -9889,7 +9891,7 @@ test "Terminal: saveCursor position" { t.saveCursor(); t.setCursorPos(1, 1); try t.print('B'); - try t.restoreCursor(); + t.restoreCursor(); try t.print('X'); { @@ -9909,7 +9911,7 @@ test "Terminal: saveCursor pending wrap state" { t.saveCursor(); t.setCursorPos(1, 1); try t.print('B'); - try t.restoreCursor(); + t.restoreCursor(); try t.print('X'); { @@ -9929,7 +9931,7 @@ test "Terminal: saveCursor origin mode" { t.modes.set(.enable_left_and_right_margin, true); t.setLeftAndRightMargin(3, 5); t.setTopAndBottomMargin(2, 4); - try t.restoreCursor(); + t.restoreCursor(); try t.print('X'); { @@ -9947,7 +9949,7 @@ test "Terminal: saveCursor resize" { t.setCursorPos(1, 10); t.saveCursor(); try t.resize(alloc, 5, 5); - try t.restoreCursor(); + t.restoreCursor(); try t.print('X'); { @@ -9968,7 +9970,7 @@ test "Terminal: saveCursor protected pen" { t.saveCursor(); t.setProtectedMode(.off); try testing.expect(!t.screens.active.cursor.protected); - try t.restoreCursor(); + t.restoreCursor(); try testing.expect(t.screens.active.cursor.protected); } @@ -9981,10 +9983,67 @@ test "Terminal: saveCursor doesn't modify hyperlink state" { const id = t.screens.active.cursor.hyperlink_id; t.saveCursor(); try testing.expectEqual(id, t.screens.active.cursor.hyperlink_id); - try t.restoreCursor(); + t.restoreCursor(); try testing.expectEqual(id, t.screens.active.cursor.hyperlink_id); } +test "Terminal: restoreCursor uses default style on OutOfSpace" { + // Tests that restoreCursor falls back to default style when + // manualStyleUpdate fails with OutOfSpace (can't split a 1-row page + // and styles are at max capacity). + const alloc = testing.allocator; + + // Use a single row so the page can't be split + var t = try init(alloc, .{ .cols = 10, .rows = 1 }); + defer t.deinit(alloc); + + // Set a style and save the cursor + try t.setAttribute(.{ .bold = {} }); + t.saveCursor(); + + // Clear the style + try t.setAttribute(.{ .unset = {} }); + try testing.expect(!t.screens.active.cursor.style.flags.bold); + + // Fill the style map to max capacity + const max_styles = std.math.maxInt(size.CellCountInt); + while (t.screens.active.cursor.page_pin.node.data.capacity.styles < max_styles) { + _ = t.screens.active.increaseCapacity( + t.screens.active.cursor.page_pin.node, + .styles, + ) catch break; + } + + const page = &t.screens.active.cursor.page_pin.node.data; + try testing.expectEqual(max_styles, page.capacity.styles); + + // Fill all style slots using the StyleSet's layout capacity which accounts + // for the load factor. The capacity in the layout is the actual max number + // of items that can be stored. + { + page.pauseIntegrityChecks(true); + defer page.pauseIntegrityChecks(false); + defer page.assertIntegrity(); + + const max_items = page.styles.layout.cap; + var n: usize = 1; + while (n < max_items) : (n += 1) { + _ = page.styles.add( + page.memory, + .{ .bg_color = .{ .rgb = @bitCast(@as(u24, @intCast(n))) } }, + ) catch break; + } + } + + // Restore cursor - should fall back to default style since page + // can't be split (1 row) and styles are at max capacity + t.restoreCursor(); + + // The style should be reset to default because OutOfSpace occurred + try testing.expect(!t.screens.active.cursor.style.flags.bold); + try testing.expectEqual(style.default_id, t.screens.active.cursor.style_id); +} + test "Terminal: setProtectedMode" { const alloc = testing.allocator; var t = try init(alloc, .{ .cols = 3, .rows = 3 }); @@ -11376,7 +11435,7 @@ test "Terminal: resize with reflow and saved cursor" { t.saveCursor(); try t.resize(alloc, 5, 3); - try t.restoreCursor(); + t.restoreCursor(); { const str = try t.plainString(testing.allocator); @@ -11417,7 +11476,7 @@ test "Terminal: resize with reflow and saved cursor pending wrap" { t.saveCursor(); try t.resize(alloc, 5, 3); - try t.restoreCursor(); + t.restoreCursor(); { const str = try t.plainString(testing.allocator); diff --git a/src/terminal/stream_readonly.zig b/src/terminal/stream_readonly.zig index 1ee4f3f08..9b4999116 100644 --- a/src/terminal/stream_readonly.zig +++ b/src/terminal/stream_readonly.zig @@ -125,7 +125,7 @@ pub const Handler = struct { } }, .save_cursor => self.terminal.saveCursor(), - .restore_cursor => try self.terminal.restoreCursor(), + .restore_cursor => self.terminal.restoreCursor(), .invoke_charset => self.terminal.invokeCharset(value.bank, value.charset, value.locking), .configure_charset => self.terminal.configureCharset(value.slot, value.charset), .set_attribute => switch (value) { @@ -240,7 +240,7 @@ pub const Handler = struct { .save_cursor => if (enabled) { self.terminal.saveCursor(); } else { - try self.terminal.restoreCursor(); + self.terminal.restoreCursor(); }, .enable_mode_3 => {}, diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index 082a0fa10..2b92c19e3 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -721,7 +721,7 @@ pub const StreamHandler = struct { if (enabled) { self.terminal.saveCursor(); } else { - try self.terminal.restoreCursor(); + self.terminal.restoreCursor(); } }, @@ -933,7 +933,7 @@ pub const StreamHandler = struct { } pub inline fn restoreCursor(self: *StreamHandler) !void { - try self.terminal.restoreCursor(); + self.terminal.restoreCursor(); } pub fn enquiry(self: *StreamHandler) !void { From ae8d2c7a3e135b96ad7d3827dc1b8f92b49c8cd4 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 19 Jan 2026 12:17:07 -0800 Subject: [PATCH 37/37] terminal: fix up some of the manual handling, comments --- src/terminal/Screen.zig | 21 +++++++++++---------- src/terminal/Terminal.zig | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 00bf4078d..d36cdac2a 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1804,7 +1804,7 @@ pub fn setAttribute( self.manualStyleUpdate() catch |err| { log.warn("setAttribute error restoring old style after failure err={}", .{err}); self.cursor.style = .{}; - self.cursor.style_id = style.default_id; + self.manualStyleUpdate() catch unreachable; }; } @@ -1951,17 +1951,18 @@ pub fn setAttribute( /// Call this whenever you manually change the cursor style. /// +/// This function can NOT fail if the cursor style is changing to the +/// default style. +/// /// If this returns an error, the style change did not take effect and -/// the cursor style is reverted back to the default. +/// the cursor style is reverted back to the default. The only scenario +/// this returns an error is if there is a physical memory allocation failure +/// or if there is no possible way to increase style capacity to store +/// the style. /// -/// Note that this can return any PageList capacity error, because it -/// is possible for the internal pagelist to not accommodate the new style -/// at all. This WILL attempt to resize our internal pages to fit the style -/// but it is possible that it cannot be done, in which case upstream callers -/// need to split the page or do something else. -/// -/// NOTE(mitchellh): I think in the future we'll do page splitting -/// automatically here and remove this failure scenario. +/// This function WILL split pages as necessary to accommodate the new style. +/// So if OutOfSpace is returned, it means that even after splitting the page +/// there was still no room for the new style. pub fn manualStyleUpdate(self: *Screen) PageList.IncreaseCapacityError!void { defer self.assertIntegrity(); var page: *Page = &self.cursor.page_pin.node.data; diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 73571874e..7e02e3a24 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1017,7 +1017,7 @@ pub fn restoreCursor(self: *Terminal) void { log.warn("restoreCursor error updating style err={}", .{err}); const screen: *Screen = self.screens.active; screen.cursor.style = .{}; - screen.cursor.style_id = style.default_id; + self.screens.active.manualStyleUpdate() catch unreachable; }; self.screens.active.charset = saved.charset;