From d05381db832d03ede3b76e376770da474e126627 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 31 Aug 2023 14:55:27 -0700 Subject: [PATCH 1/7] remove some unreachables, log errors to avoid crashes These are still TODO but we don't want to crash on bad input. --- src/terminal/Terminal.zig | 7 +++++-- src/termio/Exec.zig | 3 +-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index fdf5bf3a0..f2973d4b0 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -990,7 +990,11 @@ pub fn setCursorColAbsolute(self: *Terminal, col_req: usize) void { // TODO: test - assert(!self.modes.get(.origin)); // TODO + // TODO + if (!self.modes.get(.origin)) { + log.err("setCursorColAbsolute: cursor origin mode handling not implemented yet", .{}); + return; + } if (self.status_display != .main) return; // TODO @@ -1116,7 +1120,6 @@ pub fn eraseLine( else => { log.err("unimplemented erase line mode: {}", .{mode}); - @panic("unimplemented"); }, } } diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 4df512e7e..021e58fe8 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -1216,8 +1216,7 @@ const StreamHandler = struct { pub fn setCursorRow(self: *StreamHandler, row: u16) !void { if (self.terminal.modes.get(.origin)) { // TODO - log.err("setCursorRow: implement origin mode", .{}); - unreachable; + log.err("setCursorRow: unimplemented origin mode handling, misrendering may occur", .{}); } self.terminal.setCursorPos(row, self.terminal.screen.cursor.x + 1); From 0aebf1e406ff3f1cc151c4ad5ac9cc85ac31d5e7 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 31 Aug 2023 17:52:22 -0700 Subject: [PATCH 2/7] terminal: CSI S allows for count greater than scroll region height --- src/terminal/Screen.zig | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 09333acd2..55fff4424 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1071,12 +1071,12 @@ pub fn copyRow(self: *Screen, dst: RowIndex, src: RowIndex) !void { /// the top/bottom are within it). /// /// This can be used to implement terminal scroll regions efficiently. -pub fn scrollRegionUp(self: *Screen, top: RowIndex, bottom: RowIndex, count: usize) void { +pub fn scrollRegionUp(self: *Screen, top: RowIndex, bottom: RowIndex, count_req: usize) void { const tracy = trace(@src()); defer tracy.end(); // Avoid a lot of work if we're doing nothing. - if (count == 0) return; + if (count_req == 0) return; // Convert our top/bottom to screen y values. This is the y offset // in the entire screen buffer. @@ -1088,7 +1088,7 @@ pub fn scrollRegionUp(self: *Screen, top: RowIndex, bottom: RowIndex, count: usi // We can only scroll up to the number of rows in the region. The "+ 1" // is because our y values are 0-based and count is 1-based. - assert(count <= (bot_y - top_y + 1)); + const count = @min(count_req, bot_y - top_y + 1); // Get the storage pointer for the full scroll region. We're going to // be modifying the whole thing so we get it right away. @@ -4014,6 +4014,22 @@ test "Screen: scrollRegionUp multiple count" { } } +test "Screen: scrollRegionUp count greater than available lines" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 4, 5, 0); + defer s.deinit(); + try s.testWriteString("1ABCD\n2EFGH\n3IJKL\n4ABCD"); + + s.scrollRegionUp(.{ .active = 1 }, .{ .active = 2 }, 10); + { + // Test our contents rotated + var contents = try s.testString(alloc, .screen); + defer alloc.free(contents); + try testing.expectEqualStrings("1ABCD\n\n\n4ABCD", contents); + } +} test "Screen: scrollRegionUp fills with pen" { const testing = std.testing; const alloc = testing.allocator; From 6c13627d5136bd0c87629d20cece63b7821e7965 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 31 Aug 2023 19:42:23 -0700 Subject: [PATCH 3/7] terminal: delete chars (CSI P) tested, fixes many issues --- src/terminal/Terminal.zig | 94 +++++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 14 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index f2973d4b0..2ea6fc5c5 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1132,25 +1132,27 @@ pub fn eraseLine( /// scrolling region, it is adjusted down. /// /// Does not change the cursor position. -/// -/// TODO: test -pub fn deleteChars(self: *Terminal, count: usize) !void { +pub fn deleteChars(self: *Terminal, count_req: usize) !void { const tracy = trace(@src()); defer tracy.end(); + // Count defaults to 1 and we can't delete more than we have remaining + // in the row. + const count = @min(self.cols - self.screen.cursor.x, count_req); + if (count == 0) return; + const line = self.screen.getRow(.{ .active = self.screen.cursor.y }); + for (0..count) |i| { + const x = self.screen.cursor.x + i; + const copy_x = x + count; + if (copy_x >= self.cols) { + line.getCellPtr(x).* = self.screen.cursor.pen; + continue; + } - // Our last index is at most the end of the number of chars we have - // in the current line. - const end = self.cols - count; - - // Shift - var i: usize = self.screen.cursor.x; - while (i < end) : (i += 1) { - const j = i + count; - const j_cell = line.getCellPtr(j); - line.getCellPtr(i).* = j_cell.*; - j_cell.char = 0; + const copy_cell = line.getCellPtr(copy_x); + line.getCellPtr(x).* = copy_cell.*; + copy_cell.char = 0; } } @@ -2614,6 +2616,70 @@ test "Terminal: print wide char with 1-column width" { try t.print('😀'); // 0x1F600 } +test "Terminal: deleteChars" { + const alloc = testing.allocator; + var t = try init(alloc, 5, 5); + defer t.deinit(alloc); + + for ("ABCDE") |c| try t.print(c); + t.setCursorPos(1, 2); + + try t.deleteChars(2); + { + var str = try t.plainString(testing.allocator); + defer testing.allocator.free(str); + try testing.expectEqualStrings("ADE", str); + } +} + +test "Terminal: deleteChars zero count" { + const alloc = testing.allocator; + var t = try init(alloc, 5, 5); + defer t.deinit(alloc); + + for ("ABCDE") |c| try t.print(c); + t.setCursorPos(1, 2); + + try t.deleteChars(0); + { + var str = try t.plainString(testing.allocator); + defer testing.allocator.free(str); + try testing.expectEqualStrings("ABCDE", str); + } +} + +test "Terminal: deleteChars more than half" { + const alloc = testing.allocator; + var t = try init(alloc, 5, 5); + defer t.deinit(alloc); + + for ("ABCDE") |c| try t.print(c); + t.setCursorPos(1, 2); + + try t.deleteChars(3); + { + var str = try t.plainString(testing.allocator); + defer testing.allocator.free(str); + try testing.expectEqualStrings("AE", str); + } +} + +test "Terminal: deleteChars more than line width" { + const alloc = testing.allocator; + var t = try init(alloc, 5, 5); + defer t.deinit(alloc); + + for ("ABCDE") |c| try t.print(c); + t.setCursorPos(1, 2); + + try t.deleteChars(10); + { + var str = try t.plainString(testing.allocator); + defer testing.allocator.free(str); + try testing.expectEqualStrings("A", str); + } +} + // https://github.com/mitchellh/ghostty/issues/272 // This is also tested in depth in screen resize tests but I want to keep // this test around to ensure we don't regress at multiple layers. From 65246327ddc9383f7a6f203849f3628107f6d1dd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 31 Aug 2023 19:45:22 -0700 Subject: [PATCH 4/7] terminal: add more assertions --- src/terminal/Screen.zig | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 55fff4424..532bb352e 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -412,6 +412,8 @@ pub const Row = struct { /// Attach a grapheme codepoint to the given cell. pub fn attachGrapheme(self: Row, x: usize, cp: u21) !void { + assert(x < self.storage.len - 1); + const cell = &self.storage[x + 1].cell; const key = self.getId() + x + 1; const gop = try self.screen.graphemes.getOrPut(self.screen.alloc, key); @@ -448,6 +450,8 @@ pub const Row = struct { /// Removes all graphemes associated with a cell. pub fn clearGraphemes(self: Row, x: usize) void { + assert(x < self.storage.len - 1); + // Our row is now dirty self.storage[0].header.flags.dirty = true; From f4fef559fb227340e3ed0a942f726f2c3cb9eb66 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 31 Aug 2023 20:41:32 -0700 Subject: [PATCH 5/7] terminal: delete lines outside of scroll region should do nothing --- src/terminal/Terminal.zig | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 2ea6fc5c5..ac9eb79ef 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1425,6 +1425,13 @@ pub fn deleteLines(self: *Terminal, count: usize) !void { const tracy = trace(@src()); defer tracy.end(); + // If our cursor is outside of the scroll region, do nothing. + if (self.screen.cursor.y < self.scrolling_region.top or + self.screen.cursor.y > self.scrolling_region.bottom) + { + return; + } + // Move the cursor to the left margin self.screen.cursor.x = 0; @@ -2132,6 +2139,34 @@ test "Terminal: deleteLines with scroll region, large count" { } } +test "Terminal: deleteLines with scroll region, cursor outside of region" { + const alloc = testing.allocator; + var t = try init(alloc, 80, 80); + defer t.deinit(alloc); + + // Initial value + try t.print('A'); + t.carriageReturn(); + try t.linefeed(); + try t.print('B'); + t.carriageReturn(); + try t.linefeed(); + try t.print('C'); + t.carriageReturn(); + try t.linefeed(); + try t.print('D'); + + t.setScrollingRegion(1, 3); + t.setCursorPos(4, 1); + try t.deleteLines(1); + + { + var str = try t.plainString(testing.allocator); + defer testing.allocator.free(str); + try testing.expectEqualStrings("A\nB\nC\nD", str); + } +} + test "Terminal: insertLines" { const alloc = testing.allocator; var t = try init(alloc, 2, 5); From 36756cc8661290c2eb5a03e6a5ca65a9799eba45 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 31 Aug 2023 20:45:13 -0700 Subject: [PATCH 6/7] terminal: charset table should be len 256, not 255 --- src/terminal/charsets.zig | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/terminal/charsets.zig b/src/terminal/charsets.zig index a42d7b0bc..316238458 100644 --- a/src/terminal/charsets.zig +++ b/src/terminal/charsets.zig @@ -85,14 +85,15 @@ const dec_special = tech: { break :tech table; }; -const max_u8 = std.math.maxInt(u8); +/// Our table length is 256 so we can contain all ASCII chars. +const table_len = std.math.maxInt(u8) + 1; /// Creates a table that maps ASCII to ASCII as a getting started point. -fn initTable() [max_u8]u16 { - var result: [max_u8]u16 = undefined; +fn initTable() [table_len]u16 { + var result: [table_len]u16 = undefined; var i: usize = 0; - while (i < max_u8) : (i += 1) result[i] = @intCast(i); - assert(i == max_u8); + while (i < table_len) : (i += 1) result[i] = @intCast(i); + assert(i == table_len); return result; } @@ -105,9 +106,9 @@ test { const table = @field(Charset, field.name).table(); - // Yes, I could use `max_u8` here, but I want to explicitly use a + // Yes, I could use `table_len` here, but I want to explicitly use a // hardcoded constant so that if there are miscompilations or a comptime // issue, we catch it. - try testing.expectEqual(@as(usize, 255), table.len); + try testing.expectEqual(@as(usize, 256), table.len); } } From 3bd77259bf1ff2b8db800d46b1a3fe16a83a1e51 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 31 Aug 2023 21:13:04 -0700 Subject: [PATCH 7/7] font: don't use intCast on index This is a weird one. By using intCast on the `idx` I am periodically getting a panic on index out of bounds where the index is larger than FontIndex can possibly be. Very strange! I tried to just remove intCasts and believe it or not that worked. Previously, `cat /dev/urandom` would trigger the issue in seconds and now I've had it running 20+ minutes without the issue. The additional `if` check is just a safety mechanism --- src/font/Group.zig | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/font/Group.zig b/src/font/Group.zig index ec1e18fbf..c338adaa4 100644 --- a/src/font/Group.zig +++ b/src/font/Group.zig @@ -303,7 +303,8 @@ fn indexForCodepointExact(self: Group, cp: u32, style: Style, p: ?Presentation) /// necessarily force the font to load. pub fn hasCodepoint(self: *Group, index: FontIndex, cp: u32, p: ?Presentation) bool { const list = self.faces.getPtr(index.style); - const item = list.items[@intCast(index.idx)]; + if (index.idx >= list.items.len) return false; + const item = list.items[index.idx]; return switch (item) { .deferred => |v| v.hasCodepoint(cp, p), .loaded => |face| loaded: { @@ -330,7 +331,7 @@ pub fn presentationFromIndex(self: *Group, index: FontIndex) !font.Presentation pub fn faceFromIndex(self: *Group, index: FontIndex) !*Face { if (index.special() != null) return error.SpecialHasNoFace; const list = self.faces.getPtr(index.style); - const item = &list.items[@intCast(index.idx)]; + const item = &list.items[index.idx]; return switch (item.*) { .deferred => |*d| deferred: { const face = try d.load(self.lib, self.size); @@ -553,6 +554,30 @@ test { } } +test "face count limit" { + const testing = std.testing; + const alloc = testing.allocator; + const testFont = @import("test.zig").fontRegular; + + var atlas_greyscale = try font.Atlas.init(alloc, 512, .greyscale); + defer atlas_greyscale.deinit(alloc); + + var lib = try Library.init(); + defer lib.deinit(); + + var group = try init(alloc, lib, .{ .points = 12 }); + defer group.deinit(); + + for (0..FontIndex.Special.start - 1) |_| { + try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }); + } + + try testing.expectError(error.GroupFull, group.addFace( + .regular, + .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }, + )); +} + test "box glyph" { const testing = std.testing; const alloc = testing.allocator;