From 1692a82b330600b346fbd735b6d7b8ca139c3426 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Thu, 5 Sep 2024 18:42:46 -0400 Subject: [PATCH 1/7] test(Terminal): test DCH wide char boundary conditions --- src/terminal/Terminal.zig | 120 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 724821166..8a8f2f415 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -8909,6 +8909,126 @@ test "Terminal: deleteChars split wide character tail" { } } +test "Terminal: deleteChars wide char boundary conditions" { + const alloc = testing.allocator; + var t = try init(alloc, .{ .rows = 1, .cols = 8 }); + defer t.deinit(alloc); + + // EXPLANATION(qwerasd): + // + // There are 3 or 4 boundaries to be concerned with in deleteChars, + // depending on how you count them. Consider the following terminal: + // + // +--------+ + // 0 |.ABCDEF.| + // : ^ : (^ = cursor) + // +--------+ + // + // if we DCH 3 we get + // + // +--------+ + // 0 |.DEF....| + // +--------+ + // + // The boundaries exist at the following points then: + // + // +--------+ + // 0 |.ABCDEF.| + // :11 22 33: + // +--------+ + // + // I'm counting 2 for double since it's both the end of the deleted + // content and the start of the content that is shifted in to place. + // + // Now consider wide characters (represented as `WW`) at these boundaries: + // + // +--------+ + // 0 |WWaWWbWW| + // : ^ : (^ = cursor) + // : ^^^ : (^ = deleted by DCH 3) + // +--------+ + // + // -> DCH 3 + // -> The first 2 wide characters are split & destroyed (verified in xterm) + // + // +--------+ + // 0 |..bWW...| + // +--------+ + + try t.printString("πŸ˜€aπŸ˜€bπŸ˜€"); + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings("πŸ˜€aπŸ˜€bπŸ˜€", str); + } + + t.setCursorPos(1, 2); + t.deleteChars(3); + t.screen.cursor.page_pin.page.data.assertIntegrity(); + + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings(" bπŸ˜€", str); + } +} + +test "Terminal: deleteChars wide char wrap boundary conditions" { + const alloc = testing.allocator; + var t = try init(alloc, .{ .rows = 3, .cols = 8 }); + defer t.deinit(alloc); + + // EXPLANATION(qwerasd): + // (cont. from "Terminal: deleteChars wide char boundary conditions") + // + // Additionally consider soft-wrapped wide chars (`H` = spacer head): + // + // +--------+ + // 0 |.......H… + // 1 …WWabcdeH… + // : ^ : (^ = cursor) + // : ^^^ : (^ = deleted by DCH 3) + // 2 …WW......| + // +--------+ + // + // -> DCH 3 + // -> First wide character split and destroyed, including spacer head, + // second spacer head removed (verified in xterm). + // -> Wrap state of row reset + // + // +--------+ + // 0 |........| + // 1 |.cde....| + // 2 |WW......| + // +--------+ + // + + try t.printString(".......πŸ˜€abcdeπŸ˜€......"); + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings(".......\nπŸ˜€abcde\nπŸ˜€......", str); + + const unwrapped = try t.plainStringUnwrapped(alloc); + defer testing.allocator.free(unwrapped); + try testing.expectEqualStrings(".......πŸ˜€abcdeπŸ˜€......", unwrapped); + } + + t.setCursorPos(2, 2); + t.deleteChars(3); + t.screen.cursor.page_pin.page.data.assertIntegrity(); + + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings(".......\n cde\nπŸ˜€......", str); + + const unwrapped = try t.plainStringUnwrapped(alloc); + defer testing.allocator.free(unwrapped); + try testing.expectEqualStrings(".......\n cde\nπŸ˜€......", unwrapped); + } +} + test "Terminal: saveCursor" { const alloc = testing.allocator; var t = try init(alloc, .{ .cols = 3, .rows = 3 }); From 04271c6a07773e2c7ae840b76e67d2545744cb48 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Thu, 5 Sep 2024 21:13:56 -0400 Subject: [PATCH 2/7] test(Terminal): test ECH wide char boundary conditions --- src/terminal/Terminal.zig | 54 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 8a8f2f415..a7a277921 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -6063,6 +6063,60 @@ test "Terminal: eraseChars protected attributes ignored with dec set" { } } +test "Terminal: eraseChars wide char boundary conditions" { + const alloc = testing.allocator; + var t = try init(alloc, .{ .rows = 1, .cols = 8 }); + defer t.deinit(alloc); + + try t.printString("πŸ˜€aπŸ˜€bπŸ˜€"); + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings("πŸ˜€aπŸ˜€bπŸ˜€", str); + } + + t.setCursorPos(1, 2); + t.eraseChars(3); + t.screen.cursor.page_pin.page.data.assertIntegrity(); + + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings(" bπŸ˜€", str); + } +} + +test "Terminal: eraseChars wide char wrap boundary conditions" { + const alloc = testing.allocator; + var t = try init(alloc, .{ .rows = 3, .cols = 8 }); + defer t.deinit(alloc); + + try t.printString(".......πŸ˜€abcdeπŸ˜€......"); + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings(".......\nπŸ˜€abcde\nπŸ˜€......", str); + + const unwrapped = try t.plainStringUnwrapped(alloc); + defer testing.allocator.free(unwrapped); + try testing.expectEqualStrings(".......πŸ˜€abcdeπŸ˜€......", unwrapped); + } + + t.setCursorPos(2, 2); + t.eraseChars(3); + t.screen.cursor.page_pin.page.data.assertIntegrity(); + + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings(".......\n cde\nπŸ˜€......", str); + + const unwrapped = try t.plainStringUnwrapped(alloc); + defer testing.allocator.free(unwrapped); + try testing.expectEqualStrings(".......\n cde\nπŸ˜€......", unwrapped); + } +} + test "Terminal: reverseIndex" { const alloc = testing.allocator; var t = try init(alloc, .{ .cols = 2, .rows = 5 }); From 8d12044f1d7bd7d4337b8a55cbfe401619b4d205 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Thu, 5 Sep 2024 21:14:53 -0400 Subject: [PATCH 3/7] Terminal: fix ECH & DCH wide char boundary cond. behavior --- src/terminal/PageList.zig | 28 +++++++++ src/terminal/Screen.zig | 123 ++++++++++++++++++++++++++++++++++++++ src/terminal/Terminal.zig | 85 +++++++++++--------------- 3 files changed, 186 insertions(+), 50 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 705d6ba2d..9967d27fd 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3172,6 +3172,34 @@ pub const Pin = struct { set.set(self.y); } + /// Resets the soft-wrap state of the row this pin is on, appropriately + /// handling the wrap and wrap_continuation flags for the previous and + /// next row as well. + /// + /// DOES NOT handle clearing spacer heads. + /// Use `Screen.splitCellBoundary` for that. + /// + /// TODO: test + pub fn resetWrap(self: Pin) void { + const rac = self.rowAndCell(); + + // This row does not wrap + rac.row.wrap = false; + + // This row is not wrapped to + rac.row.wrap_continuation = false; + + // The previous row does not wrap to this row + if (self.up(1)) |prev_row| { + prev_row.rowAndCell().row.wrap = false; + } + + // The next row is not wrapped to + if (self.down(1)) |next_row| { + next_row.rowAndCell().row.wrap_continuation = false; + } + } + /// Returns true if the row of this pin should never have its background /// color extended for filling padding space in the renderer. This is /// a set of heuristics that help making our padding look better. diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 8a9127dd6..ac95d07c2 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1041,6 +1041,17 @@ pub fn cursorMarkDirty(self: *Screen) void { self.cursor.page_pin.markDirty(); } +/// Reset the cursor row's soft-wrap state and the cursor's pending wrap. +/// Also clears spacer heads from the cursor row and prior row as necessary. +pub fn cursorResetWrap(self: *Screen) void { + // Handle boundary conditions on left and right of the row. + self.splitCellBoundary(self.cursor.page_pin.*, 0); + self.splitCellBoundary(self.cursor.page_pin.*, self.cursor.page_pin.page.data.size.cols); + + self.cursor.page_pin.resetWrap(); + self.cursor.pending_wrap = false; +} + /// Options for scrolling the viewport of the terminal grid. The reason /// we have this in addition to PageList.Scroll is because we have additional /// scroll behaviors that are not part of the PageList.Scroll enum. @@ -1282,6 +1293,118 @@ pub fn clearPrompt(self: *Screen) void { } } +/// Clean up boundary conditions where a cell will become discontiguous with +/// a neighboring cell because either one of them will be moved and/or cleared. +/// +/// Handles the boundary between the cell at `x` and the cell at `x - 1`. +/// +/// So, for example, when moving a region of cells [a, b] (inclusive), call this +/// function with `x = a` and `x = b + 1`. It is okay if `x` is out of bounds by +/// 1, this will be interpreted correctly. +/// +/// DOES NOT MODIFY ROW WRAP STATE! See `resetWrap` for that. +/// +/// The following boundary conditions are handled: +/// +/// - `x - 1` is a wide character and `x` is a spacer tail: +/// o Both cells will be cleared. +/// o If `x - 1` is the start of the row and was wrapped from a previous row +/// then the previous row is checked for a spacer head, which is cleared if +/// present. +/// +/// - `x == 0` and is a wide character: +/// o If the row is a wrap continuation then the previous row will be checked +/// for a spacer head, which is cleared if present. +/// +/// - `x == cols` and `x - 1` is a spacer head: +/// o `x - 1` will be cleared. +pub fn splitCellBoundary( + self: *Screen, + row: Pin, + x: size.CellCountInt, +) void { + row.page.data.pauseIntegrityChecks(true); + defer row.page.data.pauseIntegrityChecks(false); + + const rac = row.rowAndCell(); + const cells = row.cells(.all); + const cols = row.page.data.size.cols; + + // `x` may be up to an INCLUDING `cols`, since that signifies splitting + // the boundary to the right of the final cell in the row. + assert(x <= cols); + + // [ A B C D E F|] + // ^ Boundary between final cell and row end. + if (x == cols) { + // Spacer head at end of wrapped row. + if (cells[cols - 1].wide == .spacer_head) { + self.clearCells( + &row.page.data, + rac.row, + cells[cols - 1 ..][0..1], + ); + } + + return; + } + + // [|A B C D E F ] + // ^ Boundary between first cell and row start. + // + // OR + // + // [ A|B C D E F ] + // ^ Boundary between first cell and second cell. + // + // First cell may be a wrapped wide cell with a spacer + // head on the previous row that needs to be cleared. + if (x == 0 or x == 1) { + // If the first cell in a row is wide the previous row + // may have a spacer head which needs to be cleared. + if (cells[0].wide == .wide and rac.row.wrap_continuation) { + if (row.up(1)) |p_row| { + const p_rac = p_row.rowAndCell(); + const p_cells = p_row.cells(.all); + const p_cell = p_cells[p_row.page.data.size.cols - 1]; + if (p_cell.wide == .spacer_head) { + self.clearCells( + &p_row.page.data, + p_rac.row, + p_cells[p_row.page.data.size.cols - 1 ..][0..1], + ); + } + } + } + + // If x is 0 then we're done. + if (x == 0) return; + } + + // [ ... X|Y ... ] + // ^ Boundary between two cells in the middle of the row. + assert(x > 0); + assert(x < cols); + + const left = cells[x - 1]; + switch (left.wide) { + // There should not be spacer heads in the middle of the row. + .spacer_head => unreachable, + + // We don't need to do anything for narrow cells or spacer tails. + .narrow, .spacer_tail => {}, + + // A wide char would be split, so must be cleared. + .wide => { + self.clearCells( + &row.page.data, + rac.row, + cells[x - 1 ..][0..2], + ); + }, + } +} + /// Returns the blank cell to use when doing terminal operations that /// require preserving the bg color. pub fn blankCell(self: *const Screen) Cell { diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index a7a277921..f371f51bb 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1907,29 +1907,29 @@ pub fn deleteChars(self: *Terminal, count_req: usize) void { if (self.screen.cursor.x < self.scrolling_region.left or self.screen.cursor.x > self.scrolling_region.right) return; - // This resets the soft-wrap of this line - self.screen.cursor.page_row.wrap = false; - - // This resets the pending wrap state - self.screen.cursor.pending_wrap = false; - // left is just the cursor position but as a multi-pointer const left: [*]Cell = @ptrCast(self.screen.cursor.page_cell); var page = &self.screen.cursor.page_pin.page.data; - // If our X is a wide spacer tail then we need to erase the - // previous cell too so we don't split a multi-cell character. - if (self.screen.cursor.page_cell.wide == .spacer_tail) { - assert(self.screen.cursor.x > 0); - self.screen.clearCells(page, self.screen.cursor.page_row, (left - 1)[0..2]); - } - // Remaining cols from our cursor to the right margin. const rem = self.scrolling_region.right - self.screen.cursor.x + 1; // We can only insert blanks up to our remaining cols const count = @min(count_req, rem); + self.screen.splitCellBoundary( + self.screen.cursor.page_pin.*, + self.screen.cursor.x, + ); + self.screen.splitCellBoundary( + self.screen.cursor.page_pin.*, + self.screen.cursor.x + count, + ); + self.screen.splitCellBoundary( + self.screen.cursor.page_pin.*, + self.scrolling_region.right + 1, + ); + // This is the amount of space at the right of the scroll region // that will NOT be blank, so we need to shift the correct cols right. // "scroll_amount" is the number of such cols. @@ -1941,35 +1941,6 @@ pub fn deleteChars(self: *Terminal, count_req: usize) void { const right: [*]Cell = left + (scroll_amount - 1); - const end: *Cell = @ptrCast(right + count); - switch (end.wide) { - .narrow, .wide => {}, - - // If our end is a spacer head then we need to clear it since - // spacer heads must be at the end. - .spacer_head => { - self.screen.clearCells(page, self.screen.cursor.page_row, end[0..1]); - }, - - // If our last cell we're shifting is wide, then we need to clear - // it to be empty so we don't split the multi-cell char. - .spacer_tail => { - const wide: [*]Cell = right + count - 1; - assert(wide[0].wide == .wide); - self.screen.clearCells(page, self.screen.cursor.page_row, wide[0..2]); - }, - } - - // If our first cell is a wide char then we need to also clear - // the spacer tail following it. - if (x[0].wide == .wide) { - self.screen.clearCells( - page, - self.screen.cursor.page_row, - x[0..2], - ); - } - while (@intFromPtr(x) <= @intFromPtr(right)) : (x += 1) { const src: *Cell = @ptrCast(x + count); const dst: *Cell = @ptrCast(x); @@ -1980,6 +1951,9 @@ pub fn deleteChars(self: *Terminal, count_req: usize) void { // Insert blanks. The blanks preserve the background color. self.screen.clearCells(page, self.screen.cursor.page_row, x[0 .. rem - scroll_amount]); + // Our row's soft-wrap is always reset. + self.screen.cursorResetWrap(); + // Our row is always dirty self.screen.cursorMarkDirty(); } @@ -1987,12 +1961,6 @@ pub fn deleteChars(self: *Terminal, count_req: usize) void { pub fn eraseChars(self: *Terminal, count_req: usize) void { const count = @max(count_req, 1); - // This resets the soft-wrap of this line - self.screen.cursor.page_row.wrap = false; - - // This resets the pending wrap state - self.screen.cursor.pending_wrap = false; - // Our last index is at most the end of the number of chars we have // in the current line. const end = end: { @@ -2009,6 +1977,23 @@ pub fn eraseChars(self: *Terminal, count_req: usize) void { break :end end; }; + // Handle any boundary conditions on the edges of the erased area. + // + // TODO(qwerasd): This isn't actually correct if you take in to account + // protected modes. We need to figure out how to make `clearCells` or at + // least `clearUnprotectedCells` handle boundary conditions... + self.screen.splitCellBoundary( + self.screen.cursor.page_pin.*, + self.screen.cursor.x, + ); + self.screen.splitCellBoundary( + self.screen.cursor.page_pin.*, + end, + ); + + // Reset our row's soft-wrap. + self.screen.cursorResetWrap(); + // Mark our cursor row as dirty self.screen.cursorMarkDirty(); @@ -2051,8 +2036,8 @@ pub fn eraseLine( x -= 1; } - // This resets the soft-wrap of this line - self.screen.cursor.page_row.wrap = false; + // Reset our row's soft-wrap. + self.screen.cursorResetWrap(); break :right .{ x, self.cols }; }, From 057f218c9e0fc6a67f117ee07657d5a070291f0c Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 6 Sep 2024 13:26:57 -0400 Subject: [PATCH 4/7] perf(terminal): specialize `splitCellBoundary` to cursor row + do some abstraction leakage in `cursorResetWrap`, since they're both used in hot functions for TUI stuff so performance is important. --- src/terminal/PageList.zig | 28 --------- src/terminal/Screen.zig | 126 +++++++++++++++++++++++++++----------- src/terminal/Terminal.zig | 25 ++------ 3 files changed, 96 insertions(+), 83 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 9967d27fd..705d6ba2d 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3172,34 +3172,6 @@ pub const Pin = struct { set.set(self.y); } - /// Resets the soft-wrap state of the row this pin is on, appropriately - /// handling the wrap and wrap_continuation flags for the previous and - /// next row as well. - /// - /// DOES NOT handle clearing spacer heads. - /// Use `Screen.splitCellBoundary` for that. - /// - /// TODO: test - pub fn resetWrap(self: Pin) void { - const rac = self.rowAndCell(); - - // This row does not wrap - rac.row.wrap = false; - - // This row is not wrapped to - rac.row.wrap_continuation = false; - - // The previous row does not wrap to this row - if (self.up(1)) |prev_row| { - prev_row.rowAndCell().row.wrap = false; - } - - // The next row is not wrapped to - if (self.down(1)) |next_row| { - next_row.rowAndCell().row.wrap_continuation = false; - } - } - /// Returns true if the row of this pin should never have its background /// color extended for filling padding space in the renderer. This is /// a set of heuristics that help making our padding look better. diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index ac95d07c2..1df2de713 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1044,12 +1044,57 @@ pub fn cursorMarkDirty(self: *Screen) void { /// Reset the cursor row's soft-wrap state and the cursor's pending wrap. /// Also clears spacer heads from the cursor row and prior row as necessary. pub fn cursorResetWrap(self: *Screen) void { - // Handle boundary conditions on left and right of the row. - self.splitCellBoundary(self.cursor.page_pin.*, 0); - self.splitCellBoundary(self.cursor.page_pin.*, self.cursor.page_pin.page.data.size.cols); - - self.cursor.page_pin.resetWrap(); + // Reset the cursor's pending wrap state self.cursor.pending_wrap = false; + + const page_row = self.cursor.page_row; + + if (!(page_row.wrap or page_row.wrap_continuation)) return; + + const cells = self.cursor.page_pin.cells(.all); + + // The previous row does not wrap and this row is not wrapped to + if (page_row.wrap_continuation) { + page_row.wrap_continuation = false; + + if (self.cursor.page_pin.up(1)) |prev_row| { + const p_rac = prev_row.rowAndCell(); + p_rac.row.wrap = false; + + // If the first cell in a row is wide the previous row + // may have a spacer head which needs to be cleared. + if (cells[0].wide == .wide) { + const p_cells = prev_row.cells(.all); + const p_cell = p_cells[prev_row.page.data.size.cols - 1]; + if (p_cell.wide == .spacer_head) { + self.clearCells( + &prev_row.page.data, + p_rac.row, + p_cells[prev_row.page.data.size.cols - 1 ..][0..1], + ); + } + } + } + } + + // This row does not wrap and the next row is not wrapped to + if (page_row.wrap) { + page_row.wrap = false; + + if (self.cursor.page_pin.down(1)) |next_row| { + next_row.rowAndCell().row.wrap_continuation = false; + } + + // If the last cell in the row is a spacer head we need to clear it. + const cell = cells[self.cursor.page_pin.page.data.size.cols - 1]; + if (cell.wide == .spacer_head) { + self.clearCells( + &self.cursor.page_pin.page.data, + page_row, + cells[self.cursor.page_pin.page.data.size.cols - 1 ..][0..1], + ); + } + } } /// Options for scrolling the viewport of the terminal grid. The reason @@ -1296,6 +1341,8 @@ pub fn clearPrompt(self: *Screen) void { /// Clean up boundary conditions where a cell will become discontiguous with /// a neighboring cell because either one of them will be moved and/or cleared. /// +/// For performance reasons this is specialized to operate on the cursor row. +/// /// Handles the boundary between the cell at `x` and the cell at `x - 1`. /// /// So, for example, when moving a region of cells [a, b] (inclusive), call this @@ -1320,15 +1367,14 @@ pub fn clearPrompt(self: *Screen) void { /// o `x - 1` will be cleared. pub fn splitCellBoundary( self: *Screen, - row: Pin, x: size.CellCountInt, ) void { - row.page.data.pauseIntegrityChecks(true); - defer row.page.data.pauseIntegrityChecks(false); + const page = &self.cursor.page_pin.page.data; - const rac = row.rowAndCell(); - const cells = row.cells(.all); - const cols = row.page.data.size.cols; + page.pauseIntegrityChecks(true); + defer page.pauseIntegrityChecks(false); + + const cols = self.cursor.page_pin.page.data.size.cols; // `x` may be up to an INCLUDING `cols`, since that signifies splitting // the boundary to the right of the final cell in the row. @@ -1337,11 +1383,15 @@ pub fn splitCellBoundary( // [ A B C D E F|] // ^ Boundary between final cell and row end. if (x == cols) { + if (!self.cursor.page_row.wrap) return; + + const cells = self.cursor.page_pin.cells(.all); + // Spacer head at end of wrapped row. if (cells[cols - 1].wide == .spacer_head) { self.clearCells( - &row.page.data, - rac.row, + page, + self.cursor.page_row, cells[cols - 1 ..][0..1], ); } @@ -1359,11 +1409,13 @@ pub fn splitCellBoundary( // // First cell may be a wrapped wide cell with a spacer // head on the previous row that needs to be cleared. - if (x == 0 or x == 1) { + if ((x == 0 or x == 1) and self.cursor.page_row.wrap_continuation) { + const cells = self.cursor.page_pin.cells(.all); + // If the first cell in a row is wide the previous row // may have a spacer head which needs to be cleared. - if (cells[0].wide == .wide and rac.row.wrap_continuation) { - if (row.up(1)) |p_row| { + if (cells[0].wide == .wide) { + if (self.cursor.page_pin.up(1)) |p_row| { const p_rac = p_row.rowAndCell(); const p_cells = p_row.cells(.all); const p_cell = p_cells[p_row.page.data.size.cols - 1]; @@ -1376,32 +1428,36 @@ pub fn splitCellBoundary( } } } - - // If x is 0 then we're done. - if (x == 0) return; } + // If x is 0 then we're done. + if (x == 0) return; + // [ ... X|Y ... ] // ^ Boundary between two cells in the middle of the row. - assert(x > 0); - assert(x < cols); + { + assert(x > 0); + assert(x < cols); - const left = cells[x - 1]; - switch (left.wide) { - // There should not be spacer heads in the middle of the row. - .spacer_head => unreachable, + const cells = self.cursor.page_pin.cells(.all); - // We don't need to do anything for narrow cells or spacer tails. - .narrow, .spacer_tail => {}, + const left = cells[x - 1]; + switch (left.wide) { + // There should not be spacer heads in the middle of the row. + .spacer_head => unreachable, - // A wide char would be split, so must be cleared. - .wide => { - self.clearCells( - &row.page.data, - rac.row, - cells[x - 1 ..][0..2], - ); - }, + // We don't need to do anything for narrow cells or spacer tails. + .narrow, .spacer_tail => {}, + + // A wide char would be split, so must be cleared. + .wide => { + self.clearCells( + page, + self.cursor.page_row, + cells[x - 1 ..][0..2], + ); + }, + } } } diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index f371f51bb..9f7b22103 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1917,18 +1917,9 @@ pub fn deleteChars(self: *Terminal, count_req: usize) void { // We can only insert blanks up to our remaining cols const count = @min(count_req, rem); - self.screen.splitCellBoundary( - self.screen.cursor.page_pin.*, - self.screen.cursor.x, - ); - self.screen.splitCellBoundary( - self.screen.cursor.page_pin.*, - self.screen.cursor.x + count, - ); - self.screen.splitCellBoundary( - self.screen.cursor.page_pin.*, - self.scrolling_region.right + 1, - ); + self.screen.splitCellBoundary(self.screen.cursor.x); + self.screen.splitCellBoundary(self.screen.cursor.x + count); + self.screen.splitCellBoundary(self.scrolling_region.right + 1); // This is the amount of space at the right of the scroll region // that will NOT be blank, so we need to shift the correct cols right. @@ -1982,14 +1973,8 @@ pub fn eraseChars(self: *Terminal, count_req: usize) void { // TODO(qwerasd): This isn't actually correct if you take in to account // protected modes. We need to figure out how to make `clearCells` or at // least `clearUnprotectedCells` handle boundary conditions... - self.screen.splitCellBoundary( - self.screen.cursor.page_pin.*, - self.screen.cursor.x, - ); - self.screen.splitCellBoundary( - self.screen.cursor.page_pin.*, - end, - ); + self.screen.splitCellBoundary(self.screen.cursor.x); + self.screen.splitCellBoundary(end); // Reset our row's soft-wrap. self.screen.cursorResetWrap(); From 8f47581e22d4ac741da0a0509b9a65c96a633c4c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 6 Sep 2024 14:27:55 -0700 Subject: [PATCH 5/7] terminal: add test for wide character on right margin boundary --- src/terminal/Terminal.zig | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 9f7b22103..f51e54a76 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -9053,6 +9053,49 @@ test "Terminal: deleteChars wide char wrap boundary conditions" { } } +test "Terminal: deleteChars wide char across right margin" { + const alloc = testing.allocator; + var t = try init(alloc, .{ .rows = 3, .cols = 8 }); + defer t.deinit(alloc); + + // scroll region + // VVVVVV + // +-######-+ + // |.abcdeWW| + // : ^ : (^ = cursor) + // +--------+ + // + // DCH 1 + + try t.printString("123456ζ©‹"); + t.modes.set(.enable_left_and_right_margin, true); + t.setLeftAndRightMargin(2, 7); + + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings("123456ζ©‹", str); + } + + t.setCursorPos(1, 2); + t.deleteChars(1); + t.screen.cursor.page_pin.page.data.assertIntegrity(); + + // NOTE: This behavior is slightly inconsistent with xterm. xterm + // _visually_ splits the wide character (half the wide character shows + // up in col 6 and half in col 8). In all other wide char split scenarios, + // xterm clears the cell. Therefore, we've chosen to clear the cell here. + // Given we have space, we also could actually preserve it, but I haven't + // yet found a terminal that behaves that way. We should be open to + // revisiting this behavior but for now we're going with the simpler + // impl. + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings("13456", str); + } +} + test "Terminal: saveCursor" { const alloc = testing.allocator; var t = try init(alloc, .{ .cols = 3, .rows = 3 }); From 9669332134bee01d0f88e1610175b9fcaa68820e Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 6 Sep 2024 17:47:29 -0400 Subject: [PATCH 6/7] terminal: cursorResetWrap should not reset wrap_continuation --- src/terminal/Screen.zig | 58 +++++++++++---------------------------- src/terminal/Terminal.zig | 4 +-- 2 files changed, 18 insertions(+), 44 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 1df2de713..cb6c36017 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1042,58 +1042,32 @@ pub fn cursorMarkDirty(self: *Screen) void { } /// Reset the cursor row's soft-wrap state and the cursor's pending wrap. -/// Also clears spacer heads from the cursor row and prior row as necessary. +/// Also handles clearing the spacer head on the cursor row and resetting +/// the wrap_continuation flag on the next row if necessary. pub fn cursorResetWrap(self: *Screen) void { // Reset the cursor's pending wrap state self.cursor.pending_wrap = false; const page_row = self.cursor.page_row; - if (!(page_row.wrap or page_row.wrap_continuation)) return; - - const cells = self.cursor.page_pin.cells(.all); - - // The previous row does not wrap and this row is not wrapped to - if (page_row.wrap_continuation) { - page_row.wrap_continuation = false; - - if (self.cursor.page_pin.up(1)) |prev_row| { - const p_rac = prev_row.rowAndCell(); - p_rac.row.wrap = false; - - // If the first cell in a row is wide the previous row - // may have a spacer head which needs to be cleared. - if (cells[0].wide == .wide) { - const p_cells = prev_row.cells(.all); - const p_cell = p_cells[prev_row.page.data.size.cols - 1]; - if (p_cell.wide == .spacer_head) { - self.clearCells( - &prev_row.page.data, - p_rac.row, - p_cells[prev_row.page.data.size.cols - 1 ..][0..1], - ); - } - } - } - } + if (!page_row.wrap) return; // This row does not wrap and the next row is not wrapped to - if (page_row.wrap) { - page_row.wrap = false; + page_row.wrap = false; - if (self.cursor.page_pin.down(1)) |next_row| { - next_row.rowAndCell().row.wrap_continuation = false; - } + if (self.cursor.page_pin.down(1)) |next_row| { + next_row.rowAndCell().row.wrap_continuation = false; + } - // If the last cell in the row is a spacer head we need to clear it. - const cell = cells[self.cursor.page_pin.page.data.size.cols - 1]; - if (cell.wide == .spacer_head) { - self.clearCells( - &self.cursor.page_pin.page.data, - page_row, - cells[self.cursor.page_pin.page.data.size.cols - 1 ..][0..1], - ); - } + // If the last cell in the row is a spacer head we need to clear it. + const cells = self.cursor.page_pin.cells(.all); + const cell = cells[self.cursor.page_pin.page.data.size.cols - 1]; + if (cell.wide == .spacer_head) { + self.clearCells( + &self.cursor.page_pin.page.data, + page_row, + cells[self.cursor.page_pin.page.data.size.cols - 1 ..][0..1], + ); } } diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index f51e54a76..b3e19b708 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -6083,7 +6083,7 @@ test "Terminal: eraseChars wide char wrap boundary conditions" { const unwrapped = try t.plainStringUnwrapped(alloc); defer testing.allocator.free(unwrapped); - try testing.expectEqualStrings(".......\n cde\nπŸ˜€......", unwrapped); + try testing.expectEqualStrings("....... cde\nπŸ˜€......", unwrapped); } } @@ -9049,7 +9049,7 @@ test "Terminal: deleteChars wide char wrap boundary conditions" { const unwrapped = try t.plainStringUnwrapped(alloc); defer testing.allocator.free(unwrapped); - try testing.expectEqualStrings(".......\n cde\nπŸ˜€......", unwrapped); + try testing.expectEqualStrings("....... cde\nπŸ˜€......", unwrapped); } } From 5138801b7b6fe3f8dea969710ed2ed7f6684fa25 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 6 Sep 2024 18:03:56 -0400 Subject: [PATCH 7/7] comment --- src/terminal/Screen.zig | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index cb6c36017..7927d0343 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1044,6 +1044,11 @@ pub fn cursorMarkDirty(self: *Screen) void { /// Reset the cursor row's soft-wrap state and the cursor's pending wrap. /// Also handles clearing the spacer head on the cursor row and resetting /// the wrap_continuation flag on the next row if necessary. +/// +/// NOTE(qwerasd): This method is not scrolling region aware, and cannot be +/// since it's on Screen not Terminal. This needs to be addressed down the +/// line. Not an extremely urgent issue since it's an edge case of an edge +/// case, but not ideal. pub fn cursorResetWrap(self: *Screen) void { // Reset the cursor's pending wrap state self.cursor.pending_wrap = false; @@ -1323,7 +1328,7 @@ pub fn clearPrompt(self: *Screen) void { /// function with `x = a` and `x = b + 1`. It is okay if `x` is out of bounds by /// 1, this will be interpreted correctly. /// -/// DOES NOT MODIFY ROW WRAP STATE! See `resetWrap` for that. +/// DOES NOT MODIFY ROW WRAP STATE! See `cursorResetWrap` for that. /// /// The following boundary conditions are handled: /// @@ -1339,6 +1344,11 @@ pub fn clearPrompt(self: *Screen) void { /// /// - `x == cols` and `x - 1` is a spacer head: /// o `x - 1` will be cleared. +/// +/// NOTE(qwerasd): This method is not scrolling region aware, and cannot be +/// since it's on Screen not Terminal. This needs to be addressed down the +/// line. Not an extremely urgent issue since it's an edge case of an edge +/// case, but not ideal. pub fn splitCellBoundary( self: *Screen, x: size.CellCountInt,