From ec0cb7ce1b65ed9d84f1fa8fe8de16686591f050 Mon Sep 17 00:00:00 2001 From: Mikko Perttunen Date: Thu, 23 Feb 2023 17:30:59 +0200 Subject: [PATCH] termwiz: surface: Fix cell diffing in presence of wide cells The present code for creating diffs between surface regions processes visible cells from both surfaces in lockstep. This only works when the cells at each position have the same width in each surface. When widths differ, the iterators go out of sync and end up comparing cells in different positions of the surface, resulting in incorrect diffs. Fix this by instead iterating over the source surface ('other'), and for each cell locating the corresponding cell in the target surface to compare against. To apply the fix to all three diffing functions, outline the per-line code to a new function and call it in each function. Also add a test that verifies the behavior in such case with differing cell widths. --- termwiz/src/surface/line/cellref.rs | 2 +- termwiz/src/surface/mod.rs | 108 ++++++++++++++++++++++++---- 2 files changed, 95 insertions(+), 15 deletions(-) diff --git a/termwiz/src/surface/line/cellref.rs b/termwiz/src/surface/line/cellref.rs index 10355146b..d6e86b155 100644 --- a/termwiz/src/surface/line/cellref.rs +++ b/termwiz/src/surface/line/cellref.rs @@ -2,7 +2,7 @@ use crate::cell::{Cell, CellAttributes}; use crate::emoji::Presentation; use std::hash::{Hash, Hasher}; -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] pub enum CellRef<'a> { CellRef { cell_index: usize, diff --git a/termwiz/src/surface/mod.rs b/termwiz/src/surface/mod.rs index 430ca772d..f05d2f415 100644 --- a/termwiz/src/surface/mod.rs +++ b/termwiz/src/surface/mod.rs @@ -137,6 +137,12 @@ impl DiffState { if cell.same_contents(&other_cell) { return; } + + self.set_cell(col_num, row_num, other_cell); + } + + #[inline] + fn set_cell(&mut self, col_num: usize, row_num: usize, other_cell: CellRef) { self.cursor = match self.cursor.take() { Some((cursor_row, cursor_col)) if cursor_row == row_num && cursor_col == col_num - 1 => @@ -747,14 +753,15 @@ impl Surface { .take_while(|(row_num, _)| *row_num < y + height) .zip(other.lines.iter().skip(other_y)) { - for (cell, other_cell) in line - .visible_cells() - .skip(x) - .take_while(|cell| cell.cell_index() < x + width) - .zip(other_line.visible_cells().skip(other_x)) - { - diff_state.diff_cells(cell.cell_index(), row_num, cell, other_cell); - } + diff_line( + &mut diff_state, + line, + row_num, + other_line, + x, + width, + other_x, + ); } diff_state.changes @@ -763,9 +770,7 @@ impl Surface { pub fn diff_lines(&self, other_lines: Vec<&Line>) -> Vec { let mut diff_state = DiffState::default(); for ((row_num, line), other_line) in self.lines.iter().enumerate().zip(other_lines.iter()) { - for (cell, other_cell) in line.visible_cells().zip(other_line.visible_cells()) { - diff_state.diff_cells(cell.cell_index(), row_num, cell, other_cell); - } + diff_line(&mut diff_state, line, row_num, other_line, 0, line.len(), 0); } diff_state.changes } @@ -773,9 +778,7 @@ impl Surface { pub fn diff_against_numbered_line(&self, row_num: usize, other_line: &Line) -> Vec { let mut diff_state = DiffState::default(); if let Some(line) = self.lines.get(row_num) { - for (cell, other_cell) in line.visible_cells().zip(other_line.visible_cells()) { - diff_state.diff_cells(cell.cell_index(), row_num, cell, other_cell); - } + diff_line(&mut diff_state, line, row_num, other_line, 0, line.len(), 0); } diff_state.changes } @@ -825,6 +828,58 @@ impl Surface { } } +/// Populate `diff_state` with changes to replace contents of `line` in range [x,x+width) +/// with the contents of `other_line` in range [other_x,other_x+width). +fn diff_line( + diff_state: &mut DiffState, + line: &Line, + row_num: usize, + other_line: &Line, + x: usize, + width: usize, + other_x: usize, +) { + let mut cells = line + .visible_cells() + .skip_while(|cell| cell.cell_index() < x) + .take_while(|cell| cell.cell_index() < x + width) + .peekable(); + let other_cells = other_line + .visible_cells() + .skip_while(|cell| cell.cell_index() < other_x) + .take_while(|cell| cell.cell_index() < other_x + width); + + for other_cell in other_cells { + let rel_x = other_cell.cell_index() - other_x; + let mut comparison_cell = None; + + // Advance the `cells` iterator to try to find the visible cell in `line` in the equivalent + // position to `other_cell`. If there is no visible cell in equivalent position, advance + // one past and wait for next iteration. + while let Some(cell) = cells.peek() { + let cell_rel_x = cell.cell_index() - x; + + if cell_rel_x == rel_x { + comparison_cell = Some(*cell); + break; + } else if cell_rel_x > rel_x { + break; + } + + cells.next(); + } + + // If we find a cell in the equivalent position, diff against it. If not, we know + // there is a multi-cell grapheme in `line` that partially overlaps `other_cell`, + // so we have to overwrite anyway. + if let Some(comparison_cell) = comparison_cell { + diff_state.diff_cells(x + rel_x, row_num, comparison_cell, other_cell); + } else { + diff_state.set_cell(x + rel_x, row_num, other_cell); + } + } +} + /// Applies a Position update to either the x or y position. /// The value is clamped to be in the range: 0..limit fn compute_position_change(current: usize, pos: &Position, limit: usize) -> usize { @@ -1532,6 +1587,31 @@ mod test { assert_eq!(s.screen_chars_to_string(), " ax \n"); } + #[test] + fn draw_double_width() { + let mut s = Surface::new(4, 1); + s.add_change("か a"); + assert_eq!(s.screen_chars_to_string(), "か a\n"); + + let mut s2 = Surface::new(4, 1); + s2.draw_from_screen(&s, 0, 0); + // Verify no issue when the second visible cells on both sides + // are identical (' 's) but they are at different cell indices. + assert_eq!(s2.screen_chars_to_string(), "か a\n"); + + let s3 = Surface::new(4, 1); + s2.draw_from_screen(&s3, 0, 0); + // Verify same but in other direction + assert_eq!(s2.screen_chars_to_string(), " \n"); + + let mut s4 = Surface::new(4, 1); + s4.add_change("abcd"); + s.draw_from_screen(&s4, 0, 0); + // Verify that all overlapping cells are updated when cell widths + // differ on each side. + assert_eq!(s.screen_chars_to_string(), "abcd\n"); + } + #[test] fn zero_width() { let mut s = Surface::new(4, 1);