From 971b744c9c7c2c3dc9f055f69c5630ca11f0a09e Mon Sep 17 00:00:00 2001 From: Jesse Luehrs Date: Thu, 28 Nov 2019 14:57:07 -0500 Subject: fix a couple more issues with end of line behavior --- CHANGELOG.md | 6 +++ src/grid.rs | 104 ++++++++++++++++++++++++++++++++++++++++++++--- src/screen.rs | 46 ++++++++++++++++----- src/term.rs | 10 +++++ tests/control.rs | 15 +++++++ tests/text.rs | 11 +++++ tests/window_contents.rs | 51 ++++++++++++++++++++--- 7 files changed, 221 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09ead23..ea84420 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixed + +* Fix a couple more end-of-line/wrapping bugs. + ## [0.7.0] - 2019-11-23 ### Added diff --git a/src/grid.rs b/src/grid.rs index 8076a6c..9619443 100644 --- a/src/grid.rs +++ b/src/grid.rs @@ -228,9 +228,55 @@ impl Grid { col: self.size.cols - 1, }; } - crate::term::MoveFromTo::new(prev_pos, pos).write_buf(contents); let cell = self.visible_cell(pos).unwrap(); - contents.extend(cell.contents().as_bytes()); + if cell.has_contents() { + crate::term::MoveFromTo::new(prev_pos, pos) + .write_buf(contents); + contents.extend(cell.contents().as_bytes()); + } else { + // if the cell doesn't have contents, we can't have gotten + // here by drawing a character in the last column. this means + // that as far as i'm aware, we have to have reached here from + // a newline when we were already after the end of an earlier + // row. in the case where we are already after the end of an + // earlier row, we can just write a few newlines, otherwise we + // also need to do the same as above to get ourselves to after + // the end of a row. + let orig_row = pos.row; + let mut found = false; + for i in (0..orig_row).rev() { + pos.row = i; + let cell = self.visible_cell(pos).unwrap(); + if cell.has_contents() { + if prev_pos.row != i || prev_pos.col < self.size.cols + { + crate::term::MoveFromTo::new(prev_pos, pos) + .write_buf(contents); + contents.extend(cell.contents().as_bytes()); + } + contents.extend( + "\n".repeat((orig_row - i) as usize).as_bytes(), + ); + found = true; + break; + } + } + + // this can happen if you get the cursor off the end of a row, + // and then do something to clear the current row without + // moving the cursor (IL, ED, EL, etc). i can't see any way + // for this to happen without the entire current row being + // cleared (not just the last cell), so this seems + // prooooobably fine? + if !found { + pos.row = orig_row; + crate::term::MoveFromTo::new(prev_pos, pos) + .write_buf(contents); + contents.push(b' '); + crate::term::ClearRowBackward::default() + .write_buf(contents); + } + } } else { crate::term::MoveFromTo::new(prev_pos, self.pos) .write_buf(contents); @@ -282,9 +328,55 @@ impl Grid { col: self.size.cols - 1, }; } - crate::term::MoveFromTo::new(prev_pos, pos).write_buf(contents); let cell = self.visible_cell(pos).unwrap(); - contents.extend(cell.contents().as_bytes()); + if cell.has_contents() { + crate::term::MoveFromTo::new(prev_pos, pos) + .write_buf(contents); + contents.extend(cell.contents().as_bytes()); + } else { + // if the cell doesn't have contents, we can't have gotten + // here by drawing a character in the last column. this means + // that as far as i'm aware, we have to have reached here from + // a newline when we were already after the end of an earlier + // row. in the case where we are already after the end of an + // earlier row, we can just write a few newlines, otherwise we + // also need to do the same as above to get ourselves to after + // the end of a row. + let orig_row = pos.row; + let mut found = false; + for i in (0..orig_row).rev() { + pos.row = i; + let cell = self.visible_cell(pos).unwrap(); + if cell.has_contents() { + if prev_pos.row != i || prev_pos.col < self.size.cols + { + crate::term::MoveFromTo::new(prev_pos, pos) + .write_buf(contents); + contents.extend(cell.contents().as_bytes()); + } + contents.extend( + "\n".repeat((orig_row - i) as usize).as_bytes(), + ); + found = true; + break; + } + } + + // this can happen if you get the cursor off the end of a row, + // and then do something to clear the current row without + // moving the cursor (IL, ED, EL, etc). i can't see any way + // for this to happen without the entire current row being + // cleared (not just the last cell), so this seems + // prooooobably fine? + if !found { + pos.row = orig_row; + crate::term::MoveFromTo::new(prev_pos, pos) + .write_buf(contents); + contents.push(b' '); + crate::term::ClearRowBackward::default() + .write_buf(contents); + } + } } else { crate::term::MoveFromTo::new(prev_pos, self.pos) .write_buf(contents); @@ -495,9 +587,9 @@ impl Grid { self.col_clamp(); } - pub fn col_wrap(&mut self, width: u16) { + pub fn col_wrap(&mut self, width: u16, wrap: bool) { if self.pos.col > self.size.cols - width { - self.current_row_mut().wrap(true); + self.current_row_mut().wrap(wrap); self.pos.col = 0; self.row_inc_scroll(1); } diff --git a/src/screen.rs b/src/screen.rs index ec4d204..07363b1 100644 --- a/src/screen.rs +++ b/src/screen.rs @@ -588,19 +588,45 @@ impl Screen { } } + let mut wrap = true; + // it doesn't make any sense to wrap if the last column in a row + // didn't already have contents + if pos.col > 1 { + let mut prev_cell = self + .drawing_cell_mut(crate::grid::Pos { + row: pos.row, + col: pos.col - 2, + }) + .unwrap(); + if !prev_cell.is_wide() { + prev_cell = self + .drawing_cell_mut(crate::grid::Pos { + row: pos.row, + col: pos.col - 1, + }) + .unwrap(); + } + if !prev_cell.has_contents() { + wrap = false; + } + } + let width = c.width().unwrap_or(0).try_into().unwrap(); let attrs = self.attrs; - // zero width characters still cause the cursor to wrap - this doesn't - // affect which cell they go into (the "previous cell" for both (row, - // max_col + 1) and (row + 1, 0) is (row, max_col)), but does affect - // further movement afterwards - writing an `a` at (row, max_col) - // followed by a crlf puts the cursor at (row + 1, 0), but writing a - // `à` (specifically `a` followed by a combining grave accent - the - // normalized U+00E0 "latin small letter a with grave" behaves the - // same as `a`) at (row, max_col) followed by a crlf puts the cursor - // at (row + 2, 0) - self.grid_mut().col_wrap(if width == 0 { 1 } else { width }); + self.grid_mut().col_wrap( + // zero width characters still cause the cursor to wrap - this + // doesn't affect which cell they go into (the "previous cell" for + // both (row, max_col + 1) and (row + 1, 0) is (row, max_col)), + // but does affect further movement afterwards - writing an `a` at + // (row, max_col) followed by a crlf puts the cursor at (row + 1, + // 0), but writing a `à` (specifically `a` followed by a combining + // grave accent - the normalized U+00E0 "latin small letter a with + // grave" behaves the same as `a`) at (row, max_col) followed by a + // crlf puts the cursor at (row + 2, 0) + if width == 0 { 1 } else { width }, + wrap, + ); if width == 0 { if pos.col > 0 { diff --git a/src/term.rs b/src/term.rs index d2f7c9b..5a9fbd6 100644 --- a/src/term.rs +++ b/src/term.rs @@ -24,6 +24,16 @@ impl BufWrite for ClearRowForward { } } +#[derive(Default, Debug)] +#[must_use = "this struct does nothing unless you call write_buf"] +pub struct ClearRowBackward; + +impl BufWrite for ClearRowBackward { + fn write_buf(&self, buf: &mut Vec) { + buf.extend_from_slice(b"\x1b[1K"); + } +} + #[derive(Default, Debug)] #[must_use = "this struct does nothing unless you call write_buf"] pub struct CRLF; diff --git a/tests/control.rs b/tests/control.rs index e0dc276..74e8d9c 100644 --- a/tests/control.rs +++ b/tests/control.rs @@ -97,6 +97,21 @@ fn lf_with(b: u8) { assert_eq!(parser.screen().cell(1, 5).unwrap().contents(), "r"); assert_eq!(parser.screen().cell(1, 6).unwrap().contents(), ""); assert_eq!(parser.screen().contents(), "foo\n bar"); + + parser.process(b"\x1b[H\x1b[J\x1b[4;80H"); + assert_eq!(parser.screen().cursor_position(), (3, 79)); + parser.process(b"a"); + assert_eq!(parser.screen().cursor_position(), (3, 80)); + + // note: this is a behavior that terminals disagree on - xterm and urxvt + // would leave the cursor at (4, 79) here, but alacritty, tmux, and screen + // put it at (4, 80). in general, i'm aiming for roughly tmux/screen + // compat where possible, so that's what i'm going with here. + parser.process(&[b]); + assert_eq!(parser.screen().cursor_position(), (4, 80)); + + parser.process(b"b"); + assert_eq!(parser.screen().cursor_position(), (5, 1)); } #[test] diff --git a/tests/text.rs b/tests/text.rs index 1ad16ae..4206925 100644 --- a/tests/text.rs +++ b/tests/text.rs @@ -221,6 +221,17 @@ fn wrap() { assert_eq!(parser.screen().cell(1, 1).unwrap().contents(), ""); assert_eq!(parser.screen().cell(1, 2).unwrap().contents(), "a"); assert_eq!(parser.screen().cell(1, 3).unwrap().contents(), ""); + + parser.process(b"\x1b[H\x1b[J"); + assert_eq!(parser.screen().contents(), ""); + parser.process(b" "); + assert_eq!(parser.screen().contents(), " "); + parser.process(b"\n"); + assert_eq!(parser.screen().contents(), " "); + parser.process(b"\n"); + assert_eq!(parser.screen().contents(), " "); + parser.process(b" "); + assert_eq!(parser.screen().contents(), " \n\n\n "); } #[test] diff --git a/tests/window_contents.rs b/tests/window_contents.rs index 34f5a66..352ffd5 100644 --- a/tests/window_contents.rs +++ b/tests/window_contents.rs @@ -80,33 +80,72 @@ fn empty_cells() { #[test] fn cursor_positioning() { let mut parser = vt100::Parser::default(); - let screen1 = parser.screen().clone(); + let screen = parser.screen().clone(); parser.process(b":\x1b[K"); - let screen2 = parser.screen().clone(); assert_eq!(parser.screen().cursor_position(), (0, 1)); assert_eq!( parser.screen().contents_formatted(), b"\x1b[?25h\x1b[m\x1b[H\x1b[J:" ); - assert_eq!(parser.screen().contents_diff(&screen1), b":"); + assert_eq!(parser.screen().contents_diff(&screen), b":"); + let screen = parser.screen().clone(); parser.process(b"a"); - let screen3 = parser.screen().clone(); assert_eq!(parser.screen().cursor_position(), (0, 2)); assert_eq!( parser.screen().contents_formatted(), b"\x1b[?25h\x1b[m\x1b[H\x1b[J:a" ); - assert_eq!(parser.screen().contents_diff(&screen2), b"a"); + assert_eq!(parser.screen().contents_diff(&screen), b"a"); + let screen = parser.screen().clone(); parser.process(b"\x1b[1;2H\x1b[K"); assert_eq!(parser.screen().cursor_position(), (0, 1)); assert_eq!( parser.screen().contents_formatted(), b"\x1b[?25h\x1b[m\x1b[H\x1b[J:" ); - assert_eq!(parser.screen().contents_diff(&screen3), b"\x1b[1;2H\x1b[K"); + assert_eq!(parser.screen().contents_diff(&screen), b"\x1b[1;2H\x1b[K"); + + let screen = parser.screen().clone(); + parser.process(b"\x1b[H\x1b[J\x1b[4;80H"); + assert_eq!(parser.screen().cursor_position(), (3, 79)); + assert_eq!( + parser.screen().contents_formatted(), + b"\x1b[?25h\x1b[m\x1b[H\x1b[J\x1b[4;80H" + ); + assert_eq!( + parser.screen().contents_diff(&screen), + b"\x1b[H\x1b[K\x1b[4;80H" + ); + + let screen = parser.screen().clone(); + parser.process(b"a"); + assert_eq!(parser.screen().cursor_position(), (3, 80)); + assert_eq!( + parser.screen().contents_formatted(), + b"\x1b[?25h\x1b[m\x1b[H\x1b[J\x1b[4;80Ha" + ); + assert_eq!(parser.screen().contents_diff(&screen), b"a"); + + let screen = parser.screen().clone(); + parser.process(b"\n"); + assert_eq!(parser.screen().cursor_position(), (4, 80)); + assert_eq!( + parser.screen().contents_formatted(), + b"\x1b[?25h\x1b[m\x1b[H\x1b[J\x1b[4;80Ha\n" + ); + assert_eq!(parser.screen().contents_diff(&screen), b"\n"); + + let screen = parser.screen().clone(); + parser.process(b"b"); + assert_eq!(parser.screen().cursor_position(), (5, 1)); + assert_eq!( + parser.screen().contents_formatted(), + b"\x1b[?25h\x1b[m\x1b[H\x1b[J\x1b[4;80Ha\x1b[6;1Hb" + ); + assert_eq!(parser.screen().contents_diff(&screen), b"\r\nb"); } #[test] -- cgit v1.2.3-54-g00ecf