From b88fd8a23e2a079e84f44044be915cb0186d632b Mon Sep 17 00:00:00 2001 From: Michael Brumlow Date: Sat, 7 Jan 2017 15:47:05 -0600 Subject: Better bounds checking. - Remove the use of limit. - Reduce the number of comparisons. When using numbers provided by the PTY for subtractions there is a extra step of ensuring that we won't trigger failure on testing when trying to subtract form zero. ** NOTE ** This commit fails fails the tmux_git_log test. I am submitting a PR to talk about the test. I think the test was generated before a few things were fixed the final test gird still has cells that should have been scrolled off the screen. Also, comparing output from gnome-terminal there is no difference. So this PR is here to discuss and gather information on balding test and discussing the possibility that this test may be flawed. ** NOTE ** --- src/term/mod.rs | 87 ++++++++++++++++++++++----------------------------------- 1 file changed, 33 insertions(+), 54 deletions(-) (limited to 'src') diff --git a/src/term/mod.rs b/src/term/mod.rs index 73f9ad61..63715479 100644 --- a/src/term/mod.rs +++ b/src/term/mod.rs @@ -613,67 +613,48 @@ impl ansi::Handler for Term { #[inline] fn input(&mut self, c: char) { - if self.cursor.col == self.grid.num_cols() { - debug_println!("wrapping"); - { - let location = Point { - line: self.cursor.line, - col: self.cursor.col - 1 - }; - - let cell = &mut self.grid[&location]; - cell.flags.insert(cell::WRAPLINE); - } - if (self.cursor.line + 1) >= self.scroll_region.end { - self.linefeed(); - } else { - self.cursor.line += 1; - } - self.cursor.col = Column(0); + { + let cell = &mut self.grid[&self.cursor]; + *cell = self.template_cell; + cell.c = c; + } + + if (self.cursor.col + 1) < self.grid.num_cols() { + self.cursor.col += 1; } - unsafe { - if ::util::unlikely(self.cursor.line == self.grid.num_lines()) { - panic!("cursor fell off grid"); - } - } + // TODO handle auto wrap if auto wrap is enabled. + // ESC[?h 57 code 104 - let cell = &mut self.grid[&self.cursor]; - *cell = self.template_cell; - cell.c = c; - self.cursor.col += 1; } #[inline] fn goto(&mut self, line: Line, col: Column) { - use std::cmp::min; debug_println!("goto: line={}, col={}", line, col); - self.cursor.line = min(line, self.grid.num_lines() - 1); - self.cursor.col = min(col, self.grid.num_cols() - 1); + self.cursor.line = cmp::min(line, self.grid.num_lines() - 1); + self.cursor.col = cmp::min(col, self.grid.num_cols() - 1); } #[inline] fn goto_line(&mut self, line: Line) { - use std::cmp::min; debug_println!("goto_line: {}", line); - self.cursor.line = min(line, self.grid.num_lines() - 1); + self.cursor.line = cmp::min(line, self.grid.num_lines() - 1); } #[inline] fn goto_col(&mut self, col: Column) { - use std::cmp::min; debug_println!("goto_col: {}", col); - self.cursor.col = min(col, self.grid.num_cols() - 1); + self.cursor.col = cmp::min(col, self.grid.num_cols() - 1); } #[inline] fn insert_blank(&mut self, count: Column) { // Ensure inserting within terminal bounds - let col = limit(self.cursor.col, Column(0), self.grid.num_cols()); - let count = ::std::cmp::min(count, self.size_info.cols() - col); + + let count = cmp::min(count, self.size_info.cols() - self.cursor.col); - let source = col; - let destination = col + count; + let source = self.cursor.col; + let destination = self.cursor.col + count; let num_cells = (self.size_info.cols() - destination).0; let line = self.cursor.line; // borrowck @@ -697,30 +678,27 @@ impl ansi::Handler for Term { #[inline] fn move_up(&mut self, lines: Line) { debug_println!("move_up: {}", lines); - self.cursor.line = limit(self.cursor.line - lines, Line(0), self.grid.num_lines() -1); + let lines = cmp::min(self.cursor.line, lines); + self.cursor.line = cmp::min(self.cursor.line - lines, self.grid.num_lines() -1); } #[inline] fn move_down(&mut self, lines: Line) { debug_println!("move_down: {}", lines); - self.cursor.line = limit(self.cursor.line + lines, Line(0), self.grid.num_lines() - 1); - debug_println!("move_down - > lines: {}", self.cursor.line); + self.cursor.line = cmp::min(self.cursor.line + lines, self.grid.num_lines() - 1); } #[inline] fn move_forward(&mut self, cols: Column) { debug_println!("move_forward: {}", cols); - let col = limit(self.cursor.col, Column(0), self.grid.num_cols() - 1); - self.cursor.col = limit(col + cols, Column(0), self.grid.num_cols() - 1); + self.cursor.col = cmp::min(self.cursor.col + cols, self.grid.num_cols() - 1); } #[inline] fn move_backward(&mut self, cols: Column) { debug_println!("move_backward: {}", cols); - let col = limit(self.cursor.col, Column(0), self.grid.num_cols() - 1); - if col >= cols { - self.cursor.col = col - cols; - } + let cols = cmp::min(self.cursor.col, cols); + self.cursor.col = cmp::min(self.cursor.col - cols, self.grid.num_cols() - 1); } #[inline] @@ -742,7 +720,7 @@ impl ansi::Handler for Term { fn put_tab(&mut self, mut count: i64) { debug_println!("put_tab: {}", count); - let mut col = limit(self.cursor.col, Column(0), self.grid.num_cols() - 1); + let mut col = self.cursor.col; while col < self.grid.num_cols() && count != 0 { count -= 1; loop { @@ -837,8 +815,8 @@ impl ansi::Handler for Term { #[inline] fn erase_chars(&mut self, count: Column) { debug_println!("erase_chars: {}, {}", count, self.cursor.col); - let start = limit(self.cursor.col, Column(0), self.grid.num_cols() - 1); - let end = limit(start + count, Column(0), self.grid.num_cols() - 1); + let start = self.cursor.col; + let end = cmp::min(start + count, self.grid.num_cols() - 1); let row = &mut self.grid[self.cursor.line]; let template = self.empty_cell; @@ -850,10 +828,10 @@ impl ansi::Handler for Term { #[inline] fn delete_chars(&mut self, count: Column) { // Ensure deleting within terminal bounds - let count = ::std::cmp::min(count, self.size_info.cols()); - - let start = limit(self.cursor.col, Column(0), self.grid.num_cols() - 1); - let end = limit(self.cursor.col + count, Column(0), self.grid.num_cols() - 1); + let count = cmp::min(count, self.size_info.cols()); + + let start = self.cursor.col; + let end = cmp::min(start + count, self.grid.num_cols() - 1); let n = (self.size_info.cols() - end).0; let line = self.cursor.line; // borrowck @@ -899,7 +877,8 @@ impl ansi::Handler for Term { fn clear_line(&mut self, mode: ansi::LineClearMode) { debug_println!("clear_line: {:?}", mode); let template = self.empty_cell; - let col = limit(self.cursor.col, Column(0), self.grid.num_cols() - 1); + let col = self.cursor.col; + match mode { ansi::LineClearMode::Right => { let row = &mut self.grid[self.cursor.line]; -- cgit From 24453fa698150e4e980339a11ecc3fbdb2ee4914 Mon Sep 17 00:00:00 2001 From: Michael Brumlow Date: Sat, 7 Jan 2017 17:13:42 -0600 Subject: Implementing line wrapping. This implementation of line wrapping ensures self.cursor.col is never out of bounds, thus not requiring checking. --- src/term/mod.rs | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) (limited to 'src') diff --git a/src/term/mod.rs b/src/term/mod.rs index 63715479..878229e2 100644 --- a/src/term/mod.rs +++ b/src/term/mod.rs @@ -202,6 +202,9 @@ pub struct Term { /// The grid grid: Grid, + /// Wrap on next input + wrap: bool, + /// Alternate grid alt_grid: Grid, @@ -297,6 +300,7 @@ impl Term { Term { dirty: false, + wrap: false, grid: grid, alt_grid: alt, alt: false, @@ -613,6 +617,31 @@ impl ansi::Handler for Term { #[inline] fn input(&mut self, c: char) { + if self.wrap { + + debug_println!("wrapping"); + + { + let location = Point { + line: self.cursor.line, + col: self.cursor.col + }; + + let cell = &mut self.grid[&location]; + cell.flags.insert(cell::WRAPLINE); + } + + if (self.cursor.line + 1) >= self.scroll_region.end { + self.linefeed(); + } else { + self.cursor.line += 1; + } + + self.cursor.col = Column(0); + self.wrap = false; + } + + { let cell = &mut self.grid[&self.cursor]; *cell = self.template_cell; @@ -621,6 +650,8 @@ impl ansi::Handler for Term { if (self.cursor.col + 1) < self.grid.num_cols() { self.cursor.col += 1; + } else { + self.wrap = true; } // TODO handle auto wrap if auto wrap is enabled. @@ -633,18 +664,21 @@ impl ansi::Handler for Term { debug_println!("goto: line={}, col={}", line, col); self.cursor.line = cmp::min(line, self.grid.num_lines() - 1); self.cursor.col = cmp::min(col, self.grid.num_cols() - 1); + self.wrap = false; } #[inline] fn goto_line(&mut self, line: Line) { debug_println!("goto_line: {}", line); self.cursor.line = cmp::min(line, self.grid.num_lines() - 1); + self.wrap = false; } #[inline] fn goto_col(&mut self, col: Column) { debug_println!("goto_col: {}", col); self.cursor.col = cmp::min(col, self.grid.num_cols() - 1); + self.wrap = false; } #[inline] @@ -692,6 +726,7 @@ impl ansi::Handler for Term { fn move_forward(&mut self, cols: Column) { debug_println!("move_forward: {}", cols); self.cursor.col = cmp::min(self.cursor.col + cols, self.grid.num_cols() - 1); + self.wrap = false; } #[inline] @@ -699,6 +734,7 @@ impl ansi::Handler for Term { debug_println!("move_backward: {}", cols); let cols = cmp::min(self.cursor.col, cols); self.cursor.col = cmp::min(self.cursor.col - cols, self.grid.num_cols() - 1); + self.wrap = false; } #[inline] @@ -732,6 +768,7 @@ impl ansi::Handler for Term { } self.cursor.col = col; + self.wrap = false; } /// Backspace `count` characters @@ -740,6 +777,7 @@ impl ansi::Handler for Term { debug_println!("backspace"); if self.cursor.col > Column(0) { self.cursor.col -= 1; + self.wrap = false; } } @@ -748,6 +786,7 @@ impl ansi::Handler for Term { fn carriage_return(&mut self) { debug_println!("carriage_return"); self.cursor.col = Column(0); + self.wrap = false; } /// Linefeed -- cgit From c106ee6601e655dc822ca10d16292837ad29c1e2 Mon Sep 17 00:00:00 2001 From: Michael Brumlow Date: Sat, 7 Jan 2017 17:17:46 -0600 Subject: Removing stale comment. --- src/term/mod.rs | 4 ---- 1 file changed, 4 deletions(-) (limited to 'src') diff --git a/src/term/mod.rs b/src/term/mod.rs index 878229e2..5babaf2f 100644 --- a/src/term/mod.rs +++ b/src/term/mod.rs @@ -641,7 +641,6 @@ impl ansi::Handler for Term { self.wrap = false; } - { let cell = &mut self.grid[&self.cursor]; *cell = self.template_cell; @@ -654,9 +653,6 @@ impl ansi::Handler for Term { self.wrap = true; } - // TODO handle auto wrap if auto wrap is enabled. - // ESC[?h 57 code 104 - } #[inline] -- cgit From 5642624f40c3a7f4126d692fc917edaf3577d54b Mon Sep 17 00:00:00 2001 From: Michael Brumlow Date: Sat, 7 Jan 2017 21:46:02 -0600 Subject: Changes requested. - Rename wrap to input_needs_wrap and providing documentation. - Standardize on min. - Optimization on subtracting col. --- src/term/mod.rs | 72 ++++++++++++++++++++++++++++++--------------------------- 1 file changed, 38 insertions(+), 34 deletions(-) (limited to 'src') diff --git a/src/term/mod.rs b/src/term/mod.rs index 5babaf2f..92e5463d 100644 --- a/src/term/mod.rs +++ b/src/term/mod.rs @@ -15,7 +15,7 @@ //! Exports the `Term` type which is a high-level API for the Grid use std::ops::{Deref, Range}; use std::ptr; -use std::cmp; +use std::cmp::min; use std::io; use ansi::{self, Color, NamedColor, Attr, Handler}; @@ -168,8 +168,9 @@ impl<'a> Iterator for RenderableCellsIter<'a> { /// coerce val to be between min and max #[inline] -fn limit(val: T, min: T, max: T) -> T { - cmp::min(cmp::max(min, val), max) +fn limit(val: T, min_limit: T, max_limit: T) -> T { + use std::cmp::max; + min(max(min_limit, val), max_limit) } pub mod mode { @@ -202,8 +203,12 @@ pub struct Term { /// The grid grid: Grid, - /// Wrap on next input - wrap: bool, + /// Tracks if the next call to input will need to first handle wrapping. + /// This is true after the last column is set with the input function. Any function that + /// implicitly sets the line or column needs to set this to false to avoid wrapping twice. + /// input_needs_wrap ensures that cursor.col is always valid for use into indexing into + /// arrays. Without it we wold have to sanitize cursor.col every time we used it. + input_needs_wrap: bool, /// Alternate grid alt_grid: Grid, @@ -274,8 +279,8 @@ impl SizeInfo { let line = Line(y / (self.cell_height as usize)); Some(Point { - line: cmp::min(line, self.lines() - 1), - col: cmp::min(col, self.cols() - 1) + line: min(line, self.lines() - 1), + col: min(col, self.cols() - 1) }) } } @@ -300,7 +305,7 @@ impl Term { Term { dirty: false, - wrap: false, + input_needs_wrap: false, grid: grid, alt_grid: alt, alt: false, @@ -352,7 +357,7 @@ impl Term { ) -> Option> { let line = &grid[line]; let line_length = line.line_length(); - let line_end = cmp::min(line_length, cols.end + 1); + let line_end = min(line_length, cols.end + 1); if cols.start >= line_end { None @@ -617,7 +622,7 @@ impl ansi::Handler for Term { #[inline] fn input(&mut self, c: char) { - if self.wrap { + if self.input_needs_wrap { debug_println!("wrapping"); @@ -638,7 +643,7 @@ impl ansi::Handler for Term { } self.cursor.col = Column(0); - self.wrap = false; + self.input_needs_wrap = false; } { @@ -650,7 +655,7 @@ impl ansi::Handler for Term { if (self.cursor.col + 1) < self.grid.num_cols() { self.cursor.col += 1; } else { - self.wrap = true; + self.input_needs_wrap = true; } } @@ -658,30 +663,30 @@ impl ansi::Handler for Term { #[inline] fn goto(&mut self, line: Line, col: Column) { debug_println!("goto: line={}, col={}", line, col); - self.cursor.line = cmp::min(line, self.grid.num_lines() - 1); - self.cursor.col = cmp::min(col, self.grid.num_cols() - 1); - self.wrap = false; + self.cursor.line = min(line, self.grid.num_lines() - 1); + self.cursor.col = min(col, self.grid.num_cols() - 1); + self.input_needs_wrap = false; } #[inline] fn goto_line(&mut self, line: Line) { debug_println!("goto_line: {}", line); - self.cursor.line = cmp::min(line, self.grid.num_lines() - 1); - self.wrap = false; + self.cursor.line = min(line, self.grid.num_lines() - 1); + self.input_needs_wrap = false; } #[inline] fn goto_col(&mut self, col: Column) { debug_println!("goto_col: {}", col); - self.cursor.col = cmp::min(col, self.grid.num_cols() - 1); - self.wrap = false; + self.cursor.col = min(col, self.grid.num_cols() - 1); + self.input_needs_wrap = false; } #[inline] fn insert_blank(&mut self, count: Column) { // Ensure inserting within terminal bounds - let count = cmp::min(count, self.size_info.cols() - self.cursor.col); + let count = min(count, self.size_info.cols() - self.cursor.col); let source = self.cursor.col; let destination = self.cursor.col + count; @@ -708,29 +713,28 @@ impl ansi::Handler for Term { #[inline] fn move_up(&mut self, lines: Line) { debug_println!("move_up: {}", lines); - let lines = cmp::min(self.cursor.line, lines); - self.cursor.line = cmp::min(self.cursor.line - lines, self.grid.num_lines() -1); + let lines = min(self.cursor.line, lines); + self.cursor.line = min(self.cursor.line - lines, self.grid.num_lines() -1); } #[inline] fn move_down(&mut self, lines: Line) { debug_println!("move_down: {}", lines); - self.cursor.line = cmp::min(self.cursor.line + lines, self.grid.num_lines() - 1); + self.cursor.line = min(self.cursor.line + lines, self.grid.num_lines() - 1); } #[inline] fn move_forward(&mut self, cols: Column) { debug_println!("move_forward: {}", cols); - self.cursor.col = cmp::min(self.cursor.col + cols, self.grid.num_cols() - 1); - self.wrap = false; + self.cursor.col = min(self.cursor.col + cols, self.grid.num_cols() - 1); + self.input_needs_wrap = false; } #[inline] fn move_backward(&mut self, cols: Column) { debug_println!("move_backward: {}", cols); - let cols = cmp::min(self.cursor.col, cols); - self.cursor.col = cmp::min(self.cursor.col - cols, self.grid.num_cols() - 1); - self.wrap = false; + self.cursor.col -= min(self.cursor.col, cols); + self.input_needs_wrap = false; } #[inline] @@ -764,7 +768,7 @@ impl ansi::Handler for Term { } self.cursor.col = col; - self.wrap = false; + self.input_needs_wrap = false; } /// Backspace `count` characters @@ -773,7 +777,7 @@ impl ansi::Handler for Term { debug_println!("backspace"); if self.cursor.col > Column(0) { self.cursor.col -= 1; - self.wrap = false; + self.input_needs_wrap = false; } } @@ -782,7 +786,7 @@ impl ansi::Handler for Term { fn carriage_return(&mut self) { debug_println!("carriage_return"); self.cursor.col = Column(0); - self.wrap = false; + self.input_needs_wrap = false; } /// Linefeed @@ -851,7 +855,7 @@ impl ansi::Handler for Term { fn erase_chars(&mut self, count: Column) { debug_println!("erase_chars: {}, {}", count, self.cursor.col); let start = self.cursor.col; - let end = cmp::min(start + count, self.grid.num_cols() - 1); + let end = min(start + count, self.grid.num_cols() - 1); let row = &mut self.grid[self.cursor.line]; let template = self.empty_cell; @@ -863,10 +867,10 @@ impl ansi::Handler for Term { #[inline] fn delete_chars(&mut self, count: Column) { // Ensure deleting within terminal bounds - let count = cmp::min(count, self.size_info.cols()); + let count = min(count, self.size_info.cols()); let start = self.cursor.col; - let end = cmp::min(start + count, self.grid.num_cols() - 1); + let end = min(start + count, self.grid.num_cols() - 1); let n = (self.size_info.cols() - end).0; let line = self.cursor.line; // borrowck -- cgit