Scrolling v2

The previous scrolling + scroll region implementation exhibited display
corruption bugs in several applications including tmux, irssi, htop, and
vim. The new implementation doesn't seem to suffer from any of those
issues.

This implementation is able to `find /usr` on my machine (nearly 600k
lines) in ~2.0 seconds while st is able to do the same in ~2.2 seconds.
Alacritty is officially faster!

src/grid.rs

@@ -20,18 +20,16 @@
 //! time; rotation currently reorganize Vecs in the lines Vec, and indexing with
 //! ranges is currently supported.
 
-use std::ops::{Deref, DerefMut, Range, RangeTo, RangeFrom, RangeFull, Index, IndexMut};
-use std::cmp::Ordering;
-use std::slice::{self, Iter, IterMut};
-use std::iter::IntoIterator;
 use std::borrow::ToOwned;
-
-use util::Rotate;
+use std::cmp::Ordering;
+use std::iter::IntoIterator;
+use std::ops::{Deref, DerefMut, Range, RangeTo, RangeFrom, RangeFull, Index, IndexMut};
+use std::slice::{self, Iter, IterMut};
 
 use index::{self, Cursor};
 
 /// Represents the terminal display contents
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct Grid<T> {
     /// Lines in the grid. Each row holds a list of cells corresponding to the
     /// columns in that row.
@@ -119,8 +117,45 @@ impl<T> Grid<T> {
     }
 
     #[inline]
-    pub fn scroll(&mut self, region: Range<index::Line>, positions: isize) {
-        self[region].rotate(positions)
+    pub fn scroll_down(&mut self, region: Range<index::Line>, positions: index::Line) {
+        for line in region.rev() {
+            let src = line;
+            let dst = line - positions;
+            self.swap_lines(src, dst);
+        }
+    }
+
+    #[inline]
+    pub fn scroll_up(&mut self, region: Range<index::Line>, positions: index::Line) {
+        for line in region {
+            let src = line;
+            let dst = line + positions;
+            self.swap_lines(src, dst);
+        }
+    }
+
+    /// Swap two lines in the grid
+    ///
+    /// This could have used slice::swap internally, but we are able to have
+    /// better error messages by doing the bounds checking ourselves.
+    #[inline]
+    pub fn swap_lines(&mut self, src: index::Line, dst: index::Line) {
+        // check that src/dst are in bounds. Since index::Line newtypes usize,
+        // we can assume values are positive.
+        if src >= self.lines {
+            panic!("swap_lines src out of bounds; len={}, src={}", self.raw.len(), src);
+        }
+
+        if dst >= self.lines {
+            panic!("swap_lines dst out of bounds; len={}, dst={}", self.raw.len(), dst);
+        }
+
+        unsafe {
+            let src: *mut _ = self.raw.get_unchecked_mut(src.0);
+            let dst: *mut _ = self.raw.get_unchecked_mut(dst.0);
+
+            ::std::ptr::swap(src, dst);
+        }
     }
 
     #[inline]
@@ -179,7 +214,7 @@ impl<'cursor, T> IndexMut<&'cursor Cursor> for Grid<T> {
 }
 
 /// A row in the grid
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct Row<T>(Vec<T>);
 
 impl<T: Clone> Row<T> {
@@ -414,3 +449,131 @@ macro_rules! clear_region_impl {
 clear_region_impl!(Range<index::Line>);
 clear_region_impl!(RangeTo<index::Line>);
 clear_region_impl!(RangeFrom<index::Line>);
+
+#[cfg(test)]
+mod tests {
+    use super::Grid;
+    use index::{Line, Column};
+    #[test]
+    fn grid_swap_lines_ok() {
+        let mut grid = Grid::new(Line(10), Column(1), &0);
+        println!("");
+
+        // swap test ends
+        grid[Line(0)][Column(0)] = 1;
+        grid[Line(9)][Column(0)] = 2;
+
+        assert_eq!(grid[Line(0)][Column(0)], 1);
+        assert_eq!(grid[Line(9)][Column(0)], 2);
+
+        grid.swap_lines(Line(0), Line(9));
+
+        assert_eq!(grid[Line(0)][Column(0)], 2);
+        assert_eq!(grid[Line(9)][Column(0)], 1);
+
+        // swap test mid
+        grid[Line(4)][Column(0)] = 1;
+        grid[Line(5)][Column(0)] = 2;
+
+        println!("grid: {:?}", grid);
+
+        assert_eq!(grid[Line(4)][Column(0)], 1);
+        assert_eq!(grid[Line(5)][Column(0)], 2);
+
+        grid.swap_lines(Line(4), Line(5));
+
+        println!("grid: {:?}", grid);
+
+        assert_eq!(grid[Line(4)][Column(0)], 2);
+        assert_eq!(grid[Line(5)][Column(0)], 1);
+    }
+
+    #[test]
+    #[should_panic]
+    fn grid_swap_lines_oob1() {
+        let mut grid = Grid::new(Line(10), Column(1), &0);
+        grid.swap_lines(Line(0), Line(10));
+    }
+
+    #[test]
+    #[should_panic]
+    fn grid_swap_lines_oob2() {
+        let mut grid = Grid::new(Line(10), Column(1), &0);
+        grid.swap_lines(Line(10), Line(0));
+    }
+
+    #[test]
+    #[should_panic]
+    fn grid_swap_lines_oob3() {
+        let mut grid = Grid::new(Line(10), Column(1), &0);
+        grid.swap_lines(Line(10), Line(10));
+    }
+
+    // Scroll up moves lines upwards
+    #[test]
+    fn scroll_up() {
+        println!("");
+
+        let mut grid = Grid::new(Line(10), Column(1), &0);
+        for i in 0..10 {
+            grid[Line(i)][Column(0)] = i;
+        }
+
+        println!("grid: {:?}", grid);
+
+        grid.scroll_up(Line(0)..Line(8), Line(2));
+
+        println!("grid: {:?}", grid);
+
+        let mut other = Grid::new(Line(10), Column(1), &9);
+
+        other[Line(0)][Column(0)] = 2;
+        other[Line(1)][Column(0)] = 3;
+        other[Line(2)][Column(0)] = 4;
+        other[Line(3)][Column(0)] = 5;
+        other[Line(4)][Column(0)] = 6;
+        other[Line(5)][Column(0)] = 7;
+        other[Line(6)][Column(0)] = 8;
+        other[Line(7)][Column(0)] = 9;
+        other[Line(8)][Column(0)] = 0;
+        other[Line(9)][Column(0)] = 1;
+
+        for i in 0..10 {
+            assert_eq!(grid[Line(i)][Column(0)], other[Line(i)][Column(0)]);
+        }
+    }
+
+    // Scroll down moves lines downwards
+    #[test]
+    fn scroll_down() {
+        println!("");
+
+        let mut grid = Grid::new(Line(10), Column(1), &0);
+        for i in 0..10 {
+            grid[Line(i)][Column(0)] = i;
+        }
+
+        println!("grid: {:?}", grid);
+
+        grid.scroll_down(Line(2)..Line(10), Line(2));
+
+        println!("grid: {:?}", grid);
+
+        let mut other = Grid::new(Line(10), Column(1), &9);
+
+        other[Line(0)][Column(0)] = 8;
+        other[Line(1)][Column(0)] = 9;
+        other[Line(2)][Column(0)] = 0;
+        other[Line(3)][Column(0)] = 1;
+        other[Line(4)][Column(0)] = 2;
+        other[Line(5)][Column(0)] = 3;
+        other[Line(6)][Column(0)] = 4;
+        other[Line(7)][Column(0)] = 5;
+        other[Line(8)][Column(0)] = 6;
+        other[Line(9)][Column(0)] = 7;
+
+        for i in 0..10 {
+            assert_eq!(grid[Line(i)][Column(0)], other[Line(i)][Column(0)]);
+        }
+    }
+}

src/index.rs

@@ -131,6 +131,33 @@ macro_rules! sub {
                 $construct(self.0 - rhs.0)
             }
         }
+
+        impl<'a> ops::Sub<$ty> for &'a $ty {
+            type Output = $ty;
+
+            #[inline]
+            fn sub(self, rhs: $ty) -> $ty {
+                $construct(self.0 - rhs.0)
+            }
+        }
+
+        impl<'a> ops::Sub<&'a $ty> for $ty {
+            type Output = $ty;
+
+            #[inline]
+            fn sub(self, rhs: &'a $ty) -> $ty {
+                $construct(self.0 - rhs.0)
+            }
+        }
+
+        impl<'a, 'b> ops::Sub<&'a $ty> for &'b $ty {
+            type Output = $ty;
+
+            #[inline]
+            fn sub(self, rhs: &'a $ty) -> $ty {
+                $construct(self.0 - rhs.0)
+            }
+        }
     }
 }
 

src/term.rs

@@ -384,6 +384,57 @@ impl Term {
             self.grid.clear(|c| c.reset(&template));
         }
     }
+
+    /// Scroll screen down
+    ///
+    /// Text moves down; clear at bottom
+    #[inline]
+    fn scroll_down_relative(&mut self, origin: Line, lines: Line) {
+        debug_println!("scroll_down: {}", lines);
+
+        // Copy of cell template; can't have it borrowed when calling clear/scroll
+        let template = self.template_cell.clone();
+
+        // Clear `lines` lines at bottom of area
+        {
+            let end = self.scroll_region.end;
+            let start = end - lines;
+            self.grid.clear_region(start..end, |c| c.reset(&template));
+        }
+
+        // Scroll between origin and bottom
+        {
+            let end = self.scroll_region.end;
+            println!("origin={}, lines={}", origin, lines);
+            let start = origin + lines;
+            self.grid.scroll_down(start..end, lines);
+        }
+    }
+
+    /// Scroll screen up
+    ///
+    /// Text moves up; clear at top
+    #[inline]
+    fn scroll_up_relative(&mut self, origin: Line, lines: Line) {
+        debug_println!("scroll_up: {}", lines);
+
+        // Copy of cell template; can't have it borrowed when calling clear/scroll
+        let template = self.template_cell.clone();
+
+        // Clear `lines` lines starting from origin to origin + lines
+        {
+            let start = origin;
+            let end = start + lines;
+            self.grid.clear_region(start..end, |c| c.reset(&template));
+        }
+
+        // Scroll from origin to bottom less number of lines
+        {
+            let start = origin;
+            let end = self.scroll_region.end - lines;
+            self.grid.scroll_up(start..end, lines);
+        }
+    }
 }
 
 impl ansi::TermInfo for Term {
@@ -553,41 +604,32 @@ impl ansi::Handler for Term {
     }
 
     #[inline]
-    fn scroll_down(&mut self, lines: Line) {
-        debug_println!("scroll_down: {}", lines);
-
-        // Scrolled up, clear from top
-        self.grid.scroll(self.scroll_region.clone(), -(*lines as isize));
-        let end = self.scroll_region.start + lines;
-        let template = self.template_cell.clone();
-        self.grid.clear_region(self.scroll_region.start..end, |c| c.reset(&template));
+    fn scroll_up(&mut self, lines: Line) {
+        let origin = self.scroll_region.start;
+        self.scroll_up_relative(origin, lines);
     }
 
     #[inline]
-    fn scroll_up(&mut self, lines: Line) {
-        debug_println!("scroll_up: {}", lines);
-        // Scrolled up, so need to clear from bottom
-        self.grid.scroll(self.scroll_region.clone(), *lines as isize);
-        let start = self.scroll_region.end - lines;
-        let template = self.template_cell.clone();
-        self.grid.clear_region(start..self.scroll_region.end, |c| c.reset(&template));
+    fn scroll_down(&mut self, lines: Line) {
+        let origin = self.scroll_region.start;
+        self.scroll_down_relative(origin, lines);
     }
 
     #[inline]
     fn insert_blank_lines(&mut self, lines: Line) {
         debug_println!("insert_blank_lines: {}", lines);
-        if self.scroll_region.start <= self.cursor.line &&
-            self.cursor.line <= self.scroll_region.end {
-            self.scroll_down(lines);
+        if self.scroll_region.contains(self.cursor.line) {
+            let origin = self.cursor.line;
+            self.scroll_down_relative(origin, lines);
         }
     }
 
     #[inline]
     fn delete_lines(&mut self, lines: Line) {
         debug_println!("delete_lines: {}", lines);
-        if self.scroll_region.start <= self.cursor.line &&
-            self.cursor.line <= self.scroll_region.end {
-            self.scroll_up(lines);
+        if self.scroll_region.contains(self.cursor.line) {
+            let origin = self.cursor.line;
+            self.scroll_up_relative(origin, lines);
         }
     }
 
@@ -661,10 +703,7 @@ impl ansi::Handler for Term {
         let template = self.template_cell.clone();
         match mode {
             ansi::ClearMode::Below => {
-                let start = self.cursor.line;
-                let end = self.grid.num_lines();
-
-                for row in &mut self.grid[start..end] {
+                for row in &mut self.grid[self.cursor.line..] {
                     for cell in row {
                         cell.reset(&template);
                     }
@@ -693,7 +732,7 @@ impl ansi::Handler for Term {
     fn reverse_index(&mut self) {
         debug_println!("reverse_index");
         // if cursor is at the top
-        if self.cursor.line == Line(0) {
+        if self.cursor.line == self.scroll_region.start {
             self.scroll_down(Line(1));
         } else {
             self.cursor.line -= 1;
@@ -772,6 +811,7 @@ impl ansi::Handler for Term {
     fn set_scrolling_region(&mut self, region: Range<Line>) {
         debug_println!("set scroll region: {:?}", region);
         self.scroll_region = region;
+        self.goto(Line(0), Column(0));
     }
 
     #[inline]

src/util.rs

@@ -24,80 +24,3 @@ pub mod thread {
         ::std::thread::Builder::new().name(name.into()).spawn(f).expect("thread spawn works")
     }
 }
-
-/// Types that can have their elements rotated
-pub trait Rotate {
-    fn rotate(&mut self, positions: isize);
-}
-
-impl<T> Rotate for [T] {
-    fn rotate(&mut self, positions: isize) {
-        // length is needed over and over
-        let len = self.len();
-
-        // Enforce positions in [0, len) and treat negative rotations as a
-        // posititive rotation of len - positions.
-        let positions = if positions > 0 {
-            positions as usize % len
-        } else {
-            len - (-positions as usize) % len
-        };
-
-        // If positions is 0 or the entire slice, it's a noop.
-        if positions == 0 || positions == len {
-            return;
-        }
-
-        self[..positions].reverse();
-        self[positions..].reverse();
-        self.reverse();
-    }
-}
-
-
-#[cfg(test)]
-mod tests {
-    use super::Rotate;
-
-    #[test]
-    fn rotate_forwards_works() {
-        let s = &mut [1, 2, 3, 4, 5];
-        s.rotate(1);
-        assert_eq!(&[2, 3, 4, 5, 1], s);
-    }
-
-    #[test]
-    fn rotate_backwards_works() {
-        let s = &mut [1, 2, 3, 4, 5];
-        s.rotate(-1);
-        assert_eq!(&[5, 1, 2, 3, 4], s);
-    }
-
-    #[test]
-    fn rotate_multiple_forwards() {
-        let s = &mut [1, 2, 3, 4, 5, 6, 7];
-        s.rotate(2);
-        assert_eq!(&[3, 4, 5, 6, 7, 1, 2], s);
-    }
-
-    #[test]
-    fn rotate_multiple_backwards() {
-        let s = &mut [1, 2, 3, 4, 5];
-        s.rotate(-3);
-        assert_eq!(&[3, 4, 5, 1, 2], s);
-    }
-
-    #[test]
-    fn rotate_forwards_overflow() {
-        let s = &mut [1, 2, 3, 4, 5];
-        s.rotate(6);
-        assert_eq!(&[2, 3, 4, 5, 1], s);
-    }
-
-    #[test]
-    fn rotate_backwards_overflow() {
-        let s = &mut [1, 2, 3, 4, 5];
-        s.rotate(-6);
-        assert_eq!(&[5, 1, 2, 3, 4], s);
-    }
-}