Fix selection rotating outside of scrolling region

Fixes #2983.

CHANGELOG.md

@@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Backwards bracket selection
 - Stack overflow when printing shader creation error
 - Underline position for bitmap fonts
+- Selection rotating outside of scrolling region
 
 ### Removed
 

alacritty_terminal/src/grid/mod.rs

@@ -492,6 +492,9 @@ impl<T: GridCell + PartialEq + Copy> Grid<T> {
 
     #[inline]
     pub fn scroll_down(&mut self, region: &Range<Line>, positions: Line, template: &T) {
+        let num_lines = self.num_lines().0;
+        let num_cols = self.num_cols().0;
+
         // Whether or not there is a scrolling region active, as long as it
         // starts at the top, we can do a full rotation which just involves
         // changing the start index.
@@ -501,9 +504,10 @@ impl<T: GridCell + PartialEq + Copy> Grid<T> {
             // Rotate the entire line buffer. If there's a scrolling region
             // active, the bottom lines are restored in the next step.
             self.raw.rotate_up(*positions);
-            if let Some(ref mut selection) = self.selection {
+            self.selection = self
-                selection.rotate(-(*positions as isize));
+                .selection
-            }
+                .take()
+                .and_then(|s| s.rotate(num_lines, num_cols, region, -(*positions as isize)));
 
             self.decrease_scroll_limit(*positions);
 
@@ -518,6 +522,12 @@ impl<T: GridCell + PartialEq + Copy> Grid<T> {
                 self.raw[i].reset(&template);
             }
         } else {
+            // Rotate selection to track content
+            self.selection = self
+                .selection
+                .take()
+                .and_then(|s| s.rotate(num_lines, num_cols, region, -(*positions as isize)));
+
             // Subregion rotation
             for line in IndexRange((region.start + positions)..region.end).rev() {
                 self.raw.swap_lines(line, line - positions);
@@ -533,11 +543,13 @@ impl<T: GridCell + PartialEq + Copy> Grid<T> {
     ///
     /// This is the performance-sensitive part of scrolling.
     pub fn scroll_up(&mut self, region: &Range<Line>, positions: Line, template: &T) {
+        let num_lines = self.num_lines().0;
+        let num_cols = self.num_cols().0;
+
         if region.start == Line(0) {
             // Update display offset when not pinned to active area
             if self.display_offset != 0 {
-                self.display_offset =
+                self.display_offset = min(self.display_offset + *positions, self.len() - num_lines);
-                    min(self.display_offset + *positions, self.len() - self.num_lines().0);
             }
 
             self.increase_scroll_limit(*positions, template);
@@ -545,15 +557,16 @@ impl<T: GridCell + PartialEq + Copy> Grid<T> {
             // Rotate the entire line buffer. If there's a scrolling region
             // active, the bottom lines are restored in the next step.
             self.raw.rotate(-(*positions as isize));
-            if let Some(ref mut selection) = self.selection {
+            self.selection = self
-                selection.rotate(*positions as isize);
+                .selection
-            }
+                .take()
+                .and_then(|s| s.rotate(num_lines, num_cols, region, *positions as isize));
 
             // This next loop swaps "fixed" lines outside of a scroll region
             // back into place after the rotation. The work is done in buffer-
             // space rather than terminal-space to avoid redundant
             // transformations.
-            let fixed_lines = *self.num_lines() - *region.end;
+            let fixed_lines = num_lines - *region.end;
 
             for i in 0..fixed_lines {
                 self.raw.swap(i, i + *positions);
@@ -566,6 +579,12 @@ impl<T: GridCell + PartialEq + Copy> Grid<T> {
                 self.raw[i + fixed_lines].reset(&template);
             }
         } else {
+            // Rotate selection to track content
+            self.selection = self
+                .selection
+                .take()
+                .and_then(|s| s.rotate(num_lines, num_cols, region, *positions as isize));
+
             // Subregion rotation
             for line in IndexRange(region.start..(region.end - positions)) {
                 self.raw.swap_lines(line, line + positions);

alacritty_terminal/src/selection.rs

@@ -22,12 +22,12 @@ use std::convert::TryFrom;
 use std::mem;
 use std::ops::Range;
 
-use crate::index::{Column, Point, Side};
+use crate::index::{Column, Line, Point, Side};
 use crate::term::cell::Flags;
 use crate::term::{Search, Term};
 
 /// A Point and side within that point.
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Copy, Clone, PartialEq)]
 pub struct Anchor {
     point: Point<usize>,
     side: Side,
@@ -66,87 +66,147 @@ impl<L> SelectionRange<L> {
     }
 }
 
+/// Different kinds of selection.
+#[derive(Debug, Copy, Clone, PartialEq)]
+enum SelectionType {
+    Simple,
+    Block,
+    Semantic,
+    Lines,
+}
+
 /// Describes a region of a 2-dimensional area.
 ///
-/// Used to track a text selection. There are three supported modes, each with its own constructor:
+/// Used to track a text selection. There are four supported modes, each with its own constructor:
-/// [`simple`], [`semantic`], and [`lines`]. The [`simple`] mode precisely tracks which cells are
+/// [`simple`], [`block`], [`semantic`], and [`lines`]. The [`simple`] mode precisely tracks which
-/// selected without any expansion. [`semantic`] mode expands the initial selection to the nearest
+/// cells are selected without any expansion. [`block`] will select rectangular regions.
-/// semantic escape char in either direction. [`lines`] will always select entire lines.
+/// [`semantic`] mode expands the initial selection to the nearest semantic escape char in either
+/// direction. [`lines`] will always select entire lines.
 ///
-/// Calls to [`update`] operate different based on the selection kind. The [`simple`] mode does
+/// Calls to [`update`] operate different based on the selection kind. The [`simple`] and [`block`]
-/// nothing special, simply tracks points and sides. [`semantic`] will continue to expand out to
+/// mode do nothing special, simply track points and sides. [`semantic`] will continue to expand
-/// semantic boundaries as the selection point changes. Similarly, [`lines`] will always expand the
+/// out to semantic boundaries as the selection point changes. Similarly, [`lines`] will always
-/// new point to encompass entire lines.
+/// expand the new point to encompass entire lines.
 ///
 /// [`simple`]: enum.Selection.html#method.simple
+/// [`block`]: enum.Selection.html#method.block
 /// [`semantic`]: enum.Selection.html#method.semantic
 /// [`lines`]: enum.Selection.html#method.lines
 /// [`update`]: enum.Selection.html#method.update
 #[derive(Debug, Clone, PartialEq)]
-pub enum Selection {
+pub struct Selection {
-    Simple {
+    region: Range<Anchor>,
-        /// The region representing start and end of cursor movement.
+    ty: SelectionType,
-        region: Range<Anchor>,
-    },
-    Block {
-        /// The region representing start and end of cursor movement.
-        region: Range<Anchor>,
-    },
-    Semantic {
-        /// The region representing start and end of cursor movement.
-        region: Range<Point<usize>>,
-    },
-    Lines {
-        /// The region representing start and end of cursor movement.
-        region: Range<Point<usize>>,
-    },
 }
 
 impl Selection {
     pub fn simple(location: Point<usize>, side: Side) -> Selection {
-        Selection::Simple {
+        Self {
             region: Range { start: Anchor::new(location, side), end: Anchor::new(location, side) },
+            ty: SelectionType::Simple,
         }
     }
 
     pub fn block(location: Point<usize>, side: Side) -> Selection {
-        Selection::Block {
+        Self {
             region: Range { start: Anchor::new(location, side), end: Anchor::new(location, side) },
+            ty: SelectionType::Block,
         }
     }
 
-    pub fn semantic(point: Point<usize>) -> Selection {
+    pub fn semantic(location: Point<usize>) -> Selection {
-        Selection::Semantic { region: Range { start: point, end: point } }
+        Self {
+            region: Range {
+                start: Anchor::new(location, Side::Left),
+                end: Anchor::new(location, Side::Right),
+            },
+            ty: SelectionType::Semantic,
+        }
     }
 
-    pub fn lines(point: Point<usize>) -> Selection {
+    pub fn lines(location: Point<usize>) -> Selection {
-        Selection::Lines { region: Range { start: point, end: point } }
+        Self {
+            region: Range {
+                start: Anchor::new(location, Side::Left),
+                end: Anchor::new(location, Side::Right),
+            },
+            ty: SelectionType::Lines,
+        }
     }
 
     pub fn update(&mut self, location: Point<usize>, side: Side) {
-        let (_, end) = self.points_mut();
+        self.region.end.point = location;
-        *end = location;
+        self.region.end.side = side;
-
-        if let Some((_, end_side)) = self.sides_mut() {
-            *end_side = side;
-        }
     }
 
-    pub fn rotate(&mut self, offset: isize) {
+    pub fn rotate(
-        let (start, end) = self.points_mut();
+        mut self,
-        start.line = usize::try_from(start.line as isize + offset).unwrap_or(0);
+        num_lines: usize,
-        end.line = usize::try_from(end.line as isize + offset).unwrap_or(0);
+        num_cols: usize,
+        scrolling_region: &Range<Line>,
+        offset: isize,
+    ) -> Option<Selection> {
+        // Convert scrolling region from viewport to buffer coordinates
+        let region_start = num_lines - scrolling_region.start.0;
+        let region_end = num_lines - scrolling_region.end.0;
+
+        let (mut start, mut end) = (&mut self.region.start, &mut self.region.end);
+        if Self::points_need_swap(start.point, end.point) {
+            mem::swap(&mut start, &mut end);
+        }
+
+        // Rotate start of selection
+        if (start.point.line < region_start || region_start == num_lines)
+            && start.point.line >= region_end
+        {
+            start.point.line = usize::try_from(start.point.line as isize + offset).unwrap_or(0);
+
+            // If end is within the same region, delete selection once start rotates out
+            if start.point.line < region_end && end.point.line >= region_end {
+                return None;
+            }
+
+            // Clamp selection to start of region
+            if start.point.line >= region_start && region_start != num_lines {
+                if self.ty != SelectionType::Block {
+                    start.point.col = Column(0);
+                    start.side = Side::Left;
+                }
+                start.point.line = region_start - 1;
+            }
+        }
+
+        // Rotate end of selection
+        if (end.point.line < region_start || region_start == num_lines)
+            && end.point.line >= region_end
+        {
+            end.point.line = usize::try_from(end.point.line as isize + offset).unwrap_or(0);
+
+            // Delete selection if end has overtaken the start
+            if end.point.line > start.point.line {
+                return None;
+            }
+
+            // Clamp selection to end of region
+            if end.point.line < region_end {
+                if self.ty != SelectionType::Block {
+                    end.point.col = Column(num_cols - 1);
+                    end.side = Side::Right;
+                }
+                end.point.line = region_end;
+            }
+        }
+
+        Some(self)
     }
 
     pub fn is_empty(&self) -> bool {
-        match self {
+        match self.ty {
-            Selection::Simple { ref region } => {
+            SelectionType::Simple => {
-                let (start, end) =
+                let (mut start, mut end) = (self.region.start, self.region.end);
-                    if Selection::points_need_swap(region.start.point, region.end.point) {
+                if Selection::points_need_swap(start.point, end.point) {
-                        (&region.end, &region.start)
+                    mem::swap(&mut start, &mut end);
-                    } else {
+                }
-                        (&region.start, &region.end)
-                    };
 
                 // Simple selection is empty when the points are identical
                 // or two adjacent cells have the sides right -> left
@@ -156,7 +216,9 @@ impl Selection {
                         && (start.point.line == end.point.line)
                         && start.point.col + 1 == end.point.col)
             },
-            Selection::Block { region: Range { ref start, ref end } } => {
+            SelectionType::Block => {
+                let (start, end) = (self.region.start, self.region.end);
+
                 // Block selection is empty when the points' columns and sides are identical
                 // or two cells with adjacent columns have the sides right -> left,
                 // regardless of their lines
@@ -168,7 +230,7 @@ impl Selection {
                         && start.side == Side::Left
                         && end.side == Side::Right)
             },
-            Selection::Semantic { .. } | Selection::Lines { .. } => false,
+            SelectionType::Semantic | SelectionType::Lines => false,
         }
     }
 
@@ -177,29 +239,21 @@ impl Selection {
         let grid = term.grid();
         let num_cols = grid.num_cols();
 
-        // Get selection boundaries
-        let points = self.points();
-        let (start, end) = (*points.0, *points.1);
-
-        // Get selection sides, falling back to `Side::Left` if it will not be used
-        let sides = self.sides().unwrap_or((&Side::Left, &Side::Left));
-        let (start_side, end_side) = (*sides.0, *sides.1);
-
         // Order start above the end
-        let (start, end) = if Self::points_need_swap(start, end) {
+        let (mut start, mut end) = (self.region.start, self.region.end);
-            (Anchor { point: end, side: end_side }, Anchor { point: start, side: start_side })
+        if Self::points_need_swap(start.point, end.point) {
-        } else {
+            mem::swap(&mut start, &mut end);
-            (Anchor { point: start, side: start_side }, Anchor { point: end, side: end_side })
+        }
-        };
 
         // Clamp to inside the grid buffer
-        let (start, end) = Self::grid_clamp(start, end, self.is_block(), grid.len()).ok()?;
+        let is_block = self.ty == SelectionType::Block;
+        let (start, end) = Self::grid_clamp(start, end, is_block, grid.len()).ok()?;
 
-        let range = match self {
+        let range = match self.ty {
-            Self::Simple { .. } => self.range_simple(start, end, num_cols),
+            SelectionType::Simple => self.range_simple(start, end, num_cols),
-            Self::Block { .. } => self.range_block(start, end),
+            SelectionType::Block => self.range_block(start, end),
-            Self::Semantic { .. } => Self::range_semantic(term, start.point, end.point),
+            SelectionType::Semantic => Self::range_semantic(term, start.point, end.point),
-            Self::Lines { .. } => Self::range_lines(term, start.point, end.point),
+            SelectionType::Lines => Self::range_lines(term, start.point, end.point),
         };
 
         // Expand selection across fullwidth cells
@@ -376,53 +430,6 @@ impl Selection {
 
         Some(SelectionRange { start: start.point, end: end.point, is_block: true })
     }
-
-    fn points(&self) -> (&Point<usize>, &Point<usize>) {
-        match self {
-            Self::Simple { ref region } | Self::Block { ref region } => {
-                (&region.start.point, &region.end.point)
-            },
-            Self::Semantic { ref region } | Self::Lines { ref region } => {
-                (&region.start, &region.end)
-            },
-        }
-    }
-
-    fn points_mut(&mut self) -> (&mut Point<usize>, &mut Point<usize>) {
-        match self {
-            Self::Simple { ref mut region } | Self::Block { ref mut region } => {
-                (&mut region.start.point, &mut region.end.point)
-            },
-            Self::Semantic { ref mut region } | Self::Lines { ref mut region } => {
-                (&mut region.start, &mut region.end)
-            },
-        }
-    }
-
-    fn sides(&self) -> Option<(&Side, &Side)> {
-        match self {
-            Self::Simple { ref region } | Self::Block { ref region } => {
-                Some((&region.start.side, &region.end.side))
-            },
-            Self::Semantic { .. } | Self::Lines { .. } => None,
-        }
-    }
-
-    fn sides_mut(&mut self) -> Option<(&mut Side, &mut Side)> {
-        match self {
-            Self::Simple { ref mut region } | Self::Block { ref mut region } => {
-                Some((&mut region.start.side, &mut region.end.side))
-            },
-            Self::Semantic { .. } | Self::Lines { .. } => None,
-        }
-    }
-
-    fn is_block(&self) -> bool {
-        match self {
-            Self::Block { .. } => true,
-            _ => false,
-        }
-    }
 }
 
 /// Tests for selection.
@@ -572,11 +579,13 @@ mod test {
 
     #[test]
     fn line_selection() {
+        let num_lines = 10;
+        let num_cols = 5;
         let mut selection = Selection::lines(Point::new(0, Column(1)));
         selection.update(Point::new(5, Column(1)), Side::Right);
-        selection.rotate(7);
+        selection = selection.rotate(num_lines, num_cols, &(Line(0)..Line(num_lines)), 7).unwrap();
 
-        assert_eq!(selection.to_range(&term(5, 10)).unwrap(), SelectionRange {
+        assert_eq!(selection.to_range(&term(num_cols, num_lines)).unwrap(), SelectionRange {
             start: Point::new(9, Column(0)),
             end: Point::new(7, Column(4)),
             is_block: false,
@@ -585,11 +594,13 @@ mod test {
 
     #[test]
     fn semantic_selection() {
+        let num_lines = 10;
+        let num_cols = 5;
         let mut selection = Selection::semantic(Point::new(0, Column(3)));
         selection.update(Point::new(5, Column(1)), Side::Right);
-        selection.rotate(7);
+        selection = selection.rotate(num_lines, num_cols, &(Line(0)..Line(num_lines)), 7).unwrap();
 
-        assert_eq!(selection.to_range(&term(5, 10)).unwrap(), SelectionRange {
+        assert_eq!(selection.to_range(&term(num_cols, num_lines)).unwrap(), SelectionRange {
             start: Point::new(9, Column(0)),
             end: Point::new(7, Column(3)),
             is_block: false,
@@ -598,11 +609,13 @@ mod test {
 
     #[test]
     fn simple_selection() {
+        let num_lines = 10;
+        let num_cols = 5;
         let mut selection = Selection::simple(Point::new(0, Column(3)), Side::Right);
         selection.update(Point::new(5, Column(1)), Side::Right);
-        selection.rotate(7);
+        selection = selection.rotate(num_lines, num_cols, &(Line(0)..Line(num_lines)), 7).unwrap();
 
-        assert_eq!(selection.to_range(&term(5, 10)).unwrap(), SelectionRange {
+        assert_eq!(selection.to_range(&term(num_cols, num_lines)).unwrap(), SelectionRange {
             start: Point::new(9, Column(0)),
             end: Point::new(7, Column(3)),
             is_block: false,
@@ -611,11 +624,13 @@ mod test {
 
     #[test]
     fn block_selection() {
+        let num_lines = 10;
+        let num_cols = 5;
         let mut selection = Selection::block(Point::new(0, Column(3)), Side::Right);
         selection.update(Point::new(5, Column(1)), Side::Right);
-        selection.rotate(7);
+        selection = selection.rotate(num_lines, num_cols, &(Line(0)..Line(num_lines)), 7).unwrap();
 
-        assert_eq!(selection.to_range(&term(5, 10)).unwrap(), SelectionRange {
+        assert_eq!(selection.to_range(&term(num_cols, num_lines)).unwrap(), SelectionRange {
             start: Point::new(9, Column(2)),
             end: Point::new(7, Column(3)),
             is_block: true
@@ -667,4 +682,52 @@ mod test {
         selection.update(Point::new(1, Column(1)), Side::Right);
         assert!(!selection.is_empty());
     }
+
+    #[test]
+    fn rotate_in_region_up() {
+        let num_lines = 10;
+        let num_cols = 5;
+        let mut selection = Selection::simple(Point::new(2, Column(3)), Side::Right);
+        selection.update(Point::new(5, Column(1)), Side::Right);
+        selection =
+            selection.rotate(num_lines, num_cols, &(Line(1)..Line(num_lines - 1)), 4).unwrap();
+
+        assert_eq!(selection.to_range(&term(num_cols, num_lines)).unwrap(), SelectionRange {
+            start: Point::new(8, Column(0)),
+            end: Point::new(6, Column(3)),
+            is_block: false,
+        });
+    }
+
+    #[test]
+    fn rotate_in_region_down() {
+        let num_lines = 10;
+        let num_cols = 5;
+        let mut selection = Selection::simple(Point::new(5, Column(3)), Side::Right);
+        selection.update(Point::new(8, Column(1)), Side::Left);
+        selection =
+            selection.rotate(num_lines, num_cols, &(Line(1)..Line(num_lines - 1)), -5).unwrap();
+
+        assert_eq!(selection.to_range(&term(num_cols, num_lines)).unwrap(), SelectionRange {
+            start: Point::new(3, Column(1)),
+            end: Point::new(1, Column(num_cols - 1)),
+            is_block: false,
+        });
+    }
+
+    #[test]
+    fn rotate_in_region_up_block() {
+        let num_lines = 10;
+        let num_cols = 5;
+        let mut selection = Selection::block(Point::new(2, Column(3)), Side::Right);
+        selection.update(Point::new(5, Column(1)), Side::Right);
+        selection =
+            selection.rotate(num_lines, num_cols, &(Line(1)..Line(num_lines - 1)), 4).unwrap();
+
+        assert_eq!(selection.to_range(&term(num_cols, num_lines)).unwrap(), SelectionRange {
+            start: Point::new(8, Column(2)),
+            end: Point::new(6, Column(3)),
+            is_block: true,
+        });
+    }
 }