Fix vi cursor motion with ScrollPage* actions

This fixes the regression that vi cursor doesn't move to appropriate
position to emulate vi/vim after invokes `ScrollPage*`.

To emulate vi/vim the cursor should move up/down some lines if the
viewport on topmost scrollback buffer or on bottommost one when invokes
`ScrollPage*` action. Otherwise the cursor should look like no movement
relatively on viewport.

CHANGELOG.md

@@ -16,7 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 - Regression in rendering performance with dense grids since 0.6.0
 - Crash/Freezes with partially visible fullwidth characters due to alt screen resize
-- Incorrect vi cursor position after invoking `ScrollPageHalfUp` action
+- Incorrect vi cursor position after invoking `ScrollPage*` action
 - Slow PTY read performance with extremely dense grids
 - Crash when resizing during vi mode
 

alacritty_terminal/src/term/mod.rs

@@ -1932,6 +1932,62 @@ mod tests {
     use crate::selection::{Selection, SelectionType};
     use crate::term::cell::{Cell, Flags};
 
+    #[test]
+    fn scroll_display_page_up() {
+        let size = SizeInfo::new(5., 10., 1.0, 1.0, 0.0, 0.0, false);
+        let mut term = Term::new(&MockConfig::default(), size, ());
+
+        // Create 11 lines of scrollback.
+        for _ in 0..20 {
+            term.newline();
+        }
+
+        // Scrollable amount to top is 11.
+        term.scroll_display(Scroll::PageUp);
+        assert_eq!(term.vi_mode_cursor.point, Point::new(Line(-1), Column(0)));
+        assert_eq!(term.grid.display_offset(), 10);
+
+        // Scrollable amount to top is 1.
+        term.scroll_display(Scroll::PageUp);
+        assert_eq!(term.vi_mode_cursor.point, Point::new(Line(-2), Column(0)));
+        assert_eq!(term.grid.display_offset(), 11);
+
+        // Scrollable amount to top is 0.
+        term.scroll_display(Scroll::PageUp);
+        assert_eq!(term.vi_mode_cursor.point, Point::new(Line(-2), Column(0)));
+        assert_eq!(term.grid.display_offset(), 11);
+    }
+
+    #[test]
+    fn scroll_display_page_down() {
+        let size = SizeInfo::new(5., 10., 1.0, 1.0, 0.0, 0.0, false);
+        let mut term = Term::new(&MockConfig::default(), size, ());
+
+        // Create 11 lines of scrollback.
+        for _ in 0..20 {
+            term.newline();
+        }
+
+        // Change display_offset to topmost.
+        term.grid_mut().scroll_display(Scroll::Top);
+        term.vi_mode_cursor = ViModeCursor::new(Point::new(Line(-11), Column(0)));
+
+        // Scrollable amount to bottom is 11.
+        term.scroll_display(Scroll::PageDown);
+        assert_eq!(term.vi_mode_cursor.point, Point::new(Line(-1), Column(0)));
+        assert_eq!(term.grid.display_offset(), 1);
+
+        // Scrollable amount to bottom is 1.
+        term.scroll_display(Scroll::PageDown);
+        assert_eq!(term.vi_mode_cursor.point, Point::new(Line(0), Column(0)));
+        assert_eq!(term.grid.display_offset(), 0);
+
+        // Scrollable amount to bottom is 0.
+        term.scroll_display(Scroll::PageDown);
+        assert_eq!(term.vi_mode_cursor.point, Point::new(Line(0), Column(0)));
+        assert_eq!(term.grid.display_offset(), 0);
+    }
+
     #[test]
     fn semantic_selection_works() {
         let size = SizeInfo::new(5., 3., 1.0, 1.0, 0.0, 0.0, false);

alacritty_terminal/src/vi_mode.rs

@@ -1,4 +1,4 @@
-use std::cmp::{max, min};
+use std::cmp::min;
 
 use alacritty_config_derive::ConfigDeserialize;
 
@@ -160,21 +160,11 @@ impl ViModeCursor {
     /// Get target cursor point for vim-like page movement.
     #[must_use = "this returns the result of the operation, without modifying the original"]
     pub fn scroll<T: EventListener>(mut self, term: &Term<T>, lines: i32) -> Self {
-        // Check number of lines the cursor needs to be moved.
-        let overscroll = if lines > 0 {
-            let max_scroll = term.history_size() - term.grid().display_offset();
-            max(0, lines - max_scroll as i32)
-        } else {
-            let max_scroll = term.grid().display_offset();
-            min(0, lines + max_scroll as i32)
-        };
-
         // Clamp movement to within visible region.
-        let line = (self.point.line - overscroll).grid_clamp(term, Boundary::Grid);
+        let line = (self.point.line - lines).grid_clamp(term, Boundary::Grid);
 
         // Find the first occupied cell after scrolling has been performed.
-        let target_line = (self.point.line - lines).grid_clamp(term, Boundary::Grid);
-        let column = first_occupied_in_line(term, target_line).unwrap_or_default().column;
+        let column = first_occupied_in_line(term, line).unwrap_or_default().column;
 
         // Move cursor.
         self.point = Point::new(line, column);
@@ -388,6 +378,7 @@ fn is_boundary<T>(term: &Term<T>, point: Point, direction: Direction) -> bool {
 mod tests {
     use super::*;
 
+    use crate::ansi::Handler;
     use crate::config::MockConfig;
     use crate::index::{Column, Line};
     use crate::term::{SizeInfo, Term};
@@ -756,4 +747,73 @@ mod tests {
         cursor = cursor.motion(&mut term, ViMotion::WordLeft);
         assert_eq!(cursor.point, Point::new(Line(0), Column(0)));
     }
+
+    #[test]
+    fn scroll_simple() {
+        let mut term = term();
+
+        // Create 1 line of scrollback.
+        for _ in 0..20 {
+            term.newline();
+        }
+
+        let mut cursor = ViModeCursor::new(Point::new(Line(0), Column(0)));
+
+        cursor = cursor.scroll(&term, -1);
+        assert_eq!(cursor.point, Point::new(Line(1), Column(0)));
+
+        cursor = cursor.scroll(&term, 1);
+        assert_eq!(cursor.point, Point::new(Line(0), Column(0)));
+
+        cursor = cursor.scroll(&term, 1);
+        assert_eq!(cursor.point, Point::new(Line(-1), Column(0)));
+    }
+
+    #[test]
+    fn scroll_over_top() {
+        let mut term = term();
+
+        // Create 40 lines of scrollback.
+        for _ in 0..59 {
+            term.newline();
+        }
+
+        let mut cursor = ViModeCursor::new(Point::new(Line(19), Column(0)));
+
+        cursor = cursor.scroll(&term, 20);
+        assert_eq!(cursor.point, Point::new(Line(-1), Column(0)));
+
+        cursor = cursor.scroll(&term, 20);
+        assert_eq!(cursor.point, Point::new(Line(-21), Column(0)));
+
+        cursor = cursor.scroll(&term, 20);
+        assert_eq!(cursor.point, Point::new(Line(-40), Column(0)));
+
+        cursor = cursor.scroll(&term, 20);
+        assert_eq!(cursor.point, Point::new(Line(-40), Column(0)));
+    }
+
+    #[test]
+    fn scroll_over_bottom() {
+        let mut term = term();
+
+        // Create 40 lines of scrollback.
+        for _ in 0..59 {
+            term.newline();
+        }
+
+        let mut cursor = ViModeCursor::new(Point::new(Line(-40), Column(0)));
+
+        cursor = cursor.scroll(&term, -20);
+        assert_eq!(cursor.point, Point::new(Line(-20), Column(0)));
+
+        cursor = cursor.scroll(&term, -20);
+        assert_eq!(cursor.point, Point::new(Line(0), Column(0)));
+
+        cursor = cursor.scroll(&term, -20);
+        assert_eq!(cursor.point, Point::new(Line(19), Column(0)));
+
+        cursor = cursor.scroll(&term, -20);
+        assert_eq!(cursor.point, Point::new(Line(19), Column(0)));
+    }
 }