Fix Vi cursor not being damaged on scroll

There's no need to damage intermediate Vi mode cursor points, since it
can't change the terminal content meaning that only the previous
and current vi cursor's viewport points matter to damage it properly.

alacritty/src/display/content.rs

@@ -11,12 +11,12 @@ use alacritty_terminal::selection::SelectionRange;
 use alacritty_terminal::term::cell::{Cell, Flags};
 use alacritty_terminal::term::color::{CellRgb, Rgb};
 use alacritty_terminal::term::search::{Match, RegexIter, RegexSearch};
-use alacritty_terminal::term::{RenderableContent as TerminalContent, Term, TermMode};
+use alacritty_terminal::term::{self, RenderableContent as TerminalContent, Term, TermMode};
 
 use crate::config::UiConfig;
 use crate::display::color::{List, DIM_FACTOR};
 use crate::display::hint::HintState;
-use crate::display::{self, Display, MAX_SEARCH_LINES};
+use crate::display::{Display, MAX_SEARCH_LINES};
 use crate::event::SearchState;
 
 /// Minimum contrast between a fixed cursor color and the cell's background.
@@ -63,7 +63,7 @@ impl<'a> RenderableContent<'a> {
         // Convert terminal cursor point to viewport position.
         let cursor_point = terminal_content.cursor.point;
         let display_offset = terminal_content.display_offset;
-        let cursor_point = display::point_to_viewport(display_offset, cursor_point).unwrap();
+        let cursor_point = term::point_to_viewport(display_offset, cursor_point).unwrap();
 
         let hint = if display.hint_state.active() {
             display.hint_state.update_matches(term);
@@ -250,7 +250,7 @@ impl RenderableCell {
 
         // Convert cell point to viewport position.
         let cell_point = cell.point;
-        let point = display::point_to_viewport(display_offset, cell_point).unwrap();
+        let point = term::point_to_viewport(display_offset, cell_point).unwrap();
 
         let flags = cell.flags;
         let underline = cell

alacritty/src/display/mod.rs

@@ -1,7 +1,6 @@
 //! The display subsystem including window management, font rasterization, and
 //! GPU drawing.
 
-use std::convert::TryFrom;
 use std::fmt::{self, Formatter};
 #[cfg(all(feature = "wayland", not(any(target_os = "macos", windows))))]
 use std::sync::atomic::Ordering;
@@ -31,7 +30,7 @@ use alacritty_terminal::index::{Column, Direction, Line, Point};
 use alacritty_terminal::selection::{Selection, SelectionRange};
 use alacritty_terminal::term::cell::Flags;
 use alacritty_terminal::term::color::Rgb;
-use alacritty_terminal::term::{Term, TermDamage, TermMode, MIN_COLUMNS, MIN_SCREEN_LINES};
+use alacritty_terminal::term::{self, Term, TermDamage, TermMode, MIN_COLUMNS, MIN_SCREEN_LINES};
 
 use crate::config::font::Font;
 #[cfg(not(windows))]
@@ -747,7 +746,7 @@ impl Display {
                 glyph_cache,
                 grid_cells.into_iter().map(|mut cell| {
                     // Underline hints hovered by mouse or vi mode cursor.
-                    let point = viewport_to_point(display_offset, cell.point);
+                    let point = term::viewport_to_point(display_offset, cell.point);
                     if highlighted_hint.as_ref().map_or(false, |h| h.bounds.contains(&point))
                         || vi_highlighted_hint.as_ref().map_or(false, |h| h.bounds.contains(&point))
                     {
@@ -1064,7 +1063,7 @@ impl Display {
         let display_offset = terminal.grid().display_offset();
         for hint in self.highlighted_hint.iter().chain(&self.vi_highlighted_hint) {
             for point in (hint.bounds.start().line.0..=hint.bounds.end().line.0).flat_map(|line| {
-                point_to_viewport(display_offset, Point::new(Line(line), Column(0)))
+                term::point_to_viewport(display_offset, Point::new(Line(line), Column(0)))
             }) {
                 terminal.damage_line(point.line, 0, terminal.columns() - 1);
             }
@@ -1121,20 +1120,6 @@ impl Drop for Display {
     }
 }
 
-/// Convert a terminal point to a viewport relative point.
-#[inline]
-pub fn point_to_viewport(display_offset: usize, point: Point) -> Option<Point<usize>> {
-    let viewport_line = point.line.0 + display_offset as i32;
-    usize::try_from(viewport_line).ok().map(|line| Point::new(line, point.column))
-}
-
-/// Convert a viewport relative point to a terminal point.
-#[inline]
-pub fn viewport_to_point(display_offset: usize, point: Point<usize>) -> Point {
-    let line = Line(point.line as i32) - display_offset;
-    Point::new(line, point.column)
-}
-
 /// Calculate the cell dimensions based on font metrics.
 ///
 /// This will return a tuple of the cell width and height.

alacritty/src/event.rs

@@ -32,7 +32,7 @@ use alacritty_terminal::grid::{Dimensions, Scroll};
 use alacritty_terminal::index::{Boundary, Column, Direction, Line, Point, Side};
 use alacritty_terminal::selection::{Selection, SelectionType};
 use alacritty_terminal::term::search::{Match, RegexSearch};
-use alacritty_terminal::term::{ClipboardType, Term, TermMode};
+use alacritty_terminal::term::{self, ClipboardType, Term, TermMode};
 
 use crate::cli::{Options as CliOptions, WindowOptions};
 use crate::clipboard::Clipboard;
@@ -43,7 +43,7 @@ use crate::daemon::foreground_process_path;
 use crate::daemon::spawn_daemon;
 use crate::display::hint::HintMatch;
 use crate::display::window::Window;
-use crate::display::{self, Display, SizeInfo};
+use crate::display::{Display, SizeInfo};
 use crate::input::{self, ActionContext as _, FONT_SIZE_STEP};
 use crate::message_bar::{Message, MessageBuffer};
 use crate::scheduler::{Scheduler, TimerId, Topic};
@@ -778,8 +778,7 @@ impl<'a, N: Notify + 'a, T: EventListener> input::ActionContext<T> for ActionCon
             if self.search_state.dfas.take().is_some() {
                 self.terminal.mark_fully_damaged();
             } else {
-                // Damage line indicator and Vi cursor.
-                self.terminal.damage_vi_cursor();
+                // Damage line indicator.
                 self.terminal.damage_line(0, 0, self.terminal.columns() - 1);
             }
         } else {
@@ -1021,7 +1020,7 @@ impl Mouse {
         let line = self.y.saturating_sub(size.padding_y() as usize) / (size.cell_height() as usize);
         let line = min(line, size.bottommost_line().0 as usize);
 
-        display::viewport_to_point(display_offset, Point::new(line, col))
+        term::viewport_to_point(display_offset, Point::new(line, col))
     }
 }
 

alacritty_terminal/src/term/mod.rs

@@ -73,6 +73,20 @@ impl Default for TermMode {
     }
 }
 
+/// Convert a terminal point to a viewport relative point.
+#[inline]
+pub fn point_to_viewport(display_offset: usize, point: Point) -> Option<Point<usize>> {
+    let viewport_line = point.line.0 + display_offset as i32;
+    usize::try_from(viewport_line).ok().map(|line| Point::new(line, point.column))
+}
+
+/// Convert a viewport relative point to a terminal point.
+#[inline]
+pub fn viewport_to_point(display_offset: usize, point: Point<usize>) -> Point {
+    let line = Line(point.line as i32) - display_offset;
+    Point::new(line, point.column)
+}
+
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct LineDamageBounds {
     /// Damaged line number.
@@ -149,6 +163,9 @@ struct TermDamageState {
     /// Old terminal cursor point.
     last_cursor: Point,
 
+    /// Last Vi cursor point.
+    last_vi_cursor_point: Option<Point<usize>>,
+
     /// Old selection range.
     last_selection: Option<SelectionRange>,
 }
@@ -162,6 +179,7 @@ impl TermDamageState {
             is_fully_damaged: true,
             lines,
             last_cursor: Default::default(),
+            last_vi_cursor_point: Default::default(),
             last_selection: Default::default(),
         }
     }
@@ -170,6 +188,7 @@ impl TermDamageState {
     fn resize(&mut self, num_cols: usize, num_lines: usize) {
         // Reset point, so old cursor won't end up outside of the viewport.
         self.last_cursor = Default::default();
+        self.last_vi_cursor_point = None;
         self.last_selection = None;
         self.is_fully_damaged = true;
 
@@ -373,13 +392,23 @@ impl<T> Term<T> {
         self.damage_cursor();
         self.damage.last_cursor = self.grid.cursor.point;
 
+        // Vi mode doesn't update the terminal content, thus only last vi cursor position and the
+        // new one should be damaged.
+        if let Some(last_vi_cursor_point) = self.damage.last_vi_cursor_point.take() {
+            self.damage.damage_point(last_vi_cursor_point)
+        }
+
+        let display_offset = self.grid().display_offset();
+
         // Damage Vi cursor if it's present.
         if self.mode.contains(TermMode::VI) {
-            self.damage_vi_cursor();
+            let vi_cursor_point =
+                point_to_viewport(display_offset, self.vi_mode_cursor.point).unwrap();
+            self.damage.last_vi_cursor_point = Some(vi_cursor_point);
+            self.damage.damage_point(vi_cursor_point);
         }
 
         if self.damage.last_selection != selection {
-            let display_offset = self.grid().display_offset();
             for selection in self.damage.last_selection.into_iter().chain(selection) {
                 self.damage.damage_selection(selection, display_offset, self.columns());
             }
@@ -749,9 +778,7 @@ impl<T> Term<T> {
         }
 
         // Move cursor.
-        self.damage_vi_cursor();
         self.vi_mode_cursor = self.vi_mode_cursor.motion(self, motion);
-        self.damage_vi_cursor();
         self.vi_mode_recompute_selection();
     }
 
@@ -761,15 +788,13 @@ impl<T> Term<T> {
     where
         T: EventListener,
     {
-        self.damage_vi_cursor();
         // Move viewport to make point visible.
         self.scroll_to_point(point);
 
         // Move vi cursor to the point.
         self.vi_mode_cursor.point = point;
 
         self.vi_mode_recompute_selection();
-        self.damage_vi_cursor();
     }
 
     /// Update the active selection to match the vi mode cursor position.
@@ -926,15 +951,6 @@ impl<T> Term<T> {
             Point::new(self.grid.cursor.point.line.0 as usize, self.grid.cursor.point.column);
         self.damage.damage_point(point);
     }
-
-    /// Damage `Vi` mode cursor.
-    #[inline]
-    pub fn damage_vi_cursor(&mut self) {
-        let line = (self.grid.display_offset() as i32 + self.vi_mode_cursor.point.line.0)
-            .clamp(0, self.screen_lines() as i32 - 1) as usize;
-        let vi_point = Point::new(line, self.vi_mode_cursor.point.column);
-        self.damage.damage_point(vi_point);
-    }
 }
 
 impl<T> Dimensions for Term<T> {
@@ -2698,6 +2714,19 @@ mod tests {
         let line = vi_cursor_point.line.0 as usize;
         let left = vi_cursor_point.column.0 as usize;
         let right = left;
+
+        let mut damaged_lines = match term.damage(None) {
+            TermDamage::Full => panic!("Expected partial damage, however got Full"),
+            TermDamage::Partial(damaged_lines) => damaged_lines,
+        };
+        // Skip cursor damage information, since we're just testing Vi cursor.
+        damaged_lines.next();
+        assert_eq!(damaged_lines.next(), Some(LineDamageBounds { line, left, right }));
+        assert_eq!(damaged_lines.next(), None);
+
+        // Ensure that old Vi cursor got damaged as well.
+        term.reset_damage();
+        term.toggle_vi_mode();
         let mut damaged_lines = match term.damage(None) {
             TermDamage::Full => panic!("Expected partial damage, however got Full"),
             TermDamage::Partial(damaged_lines) => damaged_lines,
@@ -2806,38 +2835,6 @@ mod tests {
         assert_eq!(term.damage.lines[6], LineDamageBounds { line: 6, left: 8, right: 8 });
     }
 
-    #[test]
-    fn damage_vi_movements() {
-        let size = TermSize::new(10, 10);
-        let mut term = Term::new(&Config::default(), &size, ());
-        let num_cols = term.columns();
-        // Reset terminal for partial damage tests since it's initialized as fully damaged.
-        term.reset_damage();
-
-        // Enable Vi mode.
-        term.toggle_vi_mode();
-
-        // NOTE While we can use `[Term::damage]` to access terminal damage information, in the
-        // following tests we will be accessing `term.damage.lines` directly to avoid adding extra
-        // damage information (like cursor and Vi cursor), which we're not testing.
-
-        term.vi_goto_point(Point::new(Line(5), Column(5)));
-        assert_eq!(term.damage.lines[0], LineDamageBounds { line: 0, left: 0, right: 0 });
-        assert_eq!(term.damage.lines[5], LineDamageBounds { line: 5, left: 5, right: 5 });
-        term.damage.reset(num_cols);
-
-        term.vi_motion(ViMotion::Up);
-        term.vi_motion(ViMotion::Right);
-        term.vi_motion(ViMotion::Up);
-        term.vi_motion(ViMotion::Left);
-        assert_eq!(term.damage.lines[3], LineDamageBounds { line: 3, left: 5, right: 6 });
-        assert_eq!(term.damage.lines[4], LineDamageBounds { line: 4, left: 5, right: 6 });
-        assert_eq!(term.damage.lines[5], LineDamageBounds { line: 5, left: 5, right: 5 });
-
-        // Ensure that we haven't damaged entire terminal during the test.
-        assert!(!term.damage.is_fully_damaged);
-    }
-
     #[test]
     fn full_damage() {
         let size = TermSize::new(100, 10);