From 63ef6c931901e895442edf5ec113d0ff609a7c24 Mon Sep 17 00:00:00 2001 From: Kirill Chibisov Date: Fri, 27 May 2022 00:30:33 +0300 Subject: [PATCH] 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 | 8 +-- alacritty/src/display/mod.rs | 21 +------ alacritty/src/event.rs | 9 ++- alacritty_terminal/src/term/mod.rs | 91 +++++++++++++++--------------- 4 files changed, 55 insertions(+), 74 deletions(-) diff --git a/alacritty/src/display/content.rs b/alacritty/src/display/content.rs index bb3c61a8ee..77571d94f1 100644 --- a/alacritty/src/display/content.rs +++ b/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 diff --git a/alacritty/src/display/mod.rs b/alacritty/src/display/mod.rs index 10ee65c092..a1788175fb 100644 --- a/alacritty/src/display/mod.rs +++ b/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> { - 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) -> 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. diff --git a/alacritty/src/event.rs b/alacritty/src/event.rs index 873e2a34c2..d16114afd1 100644 --- a/alacritty/src/event.rs +++ b/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 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)) } } diff --git a/alacritty_terminal/src/term/mod.rs b/alacritty_terminal/src/term/mod.rs index ffb4a1e894..48b62d6b3a 100644 --- a/alacritty_terminal/src/term/mod.rs +++ b/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> { + 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) -> 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>, + /// Old selection range. last_selection: Option, } @@ -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 Term { 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 Term { } // 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,7 +788,6 @@ impl Term { where T: EventListener, { - self.damage_vi_cursor(); // Move viewport to make point visible. self.scroll_to_point(point); @@ -769,7 +795,6 @@ impl Term { 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 Term { 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 Dimensions for Term { @@ -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);