From 6832b86aa7a9adb386394e1caaf373e65297be4e Mon Sep 17 00:00:00 2001 From: Christian Duerr Date: Fri, 7 Feb 2020 06:50:18 +0000 Subject: [PATCH] Fix selection expansion across full-width glyphs Instead of trying to expand the start and end of a selection across full-width glyphs, the selection should now only go from its origin to the end without any kind of expansion. Instead, the expansion is now done where the cells are actually checked for their selection status, expanding across the entire full-width glyph whenever any part of it is selected. Fixes #3106. --- CHANGELOG.md | 1 + alacritty/src/url.rs | 4 +- alacritty_terminal/src/index.rs | 28 ++++----- alacritty_terminal/src/selection.rs | 82 +------------------------ alacritty_terminal/src/term/mod.rs | 93 ++++++++++++++++++++++------- 5 files changed, 89 insertions(+), 119 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 202f0779..781c85e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Inconsistent fontconfig fallback - Backspace deleting characters while IME is open on macOS - Handling of OpenType variable fonts +- Expansion of block-selection on partially selected full-width glyphs ### Removed diff --git a/alacritty/src/url.rs b/alacritty/src/url.rs index 1840c22c..e538331d 100644 --- a/alacritty/src/url.rs +++ b/alacritty/src/url.rs @@ -42,7 +42,7 @@ impl Url { } pub fn end(&self) -> Point { - self.lines[self.lines.len() - 1].end.sub(self.num_cols, self.end_offset as usize, false) + self.lines[self.lines.len() - 1].end.sub(self.num_cols, self.end_offset as usize) } } @@ -83,7 +83,7 @@ impl Urls { let end = point; // Reset URL when empty cells have been skipped - if point != Point::default() && Some(point.sub(num_cols, 1, false)) != self.last_point { + if point != Point::default() && Some(point.sub(num_cols, 1)) != self.last_point { self.reset(); } diff --git a/alacritty_terminal/src/index.rs b/alacritty_terminal/src/index.rs index fb21baa0..51566d8d 100644 --- a/alacritty_terminal/src/index.rs +++ b/alacritty_terminal/src/index.rs @@ -44,33 +44,29 @@ impl Point { #[inline] #[must_use = "this returns the result of the operation, without modifying the original"] - pub fn sub(mut self, num_cols: usize, length: usize, absolute_indexing: bool) -> Point + pub fn sub(mut self, num_cols: usize, rhs: usize) -> Point where - L: Copy + Add + Sub, + L: Copy + Default + Into + Add + Sub, { - let line_changes = f32::ceil(length.saturating_sub(self.col.0) as f32 / num_cols as f32); - if absolute_indexing { - self.line = self.line + line_changes as usize; + let line_changes = + f32::ceil(rhs.saturating_sub(self.col.0) as f32 / num_cols as f32) as usize; + if self.line.into() > Line(line_changes) { + self.line = self.line - line_changes; } else { - self.line = self.line - line_changes as usize; + self.line = Default::default(); } - self.col = Column((num_cols + self.col.0 - length % num_cols) % num_cols); + self.col = Column((num_cols + self.col.0 - rhs % num_cols) % num_cols); self } #[inline] #[must_use = "this returns the result of the operation, without modifying the original"] - pub fn add(mut self, num_cols: usize, length: usize, absolute_indexing: bool) -> Point + pub fn add(mut self, num_cols: usize, rhs: usize) -> Point where - L: Copy + Add + Sub, + L: Add + Sub, { - let line_changes = (length + self.col.0) / num_cols; - if absolute_indexing { - self.line = self.line - line_changes; - } else { - self.line = self.line + line_changes; - } - self.col = Column((self.col.0 + length) % num_cols); + self.line = self.line + (rhs + self.col.0) / num_cols; + self.col = Column((self.col.0 + rhs) % num_cols); self } } diff --git a/alacritty_terminal/src/selection.rs b/alacritty_terminal/src/selection.rs index f2c98a55..333c31fb 100644 --- a/alacritty_terminal/src/selection.rs +++ b/alacritty_terminal/src/selection.rs @@ -23,7 +23,6 @@ use std::mem; use std::ops::Range; use crate::index::{Column, Line, Point, Side}; -use crate::term::cell::Flags; use crate::term::{Search, Term}; /// A Point and side within that point. @@ -249,67 +248,12 @@ impl Selection { let is_block = self.ty == SelectionType::Block; let (start, end) = Self::grid_clamp(start, end, is_block, grid.len()).ok()?; - let range = match self.ty { + match self.ty { SelectionType::Simple => self.range_simple(start, end, num_cols), SelectionType::Block => self.range_block(start, end), SelectionType::Semantic => Self::range_semantic(term, start.point, end.point), SelectionType::Lines => Self::range_lines(term, start.point, end.point), - }; - - // Expand selection across fullwidth cells - range.map(|range| Self::range_expand_fullwidth(term, range)) - } - - /// Expand the start/end of the selection range to account for fullwidth glyphs. - fn range_expand_fullwidth(term: &Term, mut range: SelectionRange) -> SelectionRange { - let grid = term.grid(); - let num_cols = grid.num_cols(); - - // Helper for checking if cell at `point` contains `flag` - let flag_at = |point: Point, flag: Flags| -> bool { - grid[point.line][point.col].flags.contains(flag) - }; - - // Include all double-width cells and placeholders at top left of selection - if range.start.col < num_cols { - // Expand from wide char spacer to wide char - if range.start.line + 1 != grid.len() || range.start.col.0 != 0 { - let prev = range.start.sub(num_cols.0, 1, true); - if flag_at(range.start, Flags::WIDE_CHAR_SPACER) && flag_at(prev, Flags::WIDE_CHAR) - { - range.start = prev; - } - } - - // Expand from wide char to wide char spacer for linewrapping - if range.start.line + 1 != grid.len() || range.start.col.0 != 0 { - let prev = range.start.sub(num_cols.0, 1, true); - if (prev.line + 1 != grid.len() || prev.col.0 != 0) - && flag_at(prev, Flags::WIDE_CHAR_SPACER) - && !flag_at(prev.sub(num_cols.0, 1, true), Flags::WIDE_CHAR) - { - range.start = prev; - } - } } - - // Include all double-width cells and placeholders at bottom right of selection - if range.end.line != 0 || range.end.col < num_cols { - // Expand from wide char spacer for linewrapping to wide char - if (range.end.line + 1 != grid.len() || range.end.col.0 != 0) - && flag_at(range.end, Flags::WIDE_CHAR_SPACER) - && !flag_at(range.end.sub(num_cols.0, 1, true), Flags::WIDE_CHAR) - { - range.end = range.end.add(num_cols.0, 1, true); - } - - // Expand from wide char to wide char spacer - if flag_at(range.end, Flags::WIDE_CHAR) { - range.end = range.end.add(num_cols.0, 1, true); - } - } - - range } // Bring start and end points in the correct order @@ -443,15 +387,11 @@ impl Selection { /// look like [ B] and [E ]. #[cfg(test)] mod tests { - use std::mem; - use super::{Selection, SelectionRange}; use crate::clipboard::Clipboard; use crate::config::MockConfig; use crate::event::{Event, EventListener}; - use crate::grid::Grid; use crate::index::{Column, Line, Point, Side}; - use crate::term::cell::{Cell, Flags}; use crate::term::{SizeInfo, Term}; struct Mock; @@ -637,26 +577,6 @@ mod tests { }); } - #[test] - fn double_width_expansion() { - let mut term = term(10, 1); - let mut grid = Grid::new(Line(1), Column(10), 0, Cell::default()); - grid[Line(0)][Column(0)].flags.insert(Flags::WIDE_CHAR); - grid[Line(0)][Column(1)].flags.insert(Flags::WIDE_CHAR_SPACER); - grid[Line(0)][Column(8)].flags.insert(Flags::WIDE_CHAR); - grid[Line(0)][Column(9)].flags.insert(Flags::WIDE_CHAR_SPACER); - mem::swap(term.grid_mut(), &mut grid); - - let mut selection = Selection::simple(Point::new(0, Column(1)), Side::Left); - selection.update(Point::new(0, Column(8)), Side::Right); - - assert_eq!(selection.to_range(&term).unwrap(), SelectionRange { - start: Point::new(0, Column(0)), - end: Point::new(0, Column(9)), - is_block: false, - }); - } - #[test] fn simple_is_empty() { let mut selection = Selection::simple(Point::new(0, Column(0)), Side::Right); diff --git a/alacritty_terminal/src/term/mod.rs b/alacritty_terminal/src/term/mod.rs index ddba07ef..57733bf5 100644 --- a/alacritty_terminal/src/term/mod.rs +++ b/alacritty_terminal/src/term/mod.rs @@ -281,6 +281,49 @@ impl<'a, C> RenderableCellsIter<'a, C> { cursor_style, } } + + /// Check selection state of a cell. + fn is_selected(&self, point: Point) -> bool { + let selection = match self.selection { + Some(selection) => selection, + None => return false, + }; + + // Point itself is selected + if selection.contains(point.col, point.line) { + return true; + } + + let num_cols = self.grid.num_cols().0; + let cell = self.grid[&point]; + + // Check if wide char's spacers are selected + if cell.flags.contains(Flags::WIDE_CHAR) { + let prevprev = point.sub(num_cols, 2); + let prev = point.sub(num_cols, 1); + let next = point.add(num_cols, 1); + + // Check trailing spacer + selection.contains(next.col, next.line) + // Check line-wrapping, leading spacer + || (self.grid[&prev].flags.contains(Flags::WIDE_CHAR_SPACER) + && !self.grid[&prevprev].flags.contains(Flags::WIDE_CHAR) + && selection.contains(prev.col, prev.line)) + } else if cell.flags.contains(Flags::WIDE_CHAR_SPACER) { + // Check if spacer's wide char is selected + let prev = point.sub(num_cols, 1); + + if self.grid[&prev].flags.contains(Flags::WIDE_CHAR) { + // Check previous cell for trailing spacer + self.is_selected(prev) + } else { + // Check next cell for line-wrapping, leading spacer + self.is_selected(point.add(num_cols, 1)) + } + } else { + false + } + } } #[derive(Copy, Clone, Debug)] @@ -414,11 +457,7 @@ impl<'a, C> Iterator for RenderableCellsIter<'a, C> { fn next(&mut self) -> Option { loop { if self.cursor_offset == self.inner.offset() && self.inner.column() == self.cursor.col { - let selected = self - .selection - .as_ref() - .map(|range| range.contains(self.cursor.col, self.cursor.line)) - .unwrap_or(false); + let selected = self.is_selected(Point::new(self.cursor.line, self.cursor.col)); // Handle cursor if let Some(cursor_key) = self.cursor_key.take() { @@ -458,11 +497,7 @@ impl<'a, C> Iterator for RenderableCellsIter<'a, C> { } else { let cell = self.inner.next()?; - let selected = self - .selection - .as_ref() - .map(|range| range.contains(cell.column, cell.line)) - .unwrap_or(false); + let selected = self.is_selected(Point::new(cell.line, cell.column)); if !cell.is_empty() || selected { return Some(RenderableCell::new(self.config, self.colors, cell, selected)); @@ -949,9 +984,9 @@ impl Term { if is_block { for line in (end.line + 1..=start.line).rev() { - res += &(self.line_to_string(line, start.col..end.col) + "\n"); + res += &(self.line_to_string(line, start.col..end.col, start.col.0 != 0) + "\n"); } - res += &self.line_to_string(end.line, start.col..end.col); + res += &self.line_to_string(end.line, start.col..end.col, true); } else { res = self.bounds_to_string(start, end); } @@ -967,23 +1002,31 @@ impl Term { let start_col = if line == start.line { start.col } else { Column(0) }; let end_col = if line == end.line { end.col } else { self.cols() - 1 }; - res += &self.line_to_string(line, start_col..end_col); + res += &self.line_to_string(line, start_col..end_col, line == end.line); } res } /// Convert a single line in the grid to a String. - fn line_to_string(&self, line: usize, cols: Range) -> String { + fn line_to_string( + &self, + line: usize, + mut cols: Range, + include_wrapped_wide: bool, + ) -> String { let mut text = String::new(); let grid_line = &self.grid[line]; - let line_length = grid_line.line_length(); - let line_end = min(line_length, cols.end + 1); + let line_length = min(grid_line.line_length(), cols.end + 1); + + // Include wide char when trailing spacer is selected + if grid_line[cols.start].flags.contains(Flags::WIDE_CHAR_SPACER) { + cols.start -= 1; + } let mut tab_mode = false; - - for col in IndexRange::from(cols.start..line_end) { + for col in IndexRange::from(cols.start..line_length) { let cell = grid_line[col]; // Skip over cells until next tab-stop once a tab was found @@ -1011,12 +1054,22 @@ impl Term { } if cols.end >= self.cols() - 1 - && (line_end == Column(0) - || !self.grid[line][line_end - 1].flags.contains(Flags::WRAPLINE)) + && (line_length.0 == 0 + || !self.grid[line][line_length - 1].flags.contains(Flags::WRAPLINE)) { text.push('\n'); } + // If wide char is not part of the selection, but leading spacer is, include it + if line_length == self.grid.num_cols() + && line_length.0 >= 2 + && grid_line[line_length - 1].flags.contains(Flags::WIDE_CHAR_SPACER) + && !grid_line[line_length - 2].flags.contains(Flags::WIDE_CHAR) + && include_wrapped_wide + { + text.push(self.grid[line - 1][Column(0)].c); + } + text }