From 60c06076b25ba5aa60fd4e0abb548a710bca542d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Vinyals=20Valdepe=C3=B1as?= Date: Sun, 16 Oct 2022 11:14:59 +0200 Subject: [PATCH 1/6] feat(editor): add support to highlight trailing whitespace Adds a new render configuration value `Trailing`, which can be used to selectively enable trailing whitespace of certain whitespace characters. --- helix-term/src/ui/document.rs | 112 ++++++++------- helix-term/src/ui/mod.rs | 1 + helix-term/src/ui/trailing_whitespace.rs | 129 ++++++++++++++++++ helix-view/src/editor.rs | 165 +++++++++++++++++++++++ 4 files changed, 359 insertions(+), 48 deletions(-) create mode 100644 helix-term/src/ui/trailing_whitespace.rs diff --git a/helix-term/src/ui/document.rs b/helix-term/src/ui/document.rs index 80da1c542..9213fa7f8 100644 --- a/helix-term/src/ui/document.rs +++ b/helix-term/src/ui/document.rs @@ -7,7 +7,7 @@ use helix_core::syntax::Highlight; use helix_core::syntax::HighlightEvent; use helix_core::text_annotations::TextAnnotations; use helix_core::{visual_offset_from_block, Position, RopeSlice}; -use helix_view::editor::{WhitespaceConfig, WhitespaceRenderValue}; +use helix_view::editor::WhitespaceFeature; use helix_view::graphics::Rect; use helix_view::theme::Style; use helix_view::view::ViewPosition; @@ -15,6 +15,8 @@ use helix_view::Document; use helix_view::Theme; use tui::buffer::Buffer as Surface; +use super::trailing_whitespace::{TrailingWhitespaceTracker, WhitespaceKind}; + pub trait LineDecoration { fn render_background(&mut self, _renderer: &mut TextRenderer, _pos: LinePos) {} fn render_foreground( @@ -320,6 +322,7 @@ pub struct TextRenderer<'a> { pub draw_indent_guides: bool, pub col_offset: usize, pub viewport: Rect, + pub trailing_whitespace_tracker: TrailingWhitespaceTracker, } impl<'a> TextRenderer<'a> { @@ -331,49 +334,24 @@ impl<'a> TextRenderer<'a> { viewport: Rect, ) -> TextRenderer<'a> { let editor_config = doc.config.load(); - let WhitespaceConfig { - render: ws_render, - characters: ws_chars, - } = &editor_config.whitespace; let tab_width = doc.tab_width(); - let tab = if ws_render.tab() == WhitespaceRenderValue::All { - std::iter::once(ws_chars.tab) - .chain(std::iter::repeat(ws_chars.tabpad).take(tab_width - 1)) - .collect() - } else { - " ".repeat(tab_width) - }; - let virtual_tab = " ".repeat(tab_width); - let newline = if ws_render.newline() == WhitespaceRenderValue::All { - ws_chars.newline.into() - } else { - " ".to_owned() - }; - - let space = if ws_render.space() == WhitespaceRenderValue::All { - ws_chars.space.into() - } else { - " ".to_owned() - }; - let nbsp = if ws_render.nbsp() == WhitespaceRenderValue::All { - ws_chars.nbsp.into() - } else { - " ".to_owned() - }; - let text_style = theme.get("ui.text"); - let indent_width = doc.indent_style.indent_width(tab_width) as u16; + let ws = &editor_config.whitespace; + let regular_ws = WhitespaceFeature::Regular.palette(ws, tab_width); + let trailing_ws = WhitespaceFeature::Trailing.palette(ws, tab_width); + let trailing_whitespace_tracker = TrailingWhitespaceTracker::new(&ws.render, trailing_ws); + TextRenderer { surface, indent_guide_char: editor_config.indent_guides.character.into(), - newline, - nbsp, - space, - tab, - virtual_tab, + newline: regular_ws.newline, + nbsp: regular_ws.nbsp, + space: regular_ws.space, + tab: regular_ws.tab, + virtual_tab: regular_ws.virtual_tab, whitespace_style: theme.get("ui.virtual.whitespace"), indent_width, starting_indent: col_offset / indent_width as usize @@ -388,6 +366,7 @@ impl<'a> TextRenderer<'a> { draw_indent_guides: editor_config.indent_guides.render, viewport, col_offset, + trailing_whitespace_tracker, } } @@ -417,28 +396,65 @@ impl<'a> TextRenderer<'a> { } else { &self.tab }; - let grapheme = match grapheme { + let mut whitespace_kind = WhitespaceKind::None; + let grapheme_value = match grapheme { Grapheme::Tab { width } => { + whitespace_kind = WhitespaceKind::Tab(width); let grapheme_tab_width = char_to_byte_idx(tab, width); &tab[..grapheme_tab_width] } // TODO special rendering for other whitespaces? - Grapheme::Other { ref g } if g == " " => space, - Grapheme::Other { ref g } if g == "\u{00A0}" => nbsp, + Grapheme::Other { ref g } if g == " " => { + whitespace_kind = WhitespaceKind::Space; + space + } + Grapheme::Other { ref g } if g == "\u{00A0}" => { + whitespace_kind = WhitespaceKind::NonBreakingSpace; + nbsp + } Grapheme::Other { ref g } => g, - Grapheme::Newline => &self.newline, + Grapheme::Newline => { + whitespace_kind = WhitespaceKind::Newline; + &self.newline + } }; - let in_bounds = self.col_offset <= position.col - && position.col < self.viewport.width as usize + self.col_offset; + self.trailing_whitespace_tracker + .track(position.col, whitespace_kind); + + let viewport_right_edge = self.viewport.width as usize + self.col_offset - 1; + let in_bounds = self.col_offset <= position.col && position.col <= viewport_right_edge; if in_bounds { - self.surface.set_string( - self.viewport.x + (position.col - self.col_offset) as u16, - self.viewport.y + position.row as u16, - grapheme, - style, - ); + if self.trailing_whitespace_tracker.is_enabled() + && (grapheme == Grapheme::Newline || position.col == viewport_right_edge) + { + if let Some((from, trailing_whitespace)) = self.trailing_whitespace_tracker.get() { + let offset = if from < self.col_offset { + 0 + } else { + from - self.col_offset + }; + let begin_at = if from < self.col_offset { + self.col_offset - from + } else { + 0 + }; + self.surface.set_string( + self.viewport.x + offset as u16, + self.viewport.y + position.row as u16, + &trailing_whitespace[char_to_byte_idx(&trailing_whitespace, begin_at)..], + style, + ); + } + } else { + self.surface.set_string( + self.viewport.x + (position.col - self.col_offset) as u16, + self.viewport.y + position.row as u16, + grapheme_value, + style, + ); + } } else if cut_off_start != 0 && cut_off_start < width { // partially on screen let rect = Rect::new( diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index ec328ec55..7dc8af688 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -13,6 +13,7 @@ mod prompt; mod spinner; mod statusline; mod text; +mod trailing_whitespace; use crate::compositor::{Component, Compositor}; use crate::filter_picker_entry; diff --git a/helix-term/src/ui/trailing_whitespace.rs b/helix-term/src/ui/trailing_whitespace.rs new file mode 100644 index 000000000..da6387817 --- /dev/null +++ b/helix-term/src/ui/trailing_whitespace.rs @@ -0,0 +1,129 @@ +use helix_view::editor::{WhitespacePalette, WhitespaceRender, WhitespaceRenderValue}; + +use helix_core::str_utils::char_to_byte_idx; + +#[derive(Debug, Eq, PartialEq)] +pub enum WhitespaceKind { + None, + Space, + NonBreakingSpace, + Tab(usize), + Newline, +} + +#[derive(Debug)] +pub struct TrailingWhitespaceTracker { + enabled: bool, + palette: WhitespacePalette, + tracking: bool, + tracking_from: usize, + tracking_content: Vec, +} + +impl TrailingWhitespaceTracker { + pub fn new(render: &WhitespaceRender, palette: WhitespacePalette) -> Self { + Self { + palette, + enabled: render.any(WhitespaceRenderValue::Trailing), + tracking: false, + tracking_from: 0, + tracking_content: vec![], + } + } + + pub fn track(&mut self, from: usize, kind: WhitespaceKind) { + if kind == WhitespaceKind::None { + self.tracking = false; + return; + } + if !self.tracking { + self.tracking = true; + self.tracking_from = from; + self.tracking_content.clear(); + } + self.tracking_content.push(kind); + } + + pub fn is_enabled(&self) -> bool { + self.enabled + } + + #[must_use] + pub fn get(&mut self) -> Option<(usize, String)> { + if !self.enabled || !self.tracking { + return None; + } + + self.tracking = false; + let trailing_whitespace = self + .tracking_content + .iter() + .map(|kind| match kind { + WhitespaceKind::Space => &self.palette.space, + WhitespaceKind::NonBreakingSpace => &self.palette.nbsp, + WhitespaceKind::Tab(width) => { + let grapheme_tab_width = char_to_byte_idx(&self.palette.tab, *width); + &self.palette.tab[..grapheme_tab_width] + } + WhitespaceKind::Newline => &self.palette.newline, + WhitespaceKind::None => "", + }) + .collect::>() + .join(""); + + Some((self.tracking_from, trailing_whitespace)) + } +} + +#[cfg(test)] +mod tests { + + use super::*; + + use helix_view::editor::WhitespaceRender; + + fn palette() -> WhitespacePalette { + WhitespacePalette { + space: "S".into(), + nbsp: "N".into(), + tab: "T".into(), + virtual_tab: "V".into(), + newline: "L".into(), + } + } + + #[test] + fn test_trailing_whitespace_tracker_correctly_tracks_sequences() { + let ws_render = WhitespaceRender::Basic(WhitespaceRenderValue::Trailing); + + let mut sut = TrailingWhitespaceTracker::new(&ws_render, palette()); + + sut.track(5, WhitespaceKind::Space); + sut.track(6, WhitespaceKind::NonBreakingSpace); + sut.track(7, WhitespaceKind::Tab(1)); + sut.track(8, WhitespaceKind::Newline); + + let trailing = sut.get(); + assert!(trailing.is_some()); + let (from, display) = trailing.unwrap(); + assert_eq!(5, from); + assert_eq!("SNTL", display); + + // Now we break the sequence + sut.track(6, WhitespaceKind::None); + let trailing = sut.get(); + assert!(trailing.is_none()); + + // Now we track again + sut.track(10, WhitespaceKind::Tab(1)); + sut.track(11, WhitespaceKind::NonBreakingSpace); + sut.track(12, WhitespaceKind::Space); + sut.track(13, WhitespaceKind::Newline); + + let trailing = sut.get(); + assert!(trailing.is_some()); + let (from, display) = trailing.unwrap(); + assert_eq!(10, from); + assert_eq!("TNSL", display); + } +} diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 1f27603c9..bb70a19e1 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -632,6 +632,73 @@ pub enum WhitespaceRender { }, } +impl WhitespaceRender { + pub fn any(&self, value: WhitespaceRenderValue) -> bool { + self.space() == value + || self.nbsp() == value + || self.tab() == value + || self.newline() == value + } +} + +pub enum WhitespaceFeature { + Regular, + Trailing, +} + +impl WhitespaceFeature { + pub fn is_enabled(&self, render: WhitespaceRenderValue) -> bool { + match self { + WhitespaceFeature::Regular => matches!(render, WhitespaceRenderValue::All), + WhitespaceFeature::Trailing => matches!( + render, + WhitespaceRenderValue::All | WhitespaceRenderValue::Trailing + ), + } + } + + pub fn palette(self, cfg: &WhitespaceConfig, tab_width: usize) -> WhitespacePalette { + WhitespacePalette::from(self, cfg, tab_width) + } +} + +#[derive(Debug)] +pub struct WhitespacePalette { + pub space: String, + pub nbsp: String, + pub tab: String, + pub virtual_tab: String, + pub newline: String, +} + +impl WhitespacePalette { + fn from(feature: WhitespaceFeature, cfg: &WhitespaceConfig, tab_width: usize) -> Self { + Self { + space: if feature.is_enabled(cfg.render.space()) { + cfg.characters.space.to_string() + } else { + " ".to_string() + }, + nbsp: if feature.is_enabled(cfg.render.nbsp()) { + cfg.characters.nbsp.to_string() + } else { + " ".to_string() + }, + tab: if feature.is_enabled(cfg.render.tab()) { + cfg.characters.generate_tab(tab_width) + } else { + " ".repeat(tab_width) + }, + newline: if feature.is_enabled(cfg.render.newline()) { + cfg.characters.newline.to_string() + } else { + " ".to_string() + }, + virtual_tab: " ".repeat(tab_width), + } + } +} + #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] pub enum WhitespaceRenderValue { @@ -639,6 +706,7 @@ pub enum WhitespaceRenderValue { // TODO // Selection, All, + Trailing, } impl WhitespaceRender { @@ -698,6 +766,14 @@ impl Default for WhitespaceCharacters { } } +impl WhitespaceCharacters { + pub fn generate_tab(&self, width: usize) -> String { + std::iter::once(self.tab) + .chain(std::iter::repeat(self.tabpad).take(width - 1)) + .collect() + } +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(default, rename_all = "kebab-case")] pub struct IndentGuidesConfig { @@ -1736,3 +1812,92 @@ fn try_restore_indent(doc: &mut Document, view: &mut View) { doc.apply(&transaction, view.id); } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_whitespace_render_any() { + let sut = WhitespaceRender::Basic(WhitespaceRenderValue::Trailing); + assert!(!sut.any(WhitespaceRenderValue::None)); + assert!(!sut.any(WhitespaceRenderValue::All)); + assert!(sut.any(WhitespaceRenderValue::Trailing)); + } + + #[test] + fn test_whitespace_feature_is_enabled_regular() { + let sut = WhitespaceFeature::Regular; + + assert!(!sut.is_enabled(WhitespaceRenderValue::None)); + assert!(!sut.is_enabled(WhitespaceRenderValue::Trailing)); + assert!(sut.is_enabled(WhitespaceRenderValue::All)); + } + + #[test] + fn test_whitespace_feature_is_enabled_trailing() { + let sut = WhitespaceFeature::Trailing; + + assert!(!sut.is_enabled(WhitespaceRenderValue::None)); + assert!(sut.is_enabled(WhitespaceRenderValue::Trailing)); + assert!(sut.is_enabled(WhitespaceRenderValue::All)); + } + + #[test] + fn test_whitespace_palette_regular_all() { + let cfg = WhitespaceConfig { + render: WhitespaceRender::Basic(WhitespaceRenderValue::All), + ..Default::default() + }; + + let sut = WhitespacePalette::from(WhitespaceFeature::Regular, &cfg, 2); + + assert_eq!("·", sut.space); + assert_eq!("⍽", sut.nbsp); + assert_eq!("→ ", sut.tab); + assert_eq!(" ", sut.virtual_tab); + assert_eq!("⏎", sut.newline); + } + + #[test] + fn test_whitespace_palette_regular_trailing() { + let cfg = WhitespaceConfig { + render: WhitespaceRender::Basic(WhitespaceRenderValue::Trailing), + ..Default::default() + }; + + let sut = WhitespacePalette::from(WhitespaceFeature::Regular, &cfg, 2); + + assert_eq!(" ", sut.space); + assert_eq!(" ", sut.nbsp); + assert_eq!(" ", sut.tab); + assert_eq!(" ", sut.virtual_tab); + assert_eq!(" ", sut.newline); + } + + #[test] + fn test_whitespace_palette_trailing_all() { + let cfg = WhitespaceConfig { + render: WhitespaceRender::Basic(WhitespaceRenderValue::All), + ..Default::default() + }; + + let sut = WhitespacePalette::from(WhitespaceFeature::Trailing, &cfg, 2); + + assert_eq!("·", sut.space); + assert_eq!("⍽", sut.nbsp); + assert_eq!("→ ", sut.tab); + assert_eq!(" ", sut.virtual_tab); + assert_eq!("⏎", sut.newline); + } + + #[test] + fn test_whitespace_characters_render_tab() { + let sut = WhitespaceCharacters::default(); + + assert_eq!("→", sut.generate_tab(1)); + assert_eq!("→ ", sut.generate_tab(2)); + assert_eq!("→ ", sut.generate_tab(3)); + assert_eq!("→ ", sut.generate_tab(4)); + } +} From 99aa751c7585557faef1c35531b6ae79b8ca9f67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Vinyals=20Valdepe=C3=B1as?= Date: Sat, 24 Jun 2023 11:40:38 +0200 Subject: [PATCH 2/6] feedback: apply as much feedback as possible (round 1) --- helix-term/src/ui/document.rs | 42 ++++------- helix-term/src/ui/trailing_whitespace.rs | 93 +++++++++++++++--------- 2 files changed, 76 insertions(+), 59 deletions(-) diff --git a/helix-term/src/ui/document.rs b/helix-term/src/ui/document.rs index 9213fa7f8..5c97b057f 100644 --- a/helix-term/src/ui/document.rs +++ b/helix-term/src/ui/document.rs @@ -397,9 +397,9 @@ impl<'a> TextRenderer<'a> { &self.tab }; let mut whitespace_kind = WhitespaceKind::None; - let grapheme_value = match grapheme { + let grapheme = match grapheme { Grapheme::Tab { width } => { - whitespace_kind = WhitespaceKind::Tab(width); + whitespace_kind = WhitespaceKind::Tab; let grapheme_tab_width = char_to_byte_idx(tab, width); &tab[..grapheme_tab_width] } @@ -419,41 +419,31 @@ impl<'a> TextRenderer<'a> { } }; - self.trailing_whitespace_tracker - .track(position.col, whitespace_kind); - let viewport_right_edge = self.viewport.width as usize + self.col_offset - 1; let in_bounds = self.col_offset <= position.col && position.col <= viewport_right_edge; if in_bounds { - if self.trailing_whitespace_tracker.is_enabled() - && (grapheme == Grapheme::Newline || position.col == viewport_right_edge) + let in_bounds_col = position.col - self.col_offset; + self.surface.set_string( + self.viewport.x + in_bounds_col as u16, + self.viewport.y + position.row as u16, + grapheme, + style, + ); + + if self + .trailing_whitespace_tracker + .track(in_bounds_col, whitespace_kind) + || position.col == viewport_right_edge { if let Some((from, trailing_whitespace)) = self.trailing_whitespace_tracker.get() { - let offset = if from < self.col_offset { - 0 - } else { - from - self.col_offset - }; - let begin_at = if from < self.col_offset { - self.col_offset - from - } else { - 0 - }; self.surface.set_string( - self.viewport.x + offset as u16, + self.viewport.x + from as u16, self.viewport.y + position.row as u16, - &trailing_whitespace[char_to_byte_idx(&trailing_whitespace, begin_at)..], + &trailing_whitespace, style, ); } - } else { - self.surface.set_string( - self.viewport.x + (position.col - self.col_offset) as u16, - self.viewport.y + position.row as u16, - grapheme_value, - style, - ); } } else if cut_off_start != 0 && cut_off_start < width { // partially on screen diff --git a/helix-term/src/ui/trailing_whitespace.rs b/helix-term/src/ui/trailing_whitespace.rs index da6387817..a89fd9865 100644 --- a/helix-term/src/ui/trailing_whitespace.rs +++ b/helix-term/src/ui/trailing_whitespace.rs @@ -7,17 +7,31 @@ pub enum WhitespaceKind { None, Space, NonBreakingSpace, - Tab(usize), + Tab, Newline, } +impl WhitespaceKind { + pub fn to_str<'a>(&'a self, palette: &'a WhitespacePalette) -> &'a str { + match self { + WhitespaceKind::Space => &palette.space, + WhitespaceKind::NonBreakingSpace => &palette.nbsp, + WhitespaceKind::Tab => { + let grapheme_tab_width = char_to_byte_idx(&palette.tab, 1); + &palette.tab[..grapheme_tab_width] + } + WhitespaceKind::Newline => &palette.newline, + WhitespaceKind::None => "", + } + } +} + #[derive(Debug)] pub struct TrailingWhitespaceTracker { enabled: bool, palette: WhitespacePalette, - tracking: bool, tracking_from: usize, - tracking_content: Vec, + tracking_content: Vec<(WhitespaceKind, usize)>, } impl TrailingWhitespaceTracker { @@ -25,54 +39,51 @@ impl TrailingWhitespaceTracker { Self { palette, enabled: render.any(WhitespaceRenderValue::Trailing), - tracking: false, tracking_from: 0, tracking_content: vec![], } } - pub fn track(&mut self, from: usize, kind: WhitespaceKind) { - if kind == WhitespaceKind::None { - self.tracking = false; - return; - } - if !self.tracking { - self.tracking = true; - self.tracking_from = from; + // Tracks the whitespace and returns wether [`get`] should be called right after + // to display the trailing whitespace. + pub fn track(&mut self, from: usize, kind: WhitespaceKind) -> bool { + if !self.enabled || kind == WhitespaceKind::None { self.tracking_content.clear(); + return false; } - self.tracking_content.push(kind); - } - - pub fn is_enabled(&self) -> bool { - self.enabled + if self.tracking_content.is_empty() { + self.tracking_from = from; + } + let is_newline = kind == WhitespaceKind::Newline; + self.compress(kind); + is_newline } #[must_use] pub fn get(&mut self) -> Option<(usize, String)> { - if !self.enabled || !self.tracking { + if self.tracking_content.is_empty() { return None; } - self.tracking = false; let trailing_whitespace = self .tracking_content .iter() - .map(|kind| match kind { - WhitespaceKind::Space => &self.palette.space, - WhitespaceKind::NonBreakingSpace => &self.palette.nbsp, - WhitespaceKind::Tab(width) => { - let grapheme_tab_width = char_to_byte_idx(&self.palette.tab, *width); - &self.palette.tab[..grapheme_tab_width] - } - WhitespaceKind::Newline => &self.palette.newline, - WhitespaceKind::None => "", - }) - .collect::>() - .join(""); + .map(|(kind, n)| kind.to_str(&self.palette).repeat(*n)) + .collect::(); + self.tracking_content.clear(); Some((self.tracking_from, trailing_whitespace)) } + + fn compress(&mut self, kind: WhitespaceKind) { + if let Some((last_kind, n)) = self.tracking_content.last_mut() { + if *last_kind == kind { + *n += 1; + return; + } + } + self.tracking_content.push((kind, 1)); + } } #[cfg(test)] @@ -100,7 +111,7 @@ mod tests { sut.track(5, WhitespaceKind::Space); sut.track(6, WhitespaceKind::NonBreakingSpace); - sut.track(7, WhitespaceKind::Tab(1)); + sut.track(7, WhitespaceKind::Tab); sut.track(8, WhitespaceKind::Newline); let trailing = sut.get(); @@ -115,7 +126,7 @@ mod tests { assert!(trailing.is_none()); // Now we track again - sut.track(10, WhitespaceKind::Tab(1)); + sut.track(10, WhitespaceKind::Tab); sut.track(11, WhitespaceKind::NonBreakingSpace); sut.track(12, WhitespaceKind::Space); sut.track(13, WhitespaceKind::Newline); @@ -125,5 +136,21 @@ mod tests { let (from, display) = trailing.unwrap(); assert_eq!(10, from); assert_eq!("TNSL", display); + + // Verify compression works + sut.track(20, WhitespaceKind::Space); + sut.track(21, WhitespaceKind::Space); + sut.track(22, WhitespaceKind::NonBreakingSpace); + sut.track(23, WhitespaceKind::NonBreakingSpace); + sut.track(24, WhitespaceKind::Tab); + sut.track(25, WhitespaceKind::Tab); + sut.track(26, WhitespaceKind::Tab); + sut.track(27, WhitespaceKind::Newline); + + let trailing = sut.get(); + assert!(trailing.is_some()); + let (from, display) = trailing.unwrap(); + assert_eq!(20, from); + assert_eq!("SSNNTTTL", display); } } From de13260bb603442109f41465d7032b4a70ff1db7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Vinyals=20Valdepe=C3=B1as?= Date: Thu, 6 Jul 2023 20:31:16 +0200 Subject: [PATCH 3/6] feedback: stop allocating, pass render callback instead, ignore newline --- helix-term/src/ui/document.rs | 18 +++--- helix-term/src/ui/trailing_whitespace.rs | 79 ++++++++++++++---------- 2 files changed, 55 insertions(+), 42 deletions(-) diff --git a/helix-term/src/ui/document.rs b/helix-term/src/ui/document.rs index 5c97b057f..bf5094de4 100644 --- a/helix-term/src/ui/document.rs +++ b/helix-term/src/ui/document.rs @@ -436,14 +436,16 @@ impl<'a> TextRenderer<'a> { .track(in_bounds_col, whitespace_kind) || position.col == viewport_right_edge { - if let Some((from, trailing_whitespace)) = self.trailing_whitespace_tracker.get() { - self.surface.set_string( - self.viewport.x + from as u16, - self.viewport.y + position.row as u16, - &trailing_whitespace, - style, - ); - } + self.trailing_whitespace_tracker.render( + &mut |trailing_whitespace: &str, from: usize| { + self.surface.set_string( + self.viewport.x + from as u16, + self.viewport.y + position.row as u16, + trailing_whitespace, + self.whitespace_style, + ); + }, + ); } } else if cut_off_start != 0 && cut_off_start < width { // partially on screen diff --git a/helix-term/src/ui/trailing_whitespace.rs b/helix-term/src/ui/trailing_whitespace.rs index a89fd9865..070929b83 100644 --- a/helix-term/src/ui/trailing_whitespace.rs +++ b/helix-term/src/ui/trailing_whitespace.rs @@ -20,8 +20,7 @@ impl WhitespaceKind { let grapheme_tab_width = char_to_byte_idx(&palette.tab, 1); &palette.tab[..grapheme_tab_width] } - WhitespaceKind::Newline => &palette.newline, - WhitespaceKind::None => "", + WhitespaceKind::Newline | WhitespaceKind::None => "", } } } @@ -44,35 +43,34 @@ impl TrailingWhitespaceTracker { } } - // Tracks the whitespace and returns wether [`get`] should be called right after + // Tracks the whitespace and returns wether [`render`] should be called right after // to display the trailing whitespace. pub fn track(&mut self, from: usize, kind: WhitespaceKind) -> bool { if !self.enabled || kind == WhitespaceKind::None { self.tracking_content.clear(); return false; } + if kind == WhitespaceKind::Newline { + return true; + } if self.tracking_content.is_empty() { self.tracking_from = from; } - let is_newline = kind == WhitespaceKind::Newline; self.compress(kind); - is_newline + false } - #[must_use] - pub fn get(&mut self) -> Option<(usize, String)> { + pub fn render(&mut self, callback: &mut impl FnMut(&str, usize)) { if self.tracking_content.is_empty() { - return None; + return; } - - let trailing_whitespace = self - .tracking_content - .iter() - .map(|(kind, n)| kind.to_str(&self.palette).repeat(*n)) - .collect::(); - + let mut offset = self.tracking_from; + self.tracking_content.iter().for_each(|(kind, n)| { + let ws = kind.to_str(&self.palette).repeat(*n); + callback(&ws, offset); + offset += n; + }); self.tracking_content.clear(); - Some((self.tracking_from, trailing_whitespace)) } fn compress(&mut self, kind: WhitespaceKind) { @@ -103,6 +101,22 @@ mod tests { } } + fn capture(sut: &mut TrailingWhitespaceTracker) -> (String, usize, usize) { + let mut captured_content = String::new(); + let mut from: usize = 0; + let mut to: usize = 0; + + sut.render(&mut |content: &str, pos: usize| { + captured_content.push_str(content); + if from == 0 { + from = pos; + } + to = pos; + }); + + (captured_content, from, to) + } + #[test] fn test_trailing_whitespace_tracker_correctly_tracks_sequences() { let ws_render = WhitespaceRender::Basic(WhitespaceRenderValue::Trailing); @@ -112,30 +126,29 @@ mod tests { sut.track(5, WhitespaceKind::Space); sut.track(6, WhitespaceKind::NonBreakingSpace); sut.track(7, WhitespaceKind::Tab); - sut.track(8, WhitespaceKind::Newline); - let trailing = sut.get(); - assert!(trailing.is_some()); - let (from, display) = trailing.unwrap(); + let (content, from, to) = capture(&mut sut); + assert_eq!(5, from); - assert_eq!("SNTL", display); + assert_eq!(7, to); + assert_eq!("SNT", content); // Now we break the sequence sut.track(6, WhitespaceKind::None); - let trailing = sut.get(); - assert!(trailing.is_none()); - // Now we track again + let (content, from, to) = capture(&mut sut); + assert_eq!(0, from); + assert_eq!(0, to); + assert_eq!("", content); + sut.track(10, WhitespaceKind::Tab); sut.track(11, WhitespaceKind::NonBreakingSpace); sut.track(12, WhitespaceKind::Space); - sut.track(13, WhitespaceKind::Newline); - let trailing = sut.get(); - assert!(trailing.is_some()); - let (from, display) = trailing.unwrap(); + let (content, from, to) = capture(&mut sut); assert_eq!(10, from); - assert_eq!("TNSL", display); + assert_eq!(12, to); + assert_eq!("TNS", content); // Verify compression works sut.track(20, WhitespaceKind::Space); @@ -145,12 +158,10 @@ mod tests { sut.track(24, WhitespaceKind::Tab); sut.track(25, WhitespaceKind::Tab); sut.track(26, WhitespaceKind::Tab); - sut.track(27, WhitespaceKind::Newline); - let trailing = sut.get(); - assert!(trailing.is_some()); - let (from, display) = trailing.unwrap(); + let (content, from, to) = capture(&mut sut); assert_eq!(20, from); - assert_eq!("SSNNTTTL", display); + assert_eq!(24, to); // Compression means last tracked token is on 24 instead of 26 + assert_eq!("SSNNTTT", content); } } From a86a7d6920c03cff8d87d734fc72034d119ecda2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Vinyals=20Valdepe=C3=B1as?= Date: Sun, 14 Apr 2024 08:16:21 +0200 Subject: [PATCH 4/6] Small enum, faster to pass by value than borrowing it. --- helix-term/src/ui/document.rs | 2 +- helix-term/src/ui/trailing_whitespace.rs | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/helix-term/src/ui/document.rs b/helix-term/src/ui/document.rs index 77b4b704b..e3af3a385 100644 --- a/helix-term/src/ui/document.rs +++ b/helix-term/src/ui/document.rs @@ -398,7 +398,7 @@ impl<'a> TextRenderer<'a> { let ws = &editor_config.whitespace; let regular_ws = WhitespaceFeature::Regular.palette(ws, tab_width); let trailing_ws = WhitespaceFeature::Trailing.palette(ws, tab_width); - let trailing_whitespace_tracker = TrailingWhitespaceTracker::new(&ws.render, trailing_ws); + let trailing_whitespace_tracker = TrailingWhitespaceTracker::new(ws.render, trailing_ws); TextRenderer { surface, diff --git a/helix-term/src/ui/trailing_whitespace.rs b/helix-term/src/ui/trailing_whitespace.rs index ad30a5130..96c9d93b2 100644 --- a/helix-term/src/ui/trailing_whitespace.rs +++ b/helix-term/src/ui/trailing_whitespace.rs @@ -1,8 +1,7 @@ +use helix_core::str_utils::char_to_byte_idx; use helix_view::editor::{WhitespacePalette, WhitespaceRender, WhitespaceRenderValue}; -use helix_core::str_utils::char_to_byte_idx; - -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug, Copy, Clone, Eq, PartialEq)] pub enum WhitespaceKind { None, Space, @@ -13,7 +12,7 @@ pub enum WhitespaceKind { } impl WhitespaceKind { - pub fn to_str<'a>(&'a self, palette: &'a WhitespacePalette) -> &'a str { + pub fn to_str(self, palette: &WhitespacePalette) -> &str { match self { WhitespaceKind::Space => &palette.space, WhitespaceKind::NonBreakingSpace => &palette.nbsp, @@ -36,7 +35,7 @@ pub struct TrailingWhitespaceTracker { } impl TrailingWhitespaceTracker { - pub fn new(render: &WhitespaceRender, palette: WhitespacePalette) -> Self { + pub fn new(render: WhitespaceRender, palette: WhitespacePalette) -> Self { Self { palette, enabled: render.any(WhitespaceRenderValue::Trailing), @@ -124,7 +123,7 @@ mod tests { fn test_trailing_whitespace_tracker_correctly_tracks_sequences() { let ws_render = WhitespaceRender::Basic(WhitespaceRenderValue::Trailing); - let mut sut = TrailingWhitespaceTracker::new(&ws_render, palette()); + let mut sut = TrailingWhitespaceTracker::new(ws_render, palette()); sut.track(5, WhitespaceKind::Space); sut.track(6, WhitespaceKind::NonBreakingSpace); From 970122f6d2d12e2dfaa5590da6b571e6cf5b7d13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20Vinyals=20Valdepe=C3=B1as?= Date: Sat, 20 Apr 2024 12:20:15 +0200 Subject: [PATCH 5/6] introduce custom style for trailing whitespace --- helix-term/src/ui/document.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/helix-term/src/ui/document.rs b/helix-term/src/ui/document.rs index e3af3a385..7ce597d6a 100644 --- a/helix-term/src/ui/document.rs +++ b/helix-term/src/ui/document.rs @@ -360,6 +360,7 @@ pub struct TextRenderer<'a> { pub surface: &'a mut Surface, pub text_style: Style, pub whitespace_style: Style, + pub trailing_whitespace_style: Style, pub indent_guide_char: String, pub indent_guide_style: Style, pub newline: String, @@ -410,6 +411,7 @@ impl<'a> TextRenderer<'a> { tab: regular_ws.tab, virtual_tab: regular_ws.virtual_tab, whitespace_style: theme.get("ui.virtual.whitespace"), + trailing_whitespace_style: theme.get("ui.virtual.trailing_whitespace"), indent_width, starting_indent: col_offset / indent_width as usize + (col_offset % indent_width as usize != 0) as usize @@ -506,7 +508,7 @@ impl<'a> TextRenderer<'a> { self.viewport.x + from as u16, self.viewport.y + position.row as u16, trailing_whitespace, - self.whitespace_style, + style.patch(self.trailing_whitespace_style), ); }, ); From a37f70a488f514cab363343af7da503c67a43475 Mon Sep 17 00:00:00 2001 From: Alex Vinyals Date: Mon, 14 Oct 2024 09:11:16 +0200 Subject: [PATCH 6/6] bug: tab to str conversion should check the length of the tab in the palette --- helix-term/src/ui/trailing_whitespace.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/helix-term/src/ui/trailing_whitespace.rs b/helix-term/src/ui/trailing_whitespace.rs index 96c9d93b2..6f847138c 100644 --- a/helix-term/src/ui/trailing_whitespace.rs +++ b/helix-term/src/ui/trailing_whitespace.rs @@ -18,7 +18,7 @@ impl WhitespaceKind { WhitespaceKind::NonBreakingSpace => &palette.nbsp, WhitespaceKind::NarrowNonBreakingSpace => &palette.nnbsp, WhitespaceKind::Tab => { - let grapheme_tab_width = char_to_byte_idx(&palette.tab, 1); + let grapheme_tab_width = char_to_byte_idx(&palette.tab, palette.tab.len()); &palette.tab[..grapheme_tab_width] } WhitespaceKind::Newline | WhitespaceKind::None => "", @@ -97,7 +97,7 @@ mod tests { space: "S".into(), nbsp: "N".into(), nnbsp: "M".into(), - tab: "T".into(), + tab: "".into(), virtual_tab: "V".into(), newline: "L".into(), } @@ -134,7 +134,7 @@ mod tests { assert_eq!(5, from); assert_eq!(8, to); - assert_eq!("SNMT", content); + assert_eq!("SNM", content); // Now we break the sequence sut.track(6, WhitespaceKind::None); @@ -152,7 +152,7 @@ mod tests { let (content, from, to) = capture(&mut sut); assert_eq!(10, from); assert_eq!(13, to); - assert_eq!("TNMS", content); + assert_eq!("NMS", content); // Verify compression works sut.track(20, WhitespaceKind::Space); @@ -168,6 +168,6 @@ mod tests { let (content, from, to) = capture(&mut sut); assert_eq!(20, from); assert_eq!(26, to); // Compression means last tracked token is on 26 instead of 28 - assert_eq!("SSNNMMTTT", content); + assert_eq!("SSNNMM", content); } }