diff --git a/Cargo.lock b/Cargo.lock index a03f9c921d0be..5b12aa2491757 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1142,6 +1142,7 @@ dependencies = [ "helix-parsec", "log", "lsp-types", + "once_cell", "serde", "serde_json", "thiserror", diff --git a/helix-core/src/transaction.rs b/helix-core/src/transaction.rs index 729b45ac13871..4336f36471f5f 100644 --- a/helix-core/src/transaction.rs +++ b/helix-core/src/transaction.rs @@ -1,5 +1,3 @@ -use smallvec::SmallVec; - use crate::{Range, Rope, Selection, Tendril}; use std::borrow::Cow; @@ -111,6 +109,24 @@ impl ChangeSet { } } + /// runs `f` with this changeset extended to length `len` + pub(crate) fn with_len(&mut self, len: usize, f: impl FnOnce(&Self) -> T) -> T { + use Operation::*; + assert!(len >= self.len, "length most{len}, {}", self.len); + let n = len - self.len; + self.retain(n); + let res = f(self); + self.len -= n; + self.len_after -= n; + + if let Some(Retain(count)) = self.changes.last_mut() { + *count -= n; + } else { + self.changes.pop(); + } + res + } + /// Combine two changesets together. /// In other words, If `this` goes `docA` → `docB` and `other` represents `docB` → `docC`, the /// returned value will represent the change `docA` → `docC`. @@ -477,22 +493,17 @@ impl Transaction { /// function passes `None` to the `process_changes` function if the /// `changes` iterator yields `None` **or if the change overlaps with a /// previous change** and is ignored. - pub fn change_ignore_overlapping( + pub fn change_ignore_overlapping( doc: &Rope, - changes: I, + size_hint: usize, + mut gen_next_change: impl FnMut(&ChangeSet) -> Option>, mut process_change: impl FnMut(Option), - ) -> Self - where - I: Iterator>, - { + ) -> Self { let len = doc.len_chars(); - - let (lower, upper) = changes.size_hint(); - let size = upper.unwrap_or(lower); - let mut changeset = ChangeSet::with_capacity(2 * size + 1); // rough estimate + let mut changeset = ChangeSet::with_capacity(2 * size_hint + 1); // rough estimate let mut last = 0; - for mut change in changes { + while let Some(mut change) = changeset.with_len(len, &mut gen_next_change) { change = change.filter(|&((from, _, _), _)| last <= from); let Some(((from, to, tendril), data)) = change else { process_change(None); @@ -568,7 +579,7 @@ impl Transaction { pub fn change_by_selection_ignore_overlapping( doc: &Rope, selection: &Selection, - mut create_change: impl FnMut(&Range) -> Option<(Change, T)>, + mut create_change: impl FnMut(&Range, &ChangeSet) -> Option<(Change, T)>, mut process_applied_change: impl FnMut(&Range, T), ) -> (Transaction, usize) { let mut last_selection_idx = None; @@ -586,16 +597,20 @@ impl Transaction { } idx += 1; }; + let mut selection = selection.iter(); let transaction = Self::change_ignore_overlapping( doc, - selection.iter().map(|range| { - let (change, data) = create_change(range)?; - Some((change, (range, data))) - }), + selection.len(), + |changset| { + selection.next().map(|range| { + let (change, data) = create_change(range, changset)?; + Some((change, (range, data))) + }) + }, process_change, ); - // map the transaction trough changes - ((transaction, new_primary_idx)) + // map the transaction trough changes + (transaction, new_primary_idx) } /// Insert text at each selection head. diff --git a/helix-lsp/Cargo.toml b/helix-lsp/Cargo.toml index 9d76822dc96b2..0d284820f9030 100644 --- a/helix-lsp/Cargo.toml +++ b/helix-lsp/Cargo.toml @@ -27,3 +27,4 @@ thiserror = "1.0" tokio = { version = "1.26", features = ["rt", "rt-multi-thread", "io-util", "io-std", "time", "process", "macros", "fs", "parking_lot", "sync"] } tokio-stream = "0.1.12" which = "4.4" +once_cell = "1.17" diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index f6ff82ff51b11..3d4e159bee5db 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -61,6 +61,7 @@ pub mod util { use helix_core::line_ending::{line_end_byte_index, line_end_char_index}; use helix_core::{chars, Assoc, RopeSlice, SmallVec}; use helix_core::{diagnostic::NumberOrString, Range, Rope, Selection, Tendril, Transaction}; + use once_cell::unsync::Lazy; /// Converts a diagnostic in the document to [`lsp::Diagnostic`]. /// @@ -311,7 +312,7 @@ pub mod util { let (transaction, primary_idx) = Transaction::change_by_selection_ignore_overlapping( doc, selection, - |range| { + |range, _| { let cursor = range.cursor(text); let (start, end) = completion_pos(text, start_offset, end_offset, cursor)?; Some(((start as usize, end, replacement.clone()), ())) @@ -327,6 +328,7 @@ pub mod util { /// Creates a [Transaction] from the [snippet::Snippet] in a completion response. /// The transaction applies the edit to all cursors. + #[allow(clippy::too_many_arguments)] pub fn generate_transaction_from_snippet( doc: &Rope, selection: &Selection, @@ -335,6 +337,7 @@ pub mod util { snippet: snippet::Snippet, line_ending: &str, include_placeholder: bool, + tab_width: usize, ) -> Transaction { let text = doc.slice(..); let mut new_selection: SmallVec<[_; 1]> = SmallVec::new(); @@ -342,24 +345,45 @@ pub mod util { let (transaction, primary_idx) = Transaction::change_by_selection_ignore_overlapping( doc, selection, - |range| { + |range, changeset| { let cursor = range.cursor(text); let (replacement_start, replacement_end) = completion_pos(text, start_offset, end_offset, cursor)?; - let newline_with_offset = format!( - "{line_ending}{blank:width$}", - line_ending = line_ending, - width = - replacement_start - doc.line_to_char(doc.char_to_line(replacement_start)), - blank = "" - ); + let mapped_replacement_start = changeset.map_pos(replacement_start, Assoc::Before); + // if a snippet needs access to it's position (line number or offset) we must necessarily apply previous + // edits to the rope, as this has some ovherhead we do it lazily + // TODO: can we avoid apply the previous edits so this becomes + // O(N) instead of O(NlogN) with the number of selection ranges? + let mapped_doc = Lazy::new(|| { + let mut mapped_doc = doc.clone(); + changeset.apply(&mut mapped_doc); + mapped_doc + }); + let newline_with_offset = Lazy::new(|| { + let line_idx = mapped_doc.char_to_line(mapped_replacement_start); + let pos_on_line = mapped_replacement_start - mapped_doc.line_to_char(line_idx); + // we only care about the actual offset here (not virtual text/softwrap) + // so it's ok to use the deprecated function here + #[allow(deprecated)] + let width = helix_core::visual_coords_at_pos( + mapped_doc.line(line_idx), + pos_on_line, + tab_width, + ) + .col; + format!( + "{line_ending}{blank:width$}", + line_ending = line_ending, + blank = "" + ) + }); let (replacement, tabstops) = - snippet::render(&snippet, newline_with_offset, include_placeholder); + snippet::render(&snippet, &newline_with_offset, include_placeholder); Some(( (replacement_start, replacement_end, Some(replacement.into())), - (replacement_start, tabstops), + (mapped_replacement_start, tabstops), )) }, |range, tabstops| new_selection.push((*range, tabstops)), @@ -384,7 +408,6 @@ pub mod util { mapped_selection.push(range); continue; }; - let tabstop_anchor = changes.map_pos(tabstop_anchor, Assoc::Before); // expand the selection to cover the tabstop to retain the helix selection semantic // the tabstop closest to the range simply replaces `head` while anchor remains in place // the remainaing tabstops recive their own single-width cursor @@ -398,12 +421,15 @@ pub mod util { .map(|tabstop| Range::point(tabstop_anchor + tabstop.1)); mapped_selection.extend(rem_tabstops); } else { - let last_idx = tabstops.len(); - let primary_tabstop = tabstop_anchor + tabstops[0].1; + let last_idx = tabstops.len() - 1; + let primary_tabstop = tabstop_anchor + tabstops[last_idx].1; debug_assert!(primary_tabstop >= range.anchor); - let range = Range::new(range.anchor, primary_tabstop); - let primary_tabstop = tabstop_anchor + tabstops[last_idx].0; - let range = range.put_cursor(text, primary_tabstop, true); + // we can't properly compute the the next grapheme + // here because the transaction hasn't been applied yet + // that is not a probleme because the range gets grapheme aligned anyway + // tough so just adding one will always cause head to be grapheme + // aligned correctly when applied to the document + let range = Range::new(range.anchor, primary_tabstop + 1); mapped_selection.push(range); let rem_tabstops = tabstops[..last_idx] .iter() diff --git a/helix-lsp/src/snippet.rs b/helix-lsp/src/snippet.rs index b27077e7068cf..2f4ecacb1f961 100644 --- a/helix-lsp/src/snippet.rs +++ b/helix-lsp/src/snippet.rs @@ -2,6 +2,7 @@ use std::borrow::Cow; use anyhow::{anyhow, Result}; use helix_core::{smallvec, SmallVec}; +use once_cell::unsync::Lazy; #[derive(Debug, PartialEq, Eq)] pub enum CaseChange { @@ -55,12 +56,12 @@ pub fn parse(s: &str) -> Result> { parser::parse(s).map_err(|rest| anyhow!("Failed to parse snippet. Remaining input: {}", rest)) } -fn render_elements( +fn render_elements String>( snippet_elements: &[SnippetElement<'_>], insert: &mut String, offset: &mut usize, tabstops: &mut Vec<(usize, (usize, usize))>, - newline_with_offset: &String, + newline_with_offset: &Lazy, include_placeholer: bool, ) { use SnippetElement::*; @@ -119,9 +120,9 @@ fn render_elements( } #[allow(clippy::type_complexity)] // only used one time -pub fn render( +pub fn render String>( snippet: &Snippet<'_>, - newline_with_offset: String, + newline_with_offset: &Lazy, include_placeholer: bool, ) -> (String, Vec>) { let mut insert = String::new(); @@ -133,7 +134,7 @@ pub fn render( &mut insert, &mut offset, &mut tabstops, - &newline_with_offset, + newline_with_offset, include_placeholer, ); diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 3adc3ca9cb4ec..9eae5c4bff47a 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -8,7 +8,7 @@ use tui::{buffer::Buffer as Surface, text::Span}; use std::borrow::Cow; -use helix_core::{chars, Change, Transaction}; +use helix_core::{Change, Transaction}; use helix_view::{graphics::Rect, Document, Editor}; use crate::commands; @@ -117,7 +117,6 @@ impl Completion { view_id: ViewId, item: &CompletionItem, offset_encoding: helix_lsp::OffsetEncoding, - start_offset: usize, trigger_offset: usize, include_placeholder: bool, replace_mode: bool, @@ -210,6 +209,7 @@ impl Completion { snippet, doc.line_ending.as_str(), include_placeholder, + doc.tab_width(), ), Err(err) => { log::error!( @@ -257,7 +257,6 @@ impl Completion { view.id, item, offset_encoding, - start_offset, trigger_offset, true, replace_mode, @@ -281,7 +280,6 @@ impl Completion { view.id, item, offset_encoding, - start_offset, trigger_offset, false, replace_mode,