tmp

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<T>(&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<I, T>(
+    pub fn change_ignore_overlapping<T>(
         doc: &Rope,
-        changes: I,
+        size_hint: usize,
+        mut gen_next_change: impl FnMut(&ChangeSet) -> Option<Option<(Change, T)>>,
         mut process_change: impl FnMut(Option<T>),
-    ) -> Self
-    where
-        I: Iterator<Item = Option<(Change, T)>>,
-    {
+    ) -> 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<T>(
         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.

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"

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,31 +337,53 @@ 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();
 
         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()

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<Snippet<'_>> {
     parser::parse(s).map_err(|rest| anyhow!("Failed to parse snippet. Remaining input: {}", rest))
 }
 
-fn render_elements(
+fn render_elements<F: FnOnce() -> String>(
     snippet_elements: &[SnippetElement<'_>],
     insert: &mut String,
     offset: &mut usize,
     tabstops: &mut Vec<(usize, (usize, usize))>,
-    newline_with_offset: &String,
+    newline_with_offset: &Lazy<String, F>,
     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<F: FnOnce() -> String>(
     snippet: &Snippet<'_>,
-    newline_with_offset: String,
+    newline_with_offset: &Lazy<String, F>,
     include_placeholer: bool,
 ) -> (String, Vec<SmallVec<[(usize, usize); 1]>>) {
     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,
     );
 

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,