From b1f75280909884c9621b553b43030ac39cfa47ce Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 9 Mar 2023 23:08:55 +0100 Subject: [PATCH] fix snippet bugs and multicursor completion edgecases Multicursor completions may overlap and therefore overlapping completions must be dropped to avoid crashes. Furthermore, multicursor edits might simply be out of range if the word before/after the cursor is shorter. This currently leads to crashes, instead these selections are now also removed for completions. This commit also significantly refactors snippet transaction generation so that tabstops behave correctly with the above rules. Furthermore, snippet tabstops need to be carefully mapped to ensure their position is correct and consistent with our selection semantics. Finally, we now keep a partially updated Rope while creating snippet transactions so that we can fill information into snippets that depends on the position in the document. --- helix-lsp/src/lib.rs | 241 +++++++++++++++++++++++--------- helix-term/src/ui/completion.rs | 18 +-- 2 files changed, 180 insertions(+), 79 deletions(-) diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index 147b381c2..58e8d83dc 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -59,8 +59,8 @@ pub enum OffsetEncoding { pub mod util { use super::*; use helix_core::line_ending::{line_end_byte_index, line_end_char_index}; + use helix_core::{chars, RopeSlice, SmallVec}; use helix_core::{diagnostic::NumberOrString, Range, Rope, Selection, Tendril, Transaction}; - use helix_core::{smallvec, SmallVec}; /// Converts a diagnostic in the document to [`lsp::Diagnostic`]. /// @@ -247,13 +247,46 @@ pub mod util { Some(Range::new(start, end)) } + /// If the LS did not provide a range for the completion or the range of the + /// primary cursor can not be used for the secondary cursor, this function + /// can be used to find the completion range for a cursor + fn find_completion_range(text: RopeSlice, cursor: usize) -> (usize, usize) { + let start = cursor + - text + .chars_at(cursor) + .reversed() + .take_while(|ch| chars::char_is_word(*ch)) + .count(); + (start, cursor) + } + fn completion_range( + text: RopeSlice, + edit_offset: Option<(i128, i128)>, + cursor: usize, + ) -> Option<(usize, usize)> { + let res = match edit_offset { + Some((start_offset, end_offset)) => { + let start_offset = cursor as i128 + start_offset; + if start_offset < 0 { + return None; + } + let end_offset = cursor as i128 + end_offset; + if end_offset > text.len_chars() as i128 { + return None; + } + (start_offset as usize, end_offset as usize) + } + None => find_completion_range(text, cursor), + }; + Some(res) + } + /// Creates a [Transaction] from the [lsp::TextEdit] in a completion response. /// The transaction applies the edit to all cursors. pub fn generate_transaction_from_completion_edit( doc: &Rope, selection: &Selection, - start_offset: i128, - end_offset: i128, + edit_offset: Option<(i128, i128)>, new_text: String, ) -> Transaction { let replacement: Option = if new_text.is_empty() { @@ -263,83 +296,163 @@ pub mod util { }; let text = doc.slice(..); + let (removed_start, removed_end) = + completion_range(text, edit_offset, selection.primary().cursor(text)) + .expect("transaction must be valid for primary selection"); + let removed_text = text.slice(removed_start..removed_end); - Transaction::change_by_selection(doc, selection, |range| { - let cursor = range.cursor(text); - ( - (cursor as i128 + start_offset) as usize, - (cursor as i128 + end_offset) as usize, - replacement.clone(), - ) - }) + let (transaction, mut selection) = Transaction::change_by_selection_ignore_overlapping( + doc, + selection, + |range| { + let cursor = range.cursor(text); + completion_range(text, edit_offset, cursor) + .filter(|(start, end)| text.slice(start..end) == removed_text) + .unwrap_or_else(|| find_completion_range(text, cursor)) + }, + |_, _| replacement.clone(), + ); + if transaction.changes().is_empty() { + return transaction; + } + selection = selection.map(transaction.changes()); + transaction.with_selection(selection) } /// 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, - start_offset: i128, - end_offset: i128, + edit_offset: Option<(i128, i128)>, snippet: snippet::Snippet, line_ending: &str, include_placeholder: bool, + tab_width: usize, ) -> Transaction { let text = doc.slice(..); - // For each cursor store offsets for the first tabstop - let mut cursor_tabstop_offsets = Vec::>::new(); - let transaction = Transaction::change_by_selection(doc, selection, |range| { - let cursor = range.cursor(text); - let replacement_start = (cursor as i128 + start_offset) as usize; - let replacement_end = (cursor as i128 + end_offset) as usize; - 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 (replacement, tabstops) = - snippet::render(&snippet, newline_with_offset, include_placeholder); - - let replacement_len = replacement.chars().count(); - cursor_tabstop_offsets.push( - tabstops - .first() - .unwrap_or(&smallvec![(replacement_len, replacement_len)]) - .iter() - .map(|(from, to)| -> (i128, i128) { - ( - *from as i128 - replacement_len as i128, - *to as i128 - replacement_len as i128, - ) - }) - .collect(), - ); - - (replacement_start, replacement_end, Some(replacement.into())) - }); + let mut off = 0i128; + let mut mapped_doc = doc.clone(); + let mut selection_tabstops: SmallVec<[_; 1]> = SmallVec::new(); + let (removed_start, removed_end) = + completion_range(text, edit_offset, selection.primary().cursor(text)) + .expect("transaction must be valid for primary selection"); + let removed_text = text.slice(removed_start..removed_end); - // Create new selection based on the cursor tabstop from above - let mut cursor_tabstop_offsets_iter = cursor_tabstop_offsets.iter(); - let selection = selection - .clone() - .map(transaction.changes()) - .transform_iter(|range| { - cursor_tabstop_offsets_iter - .next() - .unwrap() - .iter() - .map(move |(from, to)| { - Range::new( - (range.anchor as i128 + *from) as usize, - (range.anchor as i128 + *to) as usize, - ) - }) - }); + let (transaction, selection) = Transaction::change_by_selection_ignore_overlapping( + doc, + selection, + |range| { + let cursor = range.cursor(text); + completion_range(text, edit_offset, cursor) + .filter(|(start, end)| text.slice(start..end) == removed_text) + .unwrap_or_else(|| find_completion_range(text, cursor)) + }, + |replacement_start, replacement_end| { + let mapped_replacement_start = (replacement_start as i128 + off) as usize; + let mapped_replacement_end = (replacement_end as i128 + off) as usize; + + 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; + let newline_with_offset = format!( + "{line_ending}{blank:width$}", + line_ending = line_ending, + blank = "" + ); + + let (replacement, tabstops) = + snippet::render(&snippet, &newline_with_offset, include_placeholder); + selection_tabstops.push((mapped_replacement_start, tabstops)); + mapped_doc.remove(mapped_replacement_start..mapped_replacement_end); + mapped_doc.insert(mapped_replacement_start, &replacement); + off += + replacement_start as i128 - replacement_end as i128 + replacement.len() as i128; + + Some(replacement) + }, + ); - transaction.with_selection(selection) + let changes = transaction.changes(); + if changes.is_empty() { + return transaction; + } + + let mut mapped_selection = SmallVec::with_capacity(selection.len()); + let mut mapped_primary_idx = 0; + let primary_range = selection.primary(); + for (range, (tabstop_anchor, tabstops)) in selection.into_iter().zip(selection_tabstops) { + if range == primary_range { + mapped_primary_idx = mapped_selection.len() + } + + let range = range.map(changes); + let tabstops = tabstops.first().filter(|tabstops| !tabstops.is_empty()); + let Some(tabstops) = tabstops else{ + // no tabstop normal mapping + mapped_selection.push(range); + continue; + }; + + // 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 remaining tabstops receive their own single-width cursor + if range.head < range.anchor { + let first_tabstop = tabstop_anchor + tabstops[0].1; + + // if selection is forward but was moved to the right it is + // contained entirely in the replacement text, just do a point + // selection (fallback below) + if range.anchor >= first_tabstop { + let range = Range::new(range.anchor, first_tabstop); + mapped_selection.push(range); + let rem_tabstops = tabstops[1..] + .iter() + .map(|tabstop| Range::point(tabstop_anchor + tabstop.1)); + mapped_selection.extend(rem_tabstops); + continue; + } + } else { + let last_idx = tabstops.len() - 1; + let last_tabstop = tabstop_anchor + tabstops[last_idx].1; + + // if selection is forward but was moved to the right it is + // contained entirely in the replacement text, just do a point + // selection (fallback below) + if range.anchor <= last_tabstop { + // we can't properly compute the the next grapheme + // here because the transaction hasn't been applied yet + // that is not a problem 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, last_tabstop + 1); + mapped_selection.push(range); + let rem_tabstops = tabstops[..last_idx] + .iter() + .map(|tabstop| Range::point(tabstop_anchor + tabstop.0)); + mapped_selection.extend(rem_tabstops); + continue; + } + }; + + let tabstops = tabstops + .iter() + .map(|tabstop| Range::point(tabstop_anchor + tabstop.0)); + mapped_selection.extend(tabstops); + } + + transaction.with_selection(Selection::new(mapped_selection, mapped_primary_idx)) } pub fn generate_transaction_from_edits( diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 336b75cb4..99c337811 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -118,7 +118,6 @@ impl Completion { view_id: ViewId, item: &CompletionItem, offset_encoding: helix_lsp::OffsetEncoding, - start_offset: usize, trigger_offset: usize, include_placeholder: bool, ) -> Transaction { @@ -147,28 +146,18 @@ impl Completion { None => return Transaction::new(doc.text()), }; - (start_offset, end_offset, edit.new_text) + (Some((start_offset, end_offset)), edit.new_text) } else { let new_text = item .insert_text .clone() .unwrap_or_else(|| item.label.clone()); - // check that we are still at the correct savepoint // we can still generate a transaction regardless but if the // document changed (and not just the selection) then we will // likely delete the wrong text (same if we applied an edit sent by the LS) debug_assert!(primary_cursor == trigger_offset); - - // TODO: Respect editor.completion_replace? - // Would require detecting the end of the word boundary for every cursor individually. - // We don't do the same for normal `edits, to be consistent we would have to do it for those too - - ( - start_offset as i128 - primary_cursor as i128, - trigger_offset as i128 - primary_cursor as i128, - new_text, - ) + (None, Some(0), new_text) }; if matches!(item.kind, Some(lsp::CompletionItemKind::SNIPPET)) @@ -186,6 +175,7 @@ impl Completion { snippet, doc.line_ending.as_str(), include_placeholder, + doc.tab_width(), ), Err(err) => { log::error!( @@ -232,7 +222,6 @@ impl Completion { view.id, item, offset_encoding, - start_offset, trigger_offset, true, ); @@ -254,7 +243,6 @@ impl Completion { view.id, item, offset_encoding, - start_offset, trigger_offset, false, );