From 5406e9f629313221c8ae97583393dfd6221d3dfc Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Tue, 4 Apr 2023 23:34:47 +0200 Subject: [PATCH] correctly handle completion rerequest --- helix-term/src/commands.rs | 9 +++-- helix-term/src/ui/completion.rs | 46 ++++++++++++++++---------- helix-term/src/ui/editor.rs | 57 +++++++++++++++++++++++--------- helix-view/src/document.rs | 58 +++++++++++++++++++++++---------- helix-view/src/editor.rs | 13 +++++--- 5 files changed, 127 insertions(+), 56 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 7d86bc0b3..8d70cd9e5 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -33,7 +33,7 @@ use helix_core::{ use helix_view::{ clipboard::ClipboardType, document::{FormatterError, Mode, SCRATCH_BUFFER_NAME}, - editor::{Action, Motion}, + editor::{Action, CompleteAction, Motion}, info::Info, input::KeyEvent, keyboard::KeyCode, @@ -4254,7 +4254,12 @@ pub fn completion(cx: &mut Context) { iter.reverse(); let offset = iter.take_while(|ch| chars::char_is_word(*ch)).count(); let start_offset = cursor.saturating_sub(offset); - let savepoint = doc.savepoint(view); + let savepoint = if let Some(CompleteAction::Selected { savepoint }) = &cx.editor.last_completion + { + savepoint.clone() + } else { + doc.savepoint(view) + }; let trigger_doc = doc.id(); let trigger_view = view.id; diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index bc216509f..dd21be03f 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -209,14 +209,27 @@ impl Completion { let (view, doc) = current!(editor); - // if more text was entered, remove it - doc.restore(view, &savepoint); - match event { - PromptEvent::Abort => { - editor.last_completion = None; - } + PromptEvent::Abort => {} PromptEvent::Update => { + // Update creates "ghost" transactiosn which are not send to the + // lsp server to avoid messing up rerequesting completions. Once a + // completion has been selected (with) tab it's always accepted whenever anything + // is typed. The only way to avoid that is to explicitly abort the completion + // with esc/c-c. This will remove the "ghost" transaction. + // + // The ghost transaction is modeled with a transaction that is not send to the LS. + // (apply_temporary) and a savepoint. It's extremly important this savepoint is restored + // (also without sending the transaction to the LS) *before any further transaction is applied*. + // Otherwise incremental sync breaks (since the state of the LS doesn't match the state the transaction + // is applied to). + if editor.last_completion.is_none() { + editor.last_completion = Some(CompleteAction::Selected { + savepoint: doc.savepoint(view), + }) + } + // if more text was entered, remove it + doc.restore(view, &savepoint, false); // always present here let item = item.unwrap(); @@ -229,19 +242,20 @@ impl Completion { true, replace_mode, ); - - // initialize a savepoint - doc.apply(&transaction, view.id); - - editor.last_completion = Some(CompleteAction { - trigger_offset, - changes: completion_changes(&transaction, trigger_offset), - }); + doc.apply_temporary(&transaction, view.id); } PromptEvent::Validate => { + if let Some(CompleteAction::Selected { savepoint }) = + editor.last_completion.take() + { + doc.restore(view, &savepoint, false); + } // always present here let item = item.unwrap(); + + // if more text was entered, remove it + doc.restore(view, &savepoint, true); let transaction = item_to_transaction( doc, view.id, @@ -251,10 +265,9 @@ impl Completion { false, replace_mode, ); - doc.apply(&transaction, view.id); - editor.last_completion = Some(CompleteAction { + editor.last_completion = Some(CompleteAction::Applied { trigger_offset, changes: completion_changes(&transaction, trigger_offset), }); @@ -270,7 +283,6 @@ impl Completion { } else { Self::resolve_completion_item(doc, item.clone()) }; - if let Some(additional_edits) = resolved_item .as_ref() .and_then(|item| item.additional_text_edits.as_ref()) diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs index 1ecbc8cc1..f0989fa88 100644 --- a/helix-term/src/ui/editor.rs +++ b/helix-term/src/ui/editor.rs @@ -19,7 +19,7 @@ use helix_core::{ syntax::{self, HighlightEvent}, text_annotations::TextAnnotations, unicode::width::UnicodeWidthStr, - visual_offset_from_block, Position, Range, Selection, Transaction, + visual_offset_from_block, Change, Position, Range, Selection, Transaction, }; use helix_view::{ document::{Mode, SavePoint, SCRATCH_BUFFER_NAME}, @@ -48,7 +48,10 @@ pub struct EditorView { #[derive(Debug, Clone)] pub enum InsertEvent { Key(KeyEvent), - CompletionApply(CompleteAction), + CompletionApply { + trigger_offset: usize, + changes: Vec, + }, TriggerCompletion, RequestCompletion, } @@ -813,7 +816,7 @@ impl EditorView { } (Mode::Insert, Mode::Normal) => { // if exiting insert mode, remove completion - self.completion = None; + self.clear_completion(cxt.editor); cxt.editor.completion_request_handle = None; // TODO: Use an on_mode_change hook to remove signature help @@ -891,22 +894,25 @@ impl EditorView { for key in self.last_insert.1.clone() { match key { InsertEvent::Key(key) => self.insert_mode(cxt, key), - InsertEvent::CompletionApply(compl) => { + InsertEvent::CompletionApply { + trigger_offset, + changes, + } => { let (view, doc) = current!(cxt.editor); if let Some(last_savepoint) = last_savepoint.as_deref() { - doc.restore(view, last_savepoint); + doc.restore(view, last_savepoint, true); } let text = doc.text().slice(..); let cursor = doc.selection(view.id).primary().cursor(text); let shift_position = - |pos: usize| -> usize { pos + cursor - compl.trigger_offset }; + |pos: usize| -> usize { pos + cursor - trigger_offset }; let tx = Transaction::change( doc.text(), - compl.changes.iter().cloned().map(|(start, end, t)| { + changes.iter().cloned().map(|(start, end, t)| { (shift_position(start), shift_position(end), t) }), ); @@ -979,6 +985,21 @@ impl EditorView { pub fn clear_completion(&mut self, editor: &mut Editor) { self.completion = None; + if let Some(last_completion) = editor.last_completion.take() { + match last_completion { + CompleteAction::Applied { + trigger_offset, + changes, + } => self.last_insert.1.push(InsertEvent::CompletionApply { + trigger_offset, + changes, + }), + CompleteAction::Selected { savepoint } => { + let (view, doc) = current!(editor); + doc.restore(view, &savepoint, false); + } + } + } // Clear any savepoints editor.clear_idle_timer(); // don't retrigger @@ -1265,12 +1286,22 @@ impl Component for EditorView { jobs: cx.jobs, scroll: None, }; - completion.handle_event(event, &mut cx) - }; - if let EventResult::Consumed(callback) = res { - consumed = true; + if let EventResult::Consumed(callback) = + completion.handle_event(event, &mut cx) + { + consumed = true; + Some(callback) + } else if let EventResult::Consumed(callback) = + completion.handle_event(&Event::Key(key!(Enter)), &mut cx) + { + Some(callback) + } else { + None + } + }; + if let Some(callback) = res { if callback.is_some() { // assume close_fn self.clear_completion(cx.editor); @@ -1286,10 +1317,6 @@ impl Component for EditorView { // if completion didn't take the event, we pass it onto commands if !consumed { - if let Some(compl) = cx.editor.last_completion.take() { - self.last_insert.1.push(InsertEvent::CompletionApply(compl)); - } - self.insert_mode(&mut cx, key); // record last_insert key diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 27f69d508..4948befdd 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1034,7 +1034,12 @@ impl Document { } /// Apply a [`Transaction`] to the [`Document`] to change its text. - fn apply_impl(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { + fn apply_impl( + &mut self, + transaction: &Transaction, + view_id: ViewId, + emit_lsp_notification: bool, + ) -> bool { use helix_core::Assoc; let old_doc = self.text().clone(); @@ -1130,25 +1135,31 @@ impl Document { apply_inlay_hint_changes(padding_after_inlay_hints); } - // emit lsp notification - if let Some(language_server) = self.language_server() { - let notify = language_server.text_document_did_change( - self.versioned_identifier(), - &old_doc, - self.text(), - changes, - ); + if emit_lsp_notification { + // emit lsp notification + if let Some(language_server) = self.language_server() { + let notify = language_server.text_document_did_change( + self.versioned_identifier(), + &old_doc, + self.text(), + changes, + ); - if let Some(notify) = notify { - tokio::spawn(notify); + if let Some(notify) = notify { + tokio::spawn(notify); + } } } } success } - /// Apply a [`Transaction`] to the [`Document`] to change its text. - pub fn apply(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { + fn apply_inner( + &mut self, + transaction: &Transaction, + view_id: ViewId, + emit_lsp_notification: bool, + ) -> bool { // store the state just before any changes are made. This allows us to undo to the // state just before a transaction was applied. if self.changes.is_empty() && !transaction.changes().is_empty() { @@ -1158,7 +1169,7 @@ impl Document { }); } - let success = self.apply_impl(transaction, view_id); + let success = self.apply_impl(transaction, view_id, emit_lsp_notification); if !transaction.changes().is_empty() { // Compose this transaction with the previous one @@ -1168,12 +1179,23 @@ impl Document { } success } + /// Apply a [`Transaction`] to the [`Document`] to change its text. + pub fn apply(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { + self.apply_inner(transaction, view_id, true) + } + + /// Apply a [`Transaction`] to the [`Document`] to change its text. + /// without notifying the language servers. This is useful for temporary transactions + /// that must not influence the server. + pub fn apply_temporary(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { + self.apply_inner(transaction, view_id, false) + } fn undo_redo_impl(&mut self, view: &mut View, undo: bool) -> bool { let mut history = self.history.take(); let txn = if undo { history.undo() } else { history.redo() }; let success = if let Some(txn) = txn { - self.apply_impl(txn, view.id) + self.apply_impl(txn, view.id, true) } else { false }; @@ -1213,7 +1235,7 @@ impl Document { savepoint } - pub fn restore(&mut self, view: &mut View, savepoint: &SavePoint) { + pub fn restore(&mut self, view: &mut View, savepoint: &SavePoint, emit_lsp_notification: bool) { assert_eq!( savepoint.view, view.id, "Savepoint must not be used with a different view!" @@ -1228,7 +1250,7 @@ impl Document { let savepoint_ref = self.savepoints.remove(savepoint_idx); let mut revert = savepoint.revert.lock(); - self.apply(&revert, view.id); + self.apply_inner(&revert, view.id, emit_lsp_notification); *revert = Transaction::new(self.text()).with_selection(self.selection(view.id).clone()); self.savepoints.push(savepoint_ref) } @@ -1241,7 +1263,7 @@ impl Document { }; let mut success = false; for txn in txns { - if self.apply_impl(&txn, view.id) { + if self.apply_impl(&txn, view.id, true) { success = true; } } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 8e4dab414..43227c5f1 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -1,7 +1,7 @@ use crate::{ align_view, clipboard::{get_clipboard_provider, ClipboardProvider}, - document::{DocumentSavedEventFuture, DocumentSavedEventResult, Mode}, + document::{DocumentSavedEventFuture, DocumentSavedEventResult, Mode, SavePoint}, graphics::{CursorKind, Rect}, info::Info, input::KeyEvent, @@ -906,9 +906,14 @@ enum ThemeAction { } #[derive(Debug, Clone)] -pub struct CompleteAction { - pub trigger_offset: usize, - pub changes: Vec, +pub enum CompleteAction { + Applied { + trigger_offset: usize, + changes: Vec, + }, + /// A savepoint of the currently active completion. The completion + /// MUST be restored before sending any event to the LSP + Selected { savepoint: Arc }, } #[derive(Debug, Copy, Clone)]