From 0d7857bfc477559dd01fe49173782ba47a07f58b Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 3 Mar 2024 23:13:22 +0100 Subject: [PATCH] refactor DocumentDidChange events in the past DocumentDidChange and SelectionDidChange events were implemented in a simplistic manner to get a simple prototype out. However, if you want to use these events in more complex scenarios with interdependencies between the two handlers the system fell short. The `SelectionDidChange` event was dispatched before the DocumentDidChange (and not at all if the selection wasn't manually set) so any handlers that wants to track selection was not able to map their ranges yet. The reason for this was actually the way that apply_impl was structured. The function was slightly refactored to address these problems and enable moving other range mappings to event handlers. --- helix-term/src/handlers/signature_help.rs | 2 +- helix-view/src/document.rs | 266 +++++++++++----------- helix-view/src/events.rs | 10 +- 3 files changed, 147 insertions(+), 131 deletions(-) diff --git a/helix-term/src/handlers/signature_help.rs b/helix-term/src/handlers/signature_help.rs index 0bb1d3d16..f028d5b4f 100644 --- a/helix-term/src/handlers/signature_help.rs +++ b/helix-term/src/handlers/signature_help.rs @@ -341,7 +341,7 @@ pub(super) fn register_hooks(handlers: &Handlers) { let tx = handlers.signature_hints.clone(); register_hook!(move |event: &mut DocumentDidChange<'_>| { - if event.doc.config.load().lsp.auto_signature_help { + if event.doc.config.load().lsp.auto_signature_help && !event.ghost_transaction { send_blocking(&tx, SignatureHelpEvent::ReTrigger); } Ok(()) diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 3393fbed7..1a6cf9b72 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1189,28 +1189,12 @@ impl Document { use helix_core::Assoc; let old_doc = self.text().clone(); + let changes = transaction.changes(); + if !changes.apply(&mut self.text) { + return false; + } - let success = transaction.changes().apply(&mut self.text); - - if success { - if emit_lsp_notification { - helix_event::dispatch(DocumentDidChange { - doc: self, - view: view_id, - old_text: &old_doc, - }); - } - - for selection in self.selections.values_mut() { - *selection = selection - .clone() - // Map through changes - .map(transaction.changes()) - // Ensure all selections across all views still adhere to invariants. - .ensure_invariants(self.text.slice(..)); - } - - // if specified, the current selection should instead be replaced by transaction.selection + if changes.is_empty() { if let Some(selection) = transaction.selection() { self.selections.insert( view_id, @@ -1221,129 +1205,155 @@ impl Document { view: view_id, }); } + return true; + } - self.modified_since_accessed = true; + self.modified_since_accessed = true; + self.version += 1; + for selection in self.selections.values_mut() { + *selection = selection + .clone() + // Map through changes + .map(transaction.changes()) + // Ensure all selections across all views still adhere to invariants. + .ensure_invariants(self.text.slice(..)); } - if !transaction.changes().is_empty() { - self.version += 1; - // start computing the diff in parallel - if let Some(diff_handle) = &self.diff_handle { - diff_handle.update_document(self.text.clone(), false); - } + // generate revert to savepoint + if !self.savepoints.is_empty() { + let revert = transaction.invert(&old_doc); + self.savepoints + .retain_mut(|save_point| match save_point.upgrade() { + Some(savepoint) => { + let mut revert_to_savepoint = savepoint.revert.lock(); + *revert_to_savepoint = + revert.clone().compose(mem::take(&mut revert_to_savepoint)); + true + } + None => false, + }) + } - // generate revert to savepoint - if !self.savepoints.is_empty() { - let revert = transaction.invert(&old_doc); - self.savepoints - .retain_mut(|save_point| match save_point.upgrade() { - Some(savepoint) => { - let mut revert_to_savepoint = savepoint.revert.lock(); - *revert_to_savepoint = - revert.clone().compose(mem::take(&mut revert_to_savepoint)); - true - } - None => false, - }) + // update tree-sitter syntax tree + if let Some(syntax) = &mut self.syntax { + // TODO: no unwrap + let res = syntax.update( + old_doc.slice(..), + self.text.slice(..), + transaction.changes(), + ); + if res.is_err() { + log::error!("TS parser failed, disabling TS for the current buffer: {res:?}"); + self.syntax = None; } + } - // update tree-sitter syntax tree - if let Some(syntax) = &mut self.syntax { - // TODO: no unwrap - let res = syntax.update( - old_doc.slice(..), - self.text.slice(..), - transaction.changes(), - ); - if res.is_err() { - log::error!("TS parser failed, disabling TS for the current buffer: {res:?}"); - self.syntax = None; - } - } + // TODO: all of that should likely just be hooks + // start computing the diff in parallel + if let Some(diff_handle) = &self.diff_handle { + diff_handle.update_document(self.text.clone(), false); + } - let changes = transaction.changes(); + // map diagnostics over changes too + changes.update_positions(self.diagnostics.iter_mut().map(|diagnostic| { + let assoc = if diagnostic.starts_at_word { + Assoc::BeforeWord + } else { + Assoc::After + }; + (&mut diagnostic.range.start, assoc) + })); + changes.update_positions(self.diagnostics.iter_mut().filter_map(|diagnostic| { + if diagnostic.zero_width { + // for zero width diagnostics treat the diagnostic as a point + // rather than a range + return None; + } + let assoc = if diagnostic.ends_at_word { + Assoc::AfterWord + } else { + Assoc::Before + }; + Some((&mut diagnostic.range.end, assoc)) + })); + self.diagnostics.retain_mut(|diagnostic| { + if diagnostic.zero_width { + diagnostic.range.end = diagnostic.range.start + } else if diagnostic.range.start >= diagnostic.range.end { + return false; + } + diagnostic.line = self.text.char_to_line(diagnostic.range.start); + true + }); - // map diagnostics over changes too - changes.update_positions(self.diagnostics.iter_mut().map(|diagnostic| { - let assoc = if diagnostic.starts_at_word { - Assoc::BeforeWord - } else { - Assoc::After - }; - (&mut diagnostic.range.start, assoc) - })); - changes.update_positions(self.diagnostics.iter_mut().filter_map(|diagnostic| { - if diagnostic.zero_width { - // for zero width diagnostics treat the diagnostic as a point - // rather than a range - return None; - } - let assoc = if diagnostic.ends_at_word { - Assoc::AfterWord - } else { - Assoc::Before - }; - Some((&mut diagnostic.range.end, assoc)) - })); - self.diagnostics.retain_mut(|diagnostic| { - if diagnostic.zero_width { - diagnostic.range.end = diagnostic.range.start - } else if diagnostic.range.start >= diagnostic.range.end { - return false; - } - diagnostic.line = self.text.char_to_line(diagnostic.range.start); - true - }); + self.diagnostics.sort_unstable_by_key(|diagnostic| { + (diagnostic.range, diagnostic.severity, diagnostic.provider) + }); - self.diagnostics.sort_unstable_by_key(|diagnostic| { - (diagnostic.range, diagnostic.severity, diagnostic.provider) - }); + // Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place + let apply_inlay_hint_changes = |annotations: &mut Vec| { + changes.update_positions( + annotations + .iter_mut() + .map(|annotation| (&mut annotation.char_idx, Assoc::After)), + ); + }; - // Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place - let apply_inlay_hint_changes = |annotations: &mut Vec| { - changes.update_positions( - annotations - .iter_mut() - .map(|annotation| (&mut annotation.char_idx, Assoc::After)), - ); - }; + self.inlay_hints_oudated = true; + for text_annotation in self.inlay_hints.values_mut() { + let DocumentInlayHints { + id: _, + type_inlay_hints, + parameter_inlay_hints, + other_inlay_hints, + padding_before_inlay_hints, + padding_after_inlay_hints, + } = text_annotation; + + apply_inlay_hint_changes(padding_before_inlay_hints); + apply_inlay_hint_changes(type_inlay_hints); + apply_inlay_hint_changes(parameter_inlay_hints); + apply_inlay_hint_changes(other_inlay_hints); + apply_inlay_hint_changes(padding_after_inlay_hints); + } - self.inlay_hints_oudated = true; - for text_annotation in self.inlay_hints.values_mut() { - let DocumentInlayHints { - id: _, - type_inlay_hints, - parameter_inlay_hints, - other_inlay_hints, - padding_before_inlay_hints, - padding_after_inlay_hints, - } = text_annotation; - - apply_inlay_hint_changes(padding_before_inlay_hints); - apply_inlay_hint_changes(type_inlay_hints); - apply_inlay_hint_changes(parameter_inlay_hints); - apply_inlay_hint_changes(other_inlay_hints); - apply_inlay_hint_changes(padding_after_inlay_hints); - } + helix_event::dispatch(DocumentDidChange { + doc: self, + view: view_id, + old_text: &old_doc, + changes, + ghost_transaction: !emit_lsp_notification, + }); - if emit_lsp_notification { - // TODO: move to hook - // emit lsp notification - for language_server in self.language_servers() { - let notify = language_server.text_document_did_change( - self.versioned_identifier(), - &old_doc, - self.text(), - changes, - ); + if emit_lsp_notification { + // TODO: move to hook + // emit lsp notification + for language_server in self.language_servers() { + 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 + + if let Some(selection) = transaction.selection() { + self.selections.insert( + view_id, + selection.clone().ensure_invariants(self.text.slice(..)), + ); + helix_event::dispatch(SelectionDidChange { + doc: self, + view: view_id, + }); + } + + true } fn apply_inner( diff --git a/helix-view/src/events.rs b/helix-view/src/events.rs index 8b789cc0d..8e9b4185b 100644 --- a/helix-view/src/events.rs +++ b/helix-view/src/events.rs @@ -1,9 +1,15 @@ -use helix_core::Rope; +use helix_core::{ChangeSet, Rope}; use helix_event::events; use crate::{Document, ViewId}; events! { - DocumentDidChange<'a> { doc: &'a mut Document, view: ViewId, old_text: &'a Rope } + DocumentDidChange<'a> { + doc: &'a mut Document, + view: ViewId, + old_text: &'a Rope, + changes: &'a ChangeSet, + ghost_transaction: bool + } SelectionDidChange<'a> { doc: &'a mut Document, view: ViewId } }