From e8898fd9a8ac8120827fb2d6f4752b3cb2431a62 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 2 Mar 2023 21:56:55 +0100 Subject: [PATCH] store multiple snapshots on the document at once Fixing autocomplete required moving the document savepoint before the asynchronous completion request. However, this in turn causes new bugs: If the completion popup is open, the savepoint is restored when the popup closes (or another entry is selected). However, at that point a new completion request might already have been created which would have replaced the new savepoint (therefore leading to incorrectly applied complies). This commit fixes that bug by allowing in arbitrary number of savepoints to be tracked on the document. The savepoints are reference counted and therefore remain valid as long as any reference to them remains. Weak reference are stored on the document and any reference that can not be upgraded anymore (hence no strong reference remain) are automatically discarded. --- Cargo.lock | 1 + helix-term/src/commands.rs | 3 +- helix-term/src/ui/completion.rs | 8 ++-- helix-term/src/ui/editor.rs | 14 +++++-- helix-view/Cargo.toml | 1 + helix-view/src/document.rs | 69 ++++++++++++++++++++++++++------- 6 files changed, 72 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a03f9c921..a1a9eae4d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1241,6 +1241,7 @@ dependencies = [ "libc", "log", "once_cell", + "parking_lot", "serde", "serde_json", "slotmap", diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index bc0e8ebea..01673c895 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -4181,7 +4181,7 @@ 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); - doc.savepoint(&view); + let savepoint = doc.savepoint(view); cx.callback( future, @@ -4209,6 +4209,7 @@ pub fn completion(cx: &mut Context) { let ui = compositor.find::().unwrap(); ui.set_completion( editor, + savepoint, items, offset_encoding, start_offset, diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 179a8cf8c..ef88938fe 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -1,12 +1,13 @@ use crate::compositor::{Component, Context, Event, EventResult}; use helix_view::{ + document::SavePoint, editor::CompleteAction, theme::{Modifier, Style}, ViewId, }; use tui::{buffer::Buffer as Surface, text::Span}; -use std::borrow::Cow; +use std::{borrow::Cow, sync::Arc}; use helix_core::{Change, Transaction}; use helix_view::{graphics::Rect, Document, Editor}; @@ -101,6 +102,7 @@ impl Completion { pub fn new( editor: &Editor, + savepoint: Arc, mut items: Vec, offset_encoding: helix_lsp::OffsetEncoding, start_offset: usize, @@ -213,11 +215,10 @@ impl Completion { let (view, doc) = current!(editor); // if more text was entered, remove it - doc.restore(view); + doc.restore(view, &savepoint); match event { PromptEvent::Abort => { - doc.restore(view); editor.last_completion = None; } PromptEvent::Update => { @@ -235,7 +236,6 @@ impl Completion { ); // initialize a savepoint - doc.savepoint(&view); doc.apply(&transaction, view.id); editor.last_completion = Some(CompleteAction { diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs index 62f04cc9d..c81ae635a 100644 --- a/helix-term/src/ui/editor.rs +++ b/helix-term/src/ui/editor.rs @@ -940,17 +940,25 @@ impl EditorView { } } + #[allow(clippy::too_many_arguments)] pub fn set_completion( &mut self, editor: &mut Editor, + savepoint: Arc, items: Vec, offset_encoding: helix_lsp::OffsetEncoding, start_offset: usize, trigger_offset: usize, size: Rect, ) { - let mut completion = - Completion::new(editor, items, offset_encoding, start_offset, trigger_offset); + let mut completion = Completion::new( + editor, + savepoint, + items, + offset_encoding, + start_offset, + trigger_offset, + ); if completion.is_empty() { // skip if we got no completion results @@ -969,8 +977,6 @@ impl EditorView { self.completion = None; // Clear any savepoints - let doc = doc_mut!(editor); - doc.savepoint = None; editor.clear_idle_timer(); // don't retrigger } diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 54b679ade..e3f98a8d3 100644 --- a/helix-view/Cargo.toml +++ b/helix-view/Cargo.toml @@ -43,6 +43,7 @@ toml = "0.7" log = "~0.4" which = "4.4" +parking_lot = "0.12.1" [target.'cfg(windows)'.dependencies] diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 13ffe7948..db12fb92b 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -9,6 +9,7 @@ use helix_core::text_annotations::TextAnnotations; use helix_core::Range; use helix_vcs::{DiffHandle, DiffProviderRegistry}; +use ::parking_lot::Mutex; use serde::de::{self, Deserialize, Deserializer}; use serde::Serialize; use std::borrow::Cow; @@ -18,7 +19,7 @@ use std::fmt::Display; use std::future::Future; use std::path::{Path, PathBuf}; use std::str::FromStr; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use std::time::SystemTime; use helix_core::{ @@ -105,6 +106,13 @@ pub struct DocumentSavedEvent { pub type DocumentSavedEventResult = Result; pub type DocumentSavedEventFuture = BoxFuture<'static, DocumentSavedEventResult>; +#[derive(Debug)] +pub struct SavePoint { + /// The view this savepoint is associated with + pub view: ViewId, + revert: Mutex, +} + pub struct Document { pub(crate) id: DocumentId, text: Rope, @@ -136,7 +144,7 @@ pub struct Document { pub history: Cell, pub config: Arc>, - pub savepoint: Option, + savepoints: Vec>, // Last time we wrote to the file. This will carry the time the file was last opened if there // were no saves. @@ -389,7 +397,7 @@ impl Document { diagnostics: Vec::new(), version: 0, history: Cell::new(History::default()), - savepoint: None, + savepoints: Vec::new(), last_saved_time: SystemTime::now(), last_saved_revision: 0, modified_since_accessed: false, @@ -846,11 +854,18 @@ impl Document { } // generate revert to savepoint - if self.savepoint.is_some() { - take_with(&mut self.savepoint, |prev_revert| { - let revert = transaction.invert(&old_doc); - Some(revert.compose(prev_revert.unwrap())) - }); + 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 @@ -940,15 +955,39 @@ impl Document { self.undo_redo_impl(view, false) } - pub fn savepoint(&mut self) { - self.savepoint = - Some(Transaction::new(self.text()).with_selection(self.selection(view.id).clone())); + /// Creates a reference counted snapshot (called savpepoint) of the document. + /// + /// The snapshot will remain valid (and updated) idenfinitly as long as ereferences to it exist. + /// Restoring the snapshot will restore the selection and the contents of the document to + /// the state it had when this function was called. + pub fn savepoint(&mut self, view: &View) -> Arc { + let revert = Transaction::new(self.text()).with_selection(self.selection(view.id).clone()); + let savepoint = Arc::new(SavePoint { + view: view.id, + revert: Mutex::new(revert), + }); + self.savepoints.push(Arc::downgrade(&savepoint)); + savepoint } - pub fn restore(&mut self, view: &mut View) { - if let Some(revert) = self.savepoint.take() { - self.apply(&revert, view.id); - } + pub fn restore(&mut self, view: &mut View, savepoint: &SavePoint) { + assert_eq!( + savepoint.view, view.id, + "Savepoint must not be used with a different view!" + ); + // search and remove savepoint using a ptr comparison + // this avoids a deadlock as we need to lock the mutex + let savepoint_idx = self + .savepoints + .iter() + .position(|savepoint_ref| savepoint_ref.as_ptr() == savepoint as *const _) + .expect("Savepoint must belong to this document"); + + let savepoint_ref = self.savepoints.remove(savepoint_idx); + let mut revert = savepoint.revert.lock(); + self.apply(&revert, view.id); + *revert = Transaction::new(self.text()).with_selection(self.selection(view.id).clone()); + self.savepoints.push(savepoint_ref) } fn earlier_later_impl(&mut self, view: &mut View, uk: UndoKind, earlier: bool) -> bool {