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.

Author: pascalkuthe

Cargo.lock

@@ -4183,7 +4183,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,
@@ -4211,6 +4211,7 @@ pub fn completion(cx: &mut Context) {
             let ui = compositor.find::<ui::EditorView>().unwrap();
             ui.set_completion(
                 editor,
+                savepoint,
                 items,
                 offset_encoding,
                 start_offset,

helix-term/src/commands.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<SavePoint>,
         mut items: Vec<CompletionItem>,
         offset_encoding: helix_lsp::OffsetEncoding,
         start_offset: usize,
@@ -172,11 +174,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 => {
@@ -193,7 +194,6 @@ impl Completion {
                     );
 
                     // initialize a savepoint
-                    doc.savepoint(&view);
                     doc.apply(&transaction, view.id);
 
                     editor.last_completion = Some(CompleteAction {

helix-term/src/ui/completion.rs

@@ -943,14 +943,21 @@ impl EditorView {
     pub fn set_completion(
         &mut self,
         editor: &mut Editor,
+        savepoint: Arc<SavePoint>,
         items: Vec<helix_lsp::lsp::CompletionItem>,
         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 +976,6 @@ impl EditorView {
         self.completion = None;
 
         // Clear any savepoints
-        let doc = doc_mut!(editor);
-        doc.savepoint = None;
         editor.clear_idle_timer(); // don't retrigger
     }
 

helix-term/src/ui/editor.rs

@@ -43,6 +43,7 @@ toml = "0.7"
 log = "~0.4"
 
 which = "4.4"
+parking_lot = "0.12.1"
 
 
 [target.'cfg(windows)'.dependencies]

helix-view/Cargo.toml

@@ -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<DocumentSavedEvent, anyhow::Error>;
 pub type DocumentSavedEventFuture = BoxFuture<'static, DocumentSavedEventResult>;
 
+#[derive(Debug)]
+pub struct SavePoint {
+    /// The view this savepoint is associated here
+    pub view: ViewId,
+    revert: Mutex<Transaction>,
+}
+
 pub struct Document {
     pub(crate) id: DocumentId,
     text: Rope,
@@ -136,7 +144,7 @@ pub struct Document {
     pub history: Cell<History>,
     pub config: Arc<dyn DynAccess<Config>>,
 
-    pub savepoint: Option<Transaction>,
+    savepoints: Vec<Weak<SavePoint>>,
 
     // 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,46 @@ 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 savepoint. Until `doc.restore` is called,
+    ///
+    /// `Document::savepoint` is set to `None`, or `savepoint` is called again
+    /// a transaction to revert the document back to this savepoint is automatically
+    /// maintained. `Document::restore` can be used to reset the document contents
+    /// and view selection back to the state when this function was called.
+    ///
+    /// # Returns
+    ///
+    /// An `id` that can uniquely identify this savepoint. A document
+    /// can only track a single savepoint at a time, to check if your savepoint still
+    /// applies simply
+    pub fn savepoint(&mut self, view: &View) -> Arc<SavePoint> {
+        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 {