Merge pull request #374 from zed-industries/fix-editor-panic

Antonio Scandurra created 4 years ago

Fix editor panic when ending transaction that wasn't started by the same editor

Change summary

crates/editor/src/editor.rs       |  7 +++++
crates/editor/src/multi_buffer.rs |  6 +++++
crates/language/src/buffer.rs     |  4 +++
crates/rpc/src/conn.rs            |  4 +-
crates/text/src/tests.rs          | 35 +++++++++++++++++++++++++++++++++
crates/text/src/text.rs           | 16 ++++++++++++++
6 files changed, 68 insertions(+), 4 deletions(-)

Detailed changes

crates/editor/src/editor.rs

@@ -3527,7 +3527,11 @@ impl Editor {
             .buffer
             .update(cx, |buffer, cx| buffer.end_transaction_at(now, cx))
         {
-            self.selection_history.get_mut(&tx_id).unwrap().1 = Some(self.selections.clone());
+            if let Some((_, end_selections)) = self.selection_history.get_mut(&tx_id) {
+                *end_selections = Some(self.selections.clone());
+            } else {
+                log::error!("unexpectedly ended a transaction that wasn't started by this editor");
+            }
         }
     }
 
@@ -3952,6 +3956,7 @@ impl View for Editor {
         self.focused = true;
         self.blink_cursors(self.blink_epoch, cx);
         self.buffer.update(cx, |buffer, cx| {
+            buffer.avoid_grouping_next_transaction(cx);
             buffer.set_active_selections(&self.selections, cx)
         });
     }

crates/editor/src/multi_buffer.rs

@@ -457,6 +457,12 @@ impl MultiBuffer {
         }
     }
 
+    pub fn avoid_grouping_next_transaction(&mut self, cx: &mut ModelContext<Self>) {
+        for BufferState { buffer, .. } in self.buffers.borrow().values() {
+            buffer.update(cx, |buffer, _| buffer.avoid_grouping_next_transaction());
+        }
+    }
+
     pub fn set_active_selections(
         &mut self,
         selections: &[Selection<Anchor>],

crates/language/src/buffer.rs

@@ -1234,6 +1234,10 @@ impl Buffer {
         }
     }
 
+    pub fn avoid_grouping_next_transaction(&mut self) {
+        self.text.avoid_grouping_next_transaction();
+    }
+
     pub fn set_active_selections(
         &mut self,
         selections: Arc<[Selection<Anchor>]>,

crates/rpc/src/conn.rs

@@ -1,5 +1,5 @@
 use async_tungstenite::tungstenite::{Error as WebSocketError, Message as WebSocketMessage};
-use futures::{channel::mpsc, SinkExt as _, Stream, StreamExt as _};
+use futures::{SinkExt as _, Stream, StreamExt as _};
 use std::{io, task::Poll};
 
 pub struct Connection {
@@ -57,7 +57,7 @@ impl Connection {
         Box<dyn Send + Unpin + futures::Sink<WebSocketMessage, Error = WebSocketError>>,
         Box<dyn Send + Unpin + futures::Stream<Item = Result<WebSocketMessage, WebSocketError>>>,
     ) {
-        use futures::SinkExt as _;
+        use futures::channel::mpsc;
         use io::{Error, ErrorKind};
 
         let (tx, rx) = mpsc::unbounded::<WebSocketMessage>();

crates/text/src/tests.rs

@@ -500,6 +500,41 @@ fn test_history() {
     assert_eq!(buffer.text(), "12cde6");
 }
 
+#[test]
+fn test_avoid_grouping_next_transaction() {
+    let now = Instant::now();
+    let mut buffer = Buffer::new(0, 0, History::new("123456".into()));
+
+    buffer.start_transaction_at(now);
+    buffer.edit(vec![2..4], "cd");
+    buffer.end_transaction_at(now);
+    assert_eq!(buffer.text(), "12cd56");
+
+    buffer.avoid_grouping_next_transaction();
+    buffer.start_transaction_at(now);
+    buffer.edit(vec![4..5], "e");
+    buffer.end_transaction_at(now).unwrap();
+    assert_eq!(buffer.text(), "12cde6");
+
+    buffer.start_transaction_at(now);
+    buffer.edit(vec![0..1], "a");
+    buffer.edit(vec![1..1], "b");
+    buffer.end_transaction_at(now).unwrap();
+    assert_eq!(buffer.text(), "ab2cde6");
+
+    buffer.undo();
+    assert_eq!(buffer.text(), "12cd56");
+
+    buffer.undo();
+    assert_eq!(buffer.text(), "123456");
+
+    buffer.redo();
+    assert_eq!(buffer.text(), "12cd56");
+
+    buffer.redo();
+    assert_eq!(buffer.text(), "ab2cde6");
+}
+
 #[test]
 fn test_concurrent_edits() {
     let text = "abcdef";

crates/text/src/text.rs

@@ -72,6 +72,7 @@ pub struct Transaction {
     ranges: Vec<Range<FullOffset>>,
     first_edit_at: Instant,
     last_edit_at: Instant,
+    suppress_grouping: bool,
 }
 
 impl Transaction {
@@ -164,6 +165,7 @@ impl History {
                 ranges: Vec::new(),
                 first_edit_at: now,
                 last_edit_at: now,
+                suppress_grouping: false,
             });
             Some(id)
         } else {
@@ -194,7 +196,9 @@ impl History {
 
         if let Some(mut transaction) = transactions.next_back() {
             while let Some(prev_transaction) = transactions.next_back() {
-                if transaction.first_edit_at - prev_transaction.last_edit_at <= self.group_interval
+                if !prev_transaction.suppress_grouping
+                    && transaction.first_edit_at - prev_transaction.last_edit_at
+                        <= self.group_interval
                     && transaction.start == prev_transaction.end
                 {
                     transaction = prev_transaction;
@@ -223,6 +227,12 @@ impl History {
         self.undo_stack.last().map(|t| t.id)
     }
 
+    fn avoid_grouping_next_transaction(&mut self) {
+        if let Some(transaction) = self.undo_stack.last_mut() {
+            transaction.suppress_grouping = true;
+        }
+    }
+
     fn push_undo(&mut self, edit_id: clock::Local) {
         assert_ne!(self.transaction_depth, 0);
         let last_transaction = self.undo_stack.last_mut().unwrap();
@@ -1157,6 +1167,10 @@ impl Buffer {
         }
     }
 
+    pub fn avoid_grouping_next_transaction(&mut self) {
+        self.history.avoid_grouping_next_transaction()
+    }
+
     pub fn base_text(&self) -> &Arc<str> {
         &self.history.base_text
     }