Determine `Buffer::is_dirty` based on the rope's fingerprint

Antonio Scandurra created

Change summary

crates/collab/src/integration_tests.rs    |  2 
crates/editor/src/display_map/fold_map.rs |  2 
crates/editor/src/editor.rs               |  2 
crates/language/src/buffer.rs             | 64 ++++++++++++++++++------
crates/language/src/tests.rs              | 41 ++++++++++++---
crates/project/src/project.rs             | 36 +++++++++----
crates/project/src/worktree.rs            | 13 +++-
crates/rpc/proto/zed.proto                |  2 
crates/text/src/rope.rs                   |  8 ++
9 files changed, 127 insertions(+), 43 deletions(-)

Detailed changes

crates/collab/src/integration_tests.rs 🔗

@@ -5364,7 +5364,7 @@ impl TestClient {
                             (buffer.version(), buffer.save(cx))
                         });
                         let save = cx.background().spawn(async move {
-                            let (saved_version, _) = save
+                            let (saved_version, _, _) = save
                                 .await
                                 .map_err(|err| anyhow!("save request failed: {:?}", err))?;
                             assert!(saved_version.observed_all(&requested_version));

crates/editor/src/display_map/fold_map.rs 🔗

@@ -5,7 +5,7 @@ use crate::{
 };
 use collections::BTreeMap;
 use gpui::fonts::HighlightStyle;
-use language::{Chunk, Edit, Point, PointUtf16, TextSummary};
+use language::{Chunk, Edit, Point, TextSummary};
 use parking_lot::Mutex;
 use std::{
     any::TypeId,

crates/editor/src/editor.rs 🔗

@@ -5508,7 +5508,7 @@ impl Editor {
                 cx.emit(Event::BufferEdited);
             }
             language::Event::Reparsed => cx.emit(Event::Reparsed),
-            language::Event::Dirtied => cx.emit(Event::Dirtied),
+            language::Event::DirtyChanged => cx.emit(Event::Dirtied),
             language::Event::Saved => cx.emit(Event::Saved),
             language::Event::FileHandleChanged => cx.emit(Event::TitleChanged),
             language::Event::Reloaded => cx.emit(Event::TitleChanged),

crates/language/src/buffer.rs 🔗

@@ -51,7 +51,10 @@ pub struct Buffer {
     text: TextBuffer,
     file: Option<Arc<dyn File>>,
     saved_version: clock::Global,
+    saved_version_fingerprint: String,
     saved_mtime: SystemTime,
+    transaction_depth: usize,
+    was_dirty_before_starting_transaction: Option<bool>,
     language: Option<Arc<Language>>,
     autoindent_requests: Vec<Arc<AutoindentRequest>>,
     pending_autoindent: Option<Task<()>>,
@@ -155,7 +158,7 @@ pub enum Operation {
 pub enum Event {
     Operation(Operation),
     Edited,
-    Dirtied,
+    DirtyChanged,
     Saved,
     FileHandleChanged,
     Reloaded,
@@ -192,7 +195,7 @@ pub trait File: Send + Sync {
         text: Rope,
         version: clock::Global,
         cx: &mut MutableAppContext,
-    ) -> Task<Result<(clock::Global, SystemTime)>>;
+    ) -> Task<Result<(clock::Global, String, SystemTime)>>;
 
     fn as_any(&self) -> &dyn Any;
 
@@ -209,6 +212,7 @@ pub trait LocalFile: File {
         &self,
         buffer_id: u64,
         version: &clock::Global,
+        fingerprint: String,
         mtime: SystemTime,
         cx: &mut MutableAppContext,
     );
@@ -426,6 +430,9 @@ impl Buffer {
         Self {
             saved_mtime,
             saved_version: buffer.version(),
+            saved_version_fingerprint: buffer.text_summary().hex_fingerprint(),
+            transaction_depth: 0,
+            was_dirty_before_starting_transaction: None,
             text: buffer,
             file,
             syntax_tree: Mutex::new(None),
@@ -476,7 +483,7 @@ impl Buffer {
     pub fn save(
         &mut self,
         cx: &mut ModelContext<Self>,
-    ) -> Task<Result<(clock::Global, SystemTime)>> {
+    ) -> Task<Result<(clock::Global, String, SystemTime)>> {
         let file = if let Some(file) = self.file.as_ref() {
             file
         } else {
@@ -486,11 +493,11 @@ impl Buffer {
         let version = self.version();
         let save = file.save(self.remote_id(), text, version, cx.as_mut());
         cx.spawn(|this, mut cx| async move {
-            let (version, mtime) = save.await?;
+            let (version, fingerprint, mtime) = save.await?;
             this.update(&mut cx, |this, cx| {
-                this.did_save(version.clone(), mtime, None, cx);
+                this.did_save(version.clone(), fingerprint.clone(), mtime, None, cx);
             });
-            Ok((version, mtime))
+            Ok((version, fingerprint, mtime))
         })
     }
 
@@ -507,12 +514,14 @@ impl Buffer {
     pub fn did_save(
         &mut self,
         version: clock::Global,
+        fingerprint: String,
         mtime: SystemTime,
         new_file: Option<Arc<dyn File>>,
         cx: &mut ModelContext<Self>,
     ) {
-        self.saved_mtime = mtime;
         self.saved_version = version;
+        self.saved_version_fingerprint = fingerprint;
+        self.saved_mtime = mtime;
         if let Some(new_file) = new_file {
             self.file = Some(new_file);
             self.file_update_count += 1;
@@ -533,7 +542,12 @@ impl Buffer {
                     .await;
                 this.update(&mut cx, |this, cx| {
                     if let Some(transaction) = this.apply_diff(diff, cx).cloned() {
-                        this.did_reload(this.version(), new_mtime, cx);
+                        this.did_reload(
+                            this.version(),
+                            this.text_summary().hex_fingerprint(),
+                            new_mtime,
+                            cx,
+                        );
                         Ok(Some(transaction))
                     } else {
                         Ok(None)
@@ -548,13 +562,21 @@ impl Buffer {
     pub fn did_reload(
         &mut self,
         version: clock::Global,
+        fingerprint: String,
         mtime: SystemTime,
         cx: &mut ModelContext<Self>,
     ) {
-        self.saved_mtime = mtime;
         self.saved_version = version;
+        self.saved_version_fingerprint = fingerprint;
+        self.saved_mtime = mtime;
         if let Some(file) = self.file.as_ref().and_then(|f| f.as_local()) {
-            file.buffer_reloaded(self.remote_id(), &self.saved_version, self.saved_mtime, cx);
+            file.buffer_reloaded(
+                self.remote_id(),
+                &self.saved_version,
+                self.saved_version_fingerprint.clone(),
+                self.saved_mtime,
+                cx,
+            );
         }
         cx.emit(Event::Reloaded);
         cx.notify();
@@ -581,7 +603,7 @@ impl Buffer {
             if !old_file.is_deleted() {
                 file_changed = true;
                 if !self.is_dirty() {
-                    cx.emit(Event::Dirtied);
+                    cx.emit(Event::DirtyChanged);
                 }
             }
         } else {
@@ -972,12 +994,12 @@ impl Buffer {
     }
 
     pub fn is_dirty(&self) -> bool {
-        !self.saved_version.observed_all(&self.version)
+        self.saved_version_fingerprint != self.as_rope().summary().hex_fingerprint()
             || self.file.as_ref().map_or(false, |file| file.is_deleted())
     }
 
     pub fn has_conflict(&self) -> bool {
-        !self.saved_version.observed_all(&self.version)
+        self.saved_version_fingerprint != self.as_rope().summary().hex_fingerprint()
             && self
                 .file
                 .as_ref()
@@ -993,6 +1015,10 @@ impl Buffer {
     }
 
     pub fn start_transaction_at(&mut self, now: Instant) -> Option<TransactionId> {
+        self.transaction_depth += 1;
+        if self.was_dirty_before_starting_transaction.is_none() {
+            self.was_dirty_before_starting_transaction = Some(self.is_dirty());
+        }
         self.text.start_transaction_at(now)
     }
 
@@ -1005,8 +1031,14 @@ impl Buffer {
         now: Instant,
         cx: &mut ModelContext<Self>,
     ) -> Option<TransactionId> {
+        assert!(self.transaction_depth > 0);
+        self.transaction_depth -= 1;
+        let was_dirty = if self.transaction_depth == 0 {
+            self.was_dirty_before_starting_transaction.take().unwrap()
+        } else {
+            false
+        };
         if let Some((transaction_id, start_version)) = self.text.end_transaction_at(now) {
-            let was_dirty = start_version != self.saved_version;
             self.did_edit(&start_version, was_dirty, cx);
             Some(transaction_id)
         } else {
@@ -1217,8 +1249,8 @@ impl Buffer {
         self.reparse(cx);
 
         cx.emit(Event::Edited);
-        if !was_dirty {
-            cx.emit(Event::Dirtied);
+        if was_dirty != self.is_dirty() {
+            cx.emit(Event::DirtyChanged);
         }
         cx.notify();
     }

crates/language/src/tests.rs 🔗

@@ -91,7 +91,7 @@ fn test_edit_events(cx: &mut gpui::MutableAppContext) {
             })
             .detach();
 
-            // An edit emits an edited event, followed by a dirtied event,
+            // An edit emits an edited event, followed by a dirty changed event,
             // since the buffer was previously in a clean state.
             buffer.edit([(2..4, "XYZ")], cx);
 
@@ -112,21 +112,46 @@ fn test_edit_events(cx: &mut gpui::MutableAppContext) {
     });
 
     // Incorporating a set of remote ops emits a single edited event,
-    // followed by a dirtied event.
+    // followed by a dirty changed event.
     buffer2.update(cx, |buffer, cx| {
         buffer
             .apply_ops(buffer1_ops.borrow_mut().drain(..), cx)
             .unwrap();
     });
-
-    let buffer_1_events = buffer_1_events.borrow();
     assert_eq!(
-        *buffer_1_events,
-        vec![Event::Edited, Event::Dirtied, Event::Edited, Event::Edited]
+        mem::take(&mut *buffer_1_events.borrow_mut()),
+        vec![
+            Event::Edited,
+            Event::DirtyChanged,
+            Event::Edited,
+            Event::Edited,
+        ]
+    );
+    assert_eq!(
+        mem::take(&mut *buffer_2_events.borrow_mut()),
+        vec![Event::Edited, Event::DirtyChanged]
     );
 
-    let buffer_2_events = buffer_2_events.borrow();
-    assert_eq!(*buffer_2_events, vec![Event::Edited, Event::Dirtied]);
+    buffer1.update(cx, |buffer, cx| {
+        // Undoing the first transaction emits edited event, followed by a
+        // dirty changed event, since the buffer is again in a clean state.
+        buffer.undo(cx);
+    });
+    // Incorporating the remote ops again emits a single edited event,
+    // followed by a dirty changed event.
+    buffer2.update(cx, |buffer, cx| {
+        buffer
+            .apply_ops(buffer1_ops.borrow_mut().drain(..), cx)
+            .unwrap();
+    });
+    assert_eq!(
+        mem::take(&mut *buffer_1_events.borrow_mut()),
+        vec![Event::Edited, Event::DirtyChanged,]
+    );
+    assert_eq!(
+        mem::take(&mut *buffer_2_events.borrow_mut()),
+        vec![Event::Edited, Event::DirtyChanged]
+    );
 }
 
 #[gpui::test]

crates/project/src/project.rs 🔗

@@ -4742,12 +4742,14 @@ impl Project {
             })
             .await;
 
-        let (saved_version, mtime) = buffer.update(&mut cx, |buffer, cx| buffer.save(cx)).await?;
+        let (saved_version, fingerprint, mtime) =
+            buffer.update(&mut cx, |buffer, cx| buffer.save(cx)).await?;
         Ok(proto::BufferSaved {
             project_id,
             buffer_id,
             version: serialize_version(&saved_version),
             mtime: Some(mtime.into()),
+            fingerprint,
         })
     }
 
@@ -5306,7 +5308,7 @@ impl Project {
                 .and_then(|buffer| buffer.upgrade(cx));
             if let Some(buffer) = buffer {
                 buffer.update(cx, |buffer, cx| {
-                    buffer.did_save(version, mtime, None, cx);
+                    buffer.did_save(version, envelope.payload.fingerprint, mtime, None, cx);
                 });
             }
             Ok(())
@@ -5332,7 +5334,7 @@ impl Project {
                 .and_then(|buffer| buffer.upgrade(cx));
             if let Some(buffer) = buffer {
                 buffer.update(cx, |buffer, cx| {
-                    buffer.did_reload(version, mtime, cx);
+                    buffer.did_reload(version, payload.fingerprint, mtime, cx);
                 });
             }
             Ok(())
@@ -8105,10 +8107,16 @@ mod tests {
             assert!(buffer.is_dirty());
             assert_eq!(
                 *events.borrow(),
-                &[language::Event::Edited, language::Event::Dirtied]
+                &[language::Event::Edited, language::Event::DirtyChanged]
             );
             events.borrow_mut().clear();
-            buffer.did_save(buffer.version(), buffer.file().unwrap().mtime(), None, cx);
+            buffer.did_save(
+                buffer.version(),
+                buffer.as_rope().summary().hex_fingerprint(),
+                buffer.file().unwrap().mtime(),
+                None,
+                cx,
+            );
         });
 
         // after saving, the buffer is not dirty, and emits a saved event.
@@ -8129,20 +8137,23 @@ mod tests {
                 *events.borrow(),
                 &[
                     language::Event::Edited,
-                    language::Event::Dirtied,
+                    language::Event::DirtyChanged,
                     language::Event::Edited,
                 ],
             );
             events.borrow_mut().clear();
 
-            // TODO - currently, after restoring the buffer to its
-            // previously-saved state, the is still considered dirty.
+            // After restoring the buffer to its previously-saved state,
+            // the buffer is not considered dirty anymore.
             buffer.edit([(1..3, "")], cx);
             assert!(buffer.text() == "ac");
-            assert!(buffer.is_dirty());
+            assert!(!buffer.is_dirty());
         });
 
-        assert_eq!(*events.borrow(), &[language::Event::Edited]);
+        assert_eq!(
+            *events.borrow(),
+            &[language::Event::Edited, language::Event::DirtyChanged]
+        );
 
         // When a file is deleted, the buffer is considered dirty.
         let events = Rc::new(RefCell::new(Vec::new()));
@@ -8164,7 +8175,10 @@ mod tests {
         buffer2.condition(&cx, |b, _| b.is_dirty()).await;
         assert_eq!(
             *events.borrow(),
-            &[language::Event::Dirtied, language::Event::FileHandleChanged]
+            &[
+                language::Event::DirtyChanged,
+                language::Event::FileHandleChanged
+            ]
         );
 
         // When a file is already dirty when deleted, we don't emit a Dirtied event.

crates/project/src/worktree.rs 🔗

@@ -634,6 +634,7 @@ impl LocalWorktree {
     ) -> Task<Result<()>> {
         let buffer = buffer_handle.read(cx);
         let text = buffer.as_rope().clone();
+        let fingerprint = text.summary().hex_fingerprint();
         let version = buffer.version();
         let save = self.write_file(path, text, cx);
         let handle = cx.handle();
@@ -648,7 +649,7 @@ impl LocalWorktree {
             };
 
             buffer_handle.update(&mut cx, |buffer, cx| {
-                buffer.did_save(version, file.mtime, Some(Arc::new(file)), cx);
+                buffer.did_save(version, fingerprint, file.mtime, Some(Arc::new(file)), cx);
             });
 
             Ok(())
@@ -1702,11 +1703,12 @@ impl language::File for File {
         text: Rope,
         version: clock::Global,
         cx: &mut MutableAppContext,
-    ) -> Task<Result<(clock::Global, SystemTime)>> {
+    ) -> Task<Result<(clock::Global, String, SystemTime)>> {
         self.worktree.update(cx, |worktree, cx| match worktree {
             Worktree::Local(worktree) => {
                 let rpc = worktree.client.clone();
                 let project_id = worktree.share.as_ref().map(|share| share.project_id);
+                let fingerprint = text.summary().hex_fingerprint();
                 let save = worktree.write_file(self.path.clone(), text, cx);
                 cx.background().spawn(async move {
                     let entry = save.await?;
@@ -1716,9 +1718,10 @@ impl language::File for File {
                             buffer_id,
                             version: serialize_version(&version),
                             mtime: Some(entry.mtime.into()),
+                            fingerprint: fingerprint.clone(),
                         })?;
                     }
-                    Ok((version, entry.mtime))
+                    Ok((version, fingerprint, entry.mtime))
                 })
             }
             Worktree::Remote(worktree) => {
@@ -1737,7 +1740,7 @@ impl language::File for File {
                         .mtime
                         .ok_or_else(|| anyhow!("missing mtime"))?
                         .into();
-                    Ok((version, mtime))
+                    Ok((version, response.fingerprint, mtime))
                 })
             }
         })
@@ -1779,6 +1782,7 @@ impl language::LocalFile for File {
         &self,
         buffer_id: u64,
         version: &clock::Global,
+        fingerprint: String,
         mtime: SystemTime,
         cx: &mut MutableAppContext,
     ) {
@@ -1791,6 +1795,7 @@ impl language::LocalFile for File {
                     buffer_id,
                     version: serialize_version(&version),
                     mtime: Some(mtime.into()),
+                    fingerprint,
                 })
                 .log_err();
         }

crates/rpc/proto/zed.proto 🔗

@@ -364,6 +364,7 @@ message BufferSaved {
     uint64 buffer_id = 2;
     repeated VectorClockEntry version = 3;
     Timestamp mtime = 4;
+    string fingerprint = 5;
 }
 
 message BufferReloaded {
@@ -371,6 +372,7 @@ message BufferReloaded {
     uint64 buffer_id = 2;
     repeated VectorClockEntry version = 3;
     Timestamp mtime = 4;
+    string fingerprint = 5;
 }
 
 message ReloadBuffers {

crates/text/src/rope.rs 🔗

@@ -2,7 +2,7 @@ use crate::PointUtf16;
 
 use super::Point;
 use arrayvec::ArrayString;
-use bromberg_sl2::HashMatrix;
+use bromberg_sl2::{DigestString, HashMatrix};
 use smallvec::SmallVec;
 use std::{cmp, fmt, io, mem, ops::Range, str};
 use sum_tree::{Bias, Dimension, SumTree};
@@ -729,6 +729,12 @@ pub struct TextSummary {
     pub fingerprint: HashMatrix,
 }
 
+impl TextSummary {
+    pub fn hex_fingerprint(&self) -> String {
+        self.fingerprint.to_hex()
+    }
+}
+
 impl<'a> From<&'a str> for TextSummary {
     fn from(text: &'a str) -> Self {
         let mut lines = Point::new(0, 0);