Merge pull request #923 from zed-industries/navigate-panic

Antonio Scandurra created 3 years ago

Clip `scroll_top_row` before navigating back to it

Change summary

crates/editor/src/editor.rs  | 36 ++++++++++++++--
crates/editor/src/items.rs   | 16 ++++---
crates/language/Cargo.toml   |  1 
crates/language/src/proto.rs |  2 
crates/language/src/tests.rs |  5 +
crates/rpc/proto/zed.proto   |  1 
crates/rpc/src/rpc.rs        |  2 
crates/text/src/anchor.rs    |  3 +
crates/text/src/text.rs      |  9 ++-
crates/zed/src/zed.rs        | 77 +++++++++++++++++++++++++++++--------
10 files changed, 116 insertions(+), 36 deletions(-)

Detailed changes

crates/editor/src/editor.rs

@@ -847,14 +847,15 @@ struct ClipboardSelection {
     is_entire_line: bool,
 }
 
+#[derive(Debug)]
 pub struct NavigationData {
     // Matching offsets for anchor and scroll_top_anchor allows us to recreate the anchor if the buffer
     // has since been closed
     cursor_anchor: Anchor,
-    cursor_offset: usize,
+    cursor_position: Point,
     scroll_position: Vector2F,
     scroll_top_anchor: Anchor,
-    scroll_top_offset: usize,
+    scroll_top_row: u32,
 }
 
 pub struct EditorCreated(pub ViewHandle<Editor>);
@@ -1112,6 +1113,7 @@ impl Editor {
             self.scroll_top_anchor = anchor;
         }
 
+        self.autoscroll_request.take();
         cx.emit(Event::ScrollPositionChanged { local });
         cx.notify();
     }
@@ -3910,9 +3912,8 @@ impl Editor {
     ) {
         if let Some(nav_history) = &self.nav_history {
             let buffer = self.buffer.read(cx).read(cx);
-            let offset = position.to_offset(&buffer);
             let point = position.to_point(&buffer);
-            let scroll_top_offset = self.scroll_top_anchor.to_offset(&buffer);
+            let scroll_top_row = self.scroll_top_anchor.to_point(&buffer).row;
             drop(buffer);
 
             if let Some(new_position) = new_position {
@@ -3924,10 +3925,10 @@ impl Editor {
 
             nav_history.push(Some(NavigationData {
                 cursor_anchor: position,
-                cursor_offset: offset,
+                cursor_position: point,
                 scroll_position: self.scroll_position,
                 scroll_top_anchor: self.scroll_top_anchor.clone(),
-                scroll_top_offset,
+                scroll_top_row,
             }));
         }
     }
@@ -6826,6 +6827,29 @@ mod tests {
             assert_eq!(editor.scroll_position, original_scroll_position);
             assert_eq!(editor.scroll_top_anchor, original_scroll_top_anchor);
 
+            // Ensure we don't panic when navigation data contains invalid anchors *and* points.
+            let mut invalid_anchor = editor.scroll_top_anchor.clone();
+            invalid_anchor.text_anchor.buffer_id = Some(999);
+            let invalid_point = Point::new(9999, 0);
+            editor.navigate(
+                Box::new(NavigationData {
+                    cursor_anchor: invalid_anchor.clone(),
+                    cursor_position: invalid_point,
+                    scroll_top_anchor: invalid_anchor.clone(),
+                    scroll_top_row: invalid_point.row,
+                    scroll_position: Default::default(),
+                }),
+                cx,
+            );
+            assert_eq!(
+                editor.selected_display_ranges(cx),
+                &[editor.max_point(cx)..editor.max_point(cx)]
+            );
+            assert_eq!(
+                editor.scroll_position(cx),
+                vec2f(0., editor.max_point(cx).row() as f32)
+            );
+
             editor
         });
     }

crates/editor/src/items.rs

@@ -1,4 +1,4 @@
-use crate::{Anchor, Autoscroll, Editor, Event, ExcerptId, NavigationData, ToOffset, ToPoint as _};
+use crate::{Anchor, Autoscroll, Editor, Event, ExcerptId, NavigationData, ToPoint as _};
 use anyhow::{anyhow, Result};
 use futures::FutureExt;
 use gpui::{
@@ -245,19 +245,21 @@ fn deserialize_selection(
 
 impl Item for Editor {
     fn navigate(&mut self, data: Box<dyn std::any::Any>, cx: &mut ViewContext<Self>) -> bool {
-        if let Some(data) = data.downcast_ref::<NavigationData>() {
+        if let Ok(data) = data.downcast::<NavigationData>() {
             let buffer = self.buffer.read(cx).read(cx);
             let offset = if buffer.can_resolve(&data.cursor_anchor) {
-                data.cursor_anchor.to_offset(&buffer)
+                data.cursor_anchor.to_point(&buffer)
             } else {
-                buffer.clip_offset(data.cursor_offset, Bias::Left)
+                buffer.clip_point(data.cursor_position, Bias::Left)
             };
-            let newest_selection = self.newest_selection_with_snapshot::<usize>(&buffer);
+            let newest_selection = self.newest_selection_with_snapshot::<Point>(&buffer);
 
             let scroll_top_anchor = if buffer.can_resolve(&data.scroll_top_anchor) {
-                data.scroll_top_anchor.clone()
+                data.scroll_top_anchor
             } else {
-                buffer.anchor_at(data.scroll_top_offset, Bias::Left)
+                buffer.anchor_before(
+                    buffer.clip_point(Point::new(data.scroll_top_row, 0), Bias::Left),
+                )
             };
 
             drop(buffer);

crates/language/Cargo.toml

@@ -61,4 +61,5 @@ env_logger = "0.8"
 rand = "0.8.3"
 tree-sitter-json = "*"
 tree-sitter-rust = "*"
+tree-sitter-typescript = "*"
 unindent = "0.1.7"

crates/language/src/proto.rs

@@ -147,6 +147,7 @@ pub fn serialize_anchor(anchor: &Anchor) -> proto::Anchor {
             Bias::Left => proto::Bias::Left as i32,
             Bias::Right => proto::Bias::Right as i32,
         },
+        buffer_id: anchor.buffer_id,
     }
 }
 
@@ -330,6 +331,7 @@ pub fn deserialize_anchor(anchor: proto::Anchor) -> Option<Anchor> {
             proto::Bias::Left => Bias::Left,
             proto::Bias::Right => Bias::Right,
         },
+        buffer_id: anchor.buffer_id,
     })
 }
 

crates/language/src/tests.rs

@@ -730,10 +730,13 @@ fn test_random_collaboration(cx: &mut MutableAppContext, mut rng: StdRng) {
     let mut replica_ids = Vec::new();
     let mut buffers = Vec::new();
     let network = Rc::new(RefCell::new(Network::new(rng.clone())));
+    let base_buffer = cx.add_model(|cx| Buffer::new(0, base_text.as_str(), cx));
 
     for i in 0..rng.gen_range(min_peers..=max_peers) {
         let buffer = cx.add_model(|cx| {
-            let mut buffer = Buffer::new(i as ReplicaId, base_text.as_str(), cx);
+            let mut buffer =
+                Buffer::from_proto(i as ReplicaId, base_buffer.read(cx).to_proto(), None, cx)
+                    .unwrap();
             buffer.set_group_interval(Duration::from_millis(rng.gen_range(0..=200)));
             let network = network.clone();
             cx.subscribe(&cx.handle(), move |buffer, _, event, _| {

crates/rpc/proto/zed.proto

@@ -685,6 +685,7 @@ message Anchor {
     uint32 local_timestamp = 2;
     uint64 offset = 3;
     Bias bias = 4;
+    optional uint64 buffer_id = 5;
 }
 
 enum Bias {

crates/rpc/src/rpc.rs

@@ -5,4 +5,4 @@ pub mod proto;
 pub use conn::Connection;
 pub use peer::*;
 
-pub const PROTOCOL_VERSION: u32 = 14;
+pub const PROTOCOL_VERSION: u32 = 15;

crates/text/src/anchor.rs

@@ -9,6 +9,7 @@ pub struct Anchor {
     pub timestamp: clock::Local,
     pub offset: usize,
     pub bias: Bias,
+    pub buffer_id: Option<u64>,
 }
 
 impl Anchor {
@@ -16,12 +17,14 @@ impl Anchor {
         timestamp: clock::Local::MIN,
         offset: usize::MIN,
         bias: Bias::Left,
+        buffer_id: None,
     };
 
     pub const MAX: Self = Self {
         timestamp: clock::Local::MAX,
         offset: usize::MAX,
         bias: Bias::Right,
+        buffer_id: None,
     };
 
     pub fn cmp(&self, other: &Anchor, buffer: &BufferSnapshot) -> Ordering {

crates/text/src/text.rs

@@ -50,7 +50,6 @@ pub struct Buffer {
     deferred_ops: OperationQueue<Operation>,
     deferred_replicas: HashSet<ReplicaId>,
     replica_id: ReplicaId,
-    remote_id: u64,
     local_clock: clock::Local,
     pub lamport_clock: clock::Lamport,
     subscriptions: Topic,
@@ -61,6 +60,7 @@ pub struct Buffer {
 #[derive(Clone, Debug)]
 pub struct BufferSnapshot {
     replica_id: ReplicaId,
+    remote_id: u64,
     visible_text: Rope,
     deleted_text: Rope,
     undo_map: UndoMap,
@@ -562,6 +562,7 @@ impl Buffer {
         Buffer {
             snapshot: BufferSnapshot {
                 replica_id,
+                remote_id,
                 visible_text,
                 deleted_text: Rope::new(),
                 fragments,
@@ -573,7 +574,6 @@ impl Buffer {
             deferred_ops: OperationQueue::new(),
             deferred_replicas: HashSet::default(),
             replica_id,
-            remote_id,
             local_clock,
             lamport_clock,
             subscriptions: Default::default(),
@@ -1795,12 +1795,15 @@ impl BufferSnapshot {
                 timestamp: fragment.insertion_timestamp.local(),
                 offset: fragment.insertion_offset + overshoot,
                 bias,
+                buffer_id: Some(self.remote_id),
             }
         }
     }
 
     pub fn can_resolve(&self, anchor: &Anchor) -> bool {
-        *anchor == Anchor::MIN || *anchor == Anchor::MAX || self.version.observed(anchor.timestamp)
+        *anchor == Anchor::MIN
+            || *anchor == Anchor::MAX
+            || (Some(self.remote_id) == anchor.buffer_id && self.version.observed(anchor.timestamp))
     }
 
     pub fn clip_offset(&self, offset: usize, bias: Bias) -> usize {

crates/zed/src/zed.rs

@@ -976,12 +976,27 @@ mod tests {
             .unwrap()
             .downcast::<Editor>()
             .unwrap();
+
+        editor3
+            .update(cx, |editor, cx| {
+                editor.select_display_ranges(
+                    &[DisplayPoint::new(12, 0)..DisplayPoint::new(12, 0)],
+                    cx,
+                );
+                editor.newline(&Default::default(), cx);
+                editor.newline(&Default::default(), cx);
+                editor.move_down(&Default::default(), cx);
+                editor.move_down(&Default::default(), cx);
+                editor.save(params.project.clone(), cx)
+            })
+            .await
+            .unwrap();
         editor3.update(cx, |editor, cx| {
-            editor.select_display_ranges(&[DisplayPoint::new(15, 0)..DisplayPoint::new(15, 0)], cx);
+            editor.set_scroll_position(vec2f(0., 12.5), cx)
         });
         assert_eq!(
             active_location(&workspace, cx),
-            (file3.clone(), DisplayPoint::new(15, 0))
+            (file3.clone(), DisplayPoint::new(16, 0), 12.5)
         );
 
         workspace
@@ -989,7 +1004,7 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file3.clone(), DisplayPoint::new(0, 0))
+            (file3.clone(), DisplayPoint::new(0, 0), 0.)
         );
 
         workspace
@@ -997,7 +1012,7 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file2.clone(), DisplayPoint::new(0, 0))
+            (file2.clone(), DisplayPoint::new(0, 0), 0.)
         );
 
         workspace
@@ -1005,7 +1020,7 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file1.clone(), DisplayPoint::new(10, 0))
+            (file1.clone(), DisplayPoint::new(10, 0), 0.)
         );
 
         workspace
@@ -1013,7 +1028,7 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file1.clone(), DisplayPoint::new(0, 0))
+            (file1.clone(), DisplayPoint::new(0, 0), 0.)
         );
 
         // Go back one more time and ensure we don't navigate past the first item in the history.
@@ -1022,7 +1037,7 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file1.clone(), DisplayPoint::new(0, 0))
+            (file1.clone(), DisplayPoint::new(0, 0), 0.)
         );
 
         workspace
@@ -1030,7 +1045,7 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file1.clone(), DisplayPoint::new(10, 0))
+            (file1.clone(), DisplayPoint::new(10, 0), 0.)
         );
 
         workspace
@@ -1038,7 +1053,7 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file2.clone(), DisplayPoint::new(0, 0))
+            (file2.clone(), DisplayPoint::new(0, 0), 0.)
         );
 
         // Go forward to an item that has been closed, ensuring it gets re-opened at the same
@@ -1056,7 +1071,23 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file3.clone(), DisplayPoint::new(0, 0))
+            (file3.clone(), DisplayPoint::new(0, 0), 0.)
+        );
+
+        workspace
+            .update(cx, |w, cx| Pane::go_forward(w, None, cx))
+            .await;
+        assert_eq!(
+            active_location(&workspace, cx),
+            (file3.clone(), DisplayPoint::new(16, 0), 12.5)
+        );
+
+        workspace
+            .update(cx, |w, cx| Pane::go_back(w, None, cx))
+            .await;
+        assert_eq!(
+            active_location(&workspace, cx),
+            (file3.clone(), DisplayPoint::new(0, 0), 0.)
         );
 
         // Go back to an item that has been closed and removed from disk, ensuring it gets skipped.
@@ -1079,17 +1110,18 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file1.clone(), DisplayPoint::new(10, 0))
+            (file1.clone(), DisplayPoint::new(10, 0), 0.)
         );
         workspace
             .update(cx, |w, cx| Pane::go_forward(w, None, cx))
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file3.clone(), DisplayPoint::new(0, 0))
+            (file3.clone(), DisplayPoint::new(0, 0), 0.)
         );
 
-        // Modify file to remove nav history location, and ensure duplicates are skipped
+        // Modify file to collapse multiple nav history entries into the same location.
+        // Ensure we don't visit the same location twice when navigating.
         editor1.update(cx, |editor, cx| {
             editor.select_display_ranges(&[DisplayPoint::new(15, 0)..DisplayPoint::new(15, 0)], cx)
         });
@@ -1125,25 +1157,34 @@ mod tests {
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file1.clone(), DisplayPoint::new(2, 0))
+            (file1.clone(), DisplayPoint::new(2, 0), 0.)
         );
         workspace
             .update(cx, |w, cx| Pane::go_back(w, None, cx))
             .await;
         assert_eq!(
             active_location(&workspace, cx),
-            (file1.clone(), DisplayPoint::new(3, 0))
+            (file1.clone(), DisplayPoint::new(3, 0), 0.)
         );
 
         fn active_location(
             workspace: &ViewHandle<Workspace>,
             cx: &mut TestAppContext,
-        ) -> (ProjectPath, DisplayPoint) {
+        ) -> (ProjectPath, DisplayPoint, f32) {
             workspace.update(cx, |workspace, cx| {
                 let item = workspace.active_item(cx).unwrap();
                 let editor = item.downcast::<Editor>().unwrap();
-                let selections = editor.update(cx, |editor, cx| editor.selected_display_ranges(cx));
-                (item.project_path(cx).unwrap(), selections[0].start)
+                let (selections, scroll_position) = editor.update(cx, |editor, cx| {
+                    (
+                        editor.selected_display_ranges(cx),
+                        editor.scroll_position(cx),
+                    )
+                });
+                (
+                    item.project_path(cx).unwrap(),
+                    selections[0].start,
+                    scroll_position.y(),
+                )
             })
         }
     }