From cde5a45318a38b73088b28c0002c6258fa738475 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 27 Apr 2022 16:11:42 +0200 Subject: [PATCH 1/3] Clip `scroll_top_row` before navigating back to it Co-Authored-By: Nathan Sobo --- crates/editor/src/editor.rs | 13 +++--- crates/editor/src/items.rs | 16 ++++---- crates/language/src/proto.rs | 2 + 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 +++++++++++++++++++++++++++--------- 8 files changed, 88 insertions(+), 35 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index f60cf7eb9abaaff8001b57886e9becdc4f39ef44..b132269619e2c3e4b672f705bb8429f63dd24272 100644 --- a/crates/editor/src/editor.rs +++ b/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_point: Point, scroll_position: Vector2F, scroll_top_anchor: Anchor, - scroll_top_offset: usize, + scroll_top_row: u32, } pub struct EditorCreated(pub ViewHandle); @@ -1112,6 +1113,7 @@ impl Editor { self.scroll_top_anchor = anchor; } + self.autoscroll_request.take(); cx.emit(Event::ScrollPositionChanged { local }); cx.notify(); } @@ -3905,9 +3907,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 { @@ -3919,10 +3920,10 @@ impl Editor { nav_history.push(Some(NavigationData { cursor_anchor: position, - cursor_offset: offset, + cursor_point: point, scroll_position: self.scroll_position, scroll_top_anchor: self.scroll_top_anchor.clone(), - scroll_top_offset, + scroll_top_row, })); } } diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 9fe6e21b718c85b7804f133ba0a02a22bb50a561..c29166a88fb219806dcbce204c376834fde08b80 100644 --- a/crates/editor/src/items.rs +++ b/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, cx: &mut ViewContext) -> bool { - if let Some(data) = data.downcast_ref::() { + if let Ok(data) = data.downcast::() { 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_point, Bias::Left) }; - let newest_selection = self.newest_selection_with_snapshot::(&buffer); + let newest_selection = self.newest_selection_with_snapshot::(&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); diff --git a/crates/language/src/proto.rs b/crates/language/src/proto.rs index deedf3e88b596209457b5b50087e4655cb968a10..406d0d751eefbcf40200091d91c7a6fd264f5ddf 100644 --- a/crates/language/src/proto.rs +++ b/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 { proto::Bias::Left => Bias::Left, proto::Bias::Right => Bias::Right, }, + buffer_id: anchor.buffer_id, }) } diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 79a50e56d4b846e928fc7be1286658d12b2a409a..16a59a3effd801437363d910119485ed5a803cde 100644 --- a/crates/rpc/proto/zed.proto +++ b/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 { diff --git a/crates/rpc/src/rpc.rs b/crates/rpc/src/rpc.rs index eac21cc35d2ce868424e91063584b4de2b487430..e0f6a0133c88a086248288784f4ff530ae7e9066 100644 --- a/crates/rpc/src/rpc.rs +++ b/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; diff --git a/crates/text/src/anchor.rs b/crates/text/src/anchor.rs index 00ec288168c45468f6f51c97dd5e3c3445d23064..36781bd3311c73b37a4ab098f2bb5448467777ce 100644 --- a/crates/text/src/anchor.rs +++ b/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, } 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 { diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index ed918cd5c59a29fe03d3f4f01dd27edbf2424d94..c3b7718711f2822f3c9a16b3e4b4b00482a3879c 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -50,7 +50,6 @@ pub struct Buffer { deferred_ops: OperationQueue, deferred_replicas: HashSet, 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 { diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 71ce6f065ab6a87a09717602ac0a33bad0dba35c..fc56d283187f148080973f25ee672460f44bb4bc 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -976,12 +976,27 @@ mod tests { .unwrap() .downcast::() .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, cx: &mut TestAppContext, - ) -> (ProjectPath, DisplayPoint) { + ) -> (ProjectPath, DisplayPoint, f32) { workspace.update(cx, |workspace, cx| { let item = workspace.active_item(cx).unwrap(); let editor = item.downcast::().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(), + ) }) } } From c9478cab098350ba1c3db55eae3eecd0b41dc8de Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 27 Apr 2022 16:51:48 +0200 Subject: [PATCH 2/3] Don't panic when navigation data contains invalid anchors and/or points Co-Authored-By: Nathan Sobo --- crates/editor/src/editor.rs | 27 +++++++++++++++++++++++++-- crates/editor/src/items.rs | 2 +- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index b132269619e2c3e4b672f705bb8429f63dd24272..75d78eda04bd6cda23d62ba6d13a31873611d3db 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -852,7 +852,7 @@ 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_point: Point, + cursor_position: Point, scroll_position: Vector2F, scroll_top_anchor: Anchor, scroll_top_row: u32, @@ -3920,7 +3920,7 @@ impl Editor { nav_history.push(Some(NavigationData { cursor_anchor: position, - cursor_point: point, + cursor_position: point, scroll_position: self.scroll_position, scroll_top_anchor: self.scroll_top_anchor.clone(), scroll_top_row, @@ -6822,6 +6822,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 }); } diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index c29166a88fb219806dcbce204c376834fde08b80..f9cb36044d1cdbec15cd4272ef612c76ee6067aa 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -250,7 +250,7 @@ impl Item for Editor { let offset = if buffer.can_resolve(&data.cursor_anchor) { data.cursor_anchor.to_point(&buffer) } else { - buffer.clip_point(data.cursor_point, Bias::Left) + buffer.clip_point(data.cursor_position, Bias::Left) }; let newest_selection = self.newest_selection_with_snapshot::(&buffer); From 76d6c00e0c740b277f7598bfc1b1b36543475c81 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 28 Apr 2022 10:12:10 +0200 Subject: [PATCH 3/3] Fix randomized collaboration tests in `language` --- crates/language/Cargo.toml | 1 + crates/language/src/tests.rs | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/language/Cargo.toml b/crates/language/Cargo.toml index ab648664199c4b93b16022645600be4848145fd0..78cfcd809dfc507024787b958a99220d107f7f36 100644 --- a/crates/language/Cargo.toml +++ b/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" diff --git a/crates/language/src/tests.rs b/crates/language/src/tests.rs index a194c26625185c72514121bb26914a17dd9b0066..57f0e6bbe01c5090cc9b6be410b324f70c636f6f 100644 --- a/crates/language/src/tests.rs +++ b/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, _| {