From bd7d50f339d17abccce34c7a64dd033f9c0b51ac Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 18 Apr 2023 16:13:18 -0700 Subject: [PATCH] Fix 'invalid insertion' panic when following Wait for the necessary buffer operations to arrive before attempting to set selections and scroll top. --- crates/collab/src/tests/integration_tests.rs | 52 ++-- crates/editor/src/items.rs | 284 ++++++++++--------- crates/editor/src/multi_buffer.rs | 38 ++- crates/language/src/buffer.rs | 6 +- crates/project/src/lsp_command.rs | 8 +- crates/text/src/text.rs | 8 +- 6 files changed, 229 insertions(+), 167 deletions(-) diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index dda8035874..eb10640d59 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/crates/collab/src/tests/integration_tests.rs @@ -5862,10 +5862,17 @@ async fn test_basic_following( // Client A updates their selections in those editors editor_a1.update(cx_a, |editor, cx| { - editor.change_selections(None, cx, |s| s.select_ranges([0..1])) + editor.handle_input("a", cx); + editor.handle_input("b", cx); + editor.handle_input("c", cx); + editor.select_left(&Default::default(), cx); + assert_eq!(editor.selections.ranges(cx), vec![3..2]); }); editor_a2.update(cx_a, |editor, cx| { - editor.change_selections(None, cx, |s| s.select_ranges([2..3])) + editor.handle_input("d", cx); + editor.handle_input("e", cx); + editor.select_left(&Default::default(), cx); + assert_eq!(editor.selections.ranges(cx), vec![2..1]); }); // When client B starts following client A, all visible view states are replicated to client B. @@ -5878,6 +5885,27 @@ async fn test_basic_following( .await .unwrap(); + cx_c.foreground().run_until_parked(); + let editor_b2 = workspace_b.read_with(cx_b, |workspace, cx| { + workspace + .active_item(cx) + .unwrap() + .downcast::() + .unwrap() + }); + assert_eq!( + cx_b.read(|cx| editor_b2.project_path(cx)), + Some((worktree_id, "2.txt").into()) + ); + assert_eq!( + editor_b2.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)), + vec![2..1] + ); + assert_eq!( + editor_b1.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)), + vec![3..2] + ); + cx_c.foreground().run_until_parked(); let active_call_c = cx_c.read(ActiveCall::global); let project_c = client_c.build_remote_project(project_id, cx_c).await; @@ -6033,26 +6061,6 @@ async fn test_basic_following( }); } - let editor_b2 = workspace_b.read_with(cx_b, |workspace, cx| { - workspace - .active_item(cx) - .unwrap() - .downcast::() - .unwrap() - }); - assert_eq!( - cx_b.read(|cx| editor_b2.project_path(cx)), - Some((worktree_id, "2.txt").into()) - ); - assert_eq!( - editor_b2.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)), - vec![2..3] - ); - assert_eq!( - editor_b1.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)), - vec![0..1] - ); - // When client A activates a different editor, client B does so as well. workspace_a.update(cx_a, |workspace, cx| { workspace.activate_item(&editor_a1, cx) diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index c5e0e8993e..a9f82e0ccb 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -3,12 +3,12 @@ use crate::{ movement::surrounding_word, persistence::DB, scroll::ScrollAnchor, Anchor, Autoscroll, Editor, Event, ExcerptId, ExcerptRange, MultiBuffer, MultiBufferSnapshot, NavigationData, ToPoint as _, }; -use anyhow::{anyhow, Context, Result}; +use anyhow::{Context, Result}; use collections::HashSet; use futures::future::try_join_all; use gpui::{ - elements::*, geometry::vector::vec2f, AppContext, Entity, ModelHandle, RenderContext, - Subscription, Task, View, ViewContext, ViewHandle, WeakViewHandle, + elements::*, geometry::vector::vec2f, AppContext, AsyncAppContext, Entity, ModelHandle, + RenderContext, Subscription, Task, View, ViewContext, ViewHandle, WeakViewHandle, }; use language::{ proto::serialize_anchor as serialize_text_anchor, Bias, Buffer, OffsetRangeExt, Point, @@ -72,11 +72,11 @@ impl FollowableItem for Editor { let editor = pane.read_with(&cx, |pane, cx| { let mut editors = pane.items_of_type::(); editors.find(|editor| { - editor.remote_id(&client, cx) == Some(remote_id) - || state.singleton - && buffers.len() == 1 - && editor.read(cx).buffer.read(cx).as_singleton().as_ref() - == Some(&buffers[0]) + let ids_match = editor.remote_id(&client, cx) == Some(remote_id); + let singleton_buffer_matches = state.singleton + && buffers.first() + == editor.read(cx).buffer.read(cx).as_singleton().as_ref(); + ids_match || singleton_buffer_matches }) }); @@ -115,46 +115,29 @@ impl FollowableItem for Editor { multibuffer }); - cx.add_view(|cx| Editor::for_multibuffer(multibuffer, Some(project), cx)) + cx.add_view(|cx| { + let mut editor = + Editor::for_multibuffer(multibuffer, Some(project.clone()), cx); + editor.remote_id = Some(remote_id); + editor + }) }) }); - editor.update(&mut cx, |editor, cx| { - editor.remote_id = Some(remote_id); - let buffer = editor.buffer.read(cx).read(cx); - let selections = state - .selections - .into_iter() - .map(|selection| { - deserialize_selection(&buffer, selection) - .ok_or_else(|| anyhow!("invalid selection")) - }) - .collect::>>()?; - let pending_selection = state - .pending_selection - .map(|selection| deserialize_selection(&buffer, selection)) - .flatten(); - let scroll_top_anchor = state - .scroll_top_anchor - .and_then(|anchor| deserialize_anchor(&buffer, anchor)); - drop(buffer); - - if !selections.is_empty() || pending_selection.is_some() { - editor.set_selections_from_remote(selections, pending_selection, cx); - } - - if let Some(scroll_top_anchor) = scroll_top_anchor { - editor.set_scroll_anchor_remote( - ScrollAnchor { - top_anchor: scroll_top_anchor, - offset: vec2f(state.scroll_x, state.scroll_y), - }, - cx, - ); - } - - anyhow::Ok(()) - })?; + update_editor_from_message( + editor.clone(), + project, + proto::update_view::Editor { + selections: state.selections, + pending_selection: state.pending_selection, + scroll_top_anchor: state.scroll_top_anchor, + scroll_x: state.scroll_x, + scroll_y: state.scroll_y, + ..Default::default() + }, + &mut cx, + ) + .await?; Ok(editor) })) @@ -299,96 +282,9 @@ impl FollowableItem for Editor { cx: &mut ViewContext, ) -> Task> { let update_view::Variant::Editor(message) = message; - let multibuffer = self.buffer.read(cx); - let multibuffer = multibuffer.read(cx); - - let buffer_ids = message - .inserted_excerpts - .iter() - .filter_map(|insertion| Some(insertion.excerpt.as_ref()?.buffer_id)) - .collect::>(); - - let mut removals = message - .deleted_excerpts - .into_iter() - .map(ExcerptId::from_proto) - .collect::>(); - removals.sort_by(|a, b| a.cmp(&b, &multibuffer)); - - let selections = message - .selections - .into_iter() - .filter_map(|selection| deserialize_selection(&multibuffer, selection)) - .collect::>(); - let pending_selection = message - .pending_selection - .and_then(|selection| deserialize_selection(&multibuffer, selection)); - - let scroll_top_anchor = message - .scroll_top_anchor - .and_then(|anchor| deserialize_anchor(&multibuffer, anchor)); - drop(multibuffer); - - let buffers = project.update(cx, |project, cx| { - buffer_ids - .into_iter() - .map(|id| project.open_buffer_by_id(id, cx)) - .collect::>() - }); - let project = project.clone(); cx.spawn(|this, mut cx| async move { - let _buffers = try_join_all(buffers).await?; - this.update(&mut cx, |this, cx| { - this.buffer.update(cx, |multibuffer, cx| { - let mut insertions = message.inserted_excerpts.into_iter().peekable(); - while let Some(insertion) = insertions.next() { - let Some(excerpt) = insertion.excerpt else { continue }; - let Some(previous_excerpt_id) = insertion.previous_excerpt_id else { continue }; - let buffer_id = excerpt.buffer_id; - let Some(buffer) = project.read(cx).buffer_for_id(buffer_id, cx) else { continue }; - - let adjacent_excerpts = iter::from_fn(|| { - let insertion = insertions.peek()?; - if insertion.previous_excerpt_id.is_none() - && insertion.excerpt.as_ref()?.buffer_id == buffer_id - { - insertions.next()?.excerpt - } else { - None - } - }); - - multibuffer.insert_excerpts_with_ids_after( - ExcerptId::from_proto(previous_excerpt_id), - buffer, - [excerpt] - .into_iter() - .chain(adjacent_excerpts) - .filter_map(|excerpt| { - Some(( - ExcerptId::from_proto(excerpt.id), - deserialize_excerpt_range(excerpt)?, - )) - }), - cx, - ); - } - - multibuffer.remove_excerpts(removals, cx); - }); - - if !selections.is_empty() || pending_selection.is_some() { - this.set_selections_from_remote(selections, pending_selection, cx); - this.request_autoscroll_remotely(Autoscroll::newest(), cx); - } else if let Some(anchor) = scroll_top_anchor { - this.set_scroll_anchor_remote(ScrollAnchor { - top_anchor: anchor, - offset: vec2f(message.scroll_x, message.scroll_y) - }, cx); - } - }); - Ok(()) + update_editor_from_message(this, project, message, &mut cx).await }) } @@ -402,6 +298,128 @@ impl FollowableItem for Editor { } } +async fn update_editor_from_message( + this: ViewHandle, + project: ModelHandle, + message: proto::update_view::Editor, + cx: &mut AsyncAppContext, +) -> Result<()> { + // Open all of the buffers of which excerpts were added to the editor. + let inserted_excerpt_buffer_ids = message + .inserted_excerpts + .iter() + .filter_map(|insertion| Some(insertion.excerpt.as_ref()?.buffer_id)) + .collect::>(); + let inserted_excerpt_buffers = project.update(cx, |project, cx| { + inserted_excerpt_buffer_ids + .into_iter() + .map(|id| project.open_buffer_by_id(id, cx)) + .collect::>() + }); + let _inserted_excerpt_buffers = try_join_all(inserted_excerpt_buffers).await?; + + // Update the editor's excerpts. + this.update(cx, |editor, cx| { + editor.buffer.update(cx, |multibuffer, cx| { + let mut removed_excerpt_ids = message + .deleted_excerpts + .into_iter() + .map(ExcerptId::from_proto) + .collect::>(); + removed_excerpt_ids.sort_by({ + let multibuffer = multibuffer.read(cx); + move |a, b| a.cmp(&b, &multibuffer) + }); + + let mut insertions = message.inserted_excerpts.into_iter().peekable(); + while let Some(insertion) = insertions.next() { + let Some(excerpt) = insertion.excerpt else { continue }; + let Some(previous_excerpt_id) = insertion.previous_excerpt_id else { continue }; + let buffer_id = excerpt.buffer_id; + let Some(buffer) = project.read(cx).buffer_for_id(buffer_id, cx) else { continue }; + + let adjacent_excerpts = iter::from_fn(|| { + let insertion = insertions.peek()?; + if insertion.previous_excerpt_id.is_none() + && insertion.excerpt.as_ref()?.buffer_id == buffer_id + { + insertions.next()?.excerpt + } else { + None + } + }); + + multibuffer.insert_excerpts_with_ids_after( + ExcerptId::from_proto(previous_excerpt_id), + buffer, + [excerpt] + .into_iter() + .chain(adjacent_excerpts) + .filter_map(|excerpt| { + Some(( + ExcerptId::from_proto(excerpt.id), + deserialize_excerpt_range(excerpt)?, + )) + }), + cx, + ); + } + + multibuffer.remove_excerpts(removed_excerpt_ids, cx); + }); + }); + + // Deserialize the editor state. + let (selections, pending_selection, scroll_top_anchor) = this.update(cx, |editor, cx| { + let buffer = editor.buffer.read(cx).read(cx); + let selections = message + .selections + .into_iter() + .filter_map(|selection| deserialize_selection(&buffer, selection)) + .collect::>(); + let pending_selection = message + .pending_selection + .and_then(|selection| deserialize_selection(&buffer, selection)); + let scroll_top_anchor = message + .scroll_top_anchor + .and_then(|anchor| deserialize_anchor(&buffer, anchor)); + anyhow::Ok((selections, pending_selection, scroll_top_anchor)) + })?; + + // Wait until the buffer has received all of the operations referenced by + // the editor's new state. + this.update(cx, |editor, cx| { + editor.buffer.update(cx, |buffer, cx| { + buffer.wait_for_anchors( + selections + .iter() + .chain(pending_selection.as_ref()) + .flat_map(|selection| [selection.start, selection.end]) + .chain(scroll_top_anchor), + cx, + ) + }) + }) + .await?; + + // Update the editor's state. + this.update(cx, |editor, cx| { + if !selections.is_empty() || pending_selection.is_some() { + editor.set_selections_from_remote(selections, pending_selection, cx); + editor.request_autoscroll_remotely(Autoscroll::newest(), cx); + } else if let Some(scroll_top_anchor) = scroll_top_anchor { + editor.set_scroll_anchor_remote( + ScrollAnchor { + top_anchor: scroll_top_anchor, + offset: vec2f(message.scroll_x, message.scroll_y), + }, + cx, + ); + } + }); + Ok(()) +} + fn serialize_excerpt( buffer_id: u64, id: &ExcerptId, diff --git a/crates/editor/src/multi_buffer.rs b/crates/editor/src/multi_buffer.rs index bec9d5967c..f8a56557ab 100644 --- a/crates/editor/src/multi_buffer.rs +++ b/crates/editor/src/multi_buffer.rs @@ -1,6 +1,7 @@ mod anchor; pub use anchor::{Anchor, AnchorRangeExt}; +use anyhow::{anyhow, Result}; use clock::ReplicaId; use collections::{BTreeMap, Bound, HashMap, HashSet}; use futures::{channel::mpsc, SinkExt}; @@ -16,7 +17,9 @@ use language::{ use std::{ borrow::Cow, cell::{Ref, RefCell}, - cmp, fmt, io, + cmp, fmt, + future::Future, + io, iter::{self, FromIterator}, mem, ops::{Range, RangeBounds, Sub}, @@ -1238,6 +1241,39 @@ impl MultiBuffer { cx.notify(); } + pub fn wait_for_anchors<'a>( + &self, + anchors: impl 'a + Iterator, + cx: &mut ModelContext, + ) -> impl 'static + Future> { + let borrow = self.buffers.borrow(); + let mut error = None; + let mut futures = Vec::new(); + for anchor in anchors { + if let Some(buffer_id) = anchor.buffer_id { + if let Some(buffer) = borrow.get(&buffer_id) { + buffer.buffer.update(cx, |buffer, _| { + futures.push(buffer.wait_for_anchors([anchor.text_anchor])) + }); + } else { + error = Some(anyhow!( + "buffer {buffer_id} is not part of this multi-buffer" + )); + break; + } + } + } + async move { + if let Some(error) = error { + Err(error)?; + } + for future in futures { + future.await?; + } + Ok(()) + } + } + pub fn text_anchor_for_position( &self, position: T, diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index fa8368f20b..fb16d41640 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -1313,10 +1313,10 @@ impl Buffer { self.text.wait_for_edits(edit_ids) } - pub fn wait_for_anchors<'a>( + pub fn wait_for_anchors( &mut self, - anchors: impl IntoIterator, - ) -> impl Future> { + anchors: impl IntoIterator, + ) -> impl 'static + Future> { self.text.wait_for_anchors(anchors) } diff --git a/crates/project/src/lsp_command.rs b/crates/project/src/lsp_command.rs index d9fafceab0..fb69df8766 100644 --- a/crates/project/src/lsp_command.rs +++ b/crates/project/src/lsp_command.rs @@ -572,7 +572,7 @@ async fn location_links_from_proto( .and_then(deserialize_anchor) .ok_or_else(|| anyhow!("missing origin end"))?; buffer - .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end])) + .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end])) .await?; Some(Location { buffer, @@ -597,7 +597,7 @@ async fn location_links_from_proto( .and_then(deserialize_anchor) .ok_or_else(|| anyhow!("missing target end"))?; buffer - .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end])) + .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end])) .await?; let target = Location { buffer, @@ -868,7 +868,7 @@ impl LspCommand for GetReferences { .and_then(deserialize_anchor) .ok_or_else(|| anyhow!("missing target end"))?; target_buffer - .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end])) + .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end])) .await?; locations.push(Location { buffer: target_buffer, @@ -1012,7 +1012,7 @@ impl LspCommand for GetDocumentHighlights { .and_then(deserialize_anchor) .ok_or_else(|| anyhow!("missing target end"))?; buffer - .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end])) + .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end])) .await?; let kind = match proto::document_highlight::Kind::from_i32(highlight.kind) { Some(proto::document_highlight::Kind::Text) => DocumentHighlightKind::TEXT, diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index b857ec5d5e..3df83fa305 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -1331,15 +1331,15 @@ impl Buffer { } } - pub fn wait_for_anchors<'a>( + pub fn wait_for_anchors( &mut self, - anchors: impl IntoIterator, + anchors: impl IntoIterator, ) -> impl 'static + Future> { let mut futures = Vec::new(); for anchor in anchors { if !self.version.observed(anchor.timestamp) - && *anchor != Anchor::MAX - && *anchor != Anchor::MIN + && anchor != Anchor::MAX + && anchor != Anchor::MIN { let (tx, rx) = oneshot::channel(); self.edit_id_resolvers