From fc0b2491365cb547814880b687d5f0bda54f488c Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 1 Oct 2025 09:43:22 +0200 Subject: [PATCH] multi_buffer: Fix handling of `ExcerptId::max()` (#38887) This removes a hack from `MultiBuffer::anchor_at` that works around missing logic for handling `ExcerptId::max()` by implementing that said missing logic. Generally, `ExcerptId::min()` is already being handled correctly due to how `Cursor` seeking works, we tend to seek to or beyond a seek target, meaning `min` will always match the first excerpt as expected. `max` on the other hand will always seek beyond the last excerpt resulting in no excerpt being found, so any code path dealing with the excerpt sumtree will have to specially check for this special excerpt ID to work correctly. Release Notes: - N/A *or* Added/Fixed/Improved ... --- crates/editor/src/editor.rs | 51 ++++--- crates/editor/src/element.rs | 4 +- crates/editor/src/hover_links.rs | 14 +- crates/editor/src/hover_popover.rs | 4 +- crates/editor/src/inlay_hint_cache.rs | 2 +- crates/editor/src/items.rs | 18 +-- crates/editor/src/movement.rs | 8 +- crates/editor/src/scroll.rs | 6 +- crates/editor/src/selections_collection.rs | 9 ++ crates/multi_buffer/src/anchor.rs | 7 + crates/multi_buffer/src/multi_buffer.rs | 153 +++++++-------------- crates/vim/src/helix.rs | 2 +- 12 files changed, 114 insertions(+), 164 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 79bad1d319f459805ca82b27e451435df6e064c9..d4c068adceccb1aae23d76544370212728ee0151 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3211,22 +3211,27 @@ impl Editor { let background_executor = cx.background_executor().clone(); let editor_id = cx.entity().entity_id().as_u64() as ItemId; self.serialize_selections = cx.background_spawn(async move { - background_executor.timer(SERIALIZATION_THROTTLE_TIME).await; - let db_selections = selections - .iter() - .map(|selection| { - ( - selection.start.to_offset(&snapshot), - selection.end.to_offset(&snapshot), - ) - }) - .collect(); + background_executor.timer(SERIALIZATION_THROTTLE_TIME).await; + let db_selections = selections + .iter() + .map(|selection| { + ( + selection.start.to_offset(&snapshot), + selection.end.to_offset(&snapshot), + ) + }) + .collect(); - DB.save_editor_selections(editor_id, workspace_id, db_selections) - .await - .with_context(|| format!("persisting editor selections for editor {editor_id}, workspace {workspace_id:?}")) - .log_err(); - }); + DB.save_editor_selections(editor_id, workspace_id, db_selections) + .await + .with_context(|| { + format!( + "persisting editor selections for editor {editor_id}, \ + workspace {workspace_id:?}" + ) + }) + .log_err(); + }); } } @@ -6871,17 +6876,7 @@ impl Editor { continue; } - let range = Anchor { - buffer_id: Some(buffer_id), - excerpt_id, - text_anchor: start, - diff_base_anchor: None, - }..Anchor { - buffer_id: Some(buffer_id), - excerpt_id, - text_anchor: end, - diff_base_anchor: None, - }; + let range = Anchor::range_in_buffer(excerpt_id, buffer_id, start..end); if highlight.kind == lsp::DocumentHighlightKind::WRITE { write_ranges.push(range); } else { @@ -12886,7 +12881,7 @@ impl Editor { self.hide_mouse_cursor(HideMouseCursorOrigin::MovementAction, cx); self.change_selections(Default::default(), window, cx, |s| { s.move_heads_with(|map, head, _| (movement::right(map, head), SelectionGoal::None)); - }) + }); } pub fn move_up(&mut self, _: &MoveUp, window: &mut Window, cx: &mut Context) { @@ -15873,7 +15868,7 @@ impl Editor { let snapshot = multi_buffer.snapshot(cx); if let Some(buffer_id) = snapshot.buffer_id_for_excerpt(excerpt) && let Some(buffer) = multi_buffer.buffer(buffer_id) - && let Some(excerpt_range) = snapshot.buffer_range_for_excerpt(excerpt) + && let Some(excerpt_range) = snapshot.context_range_for_excerpt(excerpt) { let buffer_snapshot = buffer.read(cx).snapshot(); let excerpt_end_row = Point::from_anchor(&excerpt_range.end, &buffer_snapshot).row; diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 340b03a710e336263b8f98b989577b3c98af75bc..ddd49599065cf089cfc627720033c8f27c45b50c 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -1311,10 +1311,10 @@ impl EditorElement { let range = snapshot .buffer_snapshot - .anchor_at(start.to_point(&snapshot.display_snapshot), Bias::Left) + .anchor_before(start.to_point(&snapshot.display_snapshot)) ..snapshot .buffer_snapshot - .anchor_at(end.to_point(&snapshot.display_snapshot), Bias::Right); + .anchor_after(end.to_point(&snapshot.display_snapshot)); let Some(selection) = snapshot.remote_selections_in_range(&range, hub, cx).next() else { return; diff --git a/crates/editor/src/hover_links.rs b/crates/editor/src/hover_links.rs index 9f212adbd2059dc516b726cdf3574232ddcbe1a9..5bb2617679dcdd501d24964a3be6bed72485d27d 100644 --- a/crates/editor/src/hover_links.rs +++ b/crates/editor/src/hover_links.rs @@ -301,14 +301,10 @@ pub fn update_inlay_link_and_hover_points( let mut hover_updated = false; if let Some(hovered_offset) = hovered_offset { let buffer_snapshot = editor.buffer().read(cx).snapshot(cx); - let previous_valid_anchor = buffer_snapshot.anchor_at( - point_for_position.previous_valid.to_point(snapshot), - Bias::Left, - ); - let next_valid_anchor = buffer_snapshot.anchor_at( - point_for_position.next_valid.to_point(snapshot), - Bias::Right, - ); + let previous_valid_anchor = + buffer_snapshot.anchor_before(point_for_position.previous_valid.to_point(snapshot)); + let next_valid_anchor = + buffer_snapshot.anchor_after(point_for_position.next_valid.to_point(snapshot)); if let Some(hovered_hint) = editor .visible_inlay_hints(cx) .into_iter() @@ -1400,7 +1396,7 @@ mod tests { let buffer_snapshot = editor.buffer().update(cx, |buffer, cx| buffer.snapshot(cx)); let expected_highlight = InlayHighlight { inlay: InlayId::Hint(0), - inlay_position: buffer_snapshot.anchor_at(inlay_range.start, Bias::Right), + inlay_position: buffer_snapshot.anchor_after(inlay_range.start), range: 0..hint_label.len(), }; assert_set_eq!(actual_highlights, vec![&expected_highlight]); diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index 07a9f07cc5573fa0f3695e3247697d62f2bace72..e47ff135e5fb0592089386830246255c09eba6fa 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -1785,7 +1785,7 @@ mod tests { popover.symbol_range, RangeInEditor::Inlay(InlayHighlight { inlay: InlayId::Hint(0), - inlay_position: buffer_snapshot.anchor_at(inlay_range.start, Bias::Right), + inlay_position: buffer_snapshot.anchor_after(inlay_range.start), range: ": ".len()..": ".len() + new_type_label.len(), }), "Popover range should match the new type label part" @@ -1840,7 +1840,7 @@ mod tests { popover.symbol_range, RangeInEditor::Inlay(InlayHighlight { inlay: InlayId::Hint(0), - inlay_position: buffer_snapshot.anchor_at(inlay_range.start, Bias::Right), + inlay_position: buffer_snapshot.anchor_after(inlay_range.start), range: ": ".len() + new_type_label.len() + "<".len() ..": ".len() + new_type_label.len() + "<".len() + struct_label.len(), }), diff --git a/crates/editor/src/inlay_hint_cache.rs b/crates/editor/src/inlay_hint_cache.rs index f2a715420e34050bf61ae862a8dc15b88848fa19..63d74c73e12ef0c56490a2a51371c34e3c356ae3 100644 --- a/crates/editor/src/inlay_hint_cache.rs +++ b/crates/editor/src/inlay_hint_cache.rs @@ -2251,7 +2251,7 @@ pub mod tests { .unwrap(); } - #[gpui::test(iterations = 10)] + #[gpui::test(iterations = 4)] async fn test_large_buffer_inlay_requests_split(cx: &mut gpui::TestAppContext) { init_test(cx, |settings| { settings.defaults.inlay_hints = Some(InlayHintSettingsContent { diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 08174158edafe917402fc8603716351cd7b6d50b..3989653fafa543f4aa25a1f27c2a85bec2f37951 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -578,12 +578,11 @@ fn deserialize_selection( fn deserialize_anchor(buffer: &MultiBufferSnapshot, anchor: proto::EditorAnchor) -> Option { let excerpt_id = ExcerptId::from_proto(anchor.excerpt_id); - Some(Anchor { + Some(Anchor::in_buffer( excerpt_id, - text_anchor: language::proto::deserialize_anchor(anchor.anchor?)?, - buffer_id: buffer.buffer_id_for_excerpt(excerpt_id), - diff_base_anchor: None, - }) + buffer.buffer_id_for_excerpt(excerpt_id)?, + language::proto::deserialize_anchor(anchor.anchor?)?, + )) } impl Item for Editor { @@ -1752,13 +1751,8 @@ impl SearchableItem for Editor { .anchor_after(search_range.start + match_range.start); let end = search_buffer .anchor_before(search_range.start + match_range.end); - Anchor { - diff_base_anchor: Some(start), - ..deleted_hunk_anchor - }..Anchor { - diff_base_anchor: Some(end), - ..deleted_hunk_anchor - } + deleted_hunk_anchor.with_diff_base_anchor(start) + ..deleted_hunk_anchor.with_diff_base_anchor(end) } else { let start = search_buffer .anchor_after(search_range.start + match_range.start); diff --git a/crates/editor/src/movement.rs b/crates/editor/src/movement.rs index 4bd353a2873431d8102dfc15dea9a74ac2b2c241..a832898a591d77dcdc4e66d2b0252e173c46f046 100644 --- a/crates/editor/src/movement.rs +++ b/crates/editor/src/movement.rs @@ -1018,22 +1018,22 @@ mod tests { [ Inlay::edit_prediction( post_inc(&mut id), - buffer_snapshot.anchor_at(offset, Bias::Left), + buffer_snapshot.anchor_before(offset), "test", ), Inlay::edit_prediction( post_inc(&mut id), - buffer_snapshot.anchor_at(offset, Bias::Right), + buffer_snapshot.anchor_after(offset), "test", ), Inlay::mock_hint( post_inc(&mut id), - buffer_snapshot.anchor_at(offset, Bias::Left), + buffer_snapshot.anchor_before(offset), "test", ), Inlay::mock_hint( post_inc(&mut id), - buffer_snapshot.anchor_at(offset, Bias::Right), + buffer_snapshot.anchor_after(offset), "test", ), ] diff --git a/crates/editor/src/scroll.rs b/crates/editor/src/scroll.rs index 828ab0594daf09081bf10af02ec5a4a6a470cd7d..574ab5edd0e97e3a1b207ec9f4bdd88afce1283a 100644 --- a/crates/editor/src/scroll.rs +++ b/crates/editor/src/scroll.rs @@ -244,9 +244,7 @@ impl ScrollManager { Bias::Left, ) .to_point(map); - let top_anchor = map - .buffer_snapshot - .anchor_at(scroll_top_buffer_point, Bias::Right); + let top_anchor = map.buffer_snapshot.anchor_after(scroll_top_buffer_point); self.set_anchor( ScrollAnchor { @@ -767,7 +765,7 @@ impl Editor { .buffer() .read(cx) .snapshot(cx) - .anchor_at(Point::new(top_row, 0), Bias::Left); + .anchor_before(Point::new(top_row, 0)); let scroll_anchor = ScrollAnchor { offset: gpui::Point::new(x, y), anchor: top_anchor, diff --git a/crates/editor/src/selections_collection.rs b/crates/editor/src/selections_collection.rs index 7fb44ab4b413f319e5b27983489fdc21dd8b92ce..517a9dc6391caa9c60b366a229c721daaeae8533 100644 --- a/crates/editor/src/selections_collection.rs +++ b/crates/editor/src/selections_collection.rs @@ -440,6 +440,15 @@ pub struct MutableSelectionsCollection<'a> { cx: &'a mut App, } +impl<'a> fmt::Debug for MutableSelectionsCollection<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("MutableSelectionsCollection") + .field("collection", &self.collection) + .field("selections_changed", &self.selections_changed) + .finish() + } +} + impl<'a> MutableSelectionsCollection<'a> { pub fn display_map(&mut self) -> DisplaySnapshot { self.collection.display_map(self.cx) diff --git a/crates/multi_buffer/src/anchor.rs b/crates/multi_buffer/src/anchor.rs index 6bed0a4028c5c4b0816355a397046560fb6b8618..a2498cb02fb836c6a70af9407d2a4e520c9d3d3b 100644 --- a/crates/multi_buffer/src/anchor.rs +++ b/crates/multi_buffer/src/anchor.rs @@ -16,6 +16,13 @@ pub struct Anchor { } impl Anchor { + pub fn with_diff_base_anchor(self, diff_base_anchor: text::Anchor) -> Self { + Self { + diff_base_anchor: Some(diff_base_anchor), + ..self + } + } + pub fn in_buffer( excerpt_id: ExcerptId, buffer_id: BufferId, diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 8834dd9f70853ee922d6d2e5f24ce51f6f2ac079..591398d38e86bdceac6c7aafa36819e12bc621da 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -1310,11 +1310,9 @@ impl MultiBuffer { let end_locator = snapshot.excerpt_locator_for_id(selection.end.excerpt_id); cursor.seek(&Some(start_locator), Bias::Left); - while let Some(excerpt) = cursor.item() { - if excerpt.locator > *end_locator { - break; - } - + while let Some(excerpt) = cursor.item() + && excerpt.locator <= *end_locator + { let mut start = excerpt.range.context.start; let mut end = excerpt.range.context.end; if excerpt.id == selection.start.excerpt_id { @@ -4793,7 +4791,6 @@ impl MultiBufferSnapshot { where D: TextDimension, { - // let mut range = range.start..range.end; let mut summary = D::zero(()); let mut cursor = self.excerpts.cursor::(()); cursor.seek(&range.start, Bias::Right); @@ -4917,13 +4914,13 @@ impl MultiBufferSnapshot { let locator = self.excerpt_locator_for_id(anchor.excerpt_id); cursor.seek(&Some(locator), Bias::Left); - if cursor.item().is_none() { - cursor.next(); + if cursor.item().is_none() && anchor.excerpt_id == ExcerptId::max() { + cursor.prev(); } let mut position = cursor.start().1; if let Some(excerpt) = cursor.item() - && excerpt.id == anchor.excerpt_id + && (excerpt.id == anchor.excerpt_id || anchor.excerpt_id == ExcerptId::max()) { let excerpt_buffer_start = excerpt .buffer @@ -5086,20 +5083,14 @@ impl MultiBufferSnapshot { let old_locator = self.excerpt_locator_for_id(old_excerpt_id); cursor.seek_forward(&Some(old_locator), Bias::Left); - if cursor.item().is_none() { - cursor.next(); - } - let next_excerpt = cursor.item(); let prev_excerpt = cursor.prev_item(); // Process all of the anchors for this excerpt. - while let Some((_, anchor)) = anchors.peek() { - if anchor.excerpt_id != old_excerpt_id { - break; - } - let (anchor_ix, anchor) = anchors.next().unwrap(); - let mut anchor = *anchor; + while let Some((anchor_ix, &anchor)) = + anchors.next_if(|(_, anchor)| anchor.excerpt_id == old_excerpt_id) + { + let mut anchor = anchor; // Leave min and max anchors unchanged if invalid or // if the old excerpt still exists at this location @@ -5135,12 +5126,7 @@ impl MultiBufferSnapshot { { text_anchor = excerpt.range.context.end; } - Anchor { - buffer_id: Some(excerpt.buffer_id), - excerpt_id: excerpt.id, - text_anchor, - diff_base_anchor: None, - } + Anchor::in_buffer(excerpt.id, excerpt.buffer_id, text_anchor) } else if let Some(excerpt) = prev_excerpt { let mut text_anchor = excerpt .range @@ -5153,12 +5139,7 @@ impl MultiBufferSnapshot { { text_anchor = excerpt.range.context.start; } - Anchor { - buffer_id: Some(excerpt.buffer_id), - excerpt_id: excerpt.id, - text_anchor, - diff_base_anchor: None, - } + Anchor::in_buffer(excerpt.id, excerpt.buffer_id, text_anchor) } else if anchor.text_anchor.bias == Bias::Left { Anchor::min() } else { @@ -5240,24 +5221,15 @@ impl MultiBufferSnapshot { let buffer_start = excerpt.range.context.start.to_offset(&excerpt.buffer); let text_anchor = excerpt.clip_anchor(excerpt.buffer.anchor_at(buffer_start + overshoot, bias)); - Anchor { - buffer_id: Some(excerpt.buffer_id), - excerpt_id: excerpt.id, - text_anchor, - diff_base_anchor, + let anchor = Anchor::in_buffer(excerpt.id, excerpt.buffer_id, text_anchor); + match diff_base_anchor { + Some(diff_base_anchor) => anchor.with_diff_base_anchor(diff_base_anchor), + None => anchor, } + } else if excerpt_offset.is_zero() && bias == Bias::Left { + Anchor::min() } else { - let mut anchor = if excerpt_offset.is_zero() && bias == Bias::Left { - Anchor::min() - } else { - Anchor::max() - }; - - // TODO this is a hack, because all APIs should be able to handle ExcerptId::min and max. - if let Some((excerpt_id, _, _)) = self.as_singleton() { - anchor.excerpt_id = *excerpt_id; - } - anchor + Anchor::max() } } @@ -5269,22 +5241,12 @@ impl MultiBufferSnapshot { text_anchor: text::Anchor, ) -> Option { let excerpt_id = self.latest_excerpt_id(excerpt_id); - let locator = self.excerpt_locator_for_id(excerpt_id); - let mut cursor = self.excerpts.cursor::>(()); - cursor.seek(locator, Bias::Left); - if let Some(excerpt) = cursor.item() - && excerpt.id == excerpt_id - { - let text_anchor = excerpt.clip_anchor(text_anchor); - drop(cursor); - return Some(Anchor { - buffer_id: Some(excerpt.buffer_id), - excerpt_id, - text_anchor, - diff_base_anchor: None, - }); - } - None + let excerpt = self.excerpt(excerpt_id)?; + Some(Anchor::in_buffer( + excerpt_id, + excerpt.buffer_id, + text_anchor, + )) } pub fn context_range_for_excerpt(&self, excerpt_id: ExcerptId) -> Option> { @@ -5320,8 +5282,8 @@ impl MultiBufferSnapshot { } } - pub fn excerpt_before(&self, id: ExcerptId) -> Option> { - let start_locator = self.excerpt_locator_for_id(id); + pub fn excerpt_before(&self, excerpt_id: ExcerptId) -> Option> { + let start_locator = self.excerpt_locator_for_id(excerpt_id); let mut excerpts = self .excerpts .cursor::, ExcerptDimension>>(()); @@ -6240,7 +6202,14 @@ impl MultiBufferSnapshot { .excerpts .cursor::, ExcerptDimension>>(()); let locator = self.excerpt_locator_for_id(excerpt_id); - if cursor.seek(&Some(locator), Bias::Left) { + let mut sought_exact = cursor.seek(&Some(locator), Bias::Left); + if excerpt_id == ExcerptId::max() { + sought_exact = true; + cursor.prev(); + } else if excerpt_id == ExcerptId::min() { + sought_exact = true; + } + if sought_exact { let start = cursor.start().1.clone(); let end = cursor.end().1; let mut diff_transforms = self @@ -6258,17 +6227,6 @@ impl MultiBufferSnapshot { } } - pub fn buffer_range_for_excerpt(&self, excerpt_id: ExcerptId) -> Option> { - let mut cursor = self.excerpts.cursor::>(()); - let locator = self.excerpt_locator_for_id(excerpt_id); - if cursor.seek(&Some(locator), Bias::Left) - && let Some(excerpt) = cursor.item() - { - return Some(excerpt.range.context.clone()); - } - None - } - fn excerpt(&self, excerpt_id: ExcerptId) -> Option<&Excerpt> { let mut cursor = self.excerpts.cursor::>(()); let locator = self.excerpt_locator_for_id(excerpt_id); @@ -6277,6 +6235,9 @@ impl MultiBufferSnapshot { && excerpt.id == excerpt_id { return Some(excerpt); + } else if excerpt_id == ExcerptId::max() { + cursor.prev(); + return cursor.item(); } None } @@ -6345,18 +6306,10 @@ impl MultiBufferSnapshot { .selections_in_range(query_range, include_local) .flat_map(move |(replica_id, line_mode, cursor_shape, selections)| { selections.map(move |selection| { - let mut start = Anchor { - buffer_id: Some(excerpt.buffer_id), - excerpt_id: excerpt.id, - text_anchor: selection.start, - diff_base_anchor: None, - }; - let mut end = Anchor { - buffer_id: Some(excerpt.buffer_id), - excerpt_id: excerpt.id, - text_anchor: selection.end, - diff_base_anchor: None, - }; + let mut start = + Anchor::in_buffer(excerpt.id, excerpt.buffer_id, selection.start); + let mut end = + Anchor::in_buffer(excerpt.id, excerpt.buffer_id, selection.end); if range.start.cmp(&start, self).is_gt() { start = range.start; } @@ -7073,21 +7026,19 @@ impl<'a> MultiBufferExcerpt<'a> { } pub fn start_anchor(&self) -> Anchor { - Anchor { - buffer_id: Some(self.excerpt.buffer_id), - excerpt_id: self.excerpt.id, - text_anchor: self.excerpt.range.context.start, - diff_base_anchor: None, - } + Anchor::in_buffer( + self.excerpt.id, + self.excerpt.buffer_id, + self.excerpt.range.context.start, + ) } pub fn end_anchor(&self) -> Anchor { - Anchor { - buffer_id: Some(self.excerpt.buffer_id), - excerpt_id: self.excerpt.id, - text_anchor: self.excerpt.range.context.end, - diff_base_anchor: None, - } + Anchor::in_buffer( + self.excerpt.id, + self.excerpt.buffer_id, + self.excerpt.range.context.end, + ) } pub fn buffer(&self) -> &'a BufferSnapshot { diff --git a/crates/vim/src/helix.rs b/crates/vim/src/helix.rs index e2174fe1a7adee3f5b6cdac5167f2c28fb3296c1..fabffe7484ca440da2c7ff39f5d39fbe6109aef3 100644 --- a/crates/vim/src/helix.rs +++ b/crates/vim/src/helix.rs @@ -467,7 +467,7 @@ impl Vim { let was_empty = range.is_empty(); let was_reversed = selection.reversed; ( - map.buffer_snapshot.anchor_at(start_offset, Bias::Left), + map.buffer_snapshot.anchor_before(start_offset), end_offset - start_offset, was_empty, was_reversed,