Defer navigation target buffer opening

Kirill Bulatov created

Change summary

crates/editor/src/editor.rs                | 222 +++++++++++++++++------
crates/editor/src/element.rs               |   8 
crates/editor/src/link_go_to_definition.rs | 190 ++++++++------------
3 files changed, 246 insertions(+), 174 deletions(-)

Detailed changes

crates/editor/src/editor.rs 🔗

@@ -23,7 +23,7 @@ pub mod test;
 
 use ::git::diff::DiffHunk;
 use aho_corasick::AhoCorasick;
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, Context, Result};
 use blink_manager::BlinkManager;
 use client::{ClickhouseEvent, TelemetrySettings};
 use clock::{Global, ReplicaId};
@@ -60,21 +60,24 @@ use itertools::Itertools;
 pub use language::{char_kind, CharKind};
 use language::{
     language_settings::{self, all_language_settings, InlayHintSettings},
-    AutoindentMode, BracketPair, Buffer, CodeAction, CodeLabel, Completion, CursorShape,
-    Diagnostic, DiagnosticSeverity, File, IndentKind, IndentSize, Language, OffsetRangeExt,
-    OffsetUtf16, Point, Selection, SelectionGoal, TransactionId,
+    point_from_lsp, AutoindentMode, BracketPair, Buffer, CodeAction, CodeLabel, Completion,
+    CursorShape, Diagnostic, DiagnosticSeverity, File, IndentKind, IndentSize, Language,
+    LanguageServerName, OffsetRangeExt, OffsetUtf16, Point, Selection, SelectionGoal,
+    TransactionId,
 };
 use link_go_to_definition::{
-    hide_link_definition, show_link_definition, DocumentRange, InlayRange, LinkGoToDefinitionState,
+    hide_link_definition, show_link_definition, DocumentRange, GoToDefinitionLink, InlayRange,
+    LinkGoToDefinitionState,
 };
 use log::error;
+use lsp::LanguageServerId;
 use multi_buffer::ToOffsetUtf16;
 pub use multi_buffer::{
     Anchor, AnchorRangeExt, ExcerptId, ExcerptRange, MultiBuffer, MultiBufferSnapshot, ToOffset,
     ToPoint,
 };
 use ordered_float::OrderedFloat;
-use project::{FormatTrigger, Location, LocationLink, Project, ProjectPath, ProjectTransaction};
+use project::{FormatTrigger, Location, Project, ProjectPath, ProjectTransaction};
 use rand::{seq::SliceRandom, thread_rng};
 use scroll::{
     autoscroll::Autoscroll, OngoingScroll, ScrollAnchor, ScrollManager, ScrollbarAutoHide,
@@ -6551,7 +6554,14 @@ impl Editor {
         cx.spawn_labeled("Fetching Definition...", |editor, mut cx| async move {
             let definitions = definitions.await?;
             editor.update(&mut cx, |editor, cx| {
-                editor.navigate_to_definitions(definitions, split, cx);
+                editor.navigate_to_definitions(
+                    definitions
+                        .into_iter()
+                        .map(GoToDefinitionLink::Text)
+                        .collect(),
+                    split,
+                    cx,
+                );
             })?;
             Ok::<(), anyhow::Error>(())
         })
@@ -6560,7 +6570,7 @@ impl Editor {
 
     pub fn navigate_to_definitions(
         &mut self,
-        mut definitions: Vec<LocationLink>,
+        mut definitions: Vec<GoToDefinitionLink>,
         split: bool,
         cx: &mut ViewContext<Editor>,
     ) {
@@ -6571,67 +6581,167 @@ impl Editor {
         // If there is one definition, just open it directly
         if definitions.len() == 1 {
             let definition = definitions.pop().unwrap();
-            let range = definition
-                .target
-                .range
-                .to_offset(definition.target.buffer.read(cx));
-
-            let range = self.range_for_match(&range);
-            if Some(&definition.target.buffer) == self.buffer.read(cx).as_singleton().as_ref() {
-                self.change_selections(Some(Autoscroll::fit()), cx, |s| {
-                    s.select_ranges([range]);
-                });
-            } else {
-                cx.window_context().defer(move |cx| {
-                    let target_editor: ViewHandle<Self> = workspace.update(cx, |workspace, cx| {
-                        if split {
-                            workspace.split_project_item(definition.target.buffer.clone(), cx)
+            let target_task = match definition {
+                GoToDefinitionLink::Text(link) => Task::Ready(Some(Ok(Some(link.target)))),
+                GoToDefinitionLink::InlayHint(lsp_location, server_id) => {
+                    self.compute_target_location(lsp_location, server_id, cx)
+                }
+            };
+            cx.spawn(|editor, mut cx| async move {
+                let target = target_task.await.context("target resolution task")?;
+                if let Some(target) = target {
+                    editor.update(&mut cx, |editor, cx| {
+                        let range = target.range.to_offset(target.buffer.read(cx));
+                        let range = editor.range_for_match(&range);
+                        if Some(&target.buffer) == editor.buffer.read(cx).as_singleton().as_ref() {
+                            editor.change_selections(Some(Autoscroll::fit()), cx, |s| {
+                                s.select_ranges([range]);
+                            });
                         } else {
-                            workspace.open_project_item(definition.target.buffer.clone(), cx)
+                            cx.window_context().defer(move |cx| {
+                                let target_editor: ViewHandle<Self> =
+                                    workspace.update(cx, |workspace, cx| {
+                                        if split {
+                                            workspace.split_project_item(target.buffer.clone(), cx)
+                                        } else {
+                                            workspace.open_project_item(target.buffer.clone(), cx)
+                                        }
+                                    });
+                                target_editor.update(cx, |target_editor, cx| {
+                                    // When selecting a definition in a different buffer, disable the nav history
+                                    // to avoid creating a history entry at the previous cursor location.
+                                    pane.update(cx, |pane, _| pane.disable_history());
+                                    target_editor.change_selections(
+                                        Some(Autoscroll::fit()),
+                                        cx,
+                                        |s| {
+                                            s.select_ranges([range]);
+                                        },
+                                    );
+                                    pane.update(cx, |pane, _| pane.enable_history());
+                                });
+                            });
                         }
-                    });
-                    target_editor.update(cx, |target_editor, cx| {
-                        // When selecting a definition in a different buffer, disable the nav history
-                        // to avoid creating a history entry at the previous cursor location.
-                        pane.update(cx, |pane, _| pane.disable_history());
-                        target_editor.change_selections(Some(Autoscroll::fit()), cx, |s| {
-                            s.select_ranges([range]);
-                        });
-                        pane.update(cx, |pane, _| pane.enable_history());
-                    });
-                });
-            }
+                    })
+                } else {
+                    Ok(())
+                }
+            })
+            .detach_and_log_err(cx);
         } else if !definitions.is_empty() {
             let replica_id = self.replica_id(cx);
-            cx.window_context().defer(move |cx| {
-                let title = definitions
-                    .iter()
-                    .find(|definition| definition.origin.is_some())
-                    .and_then(|definition| {
-                        definition.origin.as_ref().map(|origin| {
-                            let buffer = origin.buffer.read(cx);
-                            format!(
-                                "Definitions for {}",
-                                buffer
-                                    .text_for_range(origin.range.clone())
-                                    .collect::<String>()
-                            )
-                        })
+            cx.spawn(|editor, mut cx| async move {
+                let (title, location_tasks) = editor
+                    .update(&mut cx, |editor, cx| {
+                        let title = definitions
+                            .iter()
+                            .find_map(|definition| match definition {
+                                GoToDefinitionLink::Text(link) => {
+                                    link.origin.as_ref().map(|origin| {
+                                        let buffer = origin.buffer.read(cx);
+                                        format!(
+                                            "Definitions for {}",
+                                            buffer
+                                                .text_for_range(origin.range.clone())
+                                                .collect::<String>()
+                                        )
+                                    })
+                                }
+                                GoToDefinitionLink::InlayHint(_, _) => None,
+                            })
+                            .unwrap_or("Definitions".to_string());
+                        let location_tasks = definitions
+                            .into_iter()
+                            .map(|definition| match definition {
+                                GoToDefinitionLink::Text(link) => {
+                                    Task::Ready(Some(Ok(Some(link.target))))
+                                }
+                                GoToDefinitionLink::InlayHint(lsp_location, server_id) => {
+                                    editor.compute_target_location(lsp_location, server_id, cx)
+                                }
+                            })
+                            .collect::<Vec<_>>();
+                        (title, location_tasks)
                     })
-                    .unwrap_or("Definitions".to_owned());
-                let locations = definitions
+                    .context("location tasks preparation")?;
+
+                let locations = futures::future::join_all(location_tasks)
+                    .await
                     .into_iter()
-                    .map(|definition| definition.target)
-                    .collect();
-                workspace.update(cx, |workspace, cx| {
+                    .filter_map(|location| location.transpose())
+                    .collect::<Result<_>>()
+                    .context("location tasks")?;
+                workspace.update(&mut cx, |workspace, cx| {
                     Self::open_locations_in_multibuffer(
                         workspace, locations, replica_id, title, split, cx,
                     )
                 });
-            });
+
+                anyhow::Ok(())
+            })
+            .detach_and_log_err(cx);
         }
     }
 
+    fn compute_target_location(
+        &self,
+        lsp_location: lsp::Location,
+        server_id: LanguageServerId,
+        cx: &mut ViewContext<Editor>,
+    ) -> Task<anyhow::Result<Option<Location>>> {
+        let Some(project) = self.project.clone() else {
+            return Task::Ready(Some(Ok(None)));
+        };
+
+        cx.spawn(move |editor, mut cx| async move {
+            let location_task = editor.update(&mut cx, |editor, cx| {
+                project.update(cx, |project, cx| {
+                    let language_server_name =
+                        editor.buffer.read(cx).as_singleton().and_then(|buffer| {
+                            project
+                                .language_server_for_buffer(buffer.read(cx), server_id, cx)
+                                .map(|(_, lsp_adapter)| {
+                                    LanguageServerName(Arc::from(lsp_adapter.name()))
+                                })
+                        });
+                    language_server_name.map(|language_server_name| {
+                        project.open_local_buffer_via_lsp(
+                            lsp_location.uri.clone(),
+                            server_id,
+                            language_server_name,
+                            cx,
+                        )
+                    })
+                })
+            })?;
+            let location = match location_task {
+                Some(task) => Some({
+                    let target_buffer_handle = task.await.context("open local buffer")?;
+                    let range = {
+                        target_buffer_handle.update(&mut cx, |target_buffer, _| {
+                            let target_start = target_buffer.clip_point_utf16(
+                                point_from_lsp(lsp_location.range.start),
+                                Bias::Left,
+                            );
+                            let target_end = target_buffer.clip_point_utf16(
+                                point_from_lsp(lsp_location.range.end),
+                                Bias::Left,
+                            );
+                            target_buffer.anchor_after(target_start)
+                                ..target_buffer.anchor_before(target_end)
+                        })
+                    };
+                    Location {
+                        buffer: target_buffer_handle,
+                        range,
+                    }
+                }),
+                None => None,
+            };
+            Ok(location)
+        })
+    }
+
     pub fn find_all_references(
         workspace: &mut Workspace,
         _: &FindAllReferences,

crates/editor/src/element.rs 🔗

@@ -395,9 +395,7 @@ impl EditorElement {
 
         update_go_to_definition_link(
             editor,
-            point
-                .map(GoToDefinitionTrigger::Text)
-                .unwrap_or(GoToDefinitionTrigger::None),
+            point.map(GoToDefinitionTrigger::Text),
             cmd,
             shift,
             cx,
@@ -468,7 +466,7 @@ impl EditorElement {
                 Some(point) => {
                     update_go_to_definition_link(
                         editor,
-                        GoToDefinitionTrigger::Text(point),
+                        Some(GoToDefinitionTrigger::Text(point)),
                         cmd,
                         shift,
                         cx,
@@ -487,7 +485,7 @@ impl EditorElement {
                 }
             }
         } else {
-            update_go_to_definition_link(editor, GoToDefinitionTrigger::None, cmd, shift, cx);
+            update_go_to_definition_link(editor, None, cmd, shift, cx);
             hover_at(editor, None, cx);
         }
 
@@ -4,15 +4,14 @@ use crate::{
     hover_popover::{self, InlayHover},
     Anchor, DisplayPoint, Editor, EditorSnapshot, SelectPhase,
 };
-use anyhow::Context;
 use gpui::{Task, ViewContext};
-use language::{point_from_lsp, Bias, LanguageServerName, ToOffset};
+use language::{Bias, ToOffset};
 use lsp::LanguageServerId;
 use project::{
-    HoverBlock, HoverBlockKind, InlayHintLabelPartTooltip, InlayHintTooltip, Location,
-    LocationLink, ResolveState,
+    HoverBlock, HoverBlockKind, InlayHintLabelPartTooltip, InlayHintTooltip, LocationLink,
+    ResolveState,
 };
-use std::{ops::Range, sync::Arc};
+use std::ops::Range;
 use util::TryFutureExt;
 
 #[derive(Debug, Default)]
@@ -20,14 +19,19 @@ pub struct LinkGoToDefinitionState {
     pub last_trigger_point: Option<TriggerPoint>,
     pub symbol_range: Option<DocumentRange>,
     pub kind: Option<LinkDefinitionKind>,
-    pub definitions: Vec<LocationLink>,
+    pub definitions: Vec<GoToDefinitionLink>,
     pub task: Option<Task<Option<()>>>,
 }
 
 pub enum GoToDefinitionTrigger {
     Text(DisplayPoint),
     InlayHint(InlayRange, lsp::Location, LanguageServerId),
-    None,
+}
+
+#[derive(Debug, Clone)]
+pub enum GoToDefinitionLink {
+    Text(LocationLink),
+    InlayHint(lsp::Location, LanguageServerId),
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -97,7 +101,7 @@ impl TriggerPoint {
 
 pub fn update_go_to_definition_link(
     editor: &mut Editor,
-    origin: GoToDefinitionTrigger,
+    origin: Option<GoToDefinitionTrigger>,
     cmd_held: bool,
     shift_held: bool,
     cx: &mut ViewContext<Editor>,
@@ -107,15 +111,15 @@ pub fn update_go_to_definition_link(
     // Store new mouse point as an anchor
     let snapshot = editor.snapshot(cx);
     let trigger_point = match origin {
-        GoToDefinitionTrigger::Text(p) => {
+        Some(GoToDefinitionTrigger::Text(p)) => {
             Some(TriggerPoint::Text(snapshot.buffer_snapshot.anchor_before(
                 p.to_offset(&snapshot.display_snapshot, Bias::Left),
             )))
         }
-        GoToDefinitionTrigger::InlayHint(p, lsp_location, language_server_id) => {
+        Some(GoToDefinitionTrigger::InlayHint(p, lsp_location, language_server_id)) => {
             Some(TriggerPoint::InlayHint(p, lsp_location, language_server_id))
         }
-        GoToDefinitionTrigger::None => None,
+        None => None,
     };
 
     // If the new point is the same as the previously stored one, return early
@@ -287,7 +291,7 @@ pub fn update_inlay_link_and_hover_points(
                                         go_to_definition_updated = true;
                                         update_go_to_definition_link(
                                             editor,
-                                            GoToDefinitionTrigger::InlayHint(
+                                            Some(GoToDefinitionTrigger::InlayHint(
                                                 InlayRange {
                                                     inlay_position: hovered_hint.position,
                                                     highlight_start: part_range.start,
@@ -295,7 +299,7 @@ pub fn update_inlay_link_and_hover_points(
                                                 },
                                                 location,
                                                 language_server_id,
-                                            ),
+                                            )),
                                             cmd_held,
                                             shift_held,
                                             cx,
@@ -311,13 +315,7 @@ pub fn update_inlay_link_and_hover_points(
         }
 
         if !go_to_definition_updated {
-            update_go_to_definition_link(
-                editor,
-                GoToDefinitionTrigger::None,
-                cmd_held,
-                shift_held,
-                cx,
-            );
+            update_go_to_definition_link(editor, None, cmd_held, shift_held, cx);
         }
         if !hover_updated {
             hover_popover::hover_at(editor, None, cx);
@@ -412,63 +410,20 @@ pub fn show_link_definition(
                                     DocumentRange::Text(start..end)
                                 })
                             }),
-                            definition_result,
-                        )
-                    })
-                }
-                TriggerPoint::InlayHint(trigger_source, lsp_location, server_id) => {
-                    let target = match project.update(&mut cx, |project, cx| {
-                        let language_server_name = project
-                            .language_server_for_buffer(buffer.read(cx), *server_id, cx)
-                            .map(|(_, lsp_adapter)| {
-                                LanguageServerName(Arc::from(lsp_adapter.name()))
-                            });
-                        language_server_name.map(|language_server_name| {
-                            project.open_local_buffer_via_lsp(
-                                lsp_location.uri.clone(),
-                                *server_id,
-                                language_server_name,
-                                cx,
-                            )
-                        })
-                    }) {
-                        Some(task) => Some({
-                            let target_buffer_handle = task.await.context("open local buffer")?;
-                            let range = cx.read(|cx| {
-                                let target_buffer = target_buffer_handle.read(cx);
-                                let target_start = target_buffer.clip_point_utf16(
-                                    point_from_lsp(lsp_location.range.start),
-                                    Bias::Left,
-                                );
-                                let target_end = target_buffer.clip_point_utf16(
-                                    point_from_lsp(lsp_location.range.end),
-                                    Bias::Left,
-                                );
-                                target_buffer.anchor_after(target_start)
-                                    ..target_buffer.anchor_before(target_end)
-                            });
-                            Location {
-                                buffer: target_buffer_handle,
-                                range,
-                            }
-                        }),
-                        None => None,
-                    };
-
-                    target.map(|target| {
-                        (
-                            Some(DocumentRange::Inlay(trigger_source.clone())),
-                            vec![LocationLink {
-                                origin: Some(Location {
-                                    buffer: buffer.clone(),
-                                    range: trigger_source.inlay_position.text_anchor
-                                        ..trigger_source.inlay_position.text_anchor,
-                                }),
-                                target,
-                            }],
+                            definition_result
+                                .into_iter()
+                                .map(GoToDefinitionLink::Text)
+                                .collect(),
                         )
                     })
                 }
+                TriggerPoint::InlayHint(trigger_source, lsp_location, server_id) => Some((
+                    Some(DocumentRange::Inlay(*trigger_source)),
+                    vec![GoToDefinitionLink::InlayHint(
+                        lsp_location.clone(),
+                        *server_id,
+                    )],
+                )),
             };
 
             this.update(&mut cx, |this, cx| {
@@ -488,43 +443,52 @@ pub fn show_link_definition(
                     // the current location.
                     let any_definition_does_not_contain_current_location =
                         definitions.iter().any(|definition| {
-                            let target = &definition.target;
-                            if target.buffer == buffer {
-                                let range = &target.range;
-                                // Expand range by one character as lsp definition ranges include positions adjacent
-                                // but not contained by the symbol range
-                                let start = buffer_snapshot.clip_offset(
-                                    range.start.to_offset(&buffer_snapshot).saturating_sub(1),
-                                    Bias::Left,
-                                );
-                                let end = buffer_snapshot.clip_offset(
-                                    range.end.to_offset(&buffer_snapshot) + 1,
-                                    Bias::Right,
-                                );
-                                let offset = buffer_position.to_offset(&buffer_snapshot);
-                                !(start <= offset && end >= offset)
-                            } else {
-                                true
+                            match &definition {
+                                GoToDefinitionLink::Text(link) => {
+                                    if link.target.buffer == buffer {
+                                        let range = &link.target.range;
+                                        // Expand range by one character as lsp definition ranges include positions adjacent
+                                        // but not contained by the symbol range
+                                        let start = buffer_snapshot.clip_offset(
+                                            range
+                                                .start
+                                                .to_offset(&buffer_snapshot)
+                                                .saturating_sub(1),
+                                            Bias::Left,
+                                        );
+                                        let end = buffer_snapshot.clip_offset(
+                                            range.end.to_offset(&buffer_snapshot) + 1,
+                                            Bias::Right,
+                                        );
+                                        let offset = buffer_position.to_offset(&buffer_snapshot);
+                                        !(start <= offset && end >= offset)
+                                    } else {
+                                        true
+                                    }
+                                }
+                                GoToDefinitionLink::InlayHint(_, _) => true,
                             }
                         });
 
                     if any_definition_does_not_contain_current_location {
                         // Highlight symbol using theme link definition highlight style
                         let style = theme::current(cx).editor.link_definition;
-                        let highlight_range = symbol_range.unwrap_or_else(|| match trigger_point {
-                            TriggerPoint::Text(trigger_anchor) => {
-                                let snapshot = &snapshot.buffer_snapshot;
-                                // If no symbol range returned from language server, use the surrounding word.
-                                let (offset_range, _) = snapshot.surrounding_word(trigger_anchor);
-                                DocumentRange::Text(
-                                    snapshot.anchor_before(offset_range.start)
-                                        ..snapshot.anchor_after(offset_range.end),
-                                )
-                            }
-                            TriggerPoint::InlayHint(inlay_coordinates, _, _) => {
-                                DocumentRange::Inlay(inlay_coordinates)
-                            }
-                        });
+                        let highlight_range =
+                            symbol_range.unwrap_or_else(|| match &trigger_point {
+                                TriggerPoint::Text(trigger_anchor) => {
+                                    let snapshot = &snapshot.buffer_snapshot;
+                                    // If no symbol range returned from language server, use the surrounding word.
+                                    let (offset_range, _) =
+                                        snapshot.surrounding_word(*trigger_anchor);
+                                    DocumentRange::Text(
+                                        snapshot.anchor_before(offset_range.start)
+                                            ..snapshot.anchor_after(offset_range.end),
+                                    )
+                                }
+                                TriggerPoint::InlayHint(inlay_coordinates, _, _) => {
+                                    DocumentRange::Inlay(*inlay_coordinates)
+                                }
+                            });
 
                         match highlight_range {
                             DocumentRange::Text(text_range) => this
@@ -689,7 +653,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 true,
                 true,
                 cx,
@@ -801,7 +765,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 true,
                 false,
                 cx,
@@ -841,7 +805,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 true,
                 false,
                 cx,
@@ -869,7 +833,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 true,
                 false,
                 cx,
@@ -892,7 +856,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 false,
                 false,
                 cx,
@@ -957,7 +921,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 true,
                 false,
                 cx,
@@ -977,7 +941,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 true,
                 false,
                 cx,
@@ -1079,7 +1043,7 @@ mod tests {
         cx.update_editor(|editor, cx| {
             update_go_to_definition_link(
                 editor,
-                GoToDefinitionTrigger::Text(hover_point),
+                Some(GoToDefinitionTrigger::Text(hover_point)),
                 true,
                 false,
                 cx,