More inlay hover fixes (#2898)

Kirill Bulatov created

Better handle edge cases around cmd+hover around inlays:
* distinguish between same text anchors' trigger: inlay and text
triggers can have the same anchor, but are different
* forbid cmd+click on inlay that has no label part with location
selected
* properly omit throttled inlays that are outside of the visible range

Release Notes:

- N/A

Change summary

crates/editor/src/editor.rs                |   8 
crates/editor/src/element.rs               |   2 
crates/editor/src/hover_popover.rs         |  54 ++++---
crates/editor/src/inlay_hint_cache.rs      | 167 +++++++++++------------
crates/editor/src/link_go_to_definition.rs |  80 +++++++----
5 files changed, 162 insertions(+), 149 deletions(-)

Detailed changes

crates/editor/src/editor.rs 🔗

@@ -2792,11 +2792,13 @@ impl Editor {
                 }
             }
             InlayHintRefreshReason::ExcerptsRemoved(excerpts_removed) => {
-                let InlaySplice {
+                if let Some(InlaySplice {
                     to_remove,
                     to_insert,
-                } = self.inlay_hint_cache.remove_excerpts(excerpts_removed);
-                self.splice_inlay_hints(to_remove, to_insert, cx);
+                }) = self.inlay_hint_cache.remove_excerpts(excerpts_removed)
+                {
+                    self.splice_inlay_hints(to_remove, to_insert, cx);
+                }
                 return;
             }
             InlayHintRefreshReason::NewLinesShown => (InvalidationStrategy::None, None),

crates/editor/src/element.rs 🔗

@@ -2755,7 +2755,7 @@ impl PointForPosition {
         }
     }
 
-    fn as_valid(&self) -> Option<DisplayPoint> {
+    pub fn as_valid(&self) -> Option<DisplayPoint> {
         if self.previous_valid == self.exact_unclipped && self.next_valid == self.exact_unclipped {
             Some(self.previous_valid)
         } else {

crates/editor/src/hover_popover.rs 🔗

@@ -57,8 +57,6 @@ pub struct InlayHover {
 
 pub fn find_hovered_hint_part(
     label_parts: Vec<InlayHintLabelPart>,
-    padding_left: bool,
-    padding_right: bool,
     hint_range: Range<InlayOffset>,
     hovered_offset: InlayOffset,
 ) -> Option<(InlayHintLabelPart, Range<InlayOffset>)> {
@@ -67,19 +65,11 @@ pub fn find_hovered_hint_part(
         let mut part_start = hint_range.start;
         for part in label_parts {
             let part_len = part.value.chars().count();
-            if hovered_character >= part_len {
+            if hovered_character > part_len {
                 hovered_character -= part_len;
                 part_start.0 += part_len;
             } else {
-                let mut part_end = InlayOffset(part_start.0 + part_len);
-                if padding_left {
-                    part_start.0 += 1;
-                    part_end.0 += 1;
-                }
-                if padding_right {
-                    part_start.0 += 1;
-                    part_end.0 += 1;
-                }
+                let part_end = InlayOffset(part_start.0 + part_len);
                 return Some((part, part_start..part_end));
             }
         }
@@ -1386,13 +1376,21 @@ mod tests {
             .unwrap();
         let new_type_hint_part_hover_position = cx.update_editor(|editor, cx| {
             let snapshot = editor.snapshot(cx);
+            let previous_valid = inlay_range.start.to_display_point(&snapshot);
+            let next_valid = inlay_range.end.to_display_point(&snapshot);
+            assert_eq!(previous_valid.row(), next_valid.row());
+            assert!(previous_valid.column() < next_valid.column());
+            let exact_unclipped = DisplayPoint::new(
+                previous_valid.row(),
+                previous_valid.column()
+                    + (entire_hint_label.find(new_type_label).unwrap() + new_type_label.len() / 2)
+                        as u32,
+            );
             PointForPosition {
-                previous_valid: inlay_range.start.to_display_point(&snapshot),
-                next_valid: inlay_range.end.to_display_point(&snapshot),
-                exact_unclipped: inlay_range.end.to_display_point(&snapshot),
-                column_overshoot_after_line_end: (entire_hint_label.find(new_type_label).unwrap()
-                    + new_type_label.len() / 2)
-                    as u32,
+                previous_valid,
+                next_valid,
+                exact_unclipped,
+                column_overshoot_after_line_end: 0,
             }
         });
         cx.update_editor(|editor, cx| {
@@ -1514,13 +1512,21 @@ mod tests {
 
         let struct_hint_part_hover_position = cx.update_editor(|editor, cx| {
             let snapshot = editor.snapshot(cx);
+            let previous_valid = inlay_range.start.to_display_point(&snapshot);
+            let next_valid = inlay_range.end.to_display_point(&snapshot);
+            assert_eq!(previous_valid.row(), next_valid.row());
+            assert!(previous_valid.column() < next_valid.column());
+            let exact_unclipped = DisplayPoint::new(
+                previous_valid.row(),
+                previous_valid.column()
+                    + (entire_hint_label.find(struct_label).unwrap() + struct_label.len() / 2)
+                        as u32,
+            );
             PointForPosition {
-                previous_valid: inlay_range.start.to_display_point(&snapshot),
-                next_valid: inlay_range.end.to_display_point(&snapshot),
-                exact_unclipped: inlay_range.end.to_display_point(&snapshot),
-                column_overshoot_after_line_end: (entire_hint_label.find(struct_label).unwrap()
-                    + struct_label.len() / 2)
-                    as u32,
+                previous_valid,
+                next_valid,
+                exact_unclipped,
+                column_overshoot_after_line_end: 0,
             }
         });
         cx.update_editor(|editor, cx| {

crates/editor/src/inlay_hint_cache.rs 🔗

@@ -104,37 +104,34 @@ impl TasksForRanges {
         invalidate: InvalidationStrategy,
         spawn_task: impl FnOnce(QueryRanges) -> Task<()>,
     ) {
-        let query_ranges = match invalidate {
-            InvalidationStrategy::None => {
-                let mut updated_ranges = query_ranges;
-                updated_ranges.before_visible = updated_ranges
-                    .before_visible
-                    .into_iter()
-                    .flat_map(|query_range| {
-                        self.remove_cached_ranges_from_query(buffer_snapshot, query_range)
-                    })
-                    .collect();
-                updated_ranges.visible = updated_ranges
-                    .visible
-                    .into_iter()
-                    .flat_map(|query_range| {
-                        self.remove_cached_ranges_from_query(buffer_snapshot, query_range)
-                    })
-                    .collect();
-                updated_ranges.after_visible = updated_ranges
-                    .after_visible
-                    .into_iter()
-                    .flat_map(|query_range| {
-                        self.remove_cached_ranges_from_query(buffer_snapshot, query_range)
-                    })
-                    .collect();
-                updated_ranges
-            }
-            InvalidationStrategy::RefreshRequested | InvalidationStrategy::BufferEdited => {
-                self.tasks.clear();
-                self.sorted_ranges.clear();
-                query_ranges
-            }
+        let query_ranges = if invalidate.should_invalidate() {
+            self.tasks.clear();
+            self.sorted_ranges.clear();
+            query_ranges
+        } else {
+            let mut non_cached_query_ranges = query_ranges;
+            non_cached_query_ranges.before_visible = non_cached_query_ranges
+                .before_visible
+                .into_iter()
+                .flat_map(|query_range| {
+                    self.remove_cached_ranges_from_query(buffer_snapshot, query_range)
+                })
+                .collect();
+            non_cached_query_ranges.visible = non_cached_query_ranges
+                .visible
+                .into_iter()
+                .flat_map(|query_range| {
+                    self.remove_cached_ranges_from_query(buffer_snapshot, query_range)
+                })
+                .collect();
+            non_cached_query_ranges.after_visible = non_cached_query_ranges
+                .after_visible
+                .into_iter()
+                .flat_map(|query_range| {
+                    self.remove_cached_ranges_from_query(buffer_snapshot, query_range)
+                })
+                .collect();
+            non_cached_query_ranges
         };
 
         if !query_ranges.is_empty() {
@@ -205,45 +202,31 @@ impl TasksForRanges {
         ranges_to_query
     }
 
-    fn remove_from_cached_ranges(
-        &mut self,
-        buffer: &BufferSnapshot,
-        range_to_remove: &Range<language::Anchor>,
-    ) {
+    fn invalidate_range(&mut self, buffer: &BufferSnapshot, range: &Range<language::Anchor>) {
         self.sorted_ranges = self
             .sorted_ranges
             .drain(..)
             .filter_map(|mut cached_range| {
-                if cached_range.start.cmp(&range_to_remove.end, buffer).is_gt()
-                    || cached_range.end.cmp(&range_to_remove.start, buffer).is_lt()
+                if cached_range.start.cmp(&range.end, buffer).is_gt()
+                    || cached_range.end.cmp(&range.start, buffer).is_lt()
                 {
                     Some(vec![cached_range])
-                } else if cached_range
-                    .start
-                    .cmp(&range_to_remove.start, buffer)
-                    .is_ge()
-                    && cached_range.end.cmp(&range_to_remove.end, buffer).is_le()
+                } else if cached_range.start.cmp(&range.start, buffer).is_ge()
+                    && cached_range.end.cmp(&range.end, buffer).is_le()
                 {
                     None
-                } else if range_to_remove
-                    .start
-                    .cmp(&cached_range.start, buffer)
-                    .is_ge()
-                    && range_to_remove.end.cmp(&cached_range.end, buffer).is_le()
+                } else if range.start.cmp(&cached_range.start, buffer).is_ge()
+                    && range.end.cmp(&cached_range.end, buffer).is_le()
                 {
                     Some(vec![
-                        cached_range.start..range_to_remove.start,
-                        range_to_remove.end..cached_range.end,
+                        cached_range.start..range.start,
+                        range.end..cached_range.end,
                     ])
-                } else if cached_range
-                    .start
-                    .cmp(&range_to_remove.start, buffer)
-                    .is_ge()
-                {
-                    cached_range.start = range_to_remove.end;
+                } else if cached_range.start.cmp(&range.start, buffer).is_ge() {
+                    cached_range.start = range.end;
                     Some(vec![cached_range])
                 } else {
-                    cached_range.end = range_to_remove.start;
+                    cached_range.end = range.start;
                     Some(vec![cached_range])
                 }
             })
@@ -474,7 +457,7 @@ impl InlayHintCache {
         }
     }
 
-    pub fn remove_excerpts(&mut self, excerpts_removed: Vec<ExcerptId>) -> InlaySplice {
+    pub fn remove_excerpts(&mut self, excerpts_removed: Vec<ExcerptId>) -> Option<InlaySplice> {
         let mut to_remove = Vec::new();
         for excerpt_to_remove in excerpts_removed {
             self.update_tasks.remove(&excerpt_to_remove);
@@ -483,17 +466,21 @@ impl InlayHintCache {
                 to_remove.extend(cached_hints.hints.iter().map(|(id, _)| *id));
             }
         }
-        if !to_remove.is_empty() {
+        if to_remove.is_empty() {
+            None
+        } else {
             self.version += 1;
-        }
-        InlaySplice {
-            to_remove,
-            to_insert: Vec::new(),
+            Some(InlaySplice {
+                to_remove,
+                to_insert: Vec::new(),
+            })
         }
     }
 
     pub fn clear(&mut self) {
-        self.version += 1;
+        if !self.update_tasks.is_empty() || !self.hints.is_empty() {
+            self.version += 1;
+        }
         self.update_tasks.clear();
         self.hints.clear();
     }
@@ -818,7 +805,7 @@ fn new_update_task(
                         .update_tasks
                         .get_mut(&query.excerpt_id)
                     {
-                        task_ranges.remove_from_cached_ranges(&buffer_snapshot, &range);
+                        task_ranges.invalidate_range(&buffer_snapshot, &range);
                     }
                 })
                 .ok()
@@ -877,33 +864,33 @@ async fn fetch_and_update_hints(
     let inlay_hints_fetch_task = editor
         .update(&mut cx, |editor, cx| {
             if got_throttled {
-                if let Some((_, _, current_visible_range)) = editor
-                    .excerpt_visible_offsets(None, cx)
-                    .remove(&query.excerpt_id)
-                {
-                    let visible_offset_length = current_visible_range.len();
-                    let double_visible_range = current_visible_range
-                        .start
-                        .saturating_sub(visible_offset_length)
-                        ..current_visible_range
-                            .end
-                            .saturating_add(visible_offset_length)
-                            .min(buffer_snapshot.len());
-                    if !double_visible_range
-                        .contains(&fetch_range.start.to_offset(&buffer_snapshot))
-                        && !double_visible_range
-                            .contains(&fetch_range.end.to_offset(&buffer_snapshot))
+                let query_not_around_visible_range = match editor.excerpt_visible_offsets(None, cx).remove(&query.excerpt_id) {
+                    Some((_, _, current_visible_range)) => {
+                        let visible_offset_length = current_visible_range.len();
+                        let double_visible_range = current_visible_range
+                            .start
+                            .saturating_sub(visible_offset_length)
+                            ..current_visible_range
+                                .end
+                                .saturating_add(visible_offset_length)
+                                .min(buffer_snapshot.len());
+                        !double_visible_range
+                            .contains(&fetch_range.start.to_offset(&buffer_snapshot))
+                            && !double_visible_range
+                                .contains(&fetch_range.end.to_offset(&buffer_snapshot))
+                    },
+                    None => true,
+                };
+                if query_not_around_visible_range {
+                    log::trace!("Fetching inlay hints for range {fetch_range_to_log:?} got throttled and fell off the current visible range, skipping.");
+                    if let Some(task_ranges) = editor
+                        .inlay_hint_cache
+                        .update_tasks
+                        .get_mut(&query.excerpt_id)
                     {
-                        log::trace!("Fetching inlay hints for range {fetch_range_to_log:?} got throttled and fell off the current visible range, skipping.");
-                        if let Some(task_ranges) = editor
-                            .inlay_hint_cache
-                            .update_tasks
-                            .get_mut(&query.excerpt_id)
-                        {
-                            task_ranges.remove_from_cached_ranges(&buffer_snapshot, &fetch_range);
-                        }
-                        return None;
+                        task_ranges.invalidate_range(&buffer_snapshot, &fetch_range);
                     }
+                    return None;
                 }
             }
             editor
@@ -23,6 +23,7 @@ pub struct LinkGoToDefinitionState {
     pub task: Option<Task<Option<()>>>,
 }
 
+#[derive(Debug)]
 pub enum GoToDefinitionTrigger {
     Text(DisplayPoint),
     InlayHint(InlayRange, lsp::Location, LanguageServerId),
@@ -81,7 +82,7 @@ impl TriggerPoint {
     fn anchor(&self) -> &Anchor {
         match self {
             TriggerPoint::Text(anchor) => anchor,
-            TriggerPoint::InlayHint(coordinates, _, _) => &coordinates.inlay_position,
+            TriggerPoint::InlayHint(range, _, _) => &range.inlay_position,
         }
     }
 
@@ -127,11 +128,22 @@ pub fn update_go_to_definition_link(
         &trigger_point,
         &editor.link_go_to_definition_state.last_trigger_point,
     ) {
-        if a.anchor()
-            .cmp(b.anchor(), &snapshot.buffer_snapshot)
-            .is_eq()
-        {
-            return;
+        match (a, b) {
+            (TriggerPoint::Text(anchor_a), TriggerPoint::Text(anchor_b)) => {
+                if anchor_a.cmp(anchor_b, &snapshot.buffer_snapshot).is_eq() {
+                    return;
+                }
+            }
+            (TriggerPoint::InlayHint(range_a, _, _), TriggerPoint::InlayHint(range_b, _, _)) => {
+                if range_a
+                    .inlay_position
+                    .cmp(&range_b.inlay_position, &snapshot.buffer_snapshot)
+                    .is_eq()
+                {
+                    return;
+                }
+            }
+            _ => {}
         }
     }
 
@@ -165,11 +177,8 @@ pub fn update_inlay_link_and_hover_points(
         snapshot.display_point_to_inlay_offset(point_for_position.previous_valid, Bias::Left);
     let hint_end_offset =
         snapshot.display_point_to_inlay_offset(point_for_position.next_valid, Bias::Right);
-    let offset_overshoot = point_for_position.column_overshoot_after_line_end as usize;
-    let hovered_offset = if offset_overshoot == 0 {
+    let hovered_offset = if point_for_position.column_overshoot_after_line_end == 0 {
         Some(snapshot.display_point_to_inlay_offset(point_for_position.exact_unclipped, Bias::Left))
-    } else if (hint_end_offset - hint_start_offset).0 >= offset_overshoot {
-        Some(InlayOffset(hint_start_offset.0 + offset_overshoot))
     } else {
         None
     };
@@ -215,17 +224,18 @@ pub fn update_inlay_link_and_hover_points(
                         }
                     }
                     ResolveState::Resolved => {
+                        let mut actual_hint_start = hint_start_offset;
+                        let mut actual_hint_end = hint_end_offset;
+                        if cached_hint.padding_left {
+                            actual_hint_start.0 += 1;
+                            actual_hint_end.0 += 1;
+                        }
+                        if cached_hint.padding_right {
+                            actual_hint_start.0 += 1;
+                            actual_hint_end.0 += 1;
+                        }
                         match cached_hint.label {
                             project::InlayHintLabel::String(_) => {
-                                let mut highlight_start = hint_start_offset;
-                                let mut highlight_end = hint_end_offset;
-                                if cached_hint.padding_left {
-                                    highlight_start.0 += 1;
-                                    highlight_end.0 += 1;
-                                }
-                                if cached_hint.padding_right {
-                                    highlight_end.0 -= 1;
-                                }
                                 if let Some(tooltip) = cached_hint.tooltip {
                                     hover_popover::hover_at_inlay(
                                         editor,
@@ -246,8 +256,8 @@ pub fn update_inlay_link_and_hover_points(
                                             triggered_from: hovered_offset,
                                             range: InlayRange {
                                                 inlay_position: hovered_hint.position,
-                                                highlight_start,
-                                                highlight_end,
+                                                highlight_start: actual_hint_start,
+                                                highlight_end: actual_hint_end,
                                             },
                                         },
                                         cx,
@@ -259,9 +269,7 @@ pub fn update_inlay_link_and_hover_points(
                                 if let Some((hovered_hint_part, part_range)) =
                                     hover_popover::find_hovered_hint_part(
                                         label_parts,
-                                        cached_hint.padding_left,
-                                        cached_hint.padding_right,
-                                        hint_start_offset..hint_end_offset,
+                                        actual_hint_start..actual_hint_end,
                                         hovered_offset,
                                     )
                                 {
@@ -588,9 +596,11 @@ fn go_to_fetched_definition_of_kind(
             cx,
         );
 
-        match kind {
-            LinkDefinitionKind::Symbol => editor.go_to_definition(&Default::default(), cx),
-            LinkDefinitionKind::Type => editor.go_to_type_definition(&Default::default(), cx),
+        if point.as_valid().is_some() {
+            match kind {
+                LinkDefinitionKind::Symbol => editor.go_to_definition(&Default::default(), cx),
+                LinkDefinitionKind::Type => editor.go_to_type_definition(&Default::default(), cx),
+            }
         }
     }
 }
@@ -1160,11 +1170,19 @@ mod tests {
             .unwrap();
         let hint_hover_position = cx.update_editor(|editor, cx| {
             let snapshot = editor.snapshot(cx);
+            let previous_valid = inlay_range.start.to_display_point(&snapshot);
+            let next_valid = inlay_range.end.to_display_point(&snapshot);
+            assert_eq!(previous_valid.row(), next_valid.row());
+            assert!(previous_valid.column() < next_valid.column());
+            let exact_unclipped = DisplayPoint::new(
+                previous_valid.row(),
+                previous_valid.column() + (hint_label.len() / 2) as u32,
+            );
             PointForPosition {
-                previous_valid: inlay_range.start.to_display_point(&snapshot),
-                next_valid: inlay_range.end.to_display_point(&snapshot),
-                exact_unclipped: inlay_range.end.to_display_point(&snapshot),
-                column_overshoot_after_line_end: (hint_label.len() / 2) as u32,
+                previous_valid,
+                next_valid,
+                exact_unclipped,
+                column_overshoot_after_line_end: 0,
             }
         });
         // Press cmd to trigger highlight