From 73937876b6fb3fca890805a941b203ef1adfe7dd Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Sat, 26 Aug 2023 21:12:04 +0300 Subject: [PATCH 1/6] Properly omit throttled hint queries --- crates/editor/src/inlay_hint_cache.rs | 50 +++++++++++++-------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/crates/editor/src/inlay_hint_cache.rs b/crates/editor/src/inlay_hint_cache.rs index 1ed35ef629688575dbf25d0c64fecdd9f3489fa2..2adf3caaf1c1b2782dc5187223847a4b6301c878 100644 --- a/crates/editor/src/inlay_hint_cache.rs +++ b/crates/editor/src/inlay_hint_cache.rs @@ -877,33 +877,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.remove_from_cached_ranges(&buffer_snapshot, &fetch_range); } + return None; } } editor From 5cf51211b64b0c70e442402939d08e5bee4e17c7 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Sat, 26 Aug 2023 21:37:34 +0300 Subject: [PATCH 2/6] Use better names, simplify --- crates/editor/src/editor.rs | 8 +- crates/editor/src/inlay_hint_cache.rs | 119 ++++++++++++-------------- 2 files changed, 58 insertions(+), 69 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 01aa59574dc9455c2f5caee8f53144b1ba021446..2d934a1dc8f213d35648e3fd5799ccf8c08321c3 100644 --- a/crates/editor/src/editor.rs +++ b/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), diff --git a/crates/editor/src/inlay_hint_cache.rs b/crates/editor/src/inlay_hint_cache.rs index 2adf3caaf1c1b2782dc5187223847a4b6301c878..34898aea2efe7ec45229cdb4e63de13cd48217f9 100644 --- a/crates/editor/src/inlay_hint_cache.rs +++ b/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, - ) { + fn invalidate_range(&mut self, buffer: &BufferSnapshot, range: &Range) { 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) -> InlaySplice { + pub fn remove_excerpts(&mut self, excerpts_removed: Vec) -> Option { 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() @@ -901,7 +888,7 @@ async fn fetch_and_update_hints( .update_tasks .get_mut(&query.excerpt_id) { - task_ranges.remove_from_cached_ranges(&buffer_snapshot, &fetch_range); + task_ranges.invalidate_range(&buffer_snapshot, &fetch_range); } return None; } From dad64edde107d7d6bd7d8f835c299ddf1d43d5df Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Sun, 27 Aug 2023 15:14:45 +0300 Subject: [PATCH 3/6] Better highlight hint ranges --- crates/editor/src/hover_popover.rs | 14 ++-------- crates/editor/src/link_go_to_definition.rs | 32 ++++++++++------------ 2 files changed, 16 insertions(+), 30 deletions(-) diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index f5f663660daa04ef20bb042310ccd30cd75b6a7b..4eda65fc122b947fe44e7325a022ca6885668ccd 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -57,8 +57,6 @@ pub struct InlayHover { pub fn find_hovered_hint_part( label_parts: Vec, - padding_left: bool, - padding_right: bool, hint_range: Range, hovered_offset: InlayOffset, ) -> Option<(InlayHintLabelPart, Range)> { @@ -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)); } } diff --git a/crates/editor/src/link_go_to_definition.rs b/crates/editor/src/link_go_to_definition.rs index 84fd9b5cc1ee44d2cd5cf22153e2ce0b9d27af61..a36c673eae74ab817ff17ffe49b0288ffc48a681 100644 --- a/crates/editor/src/link_go_to_definition.rs +++ b/crates/editor/src/link_go_to_definition.rs @@ -165,11 +165,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 +212,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 +244,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 +257,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, ) { From 693e91f3351d64e85f9f4fe7de466a109445d139 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Sun, 27 Aug 2023 18:23:40 +0300 Subject: [PATCH 4/6] Properly compare previous hover trigger point when hover changes --- crates/editor/src/link_go_to_definition.rs | 24 ++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/crates/editor/src/link_go_to_definition.rs b/crates/editor/src/link_go_to_definition.rs index a36c673eae74ab817ff17ffe49b0288ffc48a681..909c07880b749ca93d37abea83d6b5bc68f1fa38 100644 --- a/crates/editor/src/link_go_to_definition.rs +++ b/crates/editor/src/link_go_to_definition.rs @@ -23,6 +23,7 @@ pub struct LinkGoToDefinitionState { pub 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; + } + } + _ => {} } } From 81e70905bb20ded519ed987c0948289a541a3a19 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Sun, 27 Aug 2023 19:12:32 +0300 Subject: [PATCH 5/6] Do not allow cmd+click in invalid inlay context --- crates/editor/src/element.rs | 2 +- crates/editor/src/link_go_to_definition.rs | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 684f92a96a8ee8b11e218960d6ac556d75187bfe..62f4c8c8065e8eb24ef24e7b1c98e75168034f43 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -2755,7 +2755,7 @@ impl PointForPosition { } } - fn as_valid(&self) -> Option { + pub fn as_valid(&self) -> Option { if self.previous_valid == self.exact_unclipped && self.next_valid == self.exact_unclipped { Some(self.previous_valid) } else { diff --git a/crates/editor/src/link_go_to_definition.rs b/crates/editor/src/link_go_to_definition.rs index 909c07880b749ca93d37abea83d6b5bc68f1fa38..9ca39f9b307769ffa818f29d55c063e974c4213e 100644 --- a/crates/editor/src/link_go_to_definition.rs +++ b/crates/editor/src/link_go_to_definition.rs @@ -596,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), + } } } } From 38da2a587a74eeb37e6a715a37ea03df82a430e2 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Sun, 27 Aug 2023 19:41:15 +0300 Subject: [PATCH 6/6] Fix the tests --- crates/editor/src/hover_popover.rs | 40 +++++++++++++++------- crates/editor/src/link_go_to_definition.rs | 16 ++++++--- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index 4eda65fc122b947fe44e7325a022ca6885668ccd..2f278ce262f6dd3e0910b022e6956f26c9bdfa6b 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -1376,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| { @@ -1504,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| { diff --git a/crates/editor/src/link_go_to_definition.rs b/crates/editor/src/link_go_to_definition.rs index 9ca39f9b307769ffa818f29d55c063e974c4213e..1f9a3aab730d4d6077df836e54ba09bc12f397d1 100644 --- a/crates/editor/src/link_go_to_definition.rs +++ b/crates/editor/src/link_go_to_definition.rs @@ -1170,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