From fcb03989d2aa6948c79799fe1bb4fc0966d83fd8 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Sun, 5 Jan 2025 23:08:17 -0700 Subject: [PATCH] Remove unnecessary finding of primary diagnostic for diagnostic hover (#22697) No need to find or store the primary range ahead of time as it's found by `activate_diagnostics`. Not entirely sure we should still even have the special case when the popover is visible. It does support the keyboard interaction of opening hover followed by jumping to the primary position, but that seems pretty undiscoverable. Support for clicking the hover to navigate to the primary diagnostic was removed in https://github.com/zed-industries/zed/pull/3408 Release Notes: - N/A --- crates/editor/src/editor.rs | 13 +++++---- crates/editor/src/hover_popover.rs | 45 ++++++++++-------------------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 14851dceef..e65b28b9af 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -9148,11 +9148,12 @@ impl Editor { // If there is an active Diagnostic Popover jump to its diagnostic instead. if direction == Direction::Next { if let Some(popover) = self.hover_state.diagnostic_popover.as_ref() { - let (group_id, jump_to) = popover.activation_info(); - if self.activate_diagnostics(group_id, cx) { + self.activate_diagnostics(popover.group_id(), cx); + if let Some(active_diagnostics) = self.active_diagnostics.as_ref() { + let primary_range_start = active_diagnostics.primary_range.start; self.change_selections(Some(Autoscroll::fit()), cx, |s| { let mut new_selection = s.newest_anchor().clone(); - new_selection.collapse_to(jump_to, SelectionGoal::None); + new_selection.collapse_to(primary_range_start, SelectionGoal::None); s.select_anchors(vec![new_selection.clone()]); }); } @@ -9224,7 +9225,8 @@ impl Editor { }); if let Some((primary_range, group_id)) = group { - if self.activate_diagnostics(group_id, cx) { + self.activate_diagnostics(group_id, cx); + if self.active_diagnostics.is_some() { self.change_selections(Some(Autoscroll::fit()), cx, |s| { s.select(vec![Selection { id: selection.id, @@ -10326,7 +10328,7 @@ impl Editor { } } - fn activate_diagnostics(&mut self, group_id: usize, cx: &mut ViewContext) -> bool { + fn activate_diagnostics(&mut self, group_id: usize, cx: &mut ViewContext) { self.dismiss_diagnostics(cx); let snapshot = self.snapshot(cx); self.active_diagnostics = self.display_map.update(cx, |display_map, cx| { @@ -10388,7 +10390,6 @@ impl Editor { is_valid: true, }) }); - self.active_diagnostics.is_some() } fn dismiss_diagnostics(&mut self, cx: &mut ViewContext) { diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index 5fa5e2c359..1a6e2da5c7 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -263,24 +263,7 @@ fn show_hover( delay.await; } - // If there's a diagnostic, assign it on the hover state and notify - let mut local_diagnostic = snapshot - .buffer_snapshot - .diagnostics_in_range(anchor..anchor, false) - // Find the entry with the most specific range - .min_by_key(|entry| { - let range = entry.range.to_offset(&snapshot.buffer_snapshot); - range.end - range.start - }); - - // Pull the primary diagnostic out so we can jump to it if the popover is clicked - let primary_diagnostic = local_diagnostic.as_ref().and_then(|local_diagnostic| { - snapshot - .buffer_snapshot - .diagnostic_group(local_diagnostic.diagnostic.group_id) - .find(|diagnostic| diagnostic.diagnostic.is_primary) - }); - if let Some(invisible) = snapshot + let local_diagnostic = if let Some(invisible) = snapshot .buffer_snapshot .chars_at(anchor) .next() @@ -289,7 +272,7 @@ fn show_hover( let after = snapshot.buffer_snapshot.anchor_after( anchor.to_offset(&snapshot.buffer_snapshot) + invisible.len_utf8(), ); - local_diagnostic = Some(DiagnosticEntry { + Some(DiagnosticEntry { diagnostic: Diagnostic { severity: DiagnosticSeverity::HINT, message: format!("Unicode character U+{:02X}", invisible as u32), @@ -306,7 +289,7 @@ fn show_hover( let before = snapshot.buffer_snapshot.anchor_before( anchor.to_offset(&snapshot.buffer_snapshot) - invisible.len_utf8(), ); - local_diagnostic = Some(DiagnosticEntry { + Some(DiagnosticEntry { diagnostic: Diagnostic { severity: DiagnosticSeverity::HINT, message: format!("Unicode character U+{:02X}", invisible as u32), @@ -314,7 +297,16 @@ fn show_hover( }, range: before..anchor, }) - } + } else { + snapshot + .buffer_snapshot + .diagnostics_in_range(anchor..anchor, false) + // Find the entry with the most specific range + .min_by_key(|entry| { + let range = entry.range.to_offset(&snapshot.buffer_snapshot); + range.end - range.start + }) + }; let diagnostic_popover = if let Some(local_diagnostic) = local_diagnostic { let text = match local_diagnostic.diagnostic.source { @@ -383,7 +375,6 @@ fn show_hover( Some(DiagnosticPopover { local_diagnostic, - primary_diagnostic, parsed_content, border_color, background_color, @@ -778,7 +769,6 @@ impl InfoPopover { #[derive(Debug, Clone)] pub struct DiagnosticPopover { local_diagnostic: DiagnosticEntry, - primary_diagnostic: Option>, parsed_content: Option>, border_color: Option, background_color: Option, @@ -832,13 +822,8 @@ impl DiagnosticPopover { diagnostic_div.into_any_element() } - pub fn activation_info(&self) -> (usize, Anchor) { - let entry = self - .primary_diagnostic - .as_ref() - .unwrap_or(&self.local_diagnostic); - - (entry.diagnostic.group_id, entry.range.start) + pub fn group_id(&self) -> usize { + self.local_diagnostic.diagnostic.group_id } }