Remove unnecessary finding of primary diagnostic for diagnostic hover (#22697)

Michael Sloan created 1 year ago

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

Change summary

crates/editor/src/editor.rs        | 13 ++++----
crates/editor/src/hover_popover.rs | 45 ++++++++++---------------------
2 files changed, 22 insertions(+), 36 deletions(-)

Detailed changes

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<Self>) -> bool {
+    fn activate_diagnostics(&mut self, group_id: usize, cx: &mut ViewContext<Self>) {
         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<Self>) {

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<Anchor>,
-    primary_diagnostic: Option<DiagnosticEntry<Anchor>>,
     parsed_content: Option<View<Markdown>>,
     border_color: Option<Hsla>,
     background_color: Option<Hsla>,
@@ -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
     }
 }