hover_popover: Fix markdown selection for info and diagnostic popovers (#28642)

Closes #28638

This PR fixes markdown selection for the info and diagnostic popovers.

In the editor popover, after the changes in
https://github.com/zed-industries/zed/pull/28255, the markdown selection
state updates correctly, but it no longer triggers the editor element to
repaint like it used to. This is fixed by adding a subscription to
listen for markdown entity changes and triggering a repaint for the
editor.

I assume markdown selection works elsewhere because:

1. Either the `Markdown` entity is directly part of a struct that
implements the `Render` trait, causing it to repaint whenever the
markdown state changes. See
[here](d1ffda9bfe/crates/ui_prompt/src/ui_prompt.rs (L65)).
2. OR it's wrapped around component like Popover which implements
`RenderOnce` trait. See
[here](d1ffda9bfe/crates/editor/src/code_context_menus.rs (L645)).

Whereas info and diagnostic popovers does not do both. I do think we can
change it to use `Popover` component, but for now this works as quick
fix.

Extras:
- Remove unnecessary struct cloning.
- Refactor rendering logic to use `when_some`.

Release Notes:

- Fixed issue where selection wasn't working for info and diagnostic
popovers.

crates/editor/src/hover_popover.rs

@@ -8,8 +8,8 @@ use crate::{
 use gpui::{
     AnyElement, AsyncWindowContext, Context, Entity, Focusable as _, FontWeight, Hsla,
     InteractiveElement, IntoElement, MouseButton, ParentElement, Pixels, ScrollHandle, Size,
-    Stateful, StatefulInteractiveElement, StyleRefinement, Styled, Task, TextStyleRefinement,
+    Stateful, StatefulInteractiveElement, StyleRefinement, Styled, Subscription, Task,
-    Window, div, px,
+    TextStyleRefinement, Window, div, px,
 };
 use itertools::Itertools;
 use language::{DiagnosticEntry, Language, LanguageRegistry};
@@ -64,26 +64,31 @@ pub fn show_keyboard_hover(
     window: &mut Window,
     cx: &mut Context<Editor>,
 ) -> bool {
-    let info_popovers = editor.hover_state.info_popovers.clone();
+    if let Some(anchor) = editor.hover_state.info_popovers.iter().find_map(|p| {
-    for p in info_popovers {
+        if *p.keyboard_grace.borrow() {
-        let keyboard_grace = p.keyboard_grace.borrow();
+            p.anchor
-        if *keyboard_grace {
+        } else {
-            if let Some(anchor) = p.anchor {
+            None
-                show_hover(editor, anchor, false, window, cx);
-                return true;
-            }
         }
+    }) {
+        show_hover(editor, anchor, false, window, cx);
+        return true;
     }
 
-    let diagnostic_popover = editor.hover_state.diagnostic_popover.clone();
+    if let Some(anchor) = editor
-    if let Some(d) = diagnostic_popover {
+        .hover_state
-        let keyboard_grace = d.keyboard_grace.borrow();
+        .diagnostic_popover
-        if *keyboard_grace {
+        .as_ref()
-            if let Some(anchor) = d.anchor {
+        .and_then(|d| {
-                show_hover(editor, anchor, false, window, cx);
+            if *d.keyboard_grace.borrow() {
-                return true;
+                d.anchor
+            } else {
+                None
             }
-        }
+        })
+    {
+        show_hover(editor, anchor, false, window, cx);
+        return true;
     }
 
     false
@@ -164,6 +169,18 @@ pub fn hover_at_inlay(
                 let parsed_content = parse_blocks(&blocks, &language_registry, None, cx).await;
 
                 let scroll_handle = ScrollHandle::new();
+
+                let subscription = this
+                    .update(cx, |_, cx| {
+                        if let Some(parsed_content) = &parsed_content {
+                            Some(cx.observe(parsed_content, |_, _, cx| cx.notify()))
+                        } else {
+                            None
+                        }
+                    })
+                    .ok()
+                    .flatten();
+
                 let hover_popover = InfoPopover {
                     symbol_range: RangeInEditor::Inlay(inlay_hover.range.clone()),
                     parsed_content,
@@ -171,6 +188,7 @@ pub fn hover_at_inlay(
                     scroll_handle,
                     keyboard_grace: Rc::new(RefCell::new(false)),
                     anchor: None,
+                    _subscription: subscription,
                 };
 
                 this.update(cx, |this, cx| {
@@ -307,40 +325,44 @@ fn show_hover(
                             .anchor_after(local_diagnostic.range.end),
                 };
 
-                let mut border_color: Option<Hsla> = None;
+                let (background_color, border_color) = cx.update(|_, cx| {
-                let mut background_color: Option<Hsla> = None;
+                    let status_colors = cx.theme().status();
+                    match local_diagnostic.diagnostic.severity {
+                        DiagnosticSeverity::ERROR => {
+                            (status_colors.error_background, status_colors.error_border)
+                        }
+                        DiagnosticSeverity::WARNING => (
+                            status_colors.warning_background,
+                            status_colors.warning_border,
+                        ),
+                        DiagnosticSeverity::INFORMATION => {
+                            (status_colors.info_background, status_colors.info_border)
+                        }
+                        DiagnosticSeverity::HINT => {
+                            (status_colors.hint_background, status_colors.hint_border)
+                        }
+                        _ => (
+                            status_colors.ignored_background,
+                            status_colors.ignored_border,
+                        ),
+                    }
+                })?;
 
                 let parsed_content = cx
-                    .new_window_entity(|_window, cx| {
+                    .new(|cx| Markdown::new_text(SharedString::new(text), cx))
-                        let status_colors = cx.theme().status();
-
-                        match local_diagnostic.diagnostic.severity {
-                            DiagnosticSeverity::ERROR => {
-                                background_color = Some(status_colors.error_background);
-                                border_color = Some(status_colors.error_border);
-                            }
-                            DiagnosticSeverity::WARNING => {
-                                background_color = Some(status_colors.warning_background);
-                                border_color = Some(status_colors.warning_border);
-                            }
-                            DiagnosticSeverity::INFORMATION => {
-                                background_color = Some(status_colors.info_background);
-                                border_color = Some(status_colors.info_border);
-                            }
-                            DiagnosticSeverity::HINT => {
-                                background_color = Some(status_colors.hint_background);
-                                border_color = Some(status_colors.hint_border);
-                            }
-                            _ => {
-                                background_color = Some(status_colors.ignored_background);
-                                border_color = Some(status_colors.ignored_border);
-                            }
-                        };
-
-                        Markdown::new_text(SharedString::new(text), cx)
-                    })
                     .ok();
 
+                let subscription = this
+                    .update(cx, |_, cx| {
+                        if let Some(parsed_content) = &parsed_content {
+                            Some(cx.observe(parsed_content, |_, _, cx| cx.notify()))
+                        } else {
+                            None
+                        }
+                    })
+                    .ok()
+                    .flatten();
+
                 Some(DiagnosticPopover {
                     local_diagnostic,
                     parsed_content,
@@ -348,6 +370,7 @@ fn show_hover(
                     background_color,
                     keyboard_grace: Rc::new(RefCell::new(ignore_timeout)),
                     anchor: Some(anchor),
+                    _subscription: subscription,
                 })
             } else {
                 None
@@ -400,6 +423,16 @@ fn show_hover(
                 }];
                 let parsed_content = parse_blocks(&blocks, &language_registry, None, cx).await;
                 let scroll_handle = ScrollHandle::new();
+                let subscription = this
+                    .update(cx, |_, cx| {
+                        if let Some(parsed_content) = &parsed_content {
+                            Some(cx.observe(parsed_content, |_, _, cx| cx.notify()))
+                        } else {
+                            None
+                        }
+                    })
+                    .ok()
+                    .flatten();
                 info_popovers.push(InfoPopover {
                     symbol_range: RangeInEditor::Text(range),
                     parsed_content,
@@ -407,6 +440,7 @@ fn show_hover(
                     scroll_handle,
                     keyboard_grace: Rc::new(RefCell::new(ignore_timeout)),
                     anchor: Some(anchor),
+                    _subscription: subscription,
                 })
             }
 
@@ -440,6 +474,16 @@ fn show_hover(
                 let parsed_content = parse_blocks(&blocks, &language_registry, language, cx).await;
                 let scroll_handle = ScrollHandle::new();
                 hover_highlights.push(range.clone());
+                let subscription = this
+                    .update(cx, |_, cx| {
+                        if let Some(parsed_content) = &parsed_content {
+                            Some(cx.observe(parsed_content, |_, _, cx| cx.notify()))
+                        } else {
+                            None
+                        }
+                    })
+                    .ok()
+                    .flatten();
                 info_popovers.push(InfoPopover {
                     symbol_range: RangeInEditor::Text(range),
                     parsed_content,
@@ -447,6 +491,7 @@ fn show_hover(
                     scroll_handle,
                     keyboard_grace: Rc::new(RefCell::new(ignore_timeout)),
                     anchor: Some(anchor),
+                    _subscription: subscription,
                 });
             }
 
@@ -660,7 +705,7 @@ pub fn open_markdown_url(link: SharedString, window: &mut Window, cx: &mut App)
     cx.open_url(&link);
 }
 
-#[derive(Default, Debug)]
+#[derive(Default)]
 pub struct HoverState {
     pub info_popovers: Vec<InfoPopover>,
     pub diagnostic_popover: Option<DiagnosticPopover>,
@@ -742,7 +787,6 @@ impl HoverState {
     }
 }
 
-#[derive(Debug, Clone)]
 pub(crate) struct InfoPopover {
     pub(crate) symbol_range: RangeInEditor,
     pub(crate) parsed_content: Option<Entity<Markdown>>,
@@ -750,6 +794,7 @@ pub(crate) struct InfoPopover {
     pub(crate) scrollbar_state: ScrollbarState,
     pub(crate) keyboard_grace: Rc<RefCell<bool>>,
     pub(crate) anchor: Option<Anchor>,
+    _subscription: Option<Subscription>,
 }
 
 impl InfoPopover {
@@ -760,7 +805,7 @@ impl InfoPopover {
         cx: &mut Context<Editor>,
     ) -> AnyElement {
         let keyboard_grace = Rc::clone(&self.keyboard_grace);
-        let mut d = div()
+        div()
             .id("info_popover")
             .elevation_2(cx)
             // Prevent a mouse down/move on the popover from being propagated to the editor,
@@ -770,11 +815,9 @@ impl InfoPopover {
                 let mut keyboard_grace = keyboard_grace.borrow_mut();
                 *keyboard_grace = false;
                 cx.stop_propagation();
-            });
+            })
-
+            .when_some(self.parsed_content.clone(), |this, markdown| {
-        if let Some(markdown) = &self.parsed_content {
+                this.child(
-            d = d
-                .child(
                     div()
                         .id("info-md-container")
                         .overflow_y_scroll()
@@ -783,19 +826,16 @@ impl InfoPopover {
                         .p_2()
                         .track_scroll(&self.scroll_handle)
                         .child(
-                            MarkdownElement::new(
+                            MarkdownElement::new(markdown, hover_markdown_style(window, cx))
-                                markdown.clone(),
+                                .code_block_renderer(markdown::CodeBlockRenderer::Default {
-                                hover_markdown_style(window, cx),
+                                    copy_button: false,
-                            )
+                                })
-                            .code_block_renderer(markdown::CodeBlockRenderer::Default {
+                                .on_url_click(open_markdown_url),
-                                copy_button: false,
-                            })
-                            .on_url_click(open_markdown_url),
                         ),
                 )
-                .child(self.render_vertical_scrollbar(cx));
+                .child(self.render_vertical_scrollbar(cx))
-        }
+            })
-        d.into_any_element()
+            .into_any_element()
     }
 
     pub fn scroll(&self, amount: &ScrollAmount, window: &mut Window, cx: &mut Context<Editor>) {
@@ -842,14 +882,14 @@ impl InfoPopover {
     }
 }
 
-#[derive(Debug, Clone)]
 pub struct DiagnosticPopover {
     pub(crate) local_diagnostic: DiagnosticEntry<Anchor>,
     parsed_content: Option<Entity<Markdown>>,
-    border_color: Option<Hsla>,
+    border_color: Hsla,
-    background_color: Option<Hsla>,
+    background_color: Hsla,
     pub keyboard_grace: Rc<RefCell<bool>>,
     pub anchor: Option<Anchor>,
+    _subscription: Option<Subscription>,
 }
 
 impl DiagnosticPopover {
@@ -860,53 +900,7 @@ impl DiagnosticPopover {
         cx: &mut Context<Editor>,
     ) -> AnyElement {
         let keyboard_grace = Rc::clone(&self.keyboard_grace);
-        let mut markdown_div = div().py_1().px_2();
+        div()
-        if let Some(markdown) = &self.parsed_content {
-            let settings = ThemeSettings::get_global(cx);
-            let mut base_text_style = window.text_style();
-            base_text_style.refine(&TextStyleRefinement {
-                font_family: Some(settings.ui_font.family.clone()),
-                font_fallbacks: settings.ui_font.fallbacks.clone(),
-                font_size: Some(settings.ui_font_size(cx).into()),
-                color: Some(cx.theme().colors().editor_foreground),
-                background_color: Some(gpui::transparent_black()),
-                ..Default::default()
-            });
-            let markdown_style = MarkdownStyle {
-                base_text_style,
-                selection_background_color: { cx.theme().players().local().selection },
-                link: TextStyleRefinement {
-                    underline: Some(gpui::UnderlineStyle {
-                        thickness: px(1.),
-                        color: Some(cx.theme().colors().editor_foreground),
-                        wavy: false,
-                    }),
-                    ..Default::default()
-                },
-                ..Default::default()
-            };
-
-            markdown_div = markdown_div.child(
-                MarkdownElement::new(markdown.clone(), markdown_style)
-                    .code_block_renderer(markdown::CodeBlockRenderer::Default {
-                        copy_button: false,
-                    })
-                    .on_url_click(open_markdown_url),
-            );
-        }
-
-        if let Some(background_color) = &self.background_color {
-            markdown_div = markdown_div.bg(*background_color);
-        }
-
-        if let Some(border_color) = &self.border_color {
-            markdown_div = markdown_div
-                .border_1()
-                .border_color(*border_color)
-                .rounded_lg();
-        }
-
-        let diagnostic_div = div()
             .id("diagnostic")
             .block()
             .max_h(max_size.height)
@@ -928,9 +922,51 @@ impl DiagnosticPopover {
                 *keyboard_grace = false;
                 cx.stop_propagation();
             })
-            .child(markdown_div);
+            .when_some(self.parsed_content.clone(), |this, markdown| {
-
+                this.child(
-        diagnostic_div.into_any_element()
+                    div()
+                        .py_1()
+                        .px_2()
+                        .child(
+                            MarkdownElement::new(markdown, {
+                                let settings = ThemeSettings::get_global(cx);
+                                let mut base_text_style = window.text_style();
+                                base_text_style.refine(&TextStyleRefinement {
+                                    font_family: Some(settings.ui_font.family.clone()),
+                                    font_fallbacks: settings.ui_font.fallbacks.clone(),
+                                    font_size: Some(settings.ui_font_size(cx).into()),
+                                    color: Some(cx.theme().colors().editor_foreground),
+                                    background_color: Some(gpui::transparent_black()),
+                                    ..Default::default()
+                                });
+                                MarkdownStyle {
+                                    base_text_style,
+                                    selection_background_color: {
+                                        cx.theme().players().local().selection
+                                    },
+                                    link: TextStyleRefinement {
+                                        underline: Some(gpui::UnderlineStyle {
+                                            thickness: px(1.),
+                                            color: Some(cx.theme().colors().editor_foreground),
+                                            wavy: false,
+                                        }),
+                                        ..Default::default()
+                                    },
+                                    ..Default::default()
+                                }
+                            })
+                            .code_block_renderer(markdown::CodeBlockRenderer::Default {
+                                copy_button: false,
+                            })
+                            .on_url_click(open_markdown_url),
+                        )
+                        .bg(self.background_color)
+                        .border_1()
+                        .border_color(self.border_color)
+                        .rounded_lg(),
+                )
+            })
+            .into_any_element()
     }
 }
 
@@ -1070,7 +1106,7 @@ mod tests {
                 editor.hover_state.info_popovers.len(),
                 1,
                 "Expected exactly one hover but got: {:?}",
-                editor.hover_state.info_popovers
+                editor.hover_state.info_popovers.len()
             );
             let rendered_text = editor
                 .hover_state
@@ -1110,7 +1146,7 @@ mod tests {
                 editor.hover_state.info_popovers.len(),
                 1,
                 "Expected exactly one hover but got: {:?}",
-                editor.hover_state.info_popovers
+                editor.hover_state.info_popovers.len()
             );
             let rendered_text = editor
                 .hover_state
@@ -1205,7 +1241,7 @@ mod tests {
                 editor.hover_state.info_popovers.len(),
                 1,
                 "Expected exactly one hover but got: {:?}",
-                editor.hover_state.info_popovers
+                editor.hover_state.info_popovers.len()
             );
             let rendered_text = editor
                 .hover_state
@@ -1270,7 +1306,7 @@ mod tests {
                 editor.hover_state.info_popovers.len(),
                 0,
                 "Expected no hovers but got but got: {:?}",
-                editor.hover_state.info_popovers
+                editor.hover_state.info_popovers.len()
             );
         });
 
@@ -1294,7 +1330,7 @@ mod tests {
                 editor.hover_state.info_popovers.len(),
                 1,
                 "Expected exactly one hover but got: {:?}",
-                editor.hover_state.info_popovers
+                editor.hover_state.info_popovers.len()
             );
 
             let rendered_text = editor
@@ -1352,7 +1388,7 @@ mod tests {
                 editor.hover_state.info_popovers.len(),
                 1,
                 "Expected exactly one hover but got: {:?}",
-                editor.hover_state.info_popovers
+                editor.hover_state.info_popovers.len()
             );
             let rendered_text = editor
                 .hover_state
@@ -1418,7 +1454,7 @@ mod tests {
                 editor.hover_state.info_popovers.len(),
                 1,
                 "Expected exactly one hover but got: {:?}",
-                editor.hover_state.info_popovers
+                editor.hover_state.info_popovers.len()
             );
             let rendered_text = editor
                 .hover_state
@@ -1795,7 +1831,7 @@ mod tests {
             assert!(
                 hover_state.diagnostic_popover.is_none() && hover_state.info_popovers.len() == 1
             );
-            let popover = hover_state.info_popovers.first().cloned().unwrap();
+            let popover = hover_state.info_popovers.first().unwrap();
             let buffer_snapshot = editor.buffer().update(cx, |buffer, cx| buffer.snapshot(cx));
             assert_eq!(
                 popover.symbol_range,
@@ -1850,7 +1886,7 @@ mod tests {
             assert!(
                 hover_state.diagnostic_popover.is_none() && hover_state.info_popovers.len() == 1
             );
-            let popover = hover_state.info_popovers.first().cloned().unwrap();
+            let popover = hover_state.info_popovers.first().unwrap();
             let buffer_snapshot = editor.buffer().update(cx, |buffer, cx| buffer.snapshot(cx));
             assert_eq!(
                 popover.symbol_range,