From b864a9b0ae633006a44c02b723fe6fad07e84b93 Mon Sep 17 00:00:00 2001 From: Smit Barmase Date: Sun, 13 Apr 2025 00:32:55 +0530 Subject: [PATCH] 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](https://github.com/zed-industries/zed/blob/d1ffda9bfeccfdf9bea3f76251350bf9cf7f6e1b/crates/ui_prompt/src/ui_prompt.rs#L65). 2. OR it's wrapped around component like Popover which implements `RenderOnce` trait. See [here](https://github.com/zed-industries/zed/blob/d1ffda9bfeccfdf9bea3f76251350bf9cf7f6e1b/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 | 294 ++++++++++++++++------------- 1 file changed, 165 insertions(+), 129 deletions(-) diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index f876fab52c..fd53c4b0ad 100644 --- a/crates/editor/src/hover_popover.rs +++ b/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, - Window, div, px, + Stateful, StatefulInteractiveElement, StyleRefinement, Styled, Subscription, Task, + 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, ) -> bool { - let info_popovers = editor.hover_state.info_popovers.clone(); - for p in info_popovers { - let keyboard_grace = p.keyboard_grace.borrow(); - if *keyboard_grace { - if let Some(anchor) = p.anchor { - show_hover(editor, anchor, false, window, cx); - return true; - } + if let Some(anchor) = editor.hover_state.info_popovers.iter().find_map(|p| { + if *p.keyboard_grace.borrow() { + p.anchor + } else { + None } + }) { + show_hover(editor, anchor, false, window, cx); + return true; } - let diagnostic_popover = editor.hover_state.diagnostic_popover.clone(); - if let Some(d) = diagnostic_popover { - let keyboard_grace = d.keyboard_grace.borrow(); - if *keyboard_grace { - if let Some(anchor) = d.anchor { - show_hover(editor, anchor, false, window, cx); - return true; + if let Some(anchor) = editor + .hover_state + .diagnostic_popover + .as_ref() + .and_then(|d| { + if *d.keyboard_grace.borrow() { + 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 = None; - let mut background_color: Option = None; + let (background_color, border_color) = cx.update(|_, cx| { + 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| { - 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) - }) + .new(|cx| 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, pub diagnostic_popover: Option, @@ -742,7 +787,6 @@ impl HoverState { } } -#[derive(Debug, Clone)] pub(crate) struct InfoPopover { pub(crate) symbol_range: RangeInEditor, pub(crate) parsed_content: Option>, @@ -750,6 +794,7 @@ pub(crate) struct InfoPopover { pub(crate) scrollbar_state: ScrollbarState, pub(crate) keyboard_grace: Rc>, pub(crate) anchor: Option, + _subscription: Option, } impl InfoPopover { @@ -760,7 +805,7 @@ impl InfoPopover { cx: &mut Context, ) -> 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(); - }); - - if let Some(markdown) = &self.parsed_content { - d = d - .child( + }) + .when_some(self.parsed_content.clone(), |this, markdown| { + this.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( - markdown.clone(), - hover_markdown_style(window, cx), - ) - .code_block_renderer(markdown::CodeBlockRenderer::Default { - copy_button: false, - }) - .on_url_click(open_markdown_url), + MarkdownElement::new(markdown, hover_markdown_style(window, cx)) + .code_block_renderer(markdown::CodeBlockRenderer::Default { + copy_button: false, + }) + .on_url_click(open_markdown_url), ), ) - .child(self.render_vertical_scrollbar(cx)); - } - d.into_any_element() + .child(self.render_vertical_scrollbar(cx)) + }) + .into_any_element() } pub fn scroll(&self, amount: &ScrollAmount, window: &mut Window, cx: &mut Context) { @@ -842,14 +882,14 @@ impl InfoPopover { } } -#[derive(Debug, Clone)] pub struct DiagnosticPopover { pub(crate) local_diagnostic: DiagnosticEntry, parsed_content: Option>, - border_color: Option, - background_color: Option, + border_color: Hsla, + background_color: Hsla, pub keyboard_grace: Rc>, pub anchor: Option, + _subscription: Option, } impl DiagnosticPopover { @@ -860,53 +900,7 @@ impl DiagnosticPopover { cx: &mut Context, ) -> AnyElement { let keyboard_grace = Rc::clone(&self.keyboard_grace); - let mut markdown_div = div().py_1().px_2(); - 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() + div() .id("diagnostic") .block() .max_h(max_size.height) @@ -928,9 +922,51 @@ impl DiagnosticPopover { *keyboard_grace = false; cx.stop_propagation(); }) - .child(markdown_div); - - diagnostic_div.into_any_element() + .when_some(self.parsed_content.clone(), |this, markdown| { + this.child( + 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,