From 506beafe1080aa1394be72d9c5794335b034213f Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Tue, 27 May 2025 17:12:38 -0600 Subject: [PATCH] Add caching of parsed completion documentation markdown to reduce flicker when selecting (#31546) Related to #31460 and #28635. Release Notes: - Fixed redraw delay of documentation from language server completions and added caching to reduce flicker when using arrow keys to change selection. --- Cargo.lock | 1 + crates/editor/src/code_context_menus.rs | 206 ++++++++++++++++++------ crates/editor/src/editor.rs | 62 ++++--- crates/editor/src/hover_popover.rs | 9 +- crates/markdown/Cargo.toml | 1 + crates/markdown/examples/markdown.rs | 10 +- crates/markdown/src/markdown.rs | 10 +- crates/util/src/util.rs | 96 +++++++++-- 8 files changed, 300 insertions(+), 95 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cc4c4bda01..b80340e689 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9567,6 +9567,7 @@ dependencies = [ "assets", "base64 0.22.1", "env_logger 0.11.8", + "futures 0.3.31", "gpui", "language", "languages", diff --git a/crates/editor/src/code_context_menus.rs b/crates/editor/src/code_context_menus.rs index 6664a38271..4ec90a204e 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -4,8 +4,9 @@ use gpui::{ Size, StrikethroughStyle, StyledText, UniformListScrollHandle, div, px, uniform_list, }; use gpui::{AsyncWindowContext, WeakEntity}; -use language::Buffer; +use itertools::Itertools; use language::CodeLabel; +use language::{Buffer, LanguageName, LanguageRegistry}; use markdown::{Markdown, MarkdownElement}; use multi_buffer::{Anchor, ExcerptId}; use ordered_float::OrderedFloat; @@ -15,6 +16,8 @@ use project::{CodeAction, Completion, TaskSourceKind}; use task::DebugScenario; use task::TaskContext; +use std::collections::VecDeque; +use std::sync::Arc; use std::{ cell::RefCell, cmp::{Reverse, min}, @@ -41,6 +44,25 @@ pub const MENU_ASIDE_X_PADDING: Pixels = px(16.); pub const MENU_ASIDE_MIN_WIDTH: Pixels = px(260.); pub const MENU_ASIDE_MAX_WIDTH: Pixels = px(500.); +// Constants for the markdown cache. The purpose of this cache is to reduce flickering due to +// documentation not yet being parsed. +// +// The size of the cache is set to the number of items fetched around the current selection plus one +// for the current selection and another to avoid cases where and adjacent selection exits the +// cache. The only current benefit of a larger cache would be doing less markdown parsing when the +// selection revisits items. +// +// One future benefit of a larger cache would be reducing flicker on backspace. This would require +// not recreating the menu on every change, by not re-querying the language server when +// `is_incomplete = false`. +const MARKDOWN_CACHE_MAX_SIZE: usize = MARKDOWN_CACHE_BEFORE_ITEMS + MARKDOWN_CACHE_AFTER_ITEMS + 2; +const MARKDOWN_CACHE_BEFORE_ITEMS: usize = 2; +const MARKDOWN_CACHE_AFTER_ITEMS: usize = 2; + +// Number of items beyond the visible items to resolve documentation. +const RESOLVE_BEFORE_ITEMS: usize = 4; +const RESOLVE_AFTER_ITEMS: usize = 4; + pub enum CodeContextMenu { Completions(CompletionsMenu), CodeActions(CodeActionsMenu), @@ -148,13 +170,12 @@ impl CodeContextMenu { pub fn render_aside( &mut self, - editor: &Editor, max_size: Size, window: &mut Window, cx: &mut Context, ) -> Option { match self { - CodeContextMenu::Completions(menu) => menu.render_aside(editor, max_size, window, cx), + CodeContextMenu::Completions(menu) => menu.render_aside(max_size, window, cx), CodeContextMenu::CodeActions(_) => None, } } @@ -162,7 +183,7 @@ impl CodeContextMenu { pub fn focused(&self, window: &mut Window, cx: &mut Context) -> bool { match self { CodeContextMenu::Completions(completions_menu) => completions_menu - .markdown_element + .get_or_create_entry_markdown(completions_menu.selected_item, cx) .as_ref() .is_some_and(|markdown| markdown.focus_handle(cx).contains_focused(window, cx)), CodeContextMenu::CodeActions(_) => false, @@ -176,7 +197,7 @@ pub enum ContextMenuOrigin { QuickActionBar, } -#[derive(Clone, Debug)] +#[derive(Clone)] pub struct CompletionsMenu { pub id: CompletionId, sort_completions: bool, @@ -191,7 +212,9 @@ pub struct CompletionsMenu { show_completion_documentation: bool, pub(super) ignore_completion_provider: bool, last_rendered_range: Rc>>>, - markdown_element: Option>, + markdown_cache: Rc)>>>, + language_registry: Option>, + language: Option, snippet_sort_order: SnippetSortOrder, } @@ -205,6 +228,9 @@ impl CompletionsMenu { buffer: Entity, completions: Box<[Completion]>, snippet_sort_order: SnippetSortOrder, + language_registry: Option>, + language: Option, + cx: &mut Context, ) -> Self { let match_candidates = completions .iter() @@ -212,7 +238,7 @@ impl CompletionsMenu { .map(|(id, completion)| StringMatchCandidate::new(id, &completion.label.filter_text())) .collect(); - Self { + let completions_menu = Self { id, sort_completions, initial_position, @@ -226,9 +252,15 @@ impl CompletionsMenu { scroll_handle: UniformListScrollHandle::new(), resolve_completions: true, last_rendered_range: RefCell::new(None).into(), - markdown_element: None, + markdown_cache: RefCell::new(VecDeque::with_capacity(MARKDOWN_CACHE_MAX_SIZE)).into(), + language_registry, + language, snippet_sort_order, - } + }; + + completions_menu.start_markdown_parse_for_nearby_entries(cx); + + completions_menu } pub fn new_snippet_choices( @@ -286,7 +318,9 @@ impl CompletionsMenu { show_completion_documentation: false, ignore_completion_provider: false, last_rendered_range: RefCell::new(None).into(), - markdown_element: None, + markdown_cache: RefCell::new(VecDeque::new()).into(), + language_registry: None, + language: None, snippet_sort_order, } } @@ -359,6 +393,7 @@ impl CompletionsMenu { self.scroll_handle .scroll_to_item(self.selected_item, ScrollStrategy::Top); self.resolve_visible_completions(provider, cx); + self.start_markdown_parse_for_nearby_entries(cx); if let Some(provider) = provider { self.handle_selection_changed(provider, window, cx); } @@ -433,11 +468,10 @@ impl CompletionsMenu { // Expand the range to resolve more completions than are predicted to be visible, to reduce // jank on navigation. - const EXTRA_TO_RESOLVE: usize = 4; - let entry_indices = util::iterate_expanded_and_wrapped_usize_range( + let entry_indices = util::expanded_and_wrapped_usize_range( entry_range.clone(), - EXTRA_TO_RESOLVE, - EXTRA_TO_RESOLVE, + RESOLVE_BEFORE_ITEMS, + RESOLVE_AFTER_ITEMS, entries.len(), ); @@ -467,14 +501,120 @@ impl CompletionsMenu { cx, ); + let completion_id = self.id; cx.spawn(async move |editor, cx| { if let Some(true) = resolve_task.await.log_err() { - editor.update(cx, |_, cx| cx.notify()).ok(); + editor + .update(cx, |editor, cx| { + // `resolve_completions` modified state affecting display. + cx.notify(); + editor.with_completions_menu_matching_id( + completion_id, + || (), + |this| this.start_markdown_parse_for_nearby_entries(cx), + ); + }) + .ok(); } }) .detach(); } + fn start_markdown_parse_for_nearby_entries(&self, cx: &mut Context) { + // Enqueue parse tasks of nearer items first. + // + // TODO: This means that the nearer items will actually be further back in the cache, which + // is not ideal. In practice this is fine because `get_or_create_markdown` moves the current + // selection to the front (when `is_render = true`). + let entry_indices = util::wrapped_usize_outward_from( + self.selected_item, + MARKDOWN_CACHE_BEFORE_ITEMS, + MARKDOWN_CACHE_AFTER_ITEMS, + self.entries.borrow().len(), + ); + + for index in entry_indices { + self.get_or_create_entry_markdown(index, cx); + } + } + + fn get_or_create_entry_markdown( + &self, + index: usize, + cx: &mut Context, + ) -> Option> { + let entries = self.entries.borrow(); + if index >= entries.len() { + return None; + } + let candidate_id = entries[index].candidate_id; + match &self.completions.borrow()[candidate_id].documentation { + Some(CompletionDocumentation::MultiLineMarkdown(source)) if !source.is_empty() => Some( + self.get_or_create_markdown(candidate_id, source.clone(), false, cx) + .1, + ), + Some(_) => None, + _ => None, + } + } + + fn get_or_create_markdown( + &self, + candidate_id: usize, + source: SharedString, + is_render: bool, + cx: &mut Context, + ) -> (bool, Entity) { + let mut markdown_cache = self.markdown_cache.borrow_mut(); + if let Some((cache_index, (_, markdown))) = markdown_cache + .iter() + .find_position(|(id, _)| *id == candidate_id) + { + let markdown = if is_render && cache_index != 0 { + // Move the current selection's cache entry to the front. + markdown_cache.rotate_right(1); + let cache_len = markdown_cache.len(); + markdown_cache.swap(0, (cache_index + 1) % cache_len); + &markdown_cache[0].1 + } else { + markdown + }; + + let is_parsing = markdown.update(cx, |markdown, cx| { + // `reset` is called as it's possible for documentation to change due to resolve + // requests. It does nothing if `source` is unchanged. + markdown.reset(source, cx); + markdown.is_parsing() + }); + return (is_parsing, markdown.clone()); + } + + if markdown_cache.len() < MARKDOWN_CACHE_MAX_SIZE { + let markdown = cx.new(|cx| { + Markdown::new( + source, + self.language_registry.clone(), + self.language.clone(), + cx, + ) + }); + // Handles redraw when the markdown is done parsing. The current render is for a + // deferred draw, and so without this did not redraw when `markdown` notified. + cx.observe(&markdown, |_, _, cx| cx.notify()).detach(); + markdown_cache.push_front((candidate_id, markdown.clone())); + (true, markdown) + } else { + debug_assert_eq!(markdown_cache.capacity(), MARKDOWN_CACHE_MAX_SIZE); + // Moves the last cache entry to the start. The ring buffer is full, so this does no + // copying and just shifts indexes. + markdown_cache.rotate_right(1); + markdown_cache[0].0 = candidate_id; + let markdown = &markdown_cache[0].1; + markdown.update(cx, |markdown, cx| markdown.reset(source, cx)); + (true, markdown.clone()) + } + } + pub fn visible(&self) -> bool { !self.entries.borrow().is_empty() } @@ -625,7 +765,6 @@ impl CompletionsMenu { fn render_aside( &mut self, - editor: &Editor, max_size: Size, window: &mut Window, cx: &mut Context, @@ -644,33 +783,14 @@ impl CompletionsMenu { plain_text: Some(text), .. } => div().child(text.clone()), - CompletionDocumentation::MultiLineMarkdown(parsed) if !parsed.is_empty() => { - let markdown = self.markdown_element.get_or_insert_with(|| { - let markdown = cx.new(|cx| { - let languages = editor - .workspace - .as_ref() - .and_then(|(workspace, _)| workspace.upgrade()) - .map(|workspace| workspace.read(cx).app_state().languages.clone()); - let language = editor - .language_at(self.initial_position, cx) - .map(|l| l.name().to_proto()); - Markdown::new(SharedString::default(), languages, language, cx) - }); - // Handles redraw when the markdown is done parsing. The current render is for a - // deferred draw and so was not getting redrawn when `markdown` notified. - cx.observe(&markdown, |_, _, cx| cx.notify()).detach(); - markdown - }); - let is_parsing = markdown.update(cx, |markdown, cx| { - markdown.reset(parsed.clone(), cx); - markdown.is_parsing() - }); + CompletionDocumentation::MultiLineMarkdown(source) if !source.is_empty() => { + let (is_parsing, markdown) = + self.get_or_create_markdown(mat.candidate_id, source.clone(), true, cx); if is_parsing { return None; } div().child( - MarkdownElement::new(markdown.clone(), hover_markdown_style(window, cx)) + MarkdownElement::new(markdown, hover_markdown_style(window, cx)) .code_block_renderer(markdown::CodeBlockRenderer::Default { copy_button: false, copy_button_on_hover: false, @@ -882,13 +1002,7 @@ impl CompletionsMenu { // another opened. `provider.selection_changed` should not be called in this case. let this_menu_still_active = editor .read_with(cx, |editor, _cx| { - if let Some(CodeContextMenu::Completions(completions_menu)) = - editor.context_menu.borrow().as_ref() - { - completions_menu.id == self.id - } else { - false - } + editor.with_completions_menu_matching_id(self.id, || false, |_| true) }) .unwrap_or(false); if this_menu_still_active { diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 16e5bb0438..653772e7df 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -4987,14 +4987,12 @@ impl Editor { (buffer_position..buffer_position, None) }; - let completion_settings = language_settings( - buffer_snapshot - .language_at(buffer_position) - .map(|language| language.name()), - buffer_snapshot.file(), - cx, - ) - .completions; + let language = buffer_snapshot + .language_at(buffer_position) + .map(|language| language.name()); + + let completion_settings = + language_settings(language.clone(), buffer_snapshot.file(), cx).completions; // The document can be large, so stay in reasonable bounds when searching for words, // otherwise completion pop-up might be slow to appear. @@ -5106,16 +5104,26 @@ impl Editor { let menu = if completions.is_empty() { None } else { - let mut menu = CompletionsMenu::new( - id, - sort_completions, - show_completion_documentation, - ignore_completion_provider, - position, - buffer.clone(), - completions.into(), - snippet_sort_order, - ); + let mut menu = editor.update(cx, |editor, cx| { + let languages = editor + .workspace + .as_ref() + .and_then(|(workspace, _)| workspace.upgrade()) + .map(|workspace| workspace.read(cx).app_state().languages.clone()); + CompletionsMenu::new( + id, + sort_completions, + show_completion_documentation, + ignore_completion_provider, + position, + buffer.clone(), + completions.into(), + snippet_sort_order, + languages, + language, + cx, + ) + })?; menu.filter( if filter_completions { @@ -5190,6 +5198,22 @@ impl Editor { } } + pub fn with_completions_menu_matching_id( + &self, + id: CompletionId, + on_absent: impl FnOnce() -> R, + on_match: impl FnOnce(&mut CompletionsMenu) -> R, + ) -> R { + let mut context_menu = self.context_menu.borrow_mut(); + let Some(CodeContextMenu::Completions(completions_menu)) = &mut *context_menu else { + return on_absent(); + }; + if completions_menu.id != id { + return on_absent(); + } + on_match(completions_menu) + } + pub fn confirm_completion( &mut self, action: &ConfirmCompletion, @@ -8686,7 +8710,7 @@ impl Editor { ) -> Option { self.context_menu.borrow_mut().as_mut().and_then(|menu| { if menu.visible() { - menu.render_aside(self, max_size, window, cx) + menu.render_aside(max_size, window, cx) } else { None } diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index d0288c5871..942f3c0424 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -583,13 +583,6 @@ async fn parse_blocks( language: Option>, cx: &mut AsyncWindowContext, ) -> Option> { - let fallback_language_name = if let Some(ref l) = language { - let l = Arc::clone(l); - Some(l.lsp_id().clone()) - } else { - None - }; - let combined_text = blocks .iter() .map(|block| match &block.kind { @@ -607,7 +600,7 @@ async fn parse_blocks( Markdown::new( combined_text.into(), Some(language_registry.clone()), - fallback_language_name, + language.map(|language| language.name()), cx, ) }) diff --git a/crates/markdown/Cargo.toml b/crates/markdown/Cargo.toml index b899cfe795..b278ef1cd4 100644 --- a/crates/markdown/Cargo.toml +++ b/crates/markdown/Cargo.toml @@ -20,6 +20,7 @@ test-support = [ [dependencies] base64.workspace = true +futures.workspace = true gpui.workspace = true language.workspace = true linkify.workspace = true diff --git a/crates/markdown/examples/markdown.rs b/crates/markdown/examples/markdown.rs index 263579ef2b..16387a8000 100644 --- a/crates/markdown/examples/markdown.rs +++ b/crates/markdown/examples/markdown.rs @@ -67,14 +67,8 @@ struct MarkdownExample { impl MarkdownExample { pub fn new(text: SharedString, language_registry: Arc, cx: &mut App) -> Self { - let markdown = cx.new(|cx| { - Markdown::new( - text, - Some(language_registry), - Some("TypeScript".to_string()), - cx, - ) - }); + let markdown = cx + .new(|cx| Markdown::new(text, Some(language_registry), Some("TypeScript".into()), cx)); Self { markdown } } } diff --git a/crates/markdown/src/markdown.rs b/crates/markdown/src/markdown.rs index 455df3ef0d..da9f3fee85 100644 --- a/crates/markdown/src/markdown.rs +++ b/crates/markdown/src/markdown.rs @@ -2,6 +2,8 @@ pub mod parser; mod path_range; use base64::Engine as _; +use futures::FutureExt as _; +use language::LanguageName; use log::Level; pub use path_range::{LineCol, PathWithRange}; @@ -101,7 +103,7 @@ pub struct Markdown { pending_parse: Option>, focus_handle: FocusHandle, language_registry: Option>, - fallback_code_block_language: Option, + fallback_code_block_language: Option, options: Options, copied_code_blocks: HashSet, } @@ -144,7 +146,7 @@ impl Markdown { pub fn new( source: SharedString, language_registry: Option>, - fallback_code_block_language: Option, + fallback_code_block_language: Option, cx: &mut Context, ) -> Self { let focus_handle = cx.focus_handle(); @@ -310,9 +312,9 @@ impl Markdown { if let Some(registry) = language_registry.as_ref() { for name in language_names { let language = if !name.is_empty() { - registry.language_for_name_or_extension(&name) + registry.language_for_name_or_extension(&name).left_future() } else if let Some(fallback) = &fallback { - registry.language_for_name_or_extension(fallback) + registry.language_for_name(fallback.as_ref()).right_future() } else { continue; }; diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index f73f222503..40bc66422e 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -670,7 +670,7 @@ pub fn measure(label: &str, f: impl FnOnce() -> R) -> R { } } -pub fn iterate_expanded_and_wrapped_usize_range( +pub fn expanded_and_wrapped_usize_range( range: Range, additional_before: usize, additional_after: usize, @@ -699,6 +699,43 @@ pub fn iterate_expanded_and_wrapped_usize_range( } } +/// Yields `[i, i + 1, i - 1, i + 2, ..]`, each modulo `wrap_length` and bounded by +/// `additional_before` and `additional_after`. If the wrapping causes overlap, duplicates are not +/// emitted. If wrap_length is 0, nothing is yielded. +pub fn wrapped_usize_outward_from( + start: usize, + additional_before: usize, + additional_after: usize, + wrap_length: usize, +) -> impl Iterator { + let mut count = 0; + let mut after_offset = 1; + let mut before_offset = 1; + + std::iter::from_fn(move || { + count += 1; + if count > wrap_length { + None + } else if count == 1 { + Some(start % wrap_length) + } else if after_offset <= additional_after && after_offset <= before_offset { + let value = (start + after_offset) % wrap_length; + after_offset += 1; + Some(value) + } else if before_offset <= additional_before { + let value = (start + wrap_length - before_offset) % wrap_length; + before_offset += 1; + Some(value) + } else if after_offset <= additional_after { + let value = (start + after_offset) % wrap_length; + after_offset += 1; + Some(value) + } else { + None + } + }) +} + #[cfg(target_os = "windows")] pub fn get_windows_system_shell() -> String { use std::path::PathBuf; @@ -1462,49 +1499,88 @@ Line 3"# } #[test] - fn test_iterate_expanded_and_wrapped_usize_range() { + fn test_expanded_and_wrapped_usize_range() { // Neither wrap assert_eq!( - iterate_expanded_and_wrapped_usize_range(2..4, 1, 1, 8).collect::>(), + expanded_and_wrapped_usize_range(2..4, 1, 1, 8).collect::>(), (1..5).collect::>() ); // Start wraps assert_eq!( - iterate_expanded_and_wrapped_usize_range(2..4, 3, 1, 8).collect::>(), + expanded_and_wrapped_usize_range(2..4, 3, 1, 8).collect::>(), ((0..5).chain(7..8)).collect::>() ); // Start wraps all the way around assert_eq!( - iterate_expanded_and_wrapped_usize_range(2..4, 5, 1, 8).collect::>(), + expanded_and_wrapped_usize_range(2..4, 5, 1, 8).collect::>(), (0..8).collect::>() ); // Start wraps all the way around and past 0 assert_eq!( - iterate_expanded_and_wrapped_usize_range(2..4, 10, 1, 8).collect::>(), + expanded_and_wrapped_usize_range(2..4, 10, 1, 8).collect::>(), (0..8).collect::>() ); // End wraps assert_eq!( - iterate_expanded_and_wrapped_usize_range(3..5, 1, 4, 8).collect::>(), + expanded_and_wrapped_usize_range(3..5, 1, 4, 8).collect::>(), (0..1).chain(2..8).collect::>() ); // End wraps all the way around assert_eq!( - iterate_expanded_and_wrapped_usize_range(3..5, 1, 5, 8).collect::>(), + expanded_and_wrapped_usize_range(3..5, 1, 5, 8).collect::>(), (0..8).collect::>() ); // End wraps all the way around and past the end assert_eq!( - iterate_expanded_and_wrapped_usize_range(3..5, 1, 10, 8).collect::>(), + expanded_and_wrapped_usize_range(3..5, 1, 10, 8).collect::>(), (0..8).collect::>() ); // Both start and end wrap assert_eq!( - iterate_expanded_and_wrapped_usize_range(3..5, 4, 4, 8).collect::>(), + expanded_and_wrapped_usize_range(3..5, 4, 4, 8).collect::>(), (0..8).collect::>() ); } + #[test] + fn test_wrapped_usize_outward_from() { + // No wrapping + assert_eq!( + wrapped_usize_outward_from(4, 2, 2, 10).collect::>(), + vec![4, 5, 3, 6, 2] + ); + // Wrapping at end + assert_eq!( + wrapped_usize_outward_from(8, 2, 3, 10).collect::>(), + vec![8, 9, 7, 0, 6, 1] + ); + // Wrapping at start + assert_eq!( + wrapped_usize_outward_from(1, 3, 2, 10).collect::>(), + vec![1, 2, 0, 3, 9, 8] + ); + // All values wrap around + assert_eq!( + wrapped_usize_outward_from(5, 10, 10, 8).collect::>(), + vec![5, 6, 4, 7, 3, 0, 2, 1] + ); + // None before / after + assert_eq!( + wrapped_usize_outward_from(3, 0, 0, 8).collect::>(), + vec![3] + ); + // Starting point already wrapped + assert_eq!( + wrapped_usize_outward_from(15, 2, 2, 10).collect::>(), + vec![5, 6, 4, 7, 3] + ); + // wrap_length of 0 + assert_eq!( + wrapped_usize_outward_from(4, 2, 2, 0).collect::>(), + Vec::::new() + ); + } + #[test] fn test_truncate_lines_to_byte_limit() { let text = "Line 1\nLine 2\nLine 3\nLine 4";