diff --git a/Cargo.lock b/Cargo.lock index e8a6c7d0852da0eb5a70ff251611037f2a0f988f..cdb1fb589906ad69fd8d87e4055c4e684bded60e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13930,6 +13930,7 @@ dependencies = [ "futures-lite 1.13.0", "git2", "globset", + "itertools 0.13.0", "log", "rand 0.8.5", "regex", diff --git a/crates/editor/src/code_context_menus.rs b/crates/editor/src/code_context_menus.rs index 1817190e426b4eb954bfc707a01da15af5f290f6..2c76287f00bbc1248c74e411ab374f281d79d946 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -1,4 +1,9 @@ -use std::{cell::Cell, cmp::Reverse, ops::Range, sync::Arc}; +use std::{ + cell::Cell, + cmp::{min, Reverse}, + ops::Range, + sync::Arc, +}; use fuzzy::{StringMatch, StringMatchCandidate}; use gpui::{ @@ -11,8 +16,9 @@ use language::{CodeLabel, Documentation}; use lsp::LanguageServerId; use multi_buffer::{Anchor, ExcerptId}; use ordered_float::OrderedFloat; -use parking_lot::RwLock; +use parking_lot::{Mutex, RwLock}; use project::{CodeAction, Completion, TaskSourceKind}; +use std::iter; use task::ResolvedTask; use ui::{ h_flex, ActiveTheme as _, Color, FluentBuilder as _, InteractiveElement as _, IntoElement, @@ -145,6 +151,7 @@ pub struct CompletionsMenu { resolve_completions: bool, pub aside_was_displayed: Cell, show_completion_documentation: bool, + last_rendered_range: Arc>>>, } impl CompletionsMenu { @@ -173,7 +180,6 @@ impl CompletionsMenu { sort_completions, initial_position, buffer, - show_completion_documentation, completions: Arc::new(RwLock::new(completions)), match_candidates, matches: Vec::new().into(), @@ -181,6 +187,8 @@ impl CompletionsMenu { scroll_handle: UniformListScrollHandle::new(), resolve_completions: true, aside_was_displayed: Cell::new(aside_was_displayed), + show_completion_documentation, + last_rendered_range: Arc::new(Mutex::new(None)), } } @@ -236,6 +244,7 @@ impl CompletionsMenu { resolve_completions: false, aside_was_displayed: Cell::new(false), show_completion_documentation: false, + last_rendered_range: Arc::new(Mutex::new(None)), } } @@ -244,11 +253,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.selected_item = 0; - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(0, provider, cx); } fn select_prev( @@ -256,15 +261,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item > 0 { - self.selected_item -= 1; - } else { - self.selected_item = self.matches.len() - 1; - } - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(self.prev_match_index(), provider, cx); } fn select_next( @@ -272,15 +269,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item + 1 < self.matches.len() { - self.selected_item += 1; - } else { - self.selected_item = 0; - } - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(self.next_match_index(), provider, cx); } fn select_last( @@ -288,14 +277,41 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.selected_item = self.matches.len() - 1; - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(self.matches.len() - 1, provider, cx); + } + + fn update_selection_index( + &mut self, + match_index: usize, + provider: Option<&dyn CompletionProvider>, + cx: &mut ViewContext, + ) { + if self.selected_item != match_index { + self.selected_item = match_index; + self.scroll_handle + .scroll_to_item(self.selected_item, ScrollStrategy::Top); + self.resolve_visible_completions(provider, cx); + cx.notify(); + } + } + + fn prev_match_index(&self) -> usize { + if self.selected_item > 0 { + self.selected_item - 1 + } else { + self.matches.len() - 1 + } + } + + fn next_match_index(&self) -> usize { + if self.selected_item + 1 < self.matches.len() { + self.selected_item + 1 + } else { + 0 + } } - pub fn resolve_selected_completion( + pub fn resolve_visible_completions( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, @@ -307,10 +323,59 @@ impl CompletionsMenu { return; }; - let completion_index = self.matches[self.selected_item].candidate_id; + // Attempt to resolve completions for every item that will be displayed. This matters + // because single line documentation may be displayed inline with the completion. + // + // When navigating to the very beginning or end of completions, `last_rendered_range` may + // have no overlap with the completions that will be displayed, so instead use a range based + // on the last rendered count. + const APPROXIMATE_VISIBLE_COUNT: usize = 12; + let last_rendered_range = self.last_rendered_range.lock().clone(); + let visible_count = last_rendered_range + .clone() + .map_or(APPROXIMATE_VISIBLE_COUNT, |range| range.count()); + let matches_range = if self.selected_item == 0 { + 0..min(visible_count, self.matches.len()) + } else if self.selected_item == self.matches.len() - 1 { + self.matches.len().saturating_sub(visible_count)..self.matches.len() + } else { + last_rendered_range.unwrap_or_else(|| self.selected_item..self.selected_item + 1) + }; + + // 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 matches_indices = util::iterate_expanded_and_wrapped_usize_range( + matches_range.clone(), + EXTRA_TO_RESOLVE, + EXTRA_TO_RESOLVE, + self.matches.len(), + ); + + // Avoid work by sometimes filtering out completions that already have documentation. + // This filtering doesn't happen if the completions are currently being updated. + let candidate_ids = matches_indices.map(|i| self.matches[i].candidate_id); + let candidate_ids = match self.completions.try_read() { + None => candidate_ids.collect::>(), + Some(completions) => candidate_ids + .filter(|i| completions[*i].documentation.is_none()) + .collect::>(), + }; + + // Current selection is always resolved even if it already has documentation, to handle + // out-of-spec language servers that return more results later. + let selected_candidate_id = self.matches[self.selected_item].candidate_id; + let candidate_ids = iter::once(selected_candidate_id) + .chain( + candidate_ids + .into_iter() + .filter(|id| *id != selected_candidate_id), + ) + .collect::>(); + let resolve_task = provider.resolve_completions( self.buffer.clone(), - vec![completion_index], + candidate_ids, self.completions.clone(), cx, ); @@ -406,11 +471,14 @@ impl CompletionsMenu { .occlude() }); + let last_rendered_range = self.last_rendered_range.clone(); + let list = uniform_list( cx.view().clone(), "completions", matches.len(), move |_editor, range, cx| { + last_rendered_range.lock().replace(range.clone()); let start_ix = range.start; let completions_guard = completions.read(); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6039092be50ed0977fb3fe6618201f21033ae15c..1336a76f22194518bdfb3065fc3038fdde6f44cc 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3684,7 +3684,7 @@ impl Editor { if editor.focus_handle.is_focused(cx) && menu.is_some() { let mut menu = menu.unwrap(); - menu.resolve_selected_completion(editor.completion_provider.as_deref(), cx); + menu.resolve_visible_completions(editor.completion_provider.as_deref(), cx); *context_menu = Some(CodeContextMenu::Completions(menu)); drop(context_menu); editor.discard_inline_completion(false, cx); diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 468eec6fc3c231761640eebe1a95532e749ac575..c1f15f34b32c4b0aef0017f8241f3418a607af75 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -25,15 +25,15 @@ use language::{ use language_settings::{Formatter, FormatterList, IndentGuideSettings}; use multi_buffer::MultiBufferIndentGuide; use parking_lot::Mutex; +use pretty_assertions::{assert_eq, assert_ne}; use project::{buffer_store::BufferChangeSet, FakeFs}; use project::{ lsp_command::SIGNATURE_HELP_HIGHLIGHT_CURRENT, project_settings::{LspSettings, ProjectSettings}, }; use serde_json::{self, json}; -use std::sync::atomic::AtomicUsize; -use std::sync::atomic::{self, AtomicBool}; -use std::{cell::RefCell, future::Future, rc::Rc, time::Instant}; +use std::sync::atomic::{self, AtomicUsize}; +use std::{cell::RefCell, future::Future, iter, rc::Rc, time::Instant}; use test::editor_lsp_test_context::rust_lang; use unindent::Unindent; use util::{ @@ -10717,24 +10717,31 @@ async fn test_completions_resolve_updates_labels(cx: &mut gpui::TestAppContext) async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppContext) { init_test(cx, |_| {}); - let mut cx = EditorLspTestContext::new_rust( - lsp::ServerCapabilities { - completion_provider: Some(lsp::CompletionOptions { - trigger_characters: Some(vec![".".to_string()]), - resolve_provider: Some(true), - ..Default::default() - }), - ..Default::default() - }, - cx, - ) - .await; - - cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"}); - cx.simulate_keystroke("."); + let item_0 = lsp::CompletionItem { + label: "abs".into(), + insert_text: Some("abs".into()), + data: Some(json!({ "very": "special"})), + insert_text_mode: Some(lsp::InsertTextMode::ADJUST_INDENTATION), + text_edit: Some(lsp::CompletionTextEdit::InsertAndReplace( + lsp::InsertReplaceEdit { + new_text: "abs".to_string(), + insert: lsp::Range::default(), + replace: lsp::Range::default(), + }, + )), + ..lsp::CompletionItem::default() + }; + let items = iter::once(item_0.clone()) + .chain((11..51).map(|i| lsp::CompletionItem { + label: format!("item_{}", i), + insert_text: Some(format!("item_{}", i)), + insert_text_format: Some(lsp::InsertTextFormat::PLAIN_TEXT), + ..lsp::CompletionItem::default() + })) + .collect::>(); let default_commit_characters = vec!["?".to_string()]; - let default_data = json!({ "very": "special"}); + let default_data = json!({ "default": "data"}); let default_insert_text_format = lsp::InsertTextFormat::SNIPPET; let default_insert_text_mode = lsp::InsertTextMode::AS_IS; let default_edit_range = lsp::Range { @@ -10748,123 +10755,49 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo }, }; - let resolve_requests_number = Arc::new(AtomicUsize::new(0)); - let expect_first_item = Arc::new(AtomicBool::new(true)); - cx.lsp - .server - .on_request::({ - let closure_default_data = default_data.clone(); - let closure_resolve_requests_number = resolve_requests_number.clone(); - let closure_expect_first_item = expect_first_item.clone(); - let closure_default_commit_characters = default_commit_characters.clone(); - move |item_to_resolve, _| { - closure_resolve_requests_number.fetch_add(1, atomic::Ordering::Release); - let default_data = closure_default_data.clone(); - let default_commit_characters = closure_default_commit_characters.clone(); - let expect_first_item = closure_expect_first_item.clone(); - async move { - if expect_first_item.load(atomic::Ordering::Acquire) { - assert_eq!( - item_to_resolve.label, "Some(2)", - "Should have selected the first item" - ); - assert_eq!( - item_to_resolve.data, - Some(json!({ "very": "special"})), - "First item should bring its own data for resolving" - ); - assert_eq!( - item_to_resolve.commit_characters, - Some(default_commit_characters), - "First item had no own commit characters and should inherit the default ones" - ); - assert!( - matches!( - item_to_resolve.text_edit, - Some(lsp::CompletionTextEdit::InsertAndReplace { .. }) - ), - "First item should bring its own edit range for resolving" - ); - assert_eq!( - item_to_resolve.insert_text_format, - Some(default_insert_text_format), - "First item had no own insert text format and should inherit the default one" - ); - assert_eq!( - item_to_resolve.insert_text_mode, - Some(lsp::InsertTextMode::ADJUST_INDENTATION), - "First item should bring its own insert text mode for resolving" - ); - Ok(item_to_resolve) - } else { - assert_eq!( - item_to_resolve.label, "vec![2]", - "Should have selected the last item" - ); - assert_eq!( - item_to_resolve.data, - Some(default_data), - "Last item has no own resolve data and should inherit the default one" - ); - assert_eq!( - item_to_resolve.commit_characters, - Some(default_commit_characters), - "Last item had no own commit characters and should inherit the default ones" - ); - assert_eq!( - item_to_resolve.text_edit, - Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { - range: default_edit_range, - new_text: "vec![2]".to_string() - })), - "Last item had no own edit range and should inherit the default one" - ); - assert_eq!( - item_to_resolve.insert_text_format, - Some(lsp::InsertTextFormat::PLAIN_TEXT), - "Last item should bring its own insert text format for resolving" - ); - assert_eq!( - item_to_resolve.insert_text_mode, - Some(default_insert_text_mode), - "Last item had no own insert text mode and should inherit the default one" - ); - - Ok(item_to_resolve) - } - } - } - }).detach(); + let item_0_out = lsp::CompletionItem { + commit_characters: Some(default_commit_characters.clone()), + insert_text_format: Some(default_insert_text_format), + ..item_0 + }; + let items_out = iter::once(item_0_out) + .chain(items[1..].iter().map(|item| lsp::CompletionItem { + commit_characters: Some(default_commit_characters.clone()), + data: Some(default_data.clone()), + insert_text_mode: Some(default_insert_text_mode), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + range: default_edit_range, + new_text: item.label.clone(), + })), + ..item.clone() + })) + .collect::>(); + + let mut cx = EditorLspTestContext::new_rust( + lsp::ServerCapabilities { + completion_provider: Some(lsp::CompletionOptions { + trigger_characters: Some(vec![".".to_string()]), + resolve_provider: Some(true), + ..Default::default() + }), + ..Default::default() + }, + cx, + ) + .await; + + cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"}); + cx.simulate_keystroke("."); let completion_data = default_data.clone(); let completion_characters = default_commit_characters.clone(); cx.handle_request::(move |_, _, _| { let default_data = completion_data.clone(); let default_commit_characters = completion_characters.clone(); + let items = items.clone(); async move { Ok(Some(lsp::CompletionResponse::List(lsp::CompletionList { - items: vec![ - lsp::CompletionItem { - label: "Some(2)".into(), - insert_text: Some("Some(2)".into()), - data: Some(json!({ "very": "special"})), - insert_text_mode: Some(lsp::InsertTextMode::ADJUST_INDENTATION), - text_edit: Some(lsp::CompletionTextEdit::InsertAndReplace( - lsp::InsertReplaceEdit { - new_text: "Some(2)".to_string(), - insert: lsp::Range::default(), - replace: lsp::Range::default(), - }, - )), - ..lsp::CompletionItem::default() - }, - lsp::CompletionItem { - label: "vec![2]".into(), - insert_text: Some("vec![2]".into()), - insert_text_format: Some(lsp::InsertTextFormat::PLAIN_TEXT), - ..lsp::CompletionItem::default() - }, - ], + items, item_defaults: Some(lsp::CompletionListItemDefaults { data: Some(default_data.clone()), commit_characters: Some(default_commit_characters.clone()), @@ -10881,6 +10814,21 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo .next() .await; + let resolved_items = Arc::new(Mutex::new(Vec::new())); + cx.lsp + .server + .on_request::({ + let closure_resolved_items = resolved_items.clone(); + move |item_to_resolve, _| { + let closure_resolved_items = closure_resolved_items.clone(); + async move { + closure_resolved_items.lock().push(item_to_resolve.clone()); + Ok(item_to_resolve) + } + } + }) + .detach(); + cx.condition(|editor, _| editor.context_menu_visible()) .await; cx.run_until_parked(); @@ -10892,40 +10840,50 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo completions_menu .matches .iter() - .map(|c| c.string.as_str()) - .collect::>(), - vec!["Some(2)", "vec![2]"] + .map(|c| c.string.clone()) + .collect::>(), + items_out + .iter() + .map(|completion| completion.label.clone()) + .collect::>() ); } CodeContextMenu::CodeActions(_) => panic!("Expected to have the completions menu"), } }); + // Approximate initial displayed interval is 0..12. With extra item padding of 4 this is 0..16 + // with 4 from the end. assert_eq!( - resolve_requests_number.load(atomic::Ordering::Acquire), - 1, - "While there are 2 items in the completion list, only 1 resolve request should have been sent, for the selected item" - ); - - cx.update_editor(|editor, cx| { - editor.context_menu_first(&ContextMenuFirst, cx); - }); - cx.run_until_parked(); - assert_eq!( - resolve_requests_number.load(atomic::Ordering::Acquire), - 2, - "After re-selecting the first item, another resolve request should have been sent" + *resolved_items.lock(), + [ + &items_out[0..16], + &items_out[items_out.len() - 4..items_out.len()] + ] + .concat() + .iter() + .cloned() + .collect::>() ); + resolved_items.lock().clear(); - expect_first_item.store(false, atomic::Ordering::Release); cx.update_editor(|editor, cx| { - editor.context_menu_last(&ContextMenuLast, cx); + editor.context_menu_prev(&ContextMenuPrev, cx); }); cx.run_until_parked(); + // Completions that have already been resolved are skipped. assert_eq!( - resolve_requests_number.load(atomic::Ordering::Acquire), - 3, - "After selecting the other item, another resolve request should have been sent" + *resolved_items.lock(), + [ + // Selected item is always resolved even if it was resolved before. + &items_out[items_out.len() - 1..items_out.len()], + &items_out[items_out.len() - 16..items_out.len() - 4] + ] + .concat() + .iter() + .cloned() + .collect::>() ); + resolved_items.lock().clear(); } #[gpui::test] diff --git a/crates/util/Cargo.toml b/crates/util/Cargo.toml index 2f841144092383b28075a26e0325582fdcdd4178..0e75e5221eb9a50efd0fe00453f6f5d6e08d6c2c 100644 --- a/crates/util/Cargo.toml +++ b/crates/util/Cargo.toml @@ -24,6 +24,7 @@ futures-lite.workspace = true futures.workspace = true git2 = { workspace = true, optional = true } globset.workspace = true +itertools.workspace = true log.workspace = true rand = { workspace = true, optional = true } regex.workspace = true diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index 777b8b60dcdffdd5c7655553064b6f50c44b7f54..58290373e1c634b026861172165ac0f4d8a8f13b 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -8,6 +8,7 @@ pub mod test; use futures::Future; +use itertools::Either; use regex::Regex; use std::sync::OnceLock; use std::{ @@ -199,6 +200,35 @@ pub fn measure(label: &str, f: impl FnOnce() -> R) -> R { } } +pub fn iterate_expanded_and_wrapped_usize_range( + range: Range, + additional_before: usize, + additional_after: usize, + wrap_length: usize, +) -> impl Iterator { + let start_wraps = range.start < additional_before; + let end_wraps = wrap_length < range.end + additional_after; + if start_wraps && end_wraps { + Either::Left(0..wrap_length) + } else if start_wraps { + let wrapped_start = (range.start + wrap_length).saturating_sub(additional_before); + if wrapped_start <= range.end { + Either::Left(0..wrap_length) + } else { + Either::Right((0..range.end + additional_after).chain(wrapped_start..wrap_length)) + } + } else if end_wraps { + let wrapped_end = range.end + additional_after - wrap_length; + if range.start <= wrapped_end { + Either::Left(0..wrap_length) + } else { + Either::Right((0..wrapped_end).chain(range.start - additional_before..wrap_length)) + } + } else { + Either::Left((range.start - additional_before)..(range.end + additional_after)) + } +} + pub trait ResultExt { type Ok; @@ -733,4 +763,48 @@ Line 2 Line 3"# ); } + + #[test] + fn test_iterate_expanded_and_wrapped_usize_range() { + // Neither wrap + assert_eq!( + iterate_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::>(), + ((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::>(), + (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::>(), + (0..8).collect::>() + ); + // End wraps + assert_eq!( + iterate_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::>(), + (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::>(), + (0..8).collect::>() + ); + // Both start and end wrap + assert_eq!( + iterate_expanded_and_wrapped_usize_range(3..5, 4, 4, 8).collect::>(), + (0..8).collect::>() + ); + } }