diff --git a/Cargo.lock b/Cargo.lock index 7b5007a703..03fa792d90 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13977,6 +13977,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 26b2fdf8ba..2432e9fc97 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -1,6 +1,3 @@ -use std::cell::RefCell; -use std::{cmp::Reverse, ops::Range, rc::Rc}; - use fuzzy::{StringMatch, StringMatchCandidate}; use gpui::{ div, px, uniform_list, AnyElement, BackgroundExecutor, Div, FontWeight, ListSizingBehavior, @@ -13,6 +10,13 @@ use lsp::LanguageServerId; use multi_buffer::{Anchor, ExcerptId}; use ordered_float::OrderedFloat; use project::{CodeAction, Completion, TaskSourceKind}; +use std::{ + cell::RefCell, + cmp::{min, Reverse}, + iter, + ops::Range, + rc::Rc, +}; use task::ResolvedTask; use ui::{prelude::*, Color, IntoElement, ListItem, Pixels, Popover, Styled}; use util::ResultExt; @@ -158,6 +162,7 @@ pub struct CompletionsMenu { scroll_handle: UniformListScrollHandle, resolve_completions: bool, show_completion_documentation: bool, + last_rendered_range: Rc>>>, } #[derive(Clone, Debug)] @@ -193,6 +198,7 @@ impl CompletionsMenu { selected_item: 0, scroll_handle: UniformListScrollHandle::new(), resolve_completions: true, + last_rendered_range: RefCell::new(None).into(), } } @@ -249,6 +255,7 @@ impl CompletionsMenu { scroll_handle: UniformListScrollHandle::new(), resolve_completions: false, show_completion_documentation: false, + last_rendered_range: RefCell::new(None).into(), } } @@ -257,11 +264,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( @@ -269,15 +272,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item > 0 { - self.selected_item -= 1; - } else { - self.selected_item = self.entries.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( @@ -285,15 +280,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item + 1 < self.entries.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( @@ -301,11 +288,38 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.selected_item = self.entries.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.entries.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.entries.len() - 1 + } + } + + fn next_match_index(&self) -> usize { + if self.selected_item + 1 < self.entries.len() { + self.selected_item + 1 + } else { + 0 + } } pub fn show_inline_completion_hint(&mut self, hint: InlineCompletionMenuHint) { @@ -330,7 +344,7 @@ impl CompletionsMenu { } } - pub fn resolve_selected_completion( + pub fn resolve_visible_completions( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, @@ -342,24 +356,76 @@ impl CompletionsMenu { return; }; - match &self.entries[self.selected_item] { - CompletionEntry::Match(entry) => { - let completion_index = entry.candidate_id; - let resolve_task = provider.resolve_completions( - self.buffer.clone(), - vec![completion_index], - self.completions.clone(), - cx, - ); + // 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.borrow().clone(); + let visible_count = last_rendered_range + .clone() + .map_or(APPROXIMATE_VISIBLE_COUNT, |range| range.count()); + let entry_range = if self.selected_item == 0 { + 0..min(visible_count, self.entries.len()) + } else if self.selected_item == self.entries.len() - 1 { + self.entries.len().saturating_sub(visible_count)..self.entries.len() + } else { + last_rendered_range.map_or(0..0, |range| { + min(range.start, self.entries.len())..min(range.end, self.entries.len()) + }) + }; - cx.spawn(move |editor, mut cx| async move { - if let Some(true) = resolve_task.await.log_err() { - editor.update(&mut cx, |_, cx| cx.notify()).ok(); - } - }) - .detach(); + // 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( + entry_range.clone(), + EXTRA_TO_RESOLVE, + EXTRA_TO_RESOLVE, + self.entries.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 completions = self.completions.borrow(); + let candidate_ids = entry_indices + .flat_map(|i| Self::entry_candidate_id(&self.entries[i])) + .filter(|i| completions[*i].documentation.is_none()); + + // Current selection is always resolved even if it already has documentation, to handle + // out-of-spec language servers that return more results later. + let candidate_ids = match Self::entry_candidate_id(&self.entries[self.selected_item]) { + None => candidate_ids.collect::>(), + Some(selected_candidate_id) => iter::once(selected_candidate_id) + .chain(candidate_ids.filter(|id| *id != selected_candidate_id)) + .collect::>(), + }; + + if candidate_ids.is_empty() { + return; + } + + let resolve_task = provider.resolve_completions( + self.buffer.clone(), + candidate_ids, + self.completions.clone(), + cx, + ); + + cx.spawn(move |editor, mut cx| async move { + if let Some(true) = resolve_task.await.log_err() { + editor.update(&mut cx, |_, cx| cx.notify()).ok(); } - CompletionEntry::InlineCompletionHint { .. } => {} + }) + .detach(); + } + + fn entry_candidate_id(entry: &CompletionEntry) -> Option { + match entry { + CompletionEntry::Match(entry) => Some(entry.candidate_id), + CompletionEntry::InlineCompletionHint { .. } => None, } } @@ -408,12 +474,14 @@ impl CompletionsMenu { let selected_item = self.selected_item; let completions = self.completions.clone(); let matches = self.entries.clone(); + let last_rendered_range = self.last_rendered_range.clone(); let style = style.clone(); let list = uniform_list( cx.view().clone(), "completions", matches.len(), move |_editor, range, cx| { + last_rendered_range.borrow_mut().replace(range.clone()); let start_ix = range.start; let completions_guard = completions.borrow_mut(); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 5876f003f9..36959e055c 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3742,7 +3742,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); if editor.show_inline_completions_in_menu(cx) { if let Some(hint) = editor.inline_completion_menu_hint(cx) { diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 4882d8c217..cc7d612bbf 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -25,14 +25,18 @@ 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::{self, AtomicBool, AtomicUsize}; use std::{cell::RefCell, future::Future, rc::Rc, time::Instant}; +use std::{ + iter, + sync::atomic::{self, AtomicUsize}, +}; use test::{build_editor_with_project, editor_lsp_test_context::rust_lang}; use unindent::Unindent; use util::{ @@ -10787,6 +10791,62 @@ async fn test_completions_resolve_updates_labels_if_filter_text_matches( async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppContext) { init_test(cx, |_| {}); + 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!({ "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 { + start: lsp::Position { + line: 0, + character: 5, + }, + end: lsp::Position { + line: 0, + character: 5, + }, + }; + + 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 { @@ -10803,138 +10863,15 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"}); cx.simulate_keystroke("."); - let default_commit_characters = vec!["?".to_string()]; - let default_data = json!({ "very": "special"}); - let default_insert_text_format = lsp::InsertTextFormat::SNIPPET; - let default_insert_text_mode = lsp::InsertTextMode::AS_IS; - let default_edit_range = lsp::Range { - start: lsp::Position { - line: 0, - character: 5, - }, - end: lsp::Position { - line: 0, - character: 5, - }, - }; - - 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 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()), @@ -10951,6 +10888,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(); @@ -10959,39 +10911,56 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo match menu.as_ref().expect("should have the completions menu") { CodeContextMenu::Completions(completions_menu) => { assert_eq!( - completion_menu_entries(&completions_menu.entries), - vec!["Some(2)", "vec![2]"] + completions_menu + .entries + .iter() + .flat_map(|c| match c { + CompletionEntry::Match(mat) => Some(mat.string.clone()), + _ => None, + }) + .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" + *resolved_items.lock(), + [ + &items_out[0..16], + &items_out[items_out.len() - 4..items_out.len()] + ] + .concat() + .iter() + .cloned() + .collect::>() ); + resolved_items.lock().clear(); cx.update_editor(|editor, cx| { - editor.context_menu_first(&ContextMenuFirst, 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), - 2, - "After re-selecting the first item, another resolve request should have been sent" - ); - - expect_first_item.store(false, atomic::Ordering::Release); - cx.update_editor(|editor, cx| { - editor.context_menu_last(&ContextMenuLast, cx); - }); - cx.run_until_parked(); - 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 2f84114409..0e75e5221e 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 3ba42e33cd..560fe35d74 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::{LazyLock, 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; @@ -734,4 +764,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::>() + ); + } }