Cargo.lock 🔗
@@ -13974,7 +13974,6 @@ dependencies = [
"futures-lite 1.13.0",
"git2",
"globset",
- "itertools 0.13.0",
"log",
"rand 0.8.5",
"regex",
Revert "Resolve documentation for visible completions (#21705)" (#21985)
Conrad Irwin created
This reverts commit ab595b0d5575285f2351ff085c4a8862f2ddc1f2.
Release Notes:
- (preview only) Fixed a panic in completionsChange summary
Cargo.lock | 1
crates/editor/src/code_context_menus.rs | 136 +++----------
crates/editor/src/editor.rs | 2
crates/editor/src/editor_tests.rs | 256 +++++++++++++++-----------
crates/util/Cargo.toml | 1
crates/util/src/util.rs | 74 -------
6 files changed, 182 insertions(+), 288 deletions(-)
Detailed changes
crates/editor/src/code_context_menus.rs 🔗
@@ -1,9 +1,4 @@
-use std::{
- cell::Cell,
- cmp::{min, Reverse},
- ops::Range,
- sync::Arc,
-};
+use std::{cell::Cell, cmp::Reverse, ops::Range, sync::Arc};
use fuzzy::{StringMatch, StringMatchCandidate};
use gpui::{
@@ -16,9 +11,8 @@ use language::{CodeLabel, Documentation};
use lsp::LanguageServerId;
use multi_buffer::{Anchor, ExcerptId};
use ordered_float::OrderedFloat;
-use parking_lot::{Mutex, RwLock};
+use parking_lot::RwLock;
use project::{CodeAction, Completion, TaskSourceKind};
-use std::iter;
use task::ResolvedTask;
use ui::{
h_flex, ActiveTheme as _, Color, FluentBuilder as _, InteractiveElement as _, IntoElement,
@@ -151,7 +145,6 @@ pub struct CompletionsMenu {
resolve_completions: bool,
pub aside_was_displayed: Cell<bool>,
show_completion_documentation: bool,
- last_rendered_range: Arc<Mutex<Option<Range<usize>>>>,
}
impl CompletionsMenu {
@@ -180,6 +173,7 @@ impl CompletionsMenu {
sort_completions,
initial_position,
buffer,
+ show_completion_documentation,
completions: Arc::new(RwLock::new(completions)),
match_candidates,
matches: Vec::new().into(),
@@ -187,8 +181,6 @@ 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)),
}
}
@@ -244,7 +236,6 @@ impl CompletionsMenu {
resolve_completions: false,
aside_was_displayed: Cell::new(false),
show_completion_documentation: false,
- last_rendered_range: Arc::new(Mutex::new(None)),
}
}
@@ -253,7 +244,11 @@ impl CompletionsMenu {
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
) {
- self.update_selection_index(0, provider, cx);
+ self.selected_item = 0;
+ self.scroll_handle
+ .scroll_to_item(self.selected_item, ScrollStrategy::Top);
+ self.resolve_selected_completion(provider, cx);
+ cx.notify();
}
fn select_prev(
@@ -261,7 +256,15 @@ impl CompletionsMenu {
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
) {
- self.update_selection_index(self.prev_match_index(), provider, cx);
+ 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();
}
fn select_next(
@@ -269,7 +272,15 @@ impl CompletionsMenu {
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
) {
- self.update_selection_index(self.next_match_index(), provider, cx);
+ 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();
}
fn select_last(
@@ -277,41 +288,14 @@ impl CompletionsMenu {
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
) {
- 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<Editor>,
- ) {
- 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
- }
+ 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();
}
- pub fn resolve_visible_completions(
+ pub fn resolve_selected_completion(
&mut self,
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
@@ -323,59 +307,10 @@ impl CompletionsMenu {
return;
};
- // 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::<Vec<usize>>(),
- Some(completions) => candidate_ids
- .filter(|i| completions[*i].documentation.is_none())
- .collect::<Vec<usize>>(),
- };
-
- // 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::<Vec<usize>>();
-
+ let completion_index = self.matches[self.selected_item].candidate_id;
let resolve_task = provider.resolve_completions(
self.buffer.clone(),
- candidate_ids,
+ vec![completion_index],
self.completions.clone(),
cx,
);
@@ -471,14 +406,11 @@ 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();
crates/editor/src/editor.rs 🔗
@@ -3736,7 +3736,7 @@ impl Editor {
if editor.focus_handle.is_focused(cx) && menu.is_some() {
let mut menu = menu.unwrap();
- menu.resolve_visible_completions(editor.completion_provider.as_deref(), cx);
+ menu.resolve_selected_completion(editor.completion_provider.as_deref(), cx);
*context_menu = Some(CodeContextMenu::Completions(menu));
drop(context_menu);
cx.notify();
crates/editor/src/editor_tests.rs 🔗
@@ -25,18 +25,14 @@ 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::{
@@ -10746,62 +10742,6 @@ 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 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::<Vec<_>>();
-
- 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::<Vec<lsp::CompletionItem>>();
-
let mut cx = EditorLspTestContext::new_rust(
lsp::ServerCapabilities {
completion_provider: Some(lsp::CompletionOptions {
@@ -10818,15 +10758,138 @@ 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::<lsp::request::ResolveCompletionItem, _, _>({
+ 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::<lsp::request::Completion, _, _>(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,
+ 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()
+ },
+ ],
item_defaults: Some(lsp::CompletionListItemDefaults {
data: Some(default_data.clone()),
commit_characters: Some(default_commit_characters.clone()),
@@ -10843,21 +10906,6 @@ 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::<lsp::request::ResolveCompletionItem, _, _>({
- 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();
@@ -10869,50 +10917,40 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo
completions_menu
.matches
.iter()
- .map(|c| c.string.clone())
- .collect::<Vec<String>>(),
- items_out
- .iter()
- .map(|completion| completion.label.clone())
- .collect::<Vec<String>>()
+ .map(|c| c.string.as_str())
+ .collect::<Vec<_>>(),
+ vec!["Some(2)", "vec![2]"]
);
}
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!(
- *resolved_items.lock(),
- [
- &items_out[0..16],
- &items_out[items_out.len() - 4..items_out.len()]
- ]
- .concat()
- .iter()
- .cloned()
- .collect::<Vec<lsp::CompletionItem>>()
+ 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().clear();
cx.update_editor(|editor, cx| {
- editor.context_menu_prev(&ContextMenuPrev, cx);
+ editor.context_menu_first(&ContextMenuFirst, cx);
});
cx.run_until_parked();
- // Completions that have already been resolved are skipped.
assert_eq!(
- *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::<Vec<lsp::CompletionItem>>()
+ 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().clear();
}
#[gpui::test]
crates/util/Cargo.toml 🔗
@@ -24,7 +24,6 @@ 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
crates/util/src/util.rs 🔗
@@ -8,7 +8,6 @@ pub mod test;
use futures::Future;
-use itertools::Either;
use regex::Regex;
use std::sync::OnceLock;
use std::{
@@ -200,35 +199,6 @@ pub fn measure<R>(label: &str, f: impl FnOnce() -> R) -> R {
}
}
-pub fn iterate_expanded_and_wrapped_usize_range(
- range: Range<usize>,
- additional_before: usize,
- additional_after: usize,
- wrap_length: usize,
-) -> impl Iterator<Item = usize> {
- 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<E> {
type Ok;
@@ -763,48 +733,4 @@ 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::<Vec<usize>>(),
- (1..5).collect::<Vec<usize>>()
- );
- // Start wraps
- assert_eq!(
- iterate_expanded_and_wrapped_usize_range(2..4, 3, 1, 8).collect::<Vec<usize>>(),
- ((0..5).chain(7..8)).collect::<Vec<usize>>()
- );
- // Start wraps all the way around
- assert_eq!(
- iterate_expanded_and_wrapped_usize_range(2..4, 5, 1, 8).collect::<Vec<usize>>(),
- (0..8).collect::<Vec<usize>>()
- );
- // Start wraps all the way around and past 0
- assert_eq!(
- iterate_expanded_and_wrapped_usize_range(2..4, 10, 1, 8).collect::<Vec<usize>>(),
- (0..8).collect::<Vec<usize>>()
- );
- // End wraps
- assert_eq!(
- iterate_expanded_and_wrapped_usize_range(3..5, 1, 4, 8).collect::<Vec<usize>>(),
- (0..1).chain(2..8).collect::<Vec<usize>>()
- );
- // End wraps all the way around
- assert_eq!(
- iterate_expanded_and_wrapped_usize_range(3..5, 1, 5, 8).collect::<Vec<usize>>(),
- (0..8).collect::<Vec<usize>>()
- );
- // End wraps all the way around and past the end
- assert_eq!(
- iterate_expanded_and_wrapped_usize_range(3..5, 1, 10, 8).collect::<Vec<usize>>(),
- (0..8).collect::<Vec<usize>>()
- );
- // Both start and end wrap
- assert_eq!(
- iterate_expanded_and_wrapped_usize_range(3..5, 4, 4, 8).collect::<Vec<usize>>(),
- (0..8).collect::<Vec<usize>>()
- );
- }
}