Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Do less resolves when showing the completion menu (#21286)

Browse source 
Closes https://github.com/zed-industries/zed/issues/21205

Zed does completion resolve on every menu item selection and when
applying the edit, so resolving all completion menu list is excessive
indeed.

In addition to that, removes the documentation-centric approach of menu
resolves, as we're actually resolving these for more than that, e.g.
additionalTextEdits and have to do that always, even if we do not show
the documentation.

Potentially, we can omit the second resolve too, but that seems
relatively dangerous, and many servers remove the `data` after the first
resolve, so a 2nd one is not that harmful given that we used to do much
more

Release Notes:

- Reduced the amount of `completionItem/resolve` calls done in the
completion menu
This commit is contained in:
Kirill Bulatov 2024-11-28 18:16:37 +02:00 • committed by GitHub
parent 6cba467a4e
commit f30944543e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 130 additions and 173 deletions
Download patch file Download diff file Expand all files Collapse all files

108
crates/editor/src/editor.rs
View file

@ -596,7 +596,6 @@ pub struct Editor {
auto_signature_help: Option<bool>,
find_all_references_task_sources: Vec<Anchor>,
next_completion_id: CompletionId,
completion_documentation_pre_resolve_debounce: DebouncedDelay,
available_code_actions: Option<(Location, Arc<[AvailableCodeAction]>)>,
code_actions_task: Option<Task<Result<()>>>,
document_highlights_task: Option<Task<()>>,
@ -1006,7 +1005,7 @@ struct CompletionsMenu {
matches: Arc<[StringMatch]>,
selected_item: usize,
scroll_handle: UniformListScrollHandle,
selected_completion_documentation_resolve_debounce: Option<Arc<Mutex<DebouncedDelay>>>,
selected_completion_resolve_debounce: Option<Arc<Mutex<DebouncedDelay>>>,
}
impl CompletionsMenu {
@ -1038,9 +1037,7 @@ impl CompletionsMenu {
matches: Vec::new().into(),
selected_item: 0,
scroll_handle: UniformListScrollHandle::new(),
selected_completion_documentation_resolve_debounce: Some(Arc::new(Mutex::new(
DebouncedDelay::new(),
))),
selected_completion_resolve_debounce: Some(Arc::new(Mutex::new(DebouncedDelay::new()))),
}
}
@ -1093,15 +1090,12 @@ impl CompletionsMenu {
matches,
selected_item: 0,
scroll_handle: UniformListScrollHandle::new(),
selected_completion_documentation_resolve_debounce: Some(Arc::new(Mutex::new(
DebouncedDelay::new(),
))),
selected_completion_resolve_debounce: Some(Arc::new(Mutex::new(DebouncedDelay::new()))),
}
}
fn suppress_documentation_resolution(mut self) -> Self {
self.selected_completion_documentation_resolve_debounce
.take();
self.selected_completion_resolve_debounce.take();
self
}
@ -1113,7 +1107,7 @@ impl CompletionsMenu {
self.selected_item = 0;
self.scroll_handle
.scroll_to_item(self.selected_item, ScrollStrategy::Top);
self.attempt_resolve_selected_completion_documentation(provider, cx);
self.resolve_selected_completion(provider, cx);
cx.notify();
}
@ -1129,7 +1123,7 @@ impl CompletionsMenu {
}
self.scroll_handle
.scroll_to_item(self.selected_item, ScrollStrategy::Top);
self.attempt_resolve_selected_completion_documentation(provider, cx);
self.resolve_selected_completion(provider, cx);
cx.notify();
}
@ -1145,7 +1139,7 @@ impl CompletionsMenu {
}
self.scroll_handle
.scroll_to_item(self.selected_item, ScrollStrategy::Top);
self.attempt_resolve_selected_completion_documentation(provider, cx);
self.resolve_selected_completion(provider, cx);
cx.notify();
}
@ -1157,58 +1151,20 @@ impl CompletionsMenu {
self.selected_item = self.matches.len() - 1;
self.scroll_handle
.scroll_to_item(self.selected_item, ScrollStrategy::Top);
self.attempt_resolve_selected_completion_documentation(provider, cx);
self.resolve_selected_completion(provider, cx);
cx.notify();
}
fn pre_resolve_completion_documentation(
buffer: Model<Buffer>,
completions: Arc<RwLock<Box<[Completion]>>>,
matches: Arc<[StringMatch]>,
editor: &Editor,
cx: &mut ViewContext<Editor>,
) -> Task<()> {
let settings = EditorSettings::get_global(cx);
if !settings.show_completion_documentation {
return Task::ready(());
}
let Some(provider) = editor.completion_provider.as_ref() else {
return Task::ready(());
};
let resolve_task = provider.resolve_completions(
buffer,
matches.iter().map(|m| m.candidate_id).collect(),
completions.clone(),
cx,
);
cx.spawn(move |this, mut cx| async move {
if let Some(true) = resolve_task.await.log_err() {
this.update(&mut cx, |_, cx| cx.notify()).ok();
}
})
}
fn attempt_resolve_selected_completion_documentation(
fn resolve_selected_completion(
&mut self,
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
) {
let settings = EditorSettings::get_global(cx);
if !settings.show_completion_documentation {
return;
}
let completion_index = self.matches[self.selected_item].candidate_id;
let Some(provider) = provider else {
return;
};
let Some(documentation_resolve) = self
.selected_completion_documentation_resolve_debounce
.as_ref()
else {
let Some(completion_resolve) = self.selected_completion_resolve_debounce.as_ref() else {
return;
};
@ -1223,7 +1179,7 @@ impl CompletionsMenu {
EditorSettings::get_global(cx).completion_documentation_secondary_query_debounce;
let delay = Duration::from_millis(delay_ms);
documentation_resolve.lock().fire_new(delay, cx, |_, cx| {
completion_resolve.lock().fire_new(delay, cx, |_, cx| {
cx.spawn(move |this, mut cx| async move {
if let Some(true) = resolve_task.await.log_err() {
this.update(&mut cx, |_, cx| cx.notify()).ok();
@ -2118,7 +2074,6 @@ impl Editor {
auto_signature_help: None,
find_all_references_task_sources: Vec::new(),
next_completion_id: 0,
completion_documentation_pre_resolve_debounce: DebouncedDelay::new(),
next_inlay_id: 0,
code_action_providers,
available_code_actions: Default::default(),
@ -4523,9 +4478,9 @@ impl Editor {
let sort_completions = provider.sort_completions();
let id = post_inc(&mut self.next_completion_id);
let task = cx.spawn(|this, mut cx| {
let task = cx.spawn(|editor, mut cx| {
async move {
this.update(&mut cx, |this, _| {
editor.update(&mut cx, |this, _| {
this.completion_tasks.retain(|(task_id, _)| *task_id >= id);
})?;
let completions = completions.await.log_err();
@ -4543,34 +4498,14 @@ impl Editor {
if menu.matches.is_empty() {
None
} else {
this.update(&mut cx, |editor, cx| {
let completions = menu.completions.clone();
let matches = menu.matches.clone();
let delay_ms = EditorSettings::get_global(cx)
.completion_documentation_secondary_query_debounce;
let delay = Duration::from_millis(delay_ms);
editor
.completion_documentation_pre_resolve_debounce
.fire_new(delay, cx, |editor, cx| {
CompletionsMenu::pre_resolve_completion_documentation(
buffer,
completions,
matches,
editor,
cx,
)
});
})
.ok();
Some(menu)
}
} else {
None
};
this.update(&mut cx, |this, cx| {
let mut context_menu = this.context_menu.write();
editor.update(&mut cx, |editor, cx| {
let mut context_menu = editor.context_menu.write();
match context_menu.as_ref() {
None => {}
@ -4583,19 +4518,20 @@ impl Editor {
_ => return,
}
if this.focus_handle.is_focused(cx) && menu.is_some() {
let menu = menu.unwrap();
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);
*context_menu = Some(ContextMenu::Completions(menu));
drop(context_menu);
this.discard_inline_completion(false, cx);
editor.discard_inline_completion(false, cx);
cx.notify();
} else if this.completion_tasks.len() <= 1 {
} else if editor.completion_tasks.len() <= 1 {
// If there are no more completion tasks and the last menu was
// empty, we should hide it. If it was already hidden, we should
// also show the copilot completion when available.
drop(context_menu);
if this.hide_context_menu(cx).is_none() {
this.update_visible_inline_completion(cx);
if editor.hide_context_menu(cx).is_none() {
editor.update_visible_inline_completion(cx);
}
}
})?;

195
crates/editor/src/editor_tests.rs
View file

@ -31,8 +31,8 @@ use project::{
project_settings::{LspSettings, ProjectSettings},
};
use serde_json::{self, json};
use std::sync::atomic;
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::{self, AtomicBool};
use std::{cell::RefCell, future::Future, rc::Rc, time::Instant};
use unindent::Unindent;
use util::{
@ -10576,6 +10576,94 @@ 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::<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 |_, _, _| {
@ -10623,7 +10711,7 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo
cx.condition(|editor, _| editor.context_menu_visible())
.await;
cx.run_until_parked();
cx.update_editor(|editor, _| {
let menu = editor.context_menu.read();
match menu.as_ref().expect("should have the completions menu") {
@ -10640,99 +10728,32 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo
ContextMenu::CodeActions(_) => panic!("Expected to have the completions menu"),
}
});
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);
});
let first_item_resolve_characters = default_commit_characters.clone();
cx.handle_request::<lsp::request::ResolveCompletionItem, _, _>(move |_, item_to_resolve, _| {
let default_commit_characters = first_item_resolve_characters.clone();
async move {
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)
}
})
.next()
.await
.unwrap();
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"
);
expect_first_item.store(false, atomic::Ordering::Release);
cx.update_editor(|editor, cx| {
editor.context_menu_last(&ContextMenuLast, cx);
});
cx.handle_request::<lsp::request::ResolveCompletionItem, _, _>(move |_, item_to_resolve, _| {
let default_data = default_data.clone();
let default_commit_characters = default_commit_characters.clone();
async move {
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)
}
})
.next()
.await
.unwrap();
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"
);
}
#[gpui::test]