diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 78f0aab5a5b7c30edf9d4261ff2c303bc4877bf8..611ec9232e08da9b906489cf0ddff2a2543dc950 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -596,7 +596,6 @@ pub struct Editor { auto_signature_help: Option, find_all_references_task_sources: Vec, next_completion_id: CompletionId, - completion_documentation_pre_resolve_debounce: DebouncedDelay, available_code_actions: Option<(Location, Arc<[AvailableCodeAction]>)>, code_actions_task: Option>>, document_highlights_task: Option>, @@ -1006,7 +1005,7 @@ struct CompletionsMenu { matches: Arc<[StringMatch]>, selected_item: usize, scroll_handle: UniformListScrollHandle, - selected_completion_documentation_resolve_debounce: Option>>, + selected_completion_resolve_debounce: Option>>, } 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, - completions: Arc>>, - matches: Arc<[StringMatch]>, - editor: &Editor, - cx: &mut ViewContext, - ) -> 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, ) { - 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); } } })?; diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 669134ef1028d7e1807e18c755d03f67e9217c1f..b49b3fa33b3c00a22e32ad69990acd4ae2527af8 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -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::({ + 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 |_, _, _| { @@ -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::(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::(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]