From 75d6b6360f02143b2040c6e382c4a4e0318c5872 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 18 Apr 2023 10:24:20 +0200 Subject: [PATCH 1/2] Add failing test to demonstrate Copilot not showing enough suggestions --- crates/editor/src/editor_tests.rs | 41 +++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 74fad79fe8..9cf3c4a81e 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -5897,13 +5897,12 @@ async fn test_copilot(deterministic: Arc, cx: &mut gpui::TestAppC ) .await; + // When inserting, ensure autocompletion is favored over Copilot suggestions. cx.set_state(indoc! {" oneˇ two three "}); - - // When inserting, ensure autocompletion is favored over Copilot suggestions. cx.simulate_keystroke("."); let _ = handle_completion_request( &mut cx, @@ -5917,8 +5916,8 @@ async fn test_copilot(deterministic: Arc, cx: &mut gpui::TestAppC handle_copilot_completion_request( &copilot_lsp, vec![copilot::request::Completion { - text: "copilot1".into(), - range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 5)), + text: "one.copilot1".into(), + range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 4)), ..Default::default() }], vec![], @@ -5940,13 +5939,45 @@ async fn test_copilot(deterministic: Arc, cx: &mut gpui::TestAppC assert_eq!(editor.display_text(cx), "one.completion_a\ntwo\nthree\n"); }); + // Ensure Copilot suggestions are shown right away if no autocompletion is available. cx.set_state(indoc! {" oneˇ two three "}); + cx.simulate_keystroke("."); + let _ = handle_completion_request( + &mut cx, + indoc! {" + one.|<> + two + three + "}, + vec![], + ); + handle_copilot_completion_request( + &copilot_lsp, + vec![copilot::request::Completion { + text: "one.copilot1".into(), + range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 4)), + ..Default::default() + }], + vec![], + ); + deterministic.advance_clock(COPILOT_DEBOUNCE_TIMEOUT); + cx.update_editor(|editor, cx| { + assert!(!editor.context_menu_visible()); + assert!(editor.has_active_copilot_suggestion(cx)); + assert_eq!(editor.display_text(cx), "one.copilot1\ntwo\nthree\n"); + assert_eq!(editor.text(cx), "one.\ntwo\nthree\n"); + }); - // When inserting, ensure autocompletion is favored over Copilot suggestions. + // Reset editor, and ensure autocompletion is still favored over Copilot suggestions. + cx.set_state(indoc! {" + oneˇ + two + three + "}); cx.simulate_keystroke("."); let _ = handle_completion_request( &mut cx, From d26d0ac56fda9afd4899524dcfb5cb72e6d8e734 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 18 Apr 2023 10:34:18 +0200 Subject: [PATCH 2/2] Clean up completion tasks, even if they fail or return no results This fixes a bug where leaving the completion task in `completion_tasks` could cause the Copilot suggestion to not be shown due to the LSP not successfully return a completion. --- crates/editor/src/editor.rs | 93 +++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 40 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6649e0a699..b3f3723bc1 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -20,7 +20,7 @@ mod editor_tests; pub mod test; use aho_corasick::AhoCorasick; -use anyhow::Result; +use anyhow::{anyhow, Result}; use blink_manager::BlinkManager; use clock::ReplicaId; use collections::{BTreeMap, Bound, HashMap, HashSet, VecDeque}; @@ -2352,53 +2352,66 @@ impl Editor { let id = post_inc(&mut self.next_completion_id); let task = cx.spawn_weak(|this, mut cx| { async move { - let completions = completions.await?; - if completions.is_empty() { - return Ok(()); - } - - let mut menu = CompletionsMenu { - id, - initial_position: position, - match_candidates: completions - .iter() - .enumerate() - .map(|(id, completion)| { - StringMatchCandidate::new( - id, - completion.label.text[completion.label.filter_range.clone()].into(), - ) - }) - .collect(), - buffer, - completions: completions.into(), - matches: Vec::new().into(), - selected_item: 0, - list: Default::default(), + let menu = if let Some(completions) = completions.await.log_err() { + let mut menu = CompletionsMenu { + id, + initial_position: position, + match_candidates: completions + .iter() + .enumerate() + .map(|(id, completion)| { + StringMatchCandidate::new( + id, + completion.label.text[completion.label.filter_range.clone()] + .into(), + ) + }) + .collect(), + buffer, + completions: completions.into(), + matches: Vec::new().into(), + selected_item: 0, + list: Default::default(), + }; + menu.filter(query.as_deref(), cx.background()).await; + if menu.matches.is_empty() { + None + } else { + Some(menu) + } + } else { + None }; - menu.filter(query.as_deref(), cx.background()).await; + let this = this + .upgrade(&cx) + .ok_or_else(|| anyhow!("editor was dropped"))?; + this.update(&mut cx, |this, cx| { + this.completion_tasks.retain(|(task_id, _)| *task_id > id); - if let Some(this) = this.upgrade(&cx) { - this.update(&mut cx, |this, cx| { - match this.context_menu.as_ref() { - None => {} - Some(ContextMenu::Completions(prev_menu)) => { - if prev_menu.id > menu.id { - return; - } + match this.context_menu.as_ref() { + None => {} + Some(ContextMenu::Completions(prev_menu)) => { + if prev_menu.id > id { + return; } - _ => return, } + _ => return, + } - this.completion_tasks.retain(|(id, _)| *id > menu.id); - if this.focused && !menu.matches.is_empty() { - this.show_context_menu(ContextMenu::Completions(menu), cx); - } else if this.hide_context_menu(cx).is_none() { + if this.focused && menu.is_some() { + let menu = menu.unwrap(); + this.show_context_menu(ContextMenu::Completions(menu), cx); + } else if this.completion_tasks.is_empty() { + // 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 suggestion when available. + if this.hide_context_menu(cx).is_none() { this.update_visible_copilot_suggestion(cx); } - }); - } + } + }); + Ok::<_, anyhow::Error>(()) } .log_err()