Skip to content

Commit

Permalink
Revert "Always wait for completion resolve before applying the comple…
Browse files Browse the repository at this point in the history
…tion edits (#18907)"

This reverts commit a62a2fa.
  • Loading branch information
SomeoneToIgnore committed Oct 10, 2024
1 parent f6f5ad1 commit f4956cc
Show file tree
Hide file tree
Showing 8 changed files with 62 additions and 143 deletions.
84 changes: 27 additions & 57 deletions crates/collab/src/tests/editor_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,75 +379,51 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
.next()
.await
.unwrap();
cx_a.executor().finish_waiting();

// Open the buffer on the host.
let buffer_a = project_a
.update(cx_a, |p, cx| p.open_buffer((worktree_id, "main.rs"), cx))
.await
.unwrap();
cx_a.executor().run_until_parked();

buffer_a.read_with(cx_a, |buffer, _| {
assert_eq!(buffer.text(), "fn main() { a. }")
});

// Confirm a completion on the guest.
editor_b.update(cx_b, |editor, cx| {
assert!(editor.context_menu_visible());
editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx);
assert_eq!(editor.text(cx), "fn main() { a.first_method() }");
});

// Return a resolved completion from the host's language server.
// The resolved completion has an additional text edit.
fake_language_server.handle_request::<lsp::request::ResolveCompletionItem, _, _>(
|params, _| async move {
Ok(match params.label.as_str() {
"first_method(…)" => lsp::CompletionItem {
label: "first_method(…)".into(),
detail: Some("fn(&mut self, B) -> C".into()),
text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
new_text: "first_method($1)".to_string(),
range: lsp::Range::new(
lsp::Position::new(0, 14),
lsp::Position::new(0, 14),
),
})),
additional_text_edits: Some(vec![lsp::TextEdit {
new_text: "use d::SomeTrait;\n".to_string(),
range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)),
}]),
insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
..Default::default()
},
"second_method(…)" => lsp::CompletionItem {
label: "second_method(…)".into(),
detail: Some("fn(&mut self, C) -> D<E>".into()),
text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
new_text: "second_method()".to_string(),
range: lsp::Range::new(
lsp::Position::new(0, 14),
lsp::Position::new(0, 14),
),
})),
insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
additional_text_edits: Some(vec![lsp::TextEdit {
new_text: "use d::SomeTrait;\n".to_string(),
range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)),
}]),
..Default::default()
},
_ => panic!("unexpected completion label: {:?}", params.label),
assert_eq!(params.label, "first_method(…)");
Ok(lsp::CompletionItem {
label: "first_method(…)".into(),
detail: Some("fn(&mut self, B) -> C".into()),
text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
new_text: "first_method($1)".to_string(),
range: lsp::Range::new(lsp::Position::new(0, 14), lsp::Position::new(0, 14)),
})),
additional_text_edits: Some(vec![lsp::TextEdit {
new_text: "use d::SomeTrait;\n".to_string(),
range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)),
}]),
insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
..Default::default()
})
},
);
cx_a.executor().finish_waiting();
cx_a.executor().run_until_parked();

// Confirm a completion on the guest.
editor_b
.update(cx_b, |editor, cx| {
assert!(editor.context_menu_visible());
editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx)
})
.unwrap()
.await
.unwrap();
// The additional edit is applied.
cx_a.executor().run_until_parked();
cx_b.executor().run_until_parked();

// The additional edit is applied.
buffer_a.read_with(cx_a, |buffer, _| {
assert_eq!(
buffer.text(),
Expand Down Expand Up @@ -540,15 +516,9 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu
cx_b.executor().run_until_parked();

// When accepting the completion, the snippet is insert.
editor_b
.update(cx_b, |editor, cx| {
assert!(editor.context_menu_visible());
editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx)
})
.unwrap()
.await
.unwrap();
editor_b.update(cx_b, |editor, cx| {
assert!(editor.context_menu_visible());
editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx);
assert_eq!(
editor.text(cx),
"use d::SomeTrait;\nfn main() { a.first_method(); a.third_method(, , ) }"
Expand Down
10 changes: 4 additions & 6 deletions crates/copilot/src/copilot_completion_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,12 +363,10 @@ mod tests {

// Confirming a completion inserts it and hides the context menu, without showing
// the copilot suggestion afterwards.
editor.confirm_completion(&Default::default(), cx).unwrap()
})
.await
.unwrap();

cx.update_editor(|editor, cx| {
editor
.confirm_completion(&Default::default(), cx)
.unwrap()
.detach();
assert!(!editor.context_menu_visible());
assert!(!editor.has_active_inline_completion(cx));
assert_eq!(editor.text(cx), "one.completion_a\ntwo\nthree\n");
Expand Down
22 changes: 5 additions & 17 deletions crates/editor/src/debounced_delay.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{ops::ControlFlow, time::Duration};
use std::time::Duration;

use futures::{channel::oneshot, FutureExt};
use gpui::{Task, ViewContext};
Expand All @@ -7,7 +7,7 @@ use crate::Editor;

pub struct DebouncedDelay {
task: Option<Task<()>>,
cancel_channel: Option<oneshot::Sender<ControlFlow<()>>>,
cancel_channel: Option<oneshot::Sender<()>>,
}

impl DebouncedDelay {
Expand All @@ -23,22 +23,17 @@ impl DebouncedDelay {
F: 'static + Send + FnOnce(&mut Editor, &mut ViewContext<Editor>) -> Task<()>,
{
if let Some(channel) = self.cancel_channel.take() {
channel.send(ControlFlow::Break(())).ok();
_ = channel.send(());
}

let (sender, mut receiver) = oneshot::channel::<ControlFlow<()>>();
let (sender, mut receiver) = oneshot::channel::<()>();
self.cancel_channel = Some(sender);

drop(self.task.take());
self.task = Some(cx.spawn(move |model, mut cx| async move {
let mut timer = cx.background_executor().timer(delay).fuse();
futures::select_biased! {
interrupt = receiver => {
match interrupt {
Ok(ControlFlow::Break(())) | Err(_) => return,
Ok(ControlFlow::Continue(())) => {},
}
}
_ = receiver => return,
_ = timer => {}
}

Expand All @@ -47,11 +42,4 @@ impl DebouncedDelay {
}
}));
}

pub fn start_now(&mut self) -> Option<Task<()>> {
if let Some(channel) = self.cancel_channel.take() {
channel.send(ControlFlow::Continue(())).ok();
}
self.task.take()
}
}
47 changes: 9 additions & 38 deletions crates/editor/src/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4459,49 +4459,16 @@ impl Editor {
&mut self,
item_ix: Option<usize>,
intent: CompletionIntent,
cx: &mut ViewContext<Self>,
) -> Option<Task<anyhow::Result<()>>> {
cx: &mut ViewContext<Editor>,
) -> Option<Task<std::result::Result<(), anyhow::Error>>> {
use language::ToOffset as _;

let completions_menu = if let ContextMenu::Completions(menu) = self.hide_context_menu(cx)? {
menu
} else {
return None;
};

let mut resolve_task_store = completions_menu
.selected_completion_documentation_resolve_debounce
.lock();
let selected_completion_resolve = resolve_task_store.start_now();
let menu_pre_resolve = self
.completion_documentation_pre_resolve_debounce
.start_now();
drop(resolve_task_store);

Some(cx.spawn(|editor, mut cx| async move {
match (selected_completion_resolve, menu_pre_resolve) {
(None, None) => {}
(Some(resolve), None) | (None, Some(resolve)) => resolve.await,
(Some(resolve_1), Some(resolve_2)) => {
futures::join!(resolve_1, resolve_2);
}
}
if let Some(apply_edits_task) = editor.update(&mut cx, |editor, cx| {
editor.apply_resolved_completion(completions_menu, item_ix, intent, cx)
})? {
apply_edits_task.await?;
}
Ok(())
}))
}

fn apply_resolved_completion(
&mut self,
completions_menu: CompletionsMenu,
item_ix: Option<usize>,
intent: CompletionIntent,
cx: &mut ViewContext<'_, Editor>,
) -> Option<Task<anyhow::Result<Option<language::Transaction>>>> {
use language::ToOffset as _;

let mat = completions_menu
.matches
.get(item_ix.unwrap_or(completions_menu.selected_item))?;
Expand Down Expand Up @@ -4660,7 +4627,11 @@ impl Editor {
// so we should automatically call signature_help
self.show_signature_help(&ShowSignatureHelp, cx);
}
Some(apply_edits)

Some(cx.foreground_executor().spawn(async move {
apply_edits.await?;
Ok(())
}))
}

pub fn toggle_code_actions(&mut self, action: &ToggleCodeActions, cx: &mut ViewContext<Self>) {
Expand Down
18 changes: 8 additions & 10 deletions crates/editor/src/editor_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7996,7 +7996,7 @@ async fn test_completion(cx: &mut gpui::TestAppContext) {
.unwrap()
});
cx.assert_editor_state(indoc! {"
one.ˇ
one.second_completionˇ
two
three
"});
Expand Down Expand Up @@ -8029,9 +8029,9 @@ async fn test_completion(cx: &mut gpui::TestAppContext) {
cx.assert_editor_state(indoc! {"
one.second_completionˇ
two
thoverlapping additional editree
additional edit"});
three
additional edit
"});

cx.set_state(indoc! {"
one.second_completion
Expand Down Expand Up @@ -8091,8 +8091,8 @@ async fn test_completion(cx: &mut gpui::TestAppContext) {
});
cx.assert_editor_state(indoc! {"
one.second_completion
two siˇ
three siˇ
two sixth_completionˇ
three sixth_completionˇ
additional edit
"});

Expand Down Expand Up @@ -8133,11 +8133,9 @@ async fn test_completion(cx: &mut gpui::TestAppContext) {
.confirm_completion(&ConfirmCompletion::default(), cx)
.unwrap()
});
cx.assert_editor_state("editor.cloˇ");
cx.assert_editor_state("editor.closeˇ");
handle_resolve_completion_request(&mut cx, None).await;
apply_additional_edits.await.unwrap();
cx.assert_editor_state(indoc! {"
editor.closeˇ"});
}

#[gpui::test]
Expand Down Expand Up @@ -10142,7 +10140,7 @@ async fn test_completions_with_additional_edits(cx: &mut gpui::TestAppContext) {
.confirm_completion(&ConfirmCompletion::default(), cx)
.unwrap()
});
cx.assert_editor_state(indoc! {"fn main() { let a = 2.ˇ; }"});
cx.assert_editor_state(indoc! {"fn main() { let a = 2.Some(2)ˇ; }"});

cx.handle_request::<lsp::request::ResolveCompletionItem, _, _>(move |_, _, _| {
let task_completion_item = completion_item.clone();
Expand Down
17 changes: 7 additions & 10 deletions crates/editor/src/hover_popover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ mod tests {
hover_provider: Some(lsp::HoverProviderCapability::Simple(true)),
completion_provider: Some(lsp::CompletionOptions {
trigger_characters: Some(vec![".".to_string(), ":".to_string()]),
resolve_provider: Some(false),
resolve_provider: Some(true),
..Default::default()
}),
..Default::default()
Expand Down Expand Up @@ -902,15 +902,12 @@ mod tests {
assert_eq!(counter.load(atomic::Ordering::Acquire), 1);

//apply a completion and check it was successfully applied
let () = cx
.update_editor(|editor, cx| {
editor.context_menu_next(&Default::default(), cx);
editor
.confirm_completion(&ConfirmCompletion::default(), cx)
.unwrap()
})
.await
.unwrap();
let _apply_additional_edits = cx.update_editor(|editor, cx| {
editor.context_menu_next(&Default::default(), cx);
editor
.confirm_completion(&ConfirmCompletion::default(), cx)
.unwrap()
});
cx.assert_editor_state(indoc! {"
one.second_completionˇ
two
Expand Down
1 change: 0 additions & 1 deletion crates/editor/src/test/editor_test_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use itertools::Itertools;
use language::{Buffer, BufferSnapshot, LanguageRegistry};
use multi_buffer::{ExcerptRange, ToPoint};
use parking_lot::RwLock;
use pretty_assertions::assert_eq;
use project::{FakeFs, Project};
use std::{
any::TypeId,
Expand Down
6 changes: 2 additions & 4 deletions crates/vim/src/normal/repeat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ mod test {
lsp::ServerCapabilities {
completion_provider: Some(lsp::CompletionOptions {
trigger_characters: Some(vec![".".to_string(), ":".to_string()]),
resolve_provider: Some(false),
resolve_provider: Some(true),
..Default::default()
}),
..Default::default()
Expand Down Expand Up @@ -432,9 +432,7 @@ mod test {
request.next().await;
cx.condition(|editor, _| editor.context_menu_visible())
.await;
cx.simulate_keystrokes("down enter");
cx.run_until_parked();
cx.simulate_keystrokes("! escape");
cx.simulate_keystrokes("down enter ! escape");

cx.assert_state(
indoc! {"
Expand Down

0 comments on commit f4956cc

Please sign in to comment.