Skip to content

Commit

Permalink
vim: Fix :wq in multibuffer (#24603)
Browse files Browse the repository at this point in the history
Supercedes #24561
Closes #21059

Before this change we would skip saving multibuffers regardless of the
save intent. Now we correctly save them.

Along the way:
* Prompt to save when closing the last singleton copy of an item (even
if it's still open in a multibuffer).
* Update our file name prompt to pull out dirty project items from
multibuffers instead of counting multibuffers as untitled files.
* Fix our prompt test helpers to require passing the button name instead
of the index. A few tests were passing invalid responses to save
prompts.
* Refactor the code a bit to hopefully clarify it for the next bug.

Release Notes:

- Fixed edge-cases when closing multiple items including multibuffers.
Previously no prompt was generated when closing an item that was open in
a multibuffer, now you will be prompted.
- vim: Fix :wq in a multibuffer
  • Loading branch information
ConradIrwin authored Feb 13, 2025
1 parent 8c780ba commit 2f741c8
Show file tree
Hide file tree
Showing 11 changed files with 318 additions and 290 deletions.
2 changes: 1 addition & 1 deletion crates/collab/src/tests/following_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ async fn test_basic_following(
});
executor.run_until_parked();
// are you sure you want to leave the call?
cx_c.simulate_prompt_answer(0);
cx_c.simulate_prompt_answer("Close window and hang up");
cx_c.cx.update(|_| {
drop(workspace_c);
});
Expand Down
10 changes: 8 additions & 2 deletions crates/gpui/src/app/test_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,15 +286,21 @@ impl TestAppContext {
}

/// Simulates clicking a button in an platform-level alert dialog.
pub fn simulate_prompt_answer(&self, button_ix: usize) {
self.test_platform.simulate_prompt_answer(button_ix);
#[track_caller]
pub fn simulate_prompt_answer(&self, button: &str) {
self.test_platform.simulate_prompt_answer(button);
}

/// Returns true if there's an alert dialog open.
pub fn has_pending_prompt(&self) -> bool {
self.test_platform.has_pending_prompt()
}

/// Returns true if there's an alert dialog open.
pub fn pending_prompt(&self) -> Option<(String, String)> {
self.test_platform.pending_prompt()
}

/// All the urls that have been opened with cx.open_url() during this test.
pub fn opened_url(&self) -> Option<String> {
self.test_platform.opened_url.borrow().clone()
Expand Down
52 changes: 46 additions & 6 deletions crates/gpui/src/platform/test/platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,16 @@ impl ScreenCaptureSource for TestScreenCaptureSource {

impl ScreenCaptureStream for TestScreenCaptureStream {}

struct TestPrompt {
msg: String,
detail: Option<String>,
answers: Vec<String>,
tx: oneshot::Sender<usize>,
}

#[derive(Default)]
pub(crate) struct TestPrompts {
multiple_choice: VecDeque<oneshot::Sender<usize>>,
multiple_choice: VecDeque<TestPrompt>,
new_path: VecDeque<(PathBuf, oneshot::Sender<Result<Option<PathBuf>>>)>,
}

Expand Down Expand Up @@ -123,33 +130,64 @@ impl TestPlatform {
.new_path
.pop_front()
.expect("no pending new path prompt");
self.background_executor().set_waiting_hint(None);
tx.send(Ok(select_path(&path))).ok();
}

pub(crate) fn simulate_prompt_answer(&self, response_ix: usize) {
let tx = self
#[track_caller]
pub(crate) fn simulate_prompt_answer(&self, response: &str) {
let prompt = self
.prompts
.borrow_mut()
.multiple_choice
.pop_front()
.expect("no pending multiple choice prompt");
self.background_executor().set_waiting_hint(None);
tx.send(response_ix).ok();
let Some(ix) = prompt.answers.iter().position(|a| a == response) else {
panic!(
"PROMPT: {}\n{:?}\n{:?}\nCannot respond with {}",
prompt.msg, prompt.detail, prompt.answers, response
)
};
prompt.tx.send(ix).ok();
}

pub(crate) fn has_pending_prompt(&self) -> bool {
!self.prompts.borrow().multiple_choice.is_empty()
}

pub(crate) fn pending_prompt(&self) -> Option<(String, String)> {
let prompts = self.prompts.borrow();
let prompt = prompts.multiple_choice.front()?;
Some((
prompt.msg.clone(),
prompt.detail.clone().unwrap_or_default(),
))
}

pub(crate) fn set_screen_capture_sources(&self, sources: Vec<TestScreenCaptureSource>) {
*self.screen_capture_sources.borrow_mut() = sources;
}

pub(crate) fn prompt(&self, msg: &str, detail: Option<&str>) -> oneshot::Receiver<usize> {
pub(crate) fn prompt(
&self,
msg: &str,
detail: Option<&str>,
answers: &[&str],
) -> oneshot::Receiver<usize> {
let (tx, rx) = oneshot::channel();
let answers: Vec<String> = answers.iter().map(|&s| s.to_string()).collect();
self.background_executor()
.set_waiting_hint(Some(format!("PROMPT: {:?} {:?}", msg, detail)));
self.prompts.borrow_mut().multiple_choice.push_back(tx);
self.prompts
.borrow_mut()
.multiple_choice
.push_back(TestPrompt {
msg: msg.to_string(),
detail: detail.map(|s| s.to_string()),
answers: answers.clone(),
tx,
});
rx
}

Expand Down Expand Up @@ -292,6 +330,8 @@ impl Platform for TestPlatform {
directory: &std::path::Path,
) -> oneshot::Receiver<Result<Option<std::path::PathBuf>>> {
let (tx, rx) = oneshot::channel();
self.background_executor()
.set_waiting_hint(Some(format!("PROMPT FOR PATH: {:?}", directory)));
self.prompts
.borrow_mut()
.new_path
Expand Down
4 changes: 2 additions & 2 deletions crates/gpui/src/platform/test/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,15 @@ impl PlatformWindow for TestWindow {
_level: crate::PromptLevel,
msg: &str,
detail: Option<&str>,
_answers: &[&str],
answers: &[&str],
) -> Option<futures::channel::oneshot::Receiver<usize>> {
Some(
self.0
.lock()
.platform
.upgrade()
.expect("platform dropped")
.prompt(msg, detail),
.prompt(msg, detail, answers),
)
}

Expand Down
2 changes: 1 addition & 1 deletion crates/project_panel/src/project_panel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9551,7 +9551,7 @@ mod tests {
cx.has_pending_prompt(),
"Should have a prompt after the deletion"
);
cx.simulate_prompt_answer(0);
cx.simulate_prompt_answer("Delete");
assert!(
!cx.has_pending_prompt(),
"Should have no prompts after prompt was replied to"
Expand Down
3 changes: 1 addition & 2 deletions crates/recent_projects/src/recent_projects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -694,8 +694,7 @@ mod tests {
cx.has_pending_prompt(),
"Dirty workspace should prompt before opening the new recent project"
);
// Cancel
cx.simulate_prompt_answer(0);
cx.simulate_prompt_answer("Cancel");
assert!(
!cx.has_pending_prompt(),
"Should have no pending prompt after cancelling"
Expand Down
6 changes: 2 additions & 4 deletions crates/vim/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1695,12 +1695,10 @@ mod test {
// conflict!
cx.simulate_keystrokes("i @ escape");
cx.simulate_keystrokes(": w enter");
assert!(cx.has_pending_prompt());
// "Cancel"
cx.simulate_prompt_answer(0);
cx.simulate_prompt_answer("Cancel");

assert_eq!(fs.load(path).await.unwrap().replace("\r\n", "\n"), "oops\n");
assert!(!cx.has_pending_prompt());
// force overwrite
cx.simulate_keystrokes(": w ! enter");
assert!(!cx.has_pending_prompt());
assert_eq!(fs.load(path).await.unwrap().replace("\r\n", "\n"), "@@\n");
Expand Down
22 changes: 21 additions & 1 deletion crates/workspace/src/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1257,6 +1257,19 @@ pub mod test {
is_dirty: false,
})
}

pub fn new_dirty(id: u64, path: &str, cx: &mut App) -> Entity<Self> {
let entry_id = Some(ProjectEntryId::from_proto(id));
let project_path = Some(ProjectPath {
worktree_id: WorktreeId::from_usize(0),
path: Path::new(path).into(),
});
cx.new(|_| Self {
entry_id,
project_path,
is_dirty: true,
})
}
}

impl TestItem {
Expand Down Expand Up @@ -1460,10 +1473,17 @@ pub mod test {
_: bool,
_: Entity<Project>,
_window: &mut Window,
_: &mut Context<Self>,
cx: &mut Context<Self>,
) -> Task<anyhow::Result<()>> {
self.save_count += 1;
self.is_dirty = false;
for item in &self.project_items {
item.update(cx, |item, _| {
if item.is_dirty {
item.is_dirty = false;
}
})
}
Task::ready(Ok(()))
}

Expand Down
Loading

0 comments on commit 2f741c8

Please sign in to comment.