From 7a34dd98889f9863930319d622bfbb2496efe2c7 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Wed, 26 Feb 2025 15:16:17 -0500 Subject: [PATCH] Save buffers after restoring hunks in the project diff (#25620) This PR fixes a bug where using the project diff editor to restore hunks from a file that's not open in its own buffer would cause those reverts to be lost once the project diff drops its excerpts for that file. The fix is to save the buffers after restoring them but before the excerpts are (potentially) dropped. This is done for the project diff editor only. If we fail to save the affected files, we add their buffers to the active workspace, so that the reverted contents are preserved and the user can try again to save them. - [x] Get it working - [x] Test - [ ] ~~Clean up boolean soup~~ Co-authored-by: Max Release Notes: - N/A --- Cargo.lock | 1 + crates/collab/src/tests/integration_tests.rs | 8 +- crates/editor/src/editor.rs | 44 +++++++++- crates/file_finder/src/file_finder.rs | 1 + crates/git_ui/Cargo.toml | 8 ++ crates/git_ui/src/git_panel.rs | 2 +- crates/git_ui/src/project_diff.rs | 91 ++++++++++++++++++++ crates/project_panel/src/project_panel.rs | 1 + crates/workspace/src/pane.rs | 51 ++++++++--- crates/workspace/src/workspace.rs | 7 +- 10 files changed, 195 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9de1c22dfdb8cb302318c69befda3b72f0e1b53f..9ced7ff81094b68f3418b64a8fa83b44507c56e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5422,6 +5422,7 @@ dependencies = [ "theme", "time", "ui", + "unindent", "util", "windows 0.58.0", "workspace", diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index 81dcf4350de6b8feec0093aa6ddc62d2dc5eaddc..017367fb1fd76a1dfe4bed995f4727e4fbd8b25b 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/crates/collab/src/tests/integration_tests.rs @@ -6354,7 +6354,7 @@ async fn test_preview_tabs(cx: &mut TestAppContext) { // Open item 1 as preview workspace .update_in(cx, |workspace, window, cx| { - workspace.open_path_preview(path_1.clone(), None, true, true, window, cx) + workspace.open_path_preview(path_1.clone(), None, true, true, true, window, cx) }) .await .unwrap(); @@ -6375,7 +6375,7 @@ async fn test_preview_tabs(cx: &mut TestAppContext) { // Open item 2 as preview workspace .update_in(cx, |workspace, window, cx| { - workspace.open_path_preview(path_2.clone(), None, true, true, window, cx) + workspace.open_path_preview(path_2.clone(), None, true, true, true, window, cx) }) .await .unwrap(); @@ -6507,7 +6507,7 @@ async fn test_preview_tabs(cx: &mut TestAppContext) { // Open item 2 as preview in right pane workspace .update_in(cx, |workspace, window, cx| { - workspace.open_path_preview(path_2.clone(), None, true, true, window, cx) + workspace.open_path_preview(path_2.clone(), None, true, true, true, window, cx) }) .await .unwrap(); @@ -6545,7 +6545,7 @@ async fn test_preview_tabs(cx: &mut TestAppContext) { // Open item 2 as preview in left pane workspace .update_in(cx, |workspace, window, cx| { - workspace.open_path_preview(path_2.clone(), None, true, true, window, cx) + workspace.open_path_preview(path_2.clone(), None, true, true, true, window, cx) }) .await .unwrap(); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 46cae97f7d665b231251e8ad89b8983eef853030..5d6f138a656a264588fed84d11862a2b155889e5 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -7722,7 +7722,7 @@ impl Editor { drop(chunk_by); if !revert_changes.is_empty() { self.transact(window, cx, |editor, window, cx| { - editor.revert(revert_changes, window, cx); + editor.restore(revert_changes, window, cx); }); } } @@ -15886,13 +15886,16 @@ impl Editor { FILE_HEADER_HEIGHT } - pub fn revert( + pub fn restore( &mut self, revert_changes: HashMap, Rope)>>, window: &mut Window, cx: &mut Context, ) { - self.buffer().update(cx, |multi_buffer, cx| { + let workspace = self.workspace(); + let project = self.project.as_ref(); + let save_tasks = self.buffer().update(cx, |multi_buffer, cx| { + let mut tasks = Vec::new(); for (buffer_id, changes) in revert_changes { if let Some(buffer) = multi_buffer.buffer(buffer_id) { buffer.update(cx, |buffer, cx| { @@ -15904,9 +15907,44 @@ impl Editor { cx, ); }); + + if let Some(project) = + project.filter(|_| multi_buffer.all_diff_hunks_expanded()) + { + project.update(cx, |project, cx| { + tasks.push((buffer.clone(), project.save_buffer(buffer, cx))); + }) + } } } + tasks }); + cx.spawn_in(window, |_, mut cx| async move { + for (buffer, task) in save_tasks { + let result = task.await; + if result.is_err() { + let Some(path) = buffer + .read_with(&cx, |buffer, cx| buffer.project_path(cx)) + .ok() + else { + continue; + }; + if let Some((workspace, path)) = workspace.as_ref().zip(path) { + let Some(task) = cx + .update_window_entity(&workspace, |workspace, window, cx| { + workspace + .open_path_preview(path, None, false, false, false, window, cx) + }) + .ok() + else { + continue; + }; + task.await.log_err(); + } + } + } + }) + .detach(); self.change_selections(None, window, cx, |selections| selections.refresh()); } diff --git a/crates/file_finder/src/file_finder.rs b/crates/file_finder/src/file_finder.rs index 7d537e5cb7f27d1b44cd7a71654b7efc6f47c358..e1448010d247ba5ee03261f25fd592b143a05050 100644 --- a/crates/file_finder/src/file_finder.rs +++ b/crates/file_finder/src/file_finder.rs @@ -1195,6 +1195,7 @@ impl PickerDelegate for FileFinderDelegate { None, true, allow_preview, + true, window, cx, ) diff --git a/crates/git_ui/Cargo.toml b/crates/git_ui/Cargo.toml index 49ece4d9e240db750a152b45f263da73aedd7343..18f44122b4377685ca8fe05bbaa152b8dd9a6127 100644 --- a/crates/git_ui/Cargo.toml +++ b/crates/git_ui/Cargo.toml @@ -47,6 +47,14 @@ zed_actions.workspace = true [target.'cfg(windows)'.dependencies] windows.workspace = true +[dev-dependencies] +editor = { workspace = true, features = ["test-support"] } +gpui = { workspace = true, features = ["test-support"] } +project = { workspace = true, features = ["test-support"] } +settings = { workspace = true, features = ["test-support"] } +workspace = { workspace = true, features = ["test-support"] } +unindent.workspace = true + [features] default = [] test-support = ["multi_buffer/test-support"] diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index 21f35bb78dc324f3579fdbb7524fc4aa334e471f..edc91b9db85d342bc3775f3547e8fd025cec3810 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -693,7 +693,7 @@ impl GitPanel { self.workspace .update(cx, |workspace, cx| { workspace - .open_path_preview(path, None, false, false, window, cx) + .open_path_preview(path, None, false, false, true, window, cx) .detach_and_prompt_err("Failed to open file", window, cx, |e, _, _| { Some(format!("{e}")) }); diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index e9cb953651ce4bf300f837eec4077ffc7bd21c93..a5c72df4f82bc9d59431421ca595af9c69c36b3e 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/crates/git_ui/src/project_diff.rs @@ -47,6 +47,7 @@ pub struct ProjectDiff { _subscription: Subscription, } +#[derive(Debug)] struct DiffBuffer { path_key: PathKey, buffer: Entity, @@ -923,3 +924,93 @@ impl Render for ProjectDiffToolbar { ) } } + +#[cfg(test)] +mod tests { + use collections::HashMap; + use editor::test::editor_test_context::assert_state_with_diff; + use git::status::{StatusCode, TrackedStatus}; + use gpui::TestAppContext; + use project::FakeFs; + use serde_json::json; + use settings::SettingsStore; + use unindent::Unindent as _; + use util::path; + + use super::*; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let store = SettingsStore::test(cx); + cx.set_global(store); + theme::init(theme::LoadThemes::JustBase, cx); + language::init(cx); + Project::init_settings(cx); + workspace::init_settings(cx); + editor::init(cx); + crate::init(cx); + }); + } + + #[gpui::test] + async fn test_save_after_restore(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "foo": "FOO\n", + }), + ) + .await; + let project = Project::test(fs.clone(), [path!("/project").as_ref()], cx).await; + let (workspace, cx) = + cx.add_window_view(|window, cx| Workspace::test_new(project.clone(), window, cx)); + let diff = cx.new_window_entity(|window, cx| { + ProjectDiff::new(project.clone(), workspace, window, cx) + }); + cx.run_until_parked(); + + fs.set_head_for_repo( + path!("/project/.git").as_ref(), + &[("foo".into(), "foo\n".into())], + ); + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.statuses = HashMap::from_iter([( + "foo".into(), + TrackedStatus { + index_status: StatusCode::Unmodified, + worktree_status: StatusCode::Modified, + } + .into(), + )]); + }); + cx.run_until_parked(); + + let editor = diff.update(cx, |diff, _| diff.editor.clone()); + assert_state_with_diff( + &editor, + cx, + &" + - foo + + FOO + ˇ" + .unindent(), + ); + + editor.update_in(cx, |editor, window, cx| { + editor.restore_file(&Default::default(), window, cx); + }); + fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { + state.statuses = HashMap::default(); + }); + cx.run_until_parked(); + + assert_state_with_diff(&editor, cx, &"ˇ".unindent()); + + let text = String::from_utf8(fs.read_file_sync("/project/foo").unwrap()).unwrap(); + assert_eq!(text, "foo\n"); + } +} diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index de5a68ac3fe270932daa05a70cc2e7ab5bd8487f..341587ab564fa01207bd231dd3e7dfedcc8bd133 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -485,6 +485,7 @@ impl ProjectPanel { None, focus_opened_item, allow_preview, + true, window, cx, ) .detach_and_prompt_err("Failed to open file", window, cx, move |e, _, _| { diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index 18069c4a96b3b360eca0203cb45c58b4a59cf8d9..5ee62fdecfa36aa59f76865b46dc2d24be55f73e 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -849,6 +849,7 @@ impl Pane { project_entry_id: Option, focus_item: bool, allow_preview: bool, + activate: bool, suggested_position: Option, window: &mut Window, cx: &mut Context, @@ -876,7 +877,9 @@ impl Pane { } } } - self.activate_item(index, focus_item, focus_item, window, cx); + if activate { + self.activate_item(index, focus_item, focus_item, window, cx); + } existing_item } else { // If the item is being opened as preview and we have an existing preview tab, @@ -892,10 +895,11 @@ impl Pane { if allow_preview { self.set_preview_item_id(Some(new_item.item_id()), cx); } - self.add_item( + self.add_item_inner( new_item.clone(), true, focus_item, + activate, destination_index, window, cx, @@ -924,11 +928,13 @@ impl Pane { } } - pub fn add_item( + #[allow(clippy::too_many_arguments)] + pub fn add_item_inner( &mut self, item: Box, activate_pane: bool, focus_item: bool, + activate: bool, destination_index: Option, window: &mut Window, cx: &mut Context, @@ -1015,23 +1021,47 @@ impl Pane { cx.notify(); } - self.activate_item(insertion_index, activate_pane, focus_item, window, cx); + if activate { + self.activate_item(insertion_index, activate_pane, focus_item, window, cx); + } } else { self.items.insert(insertion_index, item.clone()); - if insertion_index <= self.active_item_index - && self.preview_item_idx() != Some(self.active_item_index) - { - self.active_item_index += 1; - } + if activate { + if insertion_index <= self.active_item_index + && self.preview_item_idx() != Some(self.active_item_index) + { + self.active_item_index += 1; + } - self.activate_item(insertion_index, activate_pane, focus_item, window, cx); + self.activate_item(insertion_index, activate_pane, focus_item, window, cx); + } cx.notify(); } cx.emit(Event::AddItem { item }); } + pub fn add_item( + &mut self, + item: Box, + activate_pane: bool, + focus_item: bool, + destination_index: Option, + window: &mut Window, + cx: &mut Context, + ) { + self.add_item_inner( + item, + activate_pane, + focus_item, + true, + destination_index, + window, + cx, + ) + } + pub fn items_len(&self) -> usize { self.items.len() } @@ -2941,6 +2971,7 @@ impl Pane { project_entry_id, true, false, + true, target, window, cx, diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 126e1306077b2b650722e3073cb5d2dfd09f6316..f28561a002a1d6f77b3a1c4720d2a48cd02f88bc 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -1499,6 +1499,7 @@ impl Workspace { project_entry_id, true, entry.is_preview, + true, None, window, cx, build_item, @@ -2801,15 +2802,17 @@ impl Workspace { window: &mut Window, cx: &mut App, ) -> Task, anyhow::Error>> { - self.open_path_preview(path, pane, focus_item, false, window, cx) + self.open_path_preview(path, pane, focus_item, false, true, window, cx) } + #[allow(clippy::too_many_arguments)] pub fn open_path_preview( &mut self, path: impl Into, pane: Option>, focus_item: bool, allow_preview: bool, + activate: bool, window: &mut Window, cx: &mut App, ) -> Task, anyhow::Error>> { @@ -2830,6 +2833,7 @@ impl Workspace { project_entry_id, focus_item, allow_preview, + activate, None, window, cx, @@ -2888,6 +2892,7 @@ impl Workspace { project_entry_id, true, allow_preview, + true, None, window, cx,