From d75d34576a5ed80142666dddc68cbbc2652aeb61 Mon Sep 17 00:00:00 2001 From: tims <0xtimsb@gmail.com> Date: Wed, 27 Nov 2024 04:53:01 +0530 Subject: [PATCH] Fix file missing or duplicated when copying multiple items in project panel + Fix marked files not being deselected after selecting a directory (#20859) Closes #20858 This fix depends on the sanitization logic implemented in PR #20577. Since that branch may undergo further changes, this branch will be periodically rebased on it. Once #20577 is merged, the dependency will no longer apply. Release Notes: - Fix missing or duplicated files when copying multiple items in the project panel. - Fix marked files not being deselected after selecting a directory on primary click. - Fix "copy path" and "copy path relative" with multiple items selected in project panel. **Problem**: In this case, `dir1` is selected while `dir2`, `dir3`, and `dir1/file` are marked. Using the `marked_entries` function results in only `dir1`, which is incorrect. Currently, the `marked_entries` function is used in five actions, which all produce incorrect results: 1. Delete (via the disjoint function) 2. Copy 3. Cut 4. Copy Path 5. Copy Path Relative **Solution**: 1. `marked_entries` function should not use "When currently selected entry is not marked, it's treated as the only marked entry." logic. There is no grand scheme behind this logic as confirmed by piotr [here](https://github.com/zed-industries/zed/issues/17746#issuecomment-2464765963). 2. `copy` and `cut` actions should use the disjoint function to prevent obivous failures. 3. `copy path` and `copy path relative` should keep using *fixed* `marked_entries` as that is expected behavior for these actions. --- 1. Before/After: Partial Copy Select `dir1` and `c.txt` (in that order, reverse order works!), and copy it and paste in `dir2`. `c.txt` is not copied in `dir2`. --- 2. Before/After: Duplicate Copy Select `a.txt`, `dir1` and `c.txt` (in that order), and copy it and paste in `dir2`. `a.txt` is duplicated in `dir2`. --- 3. Before/After: Directory Selection Simply primary click on any file, now primary click on any dir. That previous file is still marked. --- 4. Before/After: Copy Path and Copy Path Relative Upon `copy path` (ctrl + alt + c): Before: Only `/home/tims/test/dir2/a.txt` was copied. After: All three paths `/home/tims/test/dir2`, `/home/tims/test/c.txt` and `/home/tims/test/dir2/a.txt` are copied. This is also how VSCode also copies path when multiple are selected. --- crates/project_panel/src/project_panel.rs | 203 +++++++++++++++++++--- 1 file changed, 181 insertions(+), 22 deletions(-) diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index c7579247276cb..9803742966172 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -1185,7 +1185,7 @@ impl ProjectPanel { fn remove(&mut self, trash: bool, skip_prompt: bool, cx: &mut ViewContext<'_, ProjectPanel>) { maybe!({ - let items_to_delete = self.disjoint_entries_for_removal(cx); + let items_to_delete = self.disjoint_entries(cx); if items_to_delete.is_empty() { return None; } @@ -1546,7 +1546,7 @@ impl ProjectPanel { } fn cut(&mut self, _: &Cut, cx: &mut ViewContext) { - let entries = self.marked_entries(); + let entries = self.disjoint_entries(cx); if !entries.is_empty() { self.clipboard = Some(ClipboardEntry::Cut(entries)); cx.notify(); @@ -1554,7 +1554,7 @@ impl ProjectPanel { } fn copy(&mut self, _: &Copy, cx: &mut ViewContext) { - let entries = self.marked_entries(); + let entries = self.disjoint_entries(cx); if !entries.is_empty() { self.clipboard = Some(ClipboardEntry::Copied(entries)); cx.notify(); @@ -1928,7 +1928,7 @@ impl ProjectPanel { None } - fn disjoint_entries_for_removal(&self, cx: &AppContext) -> BTreeSet { + fn disjoint_entries(&self, cx: &AppContext) -> BTreeSet { let marked_entries = self.marked_entries(); let mut sanitized_entries = BTreeSet::new(); if marked_entries.is_empty() { @@ -1976,25 +1976,25 @@ impl ProjectPanel { sanitized_entries } - // Returns list of entries that should be affected by an operation. - // When currently selected entry is not marked, it's treated as the only marked entry. + // Returns the union of the currently selected entry and all marked entries. fn marked_entries(&self) -> BTreeSet { - let Some(mut selection) = self.selection else { - return Default::default(); - }; - if self.marked_entries.contains(&selection) { - self.marked_entries - .iter() - .copied() - .map(|mut entry| { - entry.entry_id = self.resolve_entry(entry.entry_id); - entry - }) - .collect() - } else { - selection.entry_id = self.resolve_entry(selection.entry_id); - BTreeSet::from_iter([selection]) + let mut entries = self + .marked_entries + .iter() + .map(|entry| SelectedEntry { + entry_id: self.resolve_entry(entry.entry_id), + worktree_id: entry.worktree_id, + }) + .collect::>(); + + if let Some(selection) = self.selection { + entries.insert(SelectedEntry { + entry_id: self.resolve_entry(selection.entry_id), + worktree_id: selection.worktree_id, + }); } + + entries } /// Finds the currently selected subentry for a given leaf entry id. If a given entry @@ -2915,6 +2915,7 @@ impl ProjectPanel { this.marked_entries.remove(&selection); } } else if kind.is_dir() { + this.marked_entries.clear(); this.toggle_expanded(entry_id, cx); } else { let preview_tabs_enabled = PreviewTabsSettings::get_global(cx).enabled; @@ -3051,7 +3052,8 @@ impl ProjectPanel { .single_line() .color(filename_text_color) .when( - is_active && index == active_index, + index == active_index + && (is_active || is_marked), |this| this.underline(true), ), ); @@ -5177,6 +5179,163 @@ mod tests { ); } + #[gpui::test] + async fn test_copy_paste_directory_with_sibling_file(cx: &mut gpui::TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor().clone()); + fs.insert_tree( + "/test", + json!({ + "dir1": { + "a.txt": "", + "b.txt": "", + }, + "dir2": {}, + "c.txt": "", + "d.txt": "", + }), + ) + .await; + + let project = Project::test(fs.clone(), ["/test".as_ref()], cx).await; + let workspace = cx.add_window(|cx| Workspace::test_new(project.clone(), cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + let panel = workspace.update(cx, ProjectPanel::new).unwrap(); + + toggle_expand_dir(&panel, "test/dir1", cx); + + cx.simulate_modifiers_change(gpui::Modifiers { + control: true, + ..Default::default() + }); + + select_path_with_mark(&panel, "test/dir1", cx); + select_path_with_mark(&panel, "test/c.txt", cx); + + assert_eq!( + visible_entries_as_strings(&panel, 0..15, cx), + &[ + "v test", + " v dir1 <== marked", + " a.txt", + " b.txt", + " > dir2", + " c.txt <== selected <== marked", + " d.txt", + ], + "Initial state before copying dir1 and c.txt" + ); + + panel.update(cx, |panel, cx| { + panel.copy(&Default::default(), cx); + }); + select_path(&panel, "test/dir2", cx); + panel.update(cx, |panel, cx| { + panel.paste(&Default::default(), cx); + }); + cx.executor().run_until_parked(); + + toggle_expand_dir(&panel, "test/dir2/dir1", cx); + + assert_eq!( + visible_entries_as_strings(&panel, 0..15, cx), + &[ + "v test", + " v dir1 <== marked", + " a.txt", + " b.txt", + " v dir2", + " v dir1 <== selected", + " a.txt", + " b.txt", + " c.txt", + " c.txt <== marked", + " d.txt", + ], + "Should copy dir1 as well as c.txt into dir2" + ); + } + + #[gpui::test] + async fn test_copy_paste_nested_and_root_entries(cx: &mut gpui::TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor().clone()); + fs.insert_tree( + "/test", + json!({ + "dir1": { + "a.txt": "", + "b.txt": "", + }, + "dir2": {}, + "c.txt": "", + "d.txt": "", + }), + ) + .await; + + let project = Project::test(fs.clone(), ["/test".as_ref()], cx).await; + let workspace = cx.add_window(|cx| Workspace::test_new(project.clone(), cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + let panel = workspace.update(cx, ProjectPanel::new).unwrap(); + + toggle_expand_dir(&panel, "test/dir1", cx); + + cx.simulate_modifiers_change(gpui::Modifiers { + control: true, + ..Default::default() + }); + + select_path_with_mark(&panel, "test/dir1/a.txt", cx); + select_path_with_mark(&panel, "test/dir1", cx); + select_path_with_mark(&panel, "test/c.txt", cx); + + assert_eq!( + visible_entries_as_strings(&panel, 0..15, cx), + &[ + "v test", + " v dir1 <== marked", + " a.txt <== marked", + " b.txt", + " > dir2", + " c.txt <== selected <== marked", + " d.txt", + ], + "Initial state before copying a.txt, dir1 and c.txt" + ); + + panel.update(cx, |panel, cx| { + panel.copy(&Default::default(), cx); + }); + select_path(&panel, "test/dir2", cx); + panel.update(cx, |panel, cx| { + panel.paste(&Default::default(), cx); + }); + cx.executor().run_until_parked(); + + toggle_expand_dir(&panel, "test/dir2/dir1", cx); + + assert_eq!( + visible_entries_as_strings(&panel, 0..20, cx), + &[ + "v test", + " v dir1 <== marked", + " a.txt <== marked", + " b.txt", + " v dir2", + " v dir1 <== selected", + " a.txt", + " b.txt", + " c.txt", + " c.txt <== marked", + " d.txt", + ], + "Should copy dir1 and c.txt into dir2. a.txt is already present in copied dir1." + ); + } + #[gpui::test] async fn test_remove_opened_file(cx: &mut gpui::TestAppContext) { init_test_with_editor(cx);