Skip to content

Commit

Permalink
Fix file missing or duplicated when copying multiple items in project…
Browse files Browse the repository at this point in the history
… panel + Fix marked files not being deselected after selecting a directory (zed-industries#20859)

Closes zed-industries#20858

This fix depends on the sanitization logic implemented in PR zed-industries#20577.
Since that branch may undergo further changes, this branch will be
periodically rebased on it. Once zed-industries#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.

<img height="120"
src="https://github.com/user-attachments/assets/d4d92cc5-c998-4948-9a58-25c4f54167f2"
/>

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](zed-industries#17746 (comment)).
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`.

<img height="170"
src="https://github.com/user-attachments/assets/a09fcb40-f38f-46ef-b0b4-e44ec01dda18"
/>
<img height="170"
src="https://github.com/user-attachments/assets/bb87dbe5-8e2e-4ca4-a565-42be5755ec8a"
/>

---
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`.

<img height="170"
src="https://github.com/user-attachments/assets/6f999d22-3607-48d7-9ff6-2e27494002f8"
/>
<img height="170"
src="https://github.com/user-attachments/assets/b4b6ff7d-0df7-45ea-83e4-50a0acb18457"
/>

---
3. Before/After: Directory Selection

Simply primary click on any file, now primary click on any dir. That
previous file is still marked.

<img height="170"
src="https://github.com/user-attachments/assets/9f1948ce-7445-4377-9733-06490ed6a324"
/>
<img height="170"
src="https://github.com/user-attachments/assets/e78203bc-96ba-424b-b588-c038992a9f0e"
/>

---
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.

<img height="170"
src="https://github.com/user-attachments/assets/e20423ea-1682-4efd-b208-631e2edd3771"
/>
  • Loading branch information
0xtimsb authored Nov 26, 2024
1 parent 57e4540 commit d75d345
Showing 1 changed file with 181 additions and 22 deletions.
203 changes: 181 additions & 22 deletions crates/project_panel/src/project_panel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -1546,15 +1546,15 @@ impl ProjectPanel {
}

fn cut(&mut self, _: &Cut, cx: &mut ViewContext<Self>) {
let entries = self.marked_entries();
let entries = self.disjoint_entries(cx);
if !entries.is_empty() {
self.clipboard = Some(ClipboardEntry::Cut(entries));
cx.notify();
}
}

fn copy(&mut self, _: &Copy, cx: &mut ViewContext<Self>) {
let entries = self.marked_entries();
let entries = self.disjoint_entries(cx);
if !entries.is_empty() {
self.clipboard = Some(ClipboardEntry::Copied(entries));
cx.notify();
Expand Down Expand Up @@ -1928,7 +1928,7 @@ impl ProjectPanel {
None
}

fn disjoint_entries_for_removal(&self, cx: &AppContext) -> BTreeSet<SelectedEntry> {
fn disjoint_entries(&self, cx: &AppContext) -> BTreeSet<SelectedEntry> {
let marked_entries = self.marked_entries();
let mut sanitized_entries = BTreeSet::new();
if marked_entries.is_empty() {
Expand Down Expand Up @@ -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<SelectedEntry> {
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::<BTreeSet<_>>();

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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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),
),
);
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit d75d345

Please sign in to comment.