From 0388bc8c74992b3027e026bc25b3f8f416b506b4 Mon Sep 17 00:00:00 2001 From: eth0net Date: Tue, 5 May 2026 15:07:49 +0100 Subject: [PATCH] Tighten worktree root drag-and-drop semantics - Reorder marked worktree roots together as a group, preserving their relative order; direction follows the active source's position. - Reject worktree-root drops onto nested entries and suppress the drag-over highlight for invalid targets, so the operation matches the visual feedback. - Filter worktree roots from copy-mode drags so a mixed selection still copies its non-root entries instead of being silently cancelled by the `?` in `create_paste_path`. - Replace the per-entry root-routing in drag_onto's move loop with a partition + batch reorder, removing the now-dead `move_entry` and `move_worktree_root` wrappers. - Add regression tests for multi-root group reorder, nested-target rejection, copy-mode mixed selections, and mixed root+file drags with a non-root active selection. --- crates/project/src/project.rs | 14 + crates/project/src/worktree_store.rs | 92 +++-- crates/project_panel/src/project_panel.rs | 103 +++-- .../project_panel/src/project_panel_tests.rs | 373 ++++++++++++++++++ 4 files changed, 529 insertions(+), 53 deletions(-) diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 4e74c4cf1fc..e4c379b4796 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -4579,6 +4579,20 @@ impl Project { }) } + /// Moves multiple worktrees as a group to `destination`'s position, + /// preserving their relative order. + pub fn move_worktrees( + &mut self, + sources: &[WorktreeId], + destination: WorktreeId, + active_source: Option, + cx: &mut Context, + ) -> Result<()> { + self.worktree_store.update(cx, |worktree_store, cx| { + worktree_store.move_worktrees(sources, destination, active_source, cx) + }) + } + /// Attempts to convert the input path to a WSL path if this is a wsl remote project and the input path is a host windows path. pub fn try_windows_path_to_wsl( &self, diff --git a/crates/project/src/worktree_store.rs b/crates/project/src/worktree_store.rs index c6abb6e1743..aa1070d56e6 100644 --- a/crates/project/src/worktree_store.rs +++ b/crates/project/src/worktree_store.rs @@ -1038,40 +1038,80 @@ impl WorktreeStore { destination: WorktreeId, cx: &mut Context, ) -> Result<()> { - if source == destination { + self.move_worktrees(&[source], destination, Some(source), cx) + } + + /// Moves multiple worktrees as a group to `destination`'s position, + /// preserving their relative order. The `active_source` (if provided and + /// in `sources`) decides whether the group lands before or after the + /// destination, mirroring the single-source semantics: a source originally + /// before destination ends up after it, and vice versa. When no active + /// source is supplied, the first source's position is used. + pub fn move_worktrees( + &mut self, + sources: &[WorktreeId], + destination: WorktreeId, + active_source: Option, + cx: &mut Context, + ) -> Result<()> { + if sources.is_empty() { return Ok(()); } - let mut source_index = None; - let mut destination_index = None; - for (i, worktree) in self.worktrees.iter().enumerate() { - if let Some(worktree) = worktree.upgrade() { - let worktree_id = worktree.read(cx).id(); - if worktree_id == source { - source_index = Some(i); - if destination_index.is_some() { - break; - } - } else if worktree_id == destination { - destination_index = Some(i); - if source_index.is_some() { - break; - } - } - } - } + let destination_index = self + .worktrees + .iter() + .position(|wt| { + wt.upgrade() + .is_some_and(|wt| wt.read(cx).id() == destination) + }) + .with_context(|| format!("Missing worktree for id {destination}"))?; - let source_index = - source_index.with_context(|| format!("Missing worktree for id {source}"))?; - let destination_index = - destination_index.with_context(|| format!("Missing worktree for id {destination}"))?; + let source_indices: Vec = self + .worktrees + .iter() + .enumerate() + .filter_map(|(i, wt)| { + let id = wt.upgrade()?.read(cx).id(); + (sources.contains(&id) && id != destination).then_some(i) + }) + .collect(); - if source_index == destination_index { + if source_indices.is_empty() { return Ok(()); } - let worktree_to_move = self.worktrees.remove(source_index); - self.worktrees.insert(destination_index, worktree_to_move); + let direction_index = active_source + .filter(|id| sources.contains(id)) + .and_then(|id| { + self.worktrees + .iter() + .position(|wt| wt.upgrade().is_some_and(|wt| wt.read(cx).id() == id)) + }) + .unwrap_or(source_indices[0]); + let insert_after_destination = direction_index < destination_index; + + let mut to_insert = Vec::with_capacity(source_indices.len()); + for &i in source_indices.iter().rev() { + to_insert.push(self.worktrees.remove(i)); + } + to_insert.reverse(); + + let removed_before_destination = source_indices + .iter() + .filter(|&&i| i < destination_index) + .count(); + let new_destination_index = destination_index - removed_before_destination; + let insert_at = if insert_after_destination { + new_destination_index + 1 + } else { + new_destination_index + }; + + for (offset, handle) in to_insert.into_iter().enumerate() { + self.worktrees.insert(insert_at + offset, handle); + } + cx.emit(WorktreeStoreEvent::WorktreeOrderChanged); cx.notify(); Ok(()) diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index 5266a3ea5e8..48de9f50755 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -3588,44 +3588,41 @@ impl ProjectPanel { } } - fn move_entry( + fn reorder_worktree_roots( &mut self, - entry_to_move: ProjectEntryId, - destination: ProjectEntryId, - destination_is_file: bool, - cx: &mut Context, - ) -> Option>> { - if self - .project - .read(cx) - .entry_is_worktree_root(entry_to_move, cx) - { - self.move_worktree_root(entry_to_move, destination, cx); - None - } else { - self.move_worktree_entry(entry_to_move, destination, destination_is_file, cx) - } - } - - fn move_worktree_root( - &mut self, - entry_to_move: ProjectEntryId, + source_entries: &[ProjectEntryId], destination: ProjectEntryId, + active_entry_id: ProjectEntryId, cx: &mut Context, ) { self.project.update(cx, |project, cx| { - let Some(worktree_to_move) = project.worktree_for_entry(entry_to_move, cx) else { + // Reorder only fires on explicit drops onto a worktree root. + if !project.entry_is_worktree_root(destination, cx) { return; - }; + } let Some(destination_worktree) = project.worktree_for_entry(destination, cx) else { return; }; - - let worktree_id = worktree_to_move.read(cx).id(); let destination_id = destination_worktree.read(cx).id(); + let source_ids: Vec = source_entries + .iter() + .filter_map(|entry_id| { + project + .worktree_for_entry(*entry_id, cx) + .map(|wt| wt.read(cx).id()) + }) + .collect(); + if source_ids.is_empty() { + return; + } + + let active_source = project + .worktree_for_entry(active_entry_id, cx) + .map(|wt| wt.read(cx).id()); + project - .move_worktree(worktree_id, destination_id, cx) + .move_worktrees(&source_ids, destination_id, active_source, cx) .log_err(); }); } @@ -4459,6 +4456,18 @@ impl ProjectPanel { let entries = self.disjoint_entries(resolved_selections, cx); if Self::is_copy_modifier_set(&window.modifiers()) { + // Worktree roots can't be copied — leaving them in would make + // `create_paste_path` return None and `?` would abort the whole copy. + let entries: BTreeSet = { + let project = self.project.read(cx); + entries + .into_iter() + .filter(|entry| !project.entry_is_worktree_root(entry.entry_id, cx)) + .collect() + }; + if entries.is_empty() { + return; + } let _ = maybe!({ let project = self.project.read(cx); let target_worktree = project.worktree_for_entry(target_entry_id, cx)?; @@ -4519,6 +4528,27 @@ impl ProjectPanel { } else { let update_marks = !self.marked_entries.is_empty(); let active_selection = selections.active_selection; + let active_entry_id = self.resolve_entry(active_selection.entry_id); + + // Reorder marked worktree roots together so their relative order is + // preserved; non-roots fall through to the normal per-entry move flow. + let (root_entry_ids, entries) = { + let project = self.project.read(cx); + let mut roots = Vec::new(); + let mut non_roots = BTreeSet::new(); + for entry in entries { + if project.entry_is_worktree_root(entry.entry_id, cx) { + roots.push(entry.entry_id); + } else { + non_roots.insert(entry); + } + } + (roots, non_roots) + }; + + if !root_entry_ids.is_empty() { + self.reorder_worktree_roots(&root_entry_ids, target_entry_id, active_entry_id, cx); + } // For folded selections, track the leaf suffix relative to the resolved // entry so we can refresh it after the move completes. @@ -4575,7 +4605,9 @@ impl ProjectPanel { // results with folded selections that need refreshing. let mut move_tasks: Vec<(ProjectEntryId, Task>)> = Vec::new(); for entry in entries { - if let Some(task) = self.move_entry(entry.entry_id, target_entry_id, is_file, cx) { + if let Some(task) = + self.move_worktree_entry(entry.entry_id, target_entry_id, is_file, cx) + { move_tasks.push((entry.entry_id, task)); } } @@ -5191,6 +5223,23 @@ impl ProjectPanel { drag_state: &DraggedSelection, cx: &Context, ) -> Option { + // Worktree-root drags are only meaningful when dropped on another + // worktree's root, so suppress highlights elsewhere — including over + // the source worktree itself, where the drop would be a no-op. + if self + .project + .read(cx) + .entry_is_worktree_root(drag_state.active_selection.entry_id, cx) + { + let root_id = target_worktree.root_entry()?.id; + if target_entry.id == root_id + && target_worktree.id() != drag_state.active_selection.worktree_id + { + return Some(root_id); + } + return None; + } + let target_parent_path = target_entry.path.parent(); // In case of single item drag, we do not highlight existing diff --git a/crates/project_panel/src/project_panel_tests.rs b/crates/project_panel/src/project_panel_tests.rs index c47b6ddd84b..46a7389a4b2 100644 --- a/crates/project_panel/src/project_panel_tests.rs +++ b/crates/project_panel/src/project_panel_tests.rs @@ -4211,6 +4211,379 @@ async fn test_drag_worktree_root_reorders_worktrees(cx: &mut gpui::TestAppContex ); } +// Marked worktree roots reorder together as a group, preserving their +// relative order, with direction (before vs after destination) chosen from +// the active source's original position. +#[gpui::test] +async fn test_drag_multiple_marked_worktree_roots_reorder_as_group( + cx: &mut gpui::TestAppContext, +) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree("/root1", json!({ "a.txt": "" })).await; + fs.insert_tree("/root2", json!({ "b.txt": "" })).await; + fs.insert_tree("/root3", json!({ "c.txt": "" })).await; + fs.insert_tree("/root4", json!({ "d.txt": "" })).await; + + let project = Project::test( + fs.clone(), + [ + "/root1".as_ref(), + "/root2".as_ref(), + "/root3".as_ref(), + "/root4".as_ref(), + ], + cx, + ) + .await; + let window = cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = window + .read_with(cx, |mw, _| mw.workspace().clone()) + .unwrap(); + let cx = &mut VisualTestContext::from_window(window.into(), cx); + let panel = workspace.update_in(cx, ProjectPanel::new); + cx.run_until_parked(); + + // Non-contiguous group [r1, r3], active=r1, drop onto r4. Active is before + // dest, so the group lands AFTER r4 with relative order (r1, r3) intact. + panel.update_in(cx, |panel, window, cx| { + let project = panel.project.read(cx); + let worktrees = project.visible_worktrees(cx).collect::>(); + let r1 = worktrees[0].read(cx); + let r3 = worktrees[2].read(cx); + let r4 = worktrees[3].read(cx); + let r1_entry = SelectedEntry { + worktree_id: r1.id(), + entry_id: r1.root_entry().unwrap().id, + }; + let r3_entry = SelectedEntry { + worktree_id: r3.id(), + entry_id: r3.root_entry().unwrap().id, + }; + let target = r4.root_entry().unwrap().id; + let drag = DraggedSelection { + active_selection: r1_entry, + marked_selections: Arc::new([r1_entry, r3_entry]), + }; + panel.drag_onto(&drag, target, false, window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + visible_entries_as_strings(&panel, 0..20, cx), + &[ + "v root2", + " b.txt", + "v root4", + " d.txt", + "v root1", + " a.txt", + "v root3", + " c.txt", + ], + "marked roots r1 and r3 should land after r4 in their original relative order" + ); + + // Now [r3, r1] (reordered above), active=r1, drop onto r2. Active is after + // dest, so the group lands BEFORE r2 with relative order (r1, r3) intact. + panel.update_in(cx, |panel, window, cx| { + let project = panel.project.read(cx); + let worktrees = project.visible_worktrees(cx).collect::>(); + // After the previous move, order is [r2, r4, r1, r3]. + let r2 = worktrees[0].read(cx); + let r1 = worktrees[2].read(cx); + let r3 = worktrees[3].read(cx); + let r1_entry = SelectedEntry { + worktree_id: r1.id(), + entry_id: r1.root_entry().unwrap().id, + }; + let r3_entry = SelectedEntry { + worktree_id: r3.id(), + entry_id: r3.root_entry().unwrap().id, + }; + let target = r2.root_entry().unwrap().id; + let drag = DraggedSelection { + active_selection: r1_entry, + marked_selections: Arc::new([r1_entry, r3_entry]), + }; + panel.drag_onto(&drag, target, false, window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + visible_entries_as_strings(&panel, 0..20, cx), + &[ + "v root1", + " a.txt", + "v root3", + " c.txt", + "v root2", + " b.txt", + "v root4", + " d.txt", + ], + "marked roots should land before r2 in their original relative order" + ); +} + +// Dropping a worktree root onto a file or subdirectory inside another worktree +// previously triggered an implicit reorder relative to the target's worktree. +// Reorder should only happen on an explicit drop onto a worktree root. +#[gpui::test] +async fn test_drag_worktree_root_onto_nested_entry_is_rejected( + cx: &mut gpui::TestAppContext, +) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree("/root1", json!({ "a.txt": "" })).await; + fs.insert_tree("/root2", json!({ "sub": { "b.txt": "" } })) + .await; + + let project = + Project::test(fs.clone(), ["/root1".as_ref(), "/root2".as_ref()], cx).await; + let window = cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = window + .read_with(cx, |mw, _| mw.workspace().clone()) + .unwrap(); + let cx = &mut VisualTestContext::from_window(window.into(), cx); + let panel = workspace.update_in(cx, ProjectPanel::new); + cx.run_until_parked(); + + toggle_expand_dir(&panel, "root2/sub", cx); + + // Drop root1 onto root2/sub/b.txt — should NOT reorder. + panel.update_in(cx, |panel, window, cx| { + let project = panel.project.read(cx); + let worktrees = project.visible_worktrees(cx).collect::>(); + let r1 = worktrees[0].read(cx); + let r2 = worktrees[1].read(cx); + let r1_root = SelectedEntry { + worktree_id: r1.id(), + entry_id: r1.root_entry().unwrap().id, + }; + let nested_file_id = r2.entry_for_path(rel_path("sub/b.txt")).unwrap().id; + let drag = DraggedSelection { + active_selection: r1_root, + marked_selections: Arc::new([r1_root]), + }; + panel.drag_onto(&drag, nested_file_id, true, window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + visible_entries_as_strings(&panel, 0..20, cx), + &[ + "v root1", + " a.txt", + "v root2", + " v sub <== selected", + " b.txt", + ], + "dropping a worktree root onto a nested file should not reorder worktrees" + ); + + // Drop root1 onto root2/sub (a subdirectory) — should also NOT reorder. + panel.update_in(cx, |panel, window, cx| { + let project = panel.project.read(cx); + let worktrees = project.visible_worktrees(cx).collect::>(); + let r1 = worktrees[0].read(cx); + let r2 = worktrees[1].read(cx); + let r1_root = SelectedEntry { + worktree_id: r1.id(), + entry_id: r1.root_entry().unwrap().id, + }; + let sub_dir_id = r2.entry_for_path(rel_path("sub")).unwrap().id; + let drag = DraggedSelection { + active_selection: r1_root, + marked_selections: Arc::new([r1_root]), + }; + panel.drag_onto(&drag, sub_dir_id, false, window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + visible_entries_as_strings(&panel, 0..20, cx), + &[ + "v root1", + " a.txt", + "v root2", + " v sub <== selected", + " b.txt", + ], + "dropping a worktree root onto a subdirectory should not reorder worktrees" + ); +} + +// Mixed selection: marked worktree root + non-root entry, with the non-root +// as the active drag. The root reorders, the file moves, both in one gesture. +// +// This test is positioned to exercise the `active_source.filter(|id| +// sources.contains(id))` fallback in `move_worktrees`: the active drag is a +// file in `root4` (idx 3), which lies AFTER the destination `root3` (idx 2). +// Without the filter, direction would follow root4's position and land the +// group before the destination; with the filter, `active_source` is treated +// as None and we fall back to the first source `root1` (idx 0), landing the +// group AFTER the destination. +#[gpui::test] +async fn test_drag_mixed_root_and_file_with_non_root_active( + cx: &mut gpui::TestAppContext, +) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree("/root1", json!({ "a.txt": "" })).await; + fs.insert_tree("/root2", json!({ "b.txt": "" })).await; + fs.insert_tree("/root3", json!({ "c.txt": "" })).await; + fs.insert_tree("/root4", json!({ "sub": { "d.txt": "" } })) + .await; + + let project = Project::test( + fs.clone(), + [ + "/root1".as_ref(), + "/root2".as_ref(), + "/root3".as_ref(), + "/root4".as_ref(), + ], + cx, + ) + .await; + let window = cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = window + .read_with(cx, |mw, _| mw.workspace().clone()) + .unwrap(); + let cx = &mut VisualTestContext::from_window(window.into(), cx); + let panel = workspace.update_in(cx, ProjectPanel::new); + cx.run_until_parked(); + + let (r1_id, r2_id, r3_id, r4_id) = panel.update_in(cx, |panel, _, cx| { + let worktrees = panel + .project + .read(cx) + .visible_worktrees(cx) + .collect::>(); + ( + worktrees[0].read(cx).id(), + worktrees[1].read(cx).id(), + worktrees[2].read(cx).id(), + worktrees[3].read(cx).id(), + ) + }); + + // Mark root1's root and root4/sub/d.txt; the file is the active selection. + select_path_with_mark(&panel, "root1", cx); + select_path_with_mark(&panel, "root4/sub/d.txt", cx); + + drag_selection_to(&panel, "root3", false, cx); + + // root1 reorders. The active drag's worktree (root4 at idx 3) isn't in + // the root sources [root1]; the filter discards it and direction falls + // back to root1's position (idx 0 < dest idx 2 → group lands after root3). + let worktree_order = panel.update_in(cx, |panel, _, cx| { + panel + .project + .read(cx) + .visible_worktrees(cx) + .map(|wt| wt.read(cx).id()) + .collect::>() + }); + assert_eq!(worktree_order, vec![r2_id, r3_id, r1_id, r4_id]); + + // d.txt should have moved from root4/sub to root3's root. + assert!( + find_project_entry(&panel, "root3/d.txt", cx).is_some(), + "d.txt should land in root3" + ); + assert_eq!( + find_project_entry(&panel, "root4/sub/d.txt", cx), + None, + "d.txt should be removed from root4/sub" + ); + // a.txt stays in root1. + assert!( + find_project_entry(&panel, "root1/a.txt", cx).is_some(), + "a.txt should remain in root1" + ); +} + +// Copying a worktree root has no meaning (`create_paste_path` returns None for +// it). Without filtering, the whole copy aborts via `?` and any sibling files +// in the same drag get silently dropped. Verify mixed selections still copy. +#[gpui::test] +async fn test_copy_drag_mixed_worktree_root_and_file_still_copies_file( + cx: &mut gpui::TestAppContext, +) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree("/root1", json!({ "a.txt": "hello" })).await; + fs.insert_tree("/root2", json!({ "b.txt": "world" })).await; + fs.insert_tree("/root3", json!({ "c.txt": "" })).await; + + let project = Project::test( + fs.clone(), + ["/root1".as_ref(), "/root2".as_ref(), "/root3".as_ref()], + cx, + ) + .await; + let window = cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let workspace = window + .read_with(cx, |mw, _| mw.workspace().clone()) + .unwrap(); + let cx = &mut VisualTestContext::from_window(window.into(), cx); + let panel = workspace.update_in(cx, ProjectPanel::new); + cx.run_until_parked(); + + // Hold the platform-appropriate copy modifier. + cx.simulate_modifiers_change(gpui::Modifiers { + alt: true, + control: true, + ..Default::default() + }); + + // Mark r1's root + r2's b.txt (in distinct worktrees so `disjoint_entries` + // doesn't filter the file out as nested under the root). Active = b.txt. + // Drag-copy onto r3's root. + panel.update_in(cx, |panel, window, cx| { + let project = panel.project.read(cx); + let worktrees = project.visible_worktrees(cx).collect::>(); + let r1 = worktrees[0].read(cx); + let r2 = worktrees[1].read(cx); + let r3 = worktrees[2].read(cx); + let r1_root_entry = SelectedEntry { + worktree_id: r1.id(), + entry_id: r1.root_entry().unwrap().id, + }; + let r2_file_entry = SelectedEntry { + worktree_id: r2.id(), + entry_id: r2.entry_for_path(rel_path("b.txt")).unwrap().id, + }; + let target_entry_id = r3.root_entry().unwrap().id; + let drag = DraggedSelection { + active_selection: r2_file_entry, + marked_selections: Arc::new([r1_root_entry, r2_file_entry]), + }; + panel.drag_onto(&drag, target_entry_id, false, window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + visible_entries_as_strings(&panel, 0..20, cx), + &[ + "v root1", + " a.txt", + "v root2", + " b.txt", + "v root3", + " b.txt <== selected", + " c.txt", + ], + "b.txt should be copied into root3 even though r1's root was also marked" + ); +} + #[gpui::test] async fn test_multiple_marked_entries(cx: &mut gpui::TestAppContext) { init_test_with_editor(cx);