From 669f8ccda4702441f872cbcbb6ff19579e7a7513 Mon Sep 17 00:00:00 2001 From: Smit Barmase Date: Fri, 6 Feb 2026 18:41:08 +0530 Subject: [PATCH] project_panel: Refactor selection state ownership (#48586) This PR moves `selection` from `State` to `ProjectPanel`. In async `update_visible_entries`, the `old.selection` copied into `new_state` was never read or updated. After async completion, we already either set the new selection (when provided) or keep the current selection value. So storing `selection` in `State` was redundant. This simplifies a confusing code path without changing behavior. Release Notes: - N/A --- crates/project_panel/src/project_panel.rs | 134 +++++++----------- .../project_panel/src/project_panel_tests.rs | 35 +++-- 2 files changed, 71 insertions(+), 98 deletions(-) diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index 406aff23c7e6fa554bcd32f071412472143db3ae..d2d451154ad9a25136f299c7c46d59d225522020 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -94,8 +94,6 @@ struct State { ancestors: HashMap, visible_entries: Vec, max_width_item_index: Option, - // Currently selected leaf entry (see auto-folding for a definition of that) in a file tree - selection: Option, edit_state: Option, unfolded_dir_ids: HashSet, expanded_dir_ids: HashMap>, @@ -110,7 +108,6 @@ impl State { max_width_item_index: None, edit_state: old.edit_state.clone(), unfolded_dir_ids: old.unfolded_dir_ids.clone(), - selection: old.selection, expanded_dir_ids: old.expanded_dir_ids.clone(), } } @@ -128,6 +125,7 @@ pub struct ProjectPanel { folded_directory_drag_target: Option, drag_target_entry: Option, marked_entries: Vec, + selection: Option, context_menu: Option<(Entity, Point, Subscription)>, filename_editor: Entity, clipboard: Option, @@ -446,7 +444,7 @@ pub fn init(cx: &mut App) { if let Some(first_marked) = panel.marked_entries.first() { let first_marked = *first_marked; panel.marked_entries.clear(); - panel.state.selection = Some(first_marked); + panel.selection = Some(first_marked); } panel.rename(action, window, cx); }); @@ -471,7 +469,7 @@ pub fn init(cx: &mut App) { workspace.register_action(|workspace, _: &git::FileHistory, window, cx| { // First try to get from project panel if it's focused if let Some(panel) = workspace.panel::(cx) { - let maybe_project_path = panel.read(cx).state.selection.and_then(|selection| { + let maybe_project_path = panel.read(cx).selection.and_then(|selection| { let project = workspace.project().read(cx); let worktree = project.worktree_for_id(selection.worktree_id, cx)?; let entry = worktree.read(cx).entry_for_id(selection.entry_id)?; @@ -811,8 +809,8 @@ impl ProjectPanel { rendered_entries_len: 0, folded_directory_drag_target: None, drag_target_entry: None, - marked_entries: Default::default(), + selection: None, context_menu: None, filename_editor, clipboard: None, @@ -831,7 +829,6 @@ impl ProjectPanel { state: State { max_width_item_index: None, edit_state: None, - selection: None, last_worktree_root_id: Default::default(), visible_entries: Default::default(), ancestors: Default::default(), @@ -896,7 +893,7 @@ impl ProjectPanel { let entry = SelectedEntry { worktree_id, entry_id }; project_panel.marked_entries.clear(); project_panel.marked_entries.push(entry); - project_panel.state.selection = Some(entry); + project_panel.selection = Some(entry); }); if !focus_opened_item { let focus_handle = project_panel.read(cx).focus_handle.clone(); @@ -1075,7 +1072,7 @@ impl ProjectPanel { return; }; - self.state.selection = Some(SelectedEntry { + self.selection = Some(SelectedEntry { worktree_id, entry_id, }); @@ -1600,7 +1597,7 @@ impl ProjectPanel { }); return; } - if let Some(selection) = self.state.selection { + if let Some(selection) = self.selection { let (mut worktree_ix, mut entry_ix, _) = self.index_for_selection(selection).unwrap_or_default(); if entry_ix > 0 { @@ -1621,7 +1618,7 @@ impl ProjectPanel { worktree_id: *worktree_id, entry_id: entries[entry_ix].id, }; - self.state.selection = Some(selection); + self.selection = Some(selection); if window.modifiers().shift { self.marked_entries.push(selection); } @@ -1808,7 +1805,7 @@ impl ProjectPanel { let edit_task; let edited_entry_id; if is_new_entry { - self.state.selection = Some(SelectedEntry { + self.selection = Some(SelectedEntry { worktree_id, entry_id: NEW_ENTRY_ID, }); @@ -1864,7 +1861,7 @@ impl ProjectPanel { } Ok(CreatedEntry::Included(new_entry)) => { project_panel.update_in(cx, |project_panel, window, cx| { - if let Some(selection) = &mut project_panel.state.selection + if let Some(selection) = &mut project_panel.selection && selection.entry_id == edited_entry_id { selection.worktree_id = worktree_id; @@ -2000,7 +1997,6 @@ impl ProjectPanel { fn add_entry(&mut self, is_dir: bool, window: &mut Window, cx: &mut Context) { let Some((worktree_id, entry_id)) = self - .state .selection .map(|entry| (entry.worktree_id, entry.entry_id)) .or_else(|| { @@ -2012,7 +2008,7 @@ impl ProjectPanel { .read(cx) .id(); - self.state.selection = Some(SelectedEntry { + self.selection = Some(SelectedEntry { worktree_id, entry_id, }); @@ -2064,7 +2060,7 @@ impl ProjectPanel { leaf_entry_id: None, is_dir, processing_filename: None, - previously_focused: self.state.selection, + previously_focused: self.selection, depth: 0, validation_state: ValidationState::None, }); @@ -2096,7 +2092,7 @@ impl ProjectPanel { if let Some(SelectedEntry { worktree_id, entry_id, - }) = self.state.selection + }) = self.selection && let Some(worktree) = self.project.read(cx).worktree_for_id(worktree_id, cx) { let sub_entry_id = self.unflatten_entry_id(entry_id); @@ -2165,7 +2161,7 @@ impl ProjectPanel { cx: &mut Context, ) { maybe!({ - let selection = self.state.selection?; + let selection = self.selection?; let project = self.project.read(cx); let (_worktree, entry) = self.selected_sub_entry(cx)?; @@ -2532,11 +2528,7 @@ impl ProjectPanel { _: &mut Window, cx: &mut Context, ) { - if let Some((_, _, index)) = self - .state - .selection - .and_then(|s| self.index_for_selection(s)) - { + if let Some((_, _, index)) = self.selection.and_then(|s| self.index_for_selection(s)) { self.scroll_handle .scroll_to_item_strict(index, ScrollStrategy::Center); cx.notify(); @@ -2544,11 +2536,7 @@ impl ProjectPanel { } fn scroll_cursor_top(&mut self, _: &ScrollCursorTop, _: &mut Window, cx: &mut Context) { - if let Some((_, _, index)) = self - .state - .selection - .and_then(|s| self.index_for_selection(s)) - { + if let Some((_, _, index)) = self.selection.and_then(|s| self.index_for_selection(s)) { self.scroll_handle .scroll_to_item_strict(index, ScrollStrategy::Top); cx.notify(); @@ -2561,11 +2549,7 @@ impl ProjectPanel { _: &mut Window, cx: &mut Context, ) { - if let Some((_, _, index)) = self - .state - .selection - .and_then(|s| self.index_for_selection(s)) - { + if let Some((_, _, index)) = self.selection.and_then(|s| self.index_for_selection(s)) { self.scroll_handle .scroll_to_item_strict(index, ScrollStrategy::Bottom); cx.notify(); @@ -2587,7 +2571,7 @@ impl ProjectPanel { }); return; } - if let Some(selection) = self.state.selection { + if let Some(selection) = self.selection { let (mut worktree_ix, mut entry_ix, _) = self.index_for_selection(selection).unwrap_or_default(); if let Some(worktree_entries) = self @@ -2615,7 +2599,7 @@ impl ProjectPanel { worktree_id: *worktree_id, entry_id: entry.id, }; - self.state.selection = Some(selection); + self.selection = Some(selection); if window.modifiers().shift { self.marked_entries.push(selection); } @@ -2635,10 +2619,10 @@ impl ProjectPanel { cx: &mut Context, ) { let selection = self.find_entry( - self.state.selection.as_ref(), + self.selection.as_ref(), true, |entry, worktree_id| { - self.state.selection.is_none_or(|selection| { + self.selection.is_none_or(|selection| { if selection.worktree_id == worktree_id { selection.entry_id != entry.id } else { @@ -2654,7 +2638,7 @@ impl ProjectPanel { ); if let Some(selection) = selection { - self.state.selection = Some(selection); + self.selection = Some(selection); self.expand_entry(selection.worktree_id, selection.entry_id, cx); self.update_visible_entries( Some((selection.worktree_id, selection.entry_id)), @@ -2674,10 +2658,10 @@ impl ProjectPanel { cx: &mut Context, ) { let selection = self.find_entry( - self.state.selection.as_ref(), + self.selection.as_ref(), false, |entry, worktree_id| { - self.state.selection.is_none_or(|selection| { + self.selection.is_none_or(|selection| { if selection.worktree_id == worktree_id { selection.entry_id != entry.id } else { @@ -2693,7 +2677,7 @@ impl ProjectPanel { ); if let Some(selection) = selection { - self.state.selection = Some(selection); + self.selection = Some(selection); self.expand_entry(selection.worktree_id, selection.entry_id, cx); self.update_visible_entries( Some((selection.worktree_id, selection.entry_id)), @@ -2713,11 +2697,11 @@ impl ProjectPanel { cx: &mut Context, ) { let selection = self.find_entry( - self.state.selection.as_ref(), + self.selection.as_ref(), true, |entry, worktree_id| { - (self.state.selection.is_none() - || self.state.selection.is_some_and(|selection| { + (self.selection.is_none() + || self.selection.is_some_and(|selection| { if selection.worktree_id == worktree_id { selection.entry_id != entry.id } else { @@ -2731,7 +2715,7 @@ impl ProjectPanel { ); if let Some(selection) = selection { - self.state.selection = Some(selection); + self.selection = Some(selection); self.expand_entry(selection.worktree_id, selection.entry_id, cx); self.update_visible_entries( Some((selection.worktree_id, selection.entry_id)), @@ -2751,10 +2735,10 @@ impl ProjectPanel { cx: &mut Context, ) { let selection = self.find_visible_entry( - self.state.selection.as_ref(), + self.selection.as_ref(), true, |entry, worktree_id| { - self.state.selection.is_none_or(|selection| { + self.selection.is_none_or(|selection| { if selection.worktree_id == worktree_id { selection.entry_id != entry.id } else { @@ -2766,7 +2750,7 @@ impl ProjectPanel { ); if let Some(selection) = selection { - self.state.selection = Some(selection); + self.selection = Some(selection); self.autoscroll(cx); cx.notify(); } @@ -2779,10 +2763,10 @@ impl ProjectPanel { cx: &mut Context, ) { let selection = self.find_visible_entry( - self.state.selection.as_ref(), + self.selection.as_ref(), false, |entry, worktree_id| { - self.state.selection.is_none_or(|selection| { + self.selection.is_none_or(|selection| { if selection.worktree_id == worktree_id { selection.entry_id != entry.id } else { @@ -2794,7 +2778,7 @@ impl ProjectPanel { ); if let Some(selection) = selection { - self.state.selection = Some(selection); + self.selection = Some(selection); self.autoscroll(cx); cx.notify(); } @@ -2807,10 +2791,10 @@ impl ProjectPanel { cx: &mut Context, ) { let selection = self.find_entry( - self.state.selection.as_ref(), + self.selection.as_ref(), false, |entry, worktree_id| { - self.state.selection.is_none_or(|selection| { + self.selection.is_none_or(|selection| { if selection.worktree_id == worktree_id { selection.entry_id != entry.id } else { @@ -2823,7 +2807,7 @@ impl ProjectPanel { ); if let Some(selection) = selection { - self.state.selection = Some(selection); + self.selection = Some(selection); self.expand_entry(selection.worktree_id, selection.entry_id, cx); self.update_visible_entries( Some((selection.worktree_id, selection.entry_id)), @@ -2841,7 +2825,7 @@ impl ProjectPanel { if let Some(parent) = entry.path.parent() { let worktree = worktree.read(cx); if let Some(parent_entry) = worktree.entry_for_path(parent) { - self.state.selection = Some(SelectedEntry { + self.selection = Some(SelectedEntry { worktree_id: worktree.id(), entry_id: parent_entry.id, }); @@ -2866,7 +2850,7 @@ impl ProjectPanel { worktree_id: *worktree_id, entry_id: entry.id, }; - self.state.selection = Some(selection); + self.selection = Some(selection); if window.modifiers().shift { self.marked_entries.push(selection); } @@ -2890,7 +2874,7 @@ impl ProjectPanel { worktree_id: *worktree_id, entry_id: entry.id, }; - self.state.selection = Some(selection); + self.selection = Some(selection); self.autoscroll(cx); cx.notify(); } @@ -2899,11 +2883,7 @@ impl ProjectPanel { } fn autoscroll(&mut self, cx: &mut Context) { - if let Some((_, _, index)) = self - .state - .selection - .and_then(|s| self.index_for_selection(s)) - { + if let Some((_, _, index)) = self.selection.and_then(|s| self.index_for_selection(s)) { self.scroll_handle.scroll_to_item_with_offset( index, ScrollStrategy::Center, @@ -3045,7 +3025,7 @@ impl ProjectPanel { if let Some(entry) = last_succeed { project_panel .update_in(cx, |project_panel, window, cx| { - project_panel.state.selection = Some(SelectedEntry { + project_panel.selection = Some(SelectedEntry { worktree_id, entry_id: entry.id, }); @@ -3625,7 +3605,7 @@ impl ProjectPanel { } fn effective_entries(&self) -> BTreeSet { - if let Some(selection) = self.state.selection { + if let Some(selection) = self.selection { let selection = SelectedEntry { entry_id: self.resolve_entry(selection.entry_id), worktree_id: selection.worktree_id, @@ -3689,7 +3669,7 @@ impl ProjectPanel { &self, cx: &'a App, ) -> Option<(Entity, &'a project::Entry)> { - let selection = self.state.selection?; + let selection = self.selection?; let project = self.project.read(cx); let worktree = project.worktree_for_id(selection.worktree_id, cx)?; let entry = worktree.read(cx).entry_for_id(selection.entry_id)?; @@ -4003,15 +3983,12 @@ impl ProjectPanel { }) .await; this.update_in(cx, |this, window, cx| { - let current_selection = this.state.selection; this.state = new_state; if let Some((worktree_id, entry_id)) = new_selected_entry { - this.state.selection = Some(SelectedEntry { + this.selection = Some(SelectedEntry { worktree_id, entry_id, }); - } else { - this.state.selection = current_selection; } let elapsed = now.elapsed(); if this.last_reported_update.elapsed() > Duration::from_secs(3600) { @@ -4256,7 +4233,7 @@ impl ProjectPanel { if let Some(entry_id) = last_succeed { project_panel .update_in(cx, |project_panel, window, cx| { - project_panel.state.selection = Some(SelectedEntry { + project_panel.selection = Some(SelectedEntry { worktree_id, entry_id, }); @@ -4980,7 +4957,6 @@ impl ProjectPanel { let is_marked = self.marked_entries.contains(&selection); let is_active = self - .state .selection .is_some_and(|selection| selection.entry_id == entry_id); @@ -5331,10 +5307,8 @@ impl ProjectPanel { } cx.stop_propagation(); - if let Some(selection) = project_panel - .state - .selection - .filter(|_| event.modifiers().shift) + if let Some(selection) = + project_panel.selection.filter(|_| event.modifiers().shift) { let current_selection = project_panel.index_for_selection(selection); let clicked_entry = SelectedEntry { @@ -5366,7 +5340,7 @@ impl ProjectPanel { } } - project_panel.state.selection = Some(clicked_entry); + project_panel.selection = Some(clicked_entry); if !project_panel.marked_entries.contains(&clicked_entry) { project_panel.marked_entries.push(clicked_entry); } @@ -5375,7 +5349,7 @@ impl ProjectPanel { if event.click_count() > 1 { project_panel.split_entry(entry_id, false, None, cx); } else { - project_panel.state.selection = Some(selection); + project_panel.selection = Some(selection); if let Some(position) = project_panel .marked_entries .iter() @@ -5854,7 +5828,7 @@ impl ProjectPanel { entry_id: entry.id, }; let is_marked = self.marked_entries.contains(&selection); - let is_selected = self.state.selection == Some(selection); + let is_selected = self.selection == Some(selection); let diagnostic_severity = self .diagnostics @@ -6626,7 +6600,7 @@ impl Render for ProjectPanel { return; } cx.stop_propagation(); - this.state.selection = None; + this.selection = None; this.marked_entries.clear(); this.focus_handle(cx).focus(window, cx); })) @@ -6662,7 +6636,7 @@ impl Render for ProjectPanel { return; }; - this.state.selection = Some(SelectedEntry { + this.selection = Some(SelectedEntry { worktree_id, entry_id, }); diff --git a/crates/project_panel/src/project_panel_tests.rs b/crates/project_panel/src/project_panel_tests.rs index 3501f952413aca096abeefbbc81bf14648f430de..f9fa85d70b2cb154a05dc3aa412de862a6a6b393 100644 --- a/crates/project_panel/src/project_panel_tests.rs +++ b/crates/project_panel/src/project_panel_tests.rs @@ -3814,7 +3814,7 @@ async fn test_rename_with_hide_root(cx: &mut gpui::TestAppContext) { let project = panel.project.read(cx); let worktree = project.visible_worktrees(cx).next().unwrap(); let root_entry = worktree.read(cx).root_entry().unwrap(); - panel.state.selection = Some(SelectedEntry { + panel.selection = Some(SelectedEntry { worktree_id: worktree.read(cx).id(), entry_id: root_entry.id, }); @@ -3974,7 +3974,7 @@ async fn test_multiple_marked_entries(cx: &mut gpui::TestAppContext) { cx.update(|window, cx| { panel.update(cx, |this, cx| { let drag = DraggedSelection { - active_selection: this.state.selection.unwrap(), + active_selection: this.selection.unwrap(), marked_selections: this.marked_entries.clone().into(), }; let target_entry = this @@ -4113,8 +4113,8 @@ async fn test_dragged_selection_resolve_entry(cx: &mut gpui::TestAppContext) { select_path(&panel, "root/a/b/c/d", cx); panel.update_in(cx, |panel, window, cx| { let drag = DraggedSelection { - active_selection: *panel.state.selection.as_ref().unwrap(), - marked_selections: Arc::new([*panel.state.selection.as_ref().unwrap()]), + active_selection: *panel.selection.as_ref().unwrap(), + marked_selections: Arc::new([*panel.selection.as_ref().unwrap()]), }; let target_entry = panel .project @@ -4143,8 +4143,8 @@ async fn test_dragged_selection_resolve_entry(cx: &mut gpui::TestAppContext) { select_path(&panel, "root/target_destination/d", cx); panel.update_in(cx, |panel, window, cx| { let drag = DraggedSelection { - active_selection: *panel.state.selection.as_ref().unwrap(), - marked_selections: Arc::new([*panel.state.selection.as_ref().unwrap()]), + active_selection: *panel.selection.as_ref().unwrap(), + marked_selections: Arc::new([*panel.selection.as_ref().unwrap()]), }; let target_entry = panel .project @@ -4163,8 +4163,8 @@ async fn test_dragged_selection_resolve_entry(cx: &mut gpui::TestAppContext) { select_path(&panel, "root/a/b", cx); panel.update_in(cx, |panel, window, cx| { let drag = DraggedSelection { - active_selection: *panel.state.selection.as_ref().unwrap(), - marked_selections: Arc::new([*panel.state.selection.as_ref().unwrap()]), + active_selection: *panel.selection.as_ref().unwrap(), + marked_selections: Arc::new([*panel.selection.as_ref().unwrap()]), }; let target_entry = panel .project @@ -4189,8 +4189,8 @@ async fn test_dragged_selection_resolve_entry(cx: &mut gpui::TestAppContext) { select_path(&panel, "root/target_destination/b", cx); panel.update_in(cx, |panel, window, cx| { let drag = DraggedSelection { - active_selection: *panel.state.selection.as_ref().unwrap(), - marked_selections: Arc::new([*panel.state.selection.as_ref().unwrap()]), + active_selection: *panel.selection.as_ref().unwrap(), + marked_selections: Arc::new([*panel.selection.as_ref().unwrap()]), }; let target_entry = panel .project @@ -4209,8 +4209,8 @@ async fn test_dragged_selection_resolve_entry(cx: &mut gpui::TestAppContext) { select_path(&panel, "root/a", cx); panel.update_in(cx, |panel, window, cx| { let drag = DraggedSelection { - active_selection: *panel.state.selection.as_ref().unwrap(), - marked_selections: Arc::new([*panel.state.selection.as_ref().unwrap()]), + active_selection: *panel.selection.as_ref().unwrap(), + marked_selections: Arc::new([*panel.selection.as_ref().unwrap()]), }; let target_entry = panel .project @@ -4283,7 +4283,7 @@ async fn test_drag_marked_entries_in_folded_directories(cx: &mut gpui::TestAppCo panel.update_in(cx, |panel, window, cx| { let drag = DraggedSelection { - active_selection: *panel.state.selection.as_ref().unwrap(), + active_selection: *panel.selection.as_ref().unwrap(), marked_selections: panel.marked_entries.clone().into(), }; let target_entry = panel @@ -7347,7 +7347,7 @@ async fn test_create_entries_without_selection_hide_root(cx: &mut gpui::TestAppC panel.update(cx, |panel, _| { assert!( - panel.state.selection.is_none(), + panel.selection.is_none(), "Should have no selection initially" ); }); @@ -7399,7 +7399,7 @@ async fn test_create_entries_without_selection_hide_root(cx: &mut gpui::TestAppC // Test 2: Create new directory when no entry is selected panel.update(cx, |panel, _| { - panel.state.selection = None; + panel.selection = None; }); panel.update_in(cx, |panel, window, cx| { @@ -8508,7 +8508,7 @@ fn select_path_with_mark(panel: &Entity, path: &str, cx: &mut Visu if !panel.marked_entries.contains(&entry) { panel.marked_entries.push(entry); } - panel.state.selection = Some(entry); + panel.selection = Some(entry); return; } } @@ -8527,7 +8527,7 @@ fn select_folded_path_with_mark( select_path_with_mark(panel, leaf_path, cx); let active_ancestor_path = rel_path(active_ancestor_path); panel.update(cx, |panel, cx| { - let leaf_entry_id = panel.state.selection.unwrap().entry_id; + let leaf_entry_id = panel.selection.unwrap().entry_id; if let Some(folded_ancestors) = panel.state.ancestors.get_mut(&leaf_entry_id) { for worktree in panel.project.read(cx).worktrees(cx).collect::>() { let worktree = worktree.read(cx); @@ -8561,7 +8561,6 @@ fn drag_selection_to( panel.update_in(cx, |panel, window, cx| { let selection = panel - .state .selection .expect("a selection is required before dragging"); let drag = DraggedSelection {