From 4e28b0a3fc6f7080de89919fef5ab3bd4c6813fa Mon Sep 17 00:00:00 2001 From: Smit Barmase Date: Wed, 6 May 2026 19:36:48 +0530 Subject: [PATCH] recent_projects: Allow bulk deleting entries without scroll jumping (#54777) Closes #52292 This PR preserves the scroll offset of the List used in Recent Projects while deleting items. It does that by: - Adding the `is_scrolled_to_end` method to the GPUI list, which helps us determine where the new selection should be, since it depends on whether items are taking the deleted slot from below or above. - Adding `ScrollBehavior` to `update_matches`, which lets you preserve the scroll offset even for `List` (not `UniformList`) after a reset. Before: https://github.com/user-attachments/assets/e3eb7092-59ec-4b54-b57a-503555addd27 After: https://github.com/user-attachments/assets/6929f6a0-04d7-44f9-a9b2-f9e5c077b368 Release Notes: - Fixed the recent projects list jumping to the top after deleting a project, so you can now bulk-delete entries by repeatedly clicking the delete icon or pressing the keybind. --- crates/gpui/src/elements/list.rs | 23 ++ crates/gpui/src/elements/uniform_list.rs | 14 +- crates/picker/src/picker.rs | 60 +++- crates/recent_projects/Cargo.toml | 1 + crates/recent_projects/src/recent_projects.rs | 313 +++++++++++++++++- 5 files changed, 388 insertions(+), 23 deletions(-) diff --git a/crates/gpui/src/elements/list.rs b/crates/gpui/src/elements/list.rs index c7409687fafb9b4e216f415ab7ea39454b56b3ed..558e89dd83e752af4d97fe828c8aac5789376373 100644 --- a/crates/gpui/src/elements/list.rs +++ b/crates/gpui/src/elements/list.rs @@ -271,6 +271,7 @@ struct ListItemSummary { unrendered_count: usize, height: Pixels, has_focus_handles: bool, + has_unknown_height: bool, } #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] @@ -399,6 +400,25 @@ impl ListState { self.0.borrow().items.summary().count } + /// Whether the list is scrolled to the end, or `None` if the list is + /// not scrollable or the total content height is not yet known. + pub fn is_scrolled_to_end(&self) -> Option { + let state = self.0.borrow(); + let bounds = state.last_layout_bounds?; + let summary = state.items.summary(); + if summary.has_unknown_height { + return None; + } + let padding = state.last_padding.unwrap_or_default(); + let content_height = summary.height + padding.top + padding.bottom; + let scroll_max = (content_height - bounds.size.height).max(px(0.)); + if scroll_max <= px(0.) { + return None; + } + let scroll_top = state.scroll_top(&state.logical_scroll_top()); + Some(scroll_top >= scroll_max) + } + /// Inform the list state that the items in `old_range` have been replaced /// by `count` new items that must be recalculated. pub fn splice(&self, old_range: Range, count: usize) { @@ -1385,6 +1405,7 @@ impl sum_tree::Item for ListItem { px(0.) }, has_focus_handles: focus_handle.is_some(), + has_unknown_height: size_hint.is_none(), }, ListItem::Measured { size, focus_handle, .. @@ -1394,6 +1415,7 @@ impl sum_tree::Item for ListItem { unrendered_count: 0, height: size.height, has_focus_handles: focus_handle.is_some(), + has_unknown_height: false, }, } } @@ -1410,6 +1432,7 @@ impl sum_tree::ContextLessSummary for ListItemSummary { self.unrendered_count += summary.unrendered_count; self.height += summary.height; self.has_focus_handles |= summary.has_focus_handles; + self.has_unknown_height |= summary.has_unknown_height; } } diff --git a/crates/gpui/src/elements/uniform_list.rs b/crates/gpui/src/elements/uniform_list.rs index a7486f0c00ac4e11ef807af90f6fb75b74b5d142..0a3314573f08568de586ca498332eac95ae52c7e 100644 --- a/crates/gpui/src/elements/uniform_list.rs +++ b/crates/gpui/src/elements/uniform_list.rs @@ -8,7 +8,7 @@ use crate::{ AnyElement, App, AvailableSpace, Bounds, ContentMask, Element, ElementId, Entity, GlobalElementId, Hitbox, InspectorElementId, InteractiveElement, Interactivity, IntoElement, IsZero, LayoutId, ListSizingBehavior, Overflow, Pixels, Point, ScrollHandle, Size, - StyleRefinement, Styled, Window, point, size, + StyleRefinement, Styled, Window, point, px, size, }; use smallvec::SmallVec; use std::{cell::RefCell, cmp, ops::Range, rc::Rc, usize}; @@ -236,6 +236,18 @@ impl UniformListScrollHandle { } } + /// Whether the list is scrolled to the end, or `None` if the list is + /// not scrollable. + pub fn is_scrolled_to_end(&self) -> Option { + let state = self.0.borrow(); + let max_offset = state.base_handle.max_offset(); + if max_offset.y <= px(0.) { + return None; + } + let offset = state.base_handle.offset(); + Some(-offset.y >= max_offset.y) + } + /// Scroll to the bottom of the list. pub fn scroll_to_bottom(&self) { self.scroll_to_item(usize::MAX, ScrollStrategy::Bottom); diff --git a/crates/picker/src/picker.rs b/crates/picker/src/picker.rs index f2f90db1e637ce9e0d2d759dcaa24182332b5d47..0ff12127237ca5f8ef1c2b1f30b8818dbed2e574 100644 --- a/crates/picker/src/picker.rs +++ b/crates/picker/src/picker.rs @@ -35,6 +35,12 @@ pub enum Direction { Down, } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ScrollBehavior { + RevealSelected, + PreserveOffset, +} + actions!( picker, [ @@ -687,9 +693,19 @@ impl Picker { } pub fn update_matches(&mut self, query: String, window: &mut Window, cx: &mut Context) { + self.update_matches_with_options(query, ScrollBehavior::RevealSelected, window, cx); + } + + pub fn update_matches_with_options( + &mut self, + query: String, + scroll_behavior: ScrollBehavior, + window: &mut Window, + cx: &mut Context, + ) { let delegate_pending_update_matches = self.delegate.update_matches(query, window, cx); - self.matches_updated(window, cx); + self.matches_updated(scroll_behavior, window, cx); // This struct ensures that we can synchronously drop the task returned by the // delegate's `update_matches` method and the task that the picker is spawning. // If we simply capture the delegate's task into the picker's task, when the picker's @@ -709,19 +725,40 @@ impl Picker { })?; delegate_pending_update_matches.await; this.update_in(cx, |this, window, cx| { - this.matches_updated(window, cx); + this.matches_updated(scroll_behavior, window, cx); }) }), }); } - fn matches_updated(&mut self, window: &mut Window, cx: &mut Context) { - if let ElementContainer::List(state) = &mut self.element_container { - state.reset(self.delegate.match_count()); + fn matches_updated( + &mut self, + scroll_behavior: ScrollBehavior, + window: &mut Window, + cx: &mut Context, + ) { + let match_count = self.delegate.match_count(); + match &mut self.element_container { + ElementContainer::List(state) => match scroll_behavior { + ScrollBehavior::RevealSelected => { + state.reset(match_count); + let index = self.delegate.selected_index(); + self.scroll_to_item_index(index); + } + ScrollBehavior::PreserveOffset => { + let offset = state.logical_scroll_top(); + state.reset(match_count); + state.scroll_to(offset); + } + }, + ElementContainer::UniformList(_) => match scroll_behavior { + ScrollBehavior::RevealSelected => { + let index = self.delegate.selected_index(); + self.scroll_to_item_index(index); + } + ScrollBehavior::PreserveOffset => {} + }, } - - let index = self.delegate.selected_index(); - self.scroll_to_item_index(index); self.pending_update_matches = None; if let Some(secondary) = self.confirm_on_update.take() { self.do_confirm(secondary, window, cx); @@ -752,6 +789,13 @@ impl Picker { } } + pub fn is_scrolled_to_end(&self) -> Option { + match &self.element_container { + ElementContainer::List(state) => state.is_scrolled_to_end(), + ElementContainer::UniformList(scroll_handle) => scroll_handle.is_scrolled_to_end(), + } + } + fn render_element( &self, window: &mut Window, diff --git a/crates/recent_projects/Cargo.toml b/crates/recent_projects/Cargo.toml index 6062aaa8a9036cac015c2442dff029504ac7de17..013e00e96483cd1b032a25e2b252606c985aa5f5 100644 --- a/crates/recent_projects/Cargo.toml +++ b/crates/recent_projects/Cargo.toml @@ -64,6 +64,7 @@ fs.workspace = true gpui = { workspace = true, features = ["test-support"] } http_client.workspace = true language = { workspace = true, features = ["test-support"] } +picker = { workspace = true, features = ["test-support"] } project = { workspace = true, features = ["test-support"] } release_channel.workspace = true remote = { workspace = true, features = ["test-support"] } diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index a5fb5f604508301540667d408083888b18a16eff..a53d524885f9cb2be69345880a8aba9de4396690 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -29,7 +29,7 @@ use gpui::{ }; use picker::{ - Picker, PickerDelegate, + Picker, PickerDelegate, ScrollBehavior, highlighted_match_with_paths::{HighlightedMatch, HighlightedMatchWithPaths}, }; use project::{Worktree, git_store::Repository}; @@ -83,6 +83,15 @@ enum ProjectPickerEntry { RecentProject(StringMatch), } +fn is_selectable_entry(entry: &ProjectPickerEntry) -> bool { + matches!( + entry, + ProjectPickerEntry::OpenFolder { .. } + | ProjectPickerEntry::ProjectGroup(_) + | ProjectPickerEntry::RecentProject(_) + ) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum ProjectPickerStyle { Modal, @@ -814,8 +823,7 @@ pub struct RecentProjectsDelegate { selected_index: usize, render_paths: bool, create_new_window: bool, - // Flag to reset index when there is a new query vs not reset index when user delete an item - reset_selected_match_index: bool, + snap_selection_to_first_non_header_match: bool, has_any_non_local_projects: bool, project_connection_options: Option, focus_handle: FocusHandle, @@ -843,7 +851,7 @@ impl RecentProjectsDelegate { selected_index: 0, create_new_window, render_paths, - reset_selected_match_index: true, + snap_selection_to_first_non_header_match: true, has_any_non_local_projects: project_connection_options.is_some(), project_connection_options, focus_handle, @@ -1067,14 +1075,14 @@ impl PickerDelegate for RecentProjectsDelegate { self.filtered_entries = entries; - if self.reset_selected_match_index { + if self.snap_selection_to_first_non_header_match { self.selected_index = self .filtered_entries .iter() .position(|e| !matches!(e, ProjectPickerEntry::Header(_))) .unwrap_or(0); } - self.reset_selected_match_index = true; + self.snap_selection_to_first_non_header_match = true; Task::ready(()) } @@ -2106,6 +2114,69 @@ impl RecentProjectsDelegate { .detach(); } + /// Returns the new selection index after the entry at `deleted_index` + /// is removed. + /// + /// - Prefers the nearest entry matching `prefer_section` so the user + /// stays in the same section they were navigating. + /// - Falls back to any other selectable entry so the picker doesn't + /// land on a header. + fn replacement_index_after_deletion( + &self, + deleted_index: usize, + prefer_previous: bool, + prefer_section: fn(&ProjectPickerEntry) -> bool, + ) -> Option { + let replacement_index = |matches_entry: fn(&ProjectPickerEntry) -> bool| { + let next_index = self + .filtered_entries + .iter() + .enumerate() + .skip(deleted_index) + .find_map(|(index, entry)| matches_entry(entry).then_some(index)); + let previous_index = self + .filtered_entries + .iter() + .enumerate() + .take(deleted_index.min(self.filtered_entries.len())) + .rev() + .find_map(|(index, entry)| matches_entry(entry).then_some(index)); + + if prefer_previous { + previous_index.or(next_index) + } else { + next_index.or(previous_index) + } + }; + + replacement_index(prefer_section).or_else(|| replacement_index(is_selectable_entry)) + } + + fn update_picker_after_recent_project_deletion( + picker: &mut Picker, + deleted_index: usize, + workspaces: Vec, + window: &mut Window, + cx: &mut Context>, + ) { + let prefer_previous = picker.is_scrolled_to_end() == Some(true); + picker.delegate.set_workspaces(workspaces); + picker.delegate.snap_selection_to_first_non_header_match = false; + picker.update_matches_with_options( + picker.query(cx), + ScrollBehavior::PreserveOffset, + window, + cx, + ); + if let Some(replacement_index) = picker.delegate.replacement_index_after_deletion( + deleted_index, + prefer_previous, + |entry| matches!(entry, ProjectPickerEntry::RecentProject(_)), + ) { + picker.set_selected_index(replacement_index, None, false, window, cx); + } + } + fn delete_recent_project( &self, ix: usize, @@ -2115,7 +2186,10 @@ impl RecentProjectsDelegate { if let Some(ProjectPickerEntry::RecentProject(selected_match)) = self.filtered_entries.get(ix) { - let recent_workspace = self.workspaces[selected_match.candidate_id].clone(); + let Some(recent_workspace) = self.workspaces.get(selected_match.candidate_id).cloned() + else { + return; + }; let fs = self .workspace .upgrade() @@ -2133,12 +2207,9 @@ impl RecentProjectsDelegate { .await .unwrap_or_default(); this.update_in(cx, move |picker, window, cx| { - picker.delegate.set_workspaces(workspaces); - picker - .delegate - .set_selected_index(ix.saturating_sub(1), window, cx); - picker.delegate.reset_selected_match_index = false; - picker.update_matches(picker.query(cx), window, cx); + Self::update_picker_after_recent_project_deletion( + picker, ix, workspaces, window, cx, + ); // After deleting a project, we want to update the history manager to reflect the change. // But we do not emit a update event when user opens a project, because it's handled in `workspace::load_workspace`. if let Some(history_manager) = HistoryManager::global(cx) { @@ -2234,7 +2305,7 @@ impl RecentProjectsDelegate { #[cfg(test)] mod tests { - use gpui::{TestAppContext, UpdateGlobal}; + use gpui::{TestAppContext, UpdateGlobal, VisualTestContext}; use serde_json::json; use settings::SettingsStore; @@ -2243,6 +2314,220 @@ mod tests { use super::*; + // Test picker for the empty query: + // + // [0] Header("Current Folders") + // [1] OpenFolder(0) + // [2] OpenFolder(1) + // [3] Header("This Window") + // [4] ProjectGroup(0) + // [5] ProjectGroup(1) + // [6] Header("Recent Projects") + // [7..=26] RecentProject(0..=19) + // + const RECENT_PROJECT_COUNT: usize = 20; + const FIRST_RECENT_PROJECT: usize = 7; + const LAST_RECENT_PROJECT: usize = FIRST_RECENT_PROJECT + RECENT_PROJECT_COUNT - 1; + + fn open_folder(index: usize) -> OpenFolderEntry { + OpenFolderEntry { + worktree_id: WorktreeId::from_usize(index), + name: format!("project-folder-{index}").into(), + path: PathBuf::from(format!("/current/project-folder-{index}")), + branch: None, + is_active: false, + } + } + + fn project_group(index: usize) -> ProjectGroupKey { + ProjectGroupKey::new( + None, + PathList::new(&[PathBuf::from(format!("/this-window/project-{index}"))]), + ) + } + + fn recent_workspace(index: usize) -> RecentWorkspace { + let paths = PathList::new(&[PathBuf::from(format!("/recent/project-{index:02}"))]); + RecentWorkspace { + workspace_id: WorkspaceId::from_i64(index as i64), + location: SerializedWorkspaceLocation::Local, + paths: paths.clone(), + identity_paths: paths, + timestamp: Utc::now(), + } + } + + fn recent_workspaces() -> Vec { + (0..RECENT_PROJECT_COUNT).map(recent_workspace).collect() + } + + fn draw(cx: &mut VisualTestContext) { + cx.update(|window, cx| window.draw(cx).clear()); + } + + fn build_picker( + cx: &mut TestAppContext, + ) -> ( + Entity>, + &mut VisualTestContext, + ) { + init_test(cx); + let (picker, cx) = cx.add_window_view(|window, cx| { + let mut delegate = RecentProjectsDelegate::new( + WeakEntity::new_invalid(), + false, + cx.focus_handle(), + vec![open_folder(0), open_folder(1)], + vec![project_group(0), project_group(1)], + None, + ProjectPickerStyle::Modal, + ); + delegate.set_workspaces(recent_workspaces()); + Picker::list(delegate, window, cx) + .list_measure_all() + .show_scrollbar(true) + .max_height(Some(px(240.).into())) + }); + draw(cx); + (picker, cx) + } + + fn scroll_to_and_select( + picker: &Entity>, + cx: &mut VisualTestContext, + index: usize, + ) -> usize { + picker.update_in(cx, |picker, window, cx| { + picker.set_selected_index(index, None, true, window, cx); + }); + draw(cx); + picker.update(cx, |picker, _| picker.logical_scroll_top_index()) + } + + fn delete_recent_project_in_picker( + picker: &Entity>, + cx: &mut VisualTestContext, + index: usize, + ) { + picker.update_in(cx, |picker, window, cx| { + let Some(ProjectPickerEntry::RecentProject(hit)) = + picker.delegate.filtered_entries.get(index) + else { + panic!("expected entry at {index} to be a recent project"); + }; + let mut workspaces = picker.delegate.workspaces.clone(); + workspaces.remove(hit.candidate_id); + RecentProjectsDelegate::update_picker_after_recent_project_deletion( + picker, index, workspaces, window, cx, + ); + }); + } + + #[track_caller] + fn assert_scroll_top_is( + picker: &Entity>, + cx: &mut VisualTestContext, + expected: usize, + phase: &str, + ) { + picker.update(cx, |picker, _| { + assert_eq!( + picker.logical_scroll_top_index(), + expected, + "scroll top should remain at {expected} ({phase})" + ); + assert_selected_entry_is_recent_project(picker); + }); + } + + #[track_caller] + fn assert_pinned_to_bottom( + picker: &Entity>, + cx: &mut VisualTestContext, + phase: &str, + ) { + picker.update(cx, |picker, _| { + assert_eq!( + picker.is_scrolled_to_end(), + Some(true), + "picker should remain pinned to the bottom ({phase})" + ); + assert!( + picker.logical_scroll_top_index() > 0, + "picker should not jump to the top while pinned to the bottom ({phase})" + ); + assert_selected_entry_is_recent_project(picker); + }); + } + + #[track_caller] + fn assert_selected_entry_is_recent_project(picker: &Picker) { + assert!(matches!( + picker + .delegate + .filtered_entries + .get(picker.delegate.selected_index), + Some(ProjectPickerEntry::RecentProject(_)) + )); + } + + #[gpui::test] + fn deleting_top_recent_project_preserves_scroll_position(cx: &mut TestAppContext) { + let target = FIRST_RECENT_PROJECT; + let (picker, cx) = build_picker(cx); + let scroll_top = scroll_to_and_select(&picker, cx, target); + assert!( + scroll_top > 0, + "test should start scrolled away from the top" + ); + + delete_recent_project_in_picker(&picker, cx, target); + assert_scroll_top_is(&picker, cx, scroll_top, "after delete"); + + // The picker re-runs layout on the next frame; the scroll position + // must still be preserved after that redraw. + draw(cx); + assert_scroll_top_is(&picker, cx, scroll_top, "after redraw"); + } + + #[gpui::test] + fn deleting_middle_recent_project_preserves_scroll_position(cx: &mut TestAppContext) { + let target = FIRST_RECENT_PROJECT + RECENT_PROJECT_COUNT / 2; + let (picker, cx) = build_picker(cx); + let scroll_top = scroll_to_and_select(&picker, cx, target); + assert!( + scroll_top > 0, + "test should start scrolled away from the top" + ); + + delete_recent_project_in_picker(&picker, cx, target); + assert_scroll_top_is(&picker, cx, scroll_top, "after delete"); + + draw(cx); + assert_scroll_top_is(&picker, cx, scroll_top, "after redraw"); + } + + #[gpui::test] + fn deleting_last_recent_project_preserves_scroll_position(cx: &mut TestAppContext) { + let target = LAST_RECENT_PROJECT; + let (picker, cx) = build_picker(cx); + scroll_to_and_select(&picker, cx, target); + + picker.update(cx, |picker, _| { + assert_eq!( + picker.is_scrolled_to_end(), + Some(true), + "selecting the last entry should leave the picker pinned to the bottom" + ); + }); + + delete_recent_project_in_picker(&picker, cx, target); + assert_pinned_to_bottom(&picker, cx, "after delete"); + + draw(cx); + assert_pinned_to_bottom(&picker, cx, "after redraw"); + } + #[gpui::test] async fn test_open_dev_container_action_with_single_config(cx: &mut TestAppContext) { let app_state = init_test(cx);