diff --git a/Cargo.lock b/Cargo.lock index d6630b7318c2e6566b80636b554417fb7df1cf22..ae4b1ba74f85a0ac8e0f5b081add26959c07eca9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12583,14 +12583,12 @@ name = "picker" version = "0.1.0" dependencies = [ "anyhow", - "ctor", "editor", - "env_logger 0.11.8", "gpui", "menu", "schemars", "serde", - "serde_json", + "settings", "theme", "ui", "ui_input", diff --git a/crates/agent_ui/src/agent_configuration/tool_picker.rs b/crates/agent_ui/src/agent_configuration/tool_picker.rs index 1c99f665ab1c8fc995d47682f92365852bbc9637..be6fcb5bd2b5eeb4d33f4aaefc31cfeb4a978564 100644 --- a/crates/agent_ui/src/agent_configuration/tool_picker.rs +++ b/crates/agent_ui/src/agent_configuration/tool_picker.rs @@ -172,12 +172,7 @@ impl PickerDelegate for ToolPickerDelegate { self.selected_index = ix; } - fn can_select( - &mut self, - ix: usize, - _window: &mut Window, - _cx: &mut Context>, - ) -> bool { + fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context>) -> bool { let item = &self.filtered_items[ix]; match item { PickerItem::Tool { .. } => true, diff --git a/crates/agent_ui/src/config_options.rs b/crates/agent_ui/src/config_options.rs index 458411d4d3af3f1c85dc57a1e940515e8aabb23a..6ec2595202490ca7474717f8985b6e4f6d7ca0b9 100644 --- a/crates/agent_ui/src/config_options.rs +++ b/crates/agent_ui/src/config_options.rs @@ -493,12 +493,7 @@ impl PickerDelegate for ConfigOptionPickerDelegate { cx.notify(); } - fn can_select( - &mut self, - ix: usize, - _window: &mut Window, - _cx: &mut Context>, - ) -> bool { + fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context>) -> bool { match self.filtered_entries.get(ix) { Some(ConfigOptionPickerEntry::Option(_)) => true, Some(ConfigOptionPickerEntry::Separator(_)) | None => false, diff --git a/crates/agent_ui/src/language_model_selector.rs b/crates/agent_ui/src/language_model_selector.rs index 9205e21be1ab796fae50a26d31aca514756e2bc2..e6e72b3197b4108d7b423470bf8bb4b75cd055b7 100644 --- a/crates/agent_ui/src/language_model_selector.rs +++ b/crates/agent_ui/src/language_model_selector.rs @@ -455,12 +455,7 @@ impl PickerDelegate for LanguageModelPickerDelegate { cx.notify(); } - fn can_select( - &mut self, - ix: usize, - _window: &mut Window, - _cx: &mut Context>, - ) -> bool { + fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context>) -> bool { match self.filtered_entries.get(ix) { Some(LanguageModelPickerEntry::Model(_)) => true, Some(LanguageModelPickerEntry::Separator(_)) | None => false, diff --git a/crates/agent_ui/src/model_selector.rs b/crates/agent_ui/src/model_selector.rs index 307eda507410a060f551741a998779c44b303b60..89ed3e490b33ca83cbdab25cfce77fee7cf9ccb6 100644 --- a/crates/agent_ui/src/model_selector.rs +++ b/crates/agent_ui/src/model_selector.rs @@ -212,12 +212,7 @@ impl PickerDelegate for ModelPickerDelegate { cx.notify(); } - fn can_select( - &mut self, - ix: usize, - _window: &mut Window, - _cx: &mut Context>, - ) -> bool { + fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context>) -> bool { match self.filtered_entries.get(ix) { Some(ModelPickerEntry::Model(_, _)) => true, Some(ModelPickerEntry::Separator(_)) | None => false, diff --git a/crates/agent_ui/src/profile_selector.rs b/crates/agent_ui/src/profile_selector.rs index 45d7232e0dff8b2ab1056b522b5994e11236d843..926549c22f88bcb0937dddf7c3ff1b32060ed297 100644 --- a/crates/agent_ui/src/profile_selector.rs +++ b/crates/agent_ui/src/profile_selector.rs @@ -443,12 +443,7 @@ impl PickerDelegate for ProfilePickerDelegate { cx.notify(); } - fn can_select( - &mut self, - ix: usize, - _window: &mut Window, - _cx: &mut Context>, - ) -> bool { + fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context>) -> bool { match self.filtered_entries.get(ix) { Some(ProfilePickerEntry::Profile(_)) => true, Some(ProfilePickerEntry::Header(_)) | None => false, diff --git a/crates/picker/Cargo.toml b/crates/picker/Cargo.toml index f85c55b9f27bcb8fd87101c341058e1a3962934e..8c76aa746453866755be322df576a519ba147b24 100644 --- a/crates/picker/Cargo.toml +++ b/crates/picker/Cargo.toml @@ -28,8 +28,6 @@ workspace.workspace = true zed_actions.workspace = true [dev-dependencies] -ctor.workspace = true editor = { workspace = true, features = ["test-support"] } -env_logger.workspace = true gpui = { workspace = true, features = ["test-support"] } -serde_json.workspace = true +settings.workspace = true diff --git a/crates/picker/src/picker.rs b/crates/picker/src/picker.rs index 716653d89642fe6d8f457f145ed15b8972432a09..e87ec3415cf6d70d840d8566accb94ac6de1547c 100644 --- a/crates/picker/src/picker.rs +++ b/crates/picker/src/picker.rs @@ -114,7 +114,7 @@ pub trait PickerDelegate: Sized + 'static { None } fn can_select( - &mut self, + &self, _ix: usize, _window: &mut Window, _cx: &mut Context>, @@ -619,6 +619,9 @@ impl Picker { ) { cx.stop_propagation(); window.prevent_default(); + if !self.delegate.can_select(ix, window, cx) { + return; + } self.set_selected_index(ix, None, false, window, cx); self.do_confirm(secondary, window, cx) } @@ -753,10 +756,11 @@ impl Picker { ix: usize, ) -> impl IntoElement + use { let item_bounds = self.item_bounds.clone(); + let selectable = self.delegate.can_select(ix, window, cx); div() .id(("item", ix)) - .cursor_pointer() + .when(selectable, |this| this.cursor_pointer()) .child( canvas( move |bounds, _window, _cx| { @@ -850,6 +854,175 @@ impl Picker { } } +#[cfg(test)] +mod tests { + use super::*; + use gpui::TestAppContext; + use std::cell::Cell; + + struct TestDelegate { + items: Vec, + selected_index: usize, + confirmed_index: Rc>>, + } + + impl TestDelegate { + fn new(items: Vec) -> Self { + Self { + items, + selected_index: 0, + confirmed_index: Rc::new(Cell::new(None)), + } + } + } + + impl PickerDelegate for TestDelegate { + type ListItem = ui::ListItem; + + fn match_count(&self) -> usize { + self.items.len() + } + + fn selected_index(&self) -> usize { + self.selected_index + } + + fn set_selected_index( + &mut self, + ix: usize, + _window: &mut Window, + _cx: &mut Context>, + ) { + self.selected_index = ix; + } + + fn can_select( + &self, + ix: usize, + _window: &mut Window, + _cx: &mut Context>, + ) -> bool { + self.items.get(ix).copied().unwrap_or(false) + } + + fn placeholder_text(&self, _window: &mut Window, _cx: &mut App) -> Arc { + "Test".into() + } + + fn update_matches( + &mut self, + _query: String, + _window: &mut Window, + _cx: &mut Context>, + ) -> Task<()> { + Task::ready(()) + } + + fn confirm( + &mut self, + _secondary: bool, + _window: &mut Window, + _cx: &mut Context>, + ) { + self.confirmed_index.set(Some(self.selected_index)); + } + + fn dismissed(&mut self, _window: &mut Window, _cx: &mut Context>) {} + + fn render_match( + &self, + ix: usize, + selected: bool, + _window: &mut Window, + _cx: &mut Context>, + ) -> Option { + Some( + ui::ListItem::new(ix) + .inset(true) + .toggle_state(selected) + .child(ui::Label::new(format!("Item {ix}"))), + ) + } + } + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let store = settings::SettingsStore::test(cx); + cx.set_global(store); + theme::init(theme::LoadThemes::JustBase, cx); + editor::init(cx); + }); + } + + #[gpui::test] + async fn test_clicking_non_selectable_item_does_not_confirm(cx: &mut TestAppContext) { + init_test(cx); + + let confirmed_index = Rc::new(Cell::new(None)); + let (picker, cx) = cx.add_window_view(|window, cx| { + let mut delegate = TestDelegate::new(vec![true, false, true]); + delegate.confirmed_index = confirmed_index.clone(); + Picker::uniform_list(delegate, window, cx) + }); + + picker.update(cx, |picker, _cx| { + assert_eq!(picker.delegate.selected_index(), 0); + }); + + picker.update_in(cx, |picker, window, cx| { + picker.handle_click(1, false, window, cx); + }); + assert!( + confirmed_index.get().is_none(), + "clicking a non-selectable item should not confirm" + ); + + picker.update_in(cx, |picker, window, cx| { + picker.handle_click(0, false, window, cx); + }); + assert_eq!( + confirmed_index.get(), + Some(0), + "clicking a selectable item should confirm" + ); + } + + #[gpui::test] + async fn test_keyboard_navigation_skips_non_selectable_items(cx: &mut TestAppContext) { + init_test(cx); + + let (picker, cx) = cx.add_window_view(|window, cx| { + Picker::uniform_list(TestDelegate::new(vec![true, false, true]), window, cx) + }); + + picker.update(cx, |picker, _cx| { + assert_eq!(picker.delegate.selected_index(), 0); + }); + + picker.update_in(cx, |picker, window, cx| { + picker.select_next(&menu::SelectNext, window, cx); + }); + picker.update(cx, |picker, _cx| { + assert_eq!( + picker.delegate.selected_index(), + 2, + "select_next should skip non-selectable item at index 1" + ); + }); + + picker.update_in(cx, |picker, window, cx| { + picker.select_previous(&menu::SelectPrevious, window, cx); + }); + picker.update(cx, |picker, _cx| { + assert_eq!( + picker.delegate.selected_index(), + 0, + "select_previous should skip non-selectable item at index 1" + ); + }); + } +} + impl EventEmitter for Picker {} impl ModalView for Picker {} diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 110a702437d463d6f296510c8f4a3a68d28d7d60..e7a8358da6f8daa1778837073129e70b04ee2317 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -750,12 +750,7 @@ impl PickerDelegate for RecentProjectsDelegate { self.selected_index = ix; } - fn can_select( - &mut self, - ix: usize, - _window: &mut Window, - _cx: &mut Context>, - ) -> bool { + fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context>) -> bool { matches!( self.filtered_entries.get(ix), Some(ProjectPickerEntry::OpenFolder { .. } | ProjectPickerEntry::RecentProject(_)) diff --git a/crates/rules_library/src/rules_library.rs b/crates/rules_library/src/rules_library.rs index a89657e29680ccfd759fe63efcc837d883ef7590..73bf5fdd8fcaaf1437013d300102a9e593823c7b 100644 --- a/crates/rules_library/src/rules_library.rs +++ b/crates/rules_library/src/rules_library.rs @@ -222,7 +222,7 @@ impl PickerDelegate for RulePickerDelegate { cx.notify(); } - fn can_select(&mut self, ix: usize, _: &mut Window, _: &mut Context>) -> bool { + fn can_select(&self, ix: usize, _: &mut Window, _: &mut Context>) -> bool { match self.filtered_entries.get(ix) { Some(RulePickerEntry::Rule(_)) => true, Some(RulePickerEntry::Header(_)) | Some(RulePickerEntry::Separator) | None => false, diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index 24974512cda12276b5fcdc51ebd71d091782dff6..e8fc3876e29aae6d7a3490bd7a80245c8c3bab1c 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -386,12 +386,7 @@ impl PickerDelegate for WorkspacePickerDelegate { self.selected_index = ix; } - fn can_select( - &mut self, - ix: usize, - _window: &mut Window, - _cx: &mut Context>, - ) -> bool { + fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context>) -> bool { match self.matches.get(ix) { Some(SidebarMatch { entry: SidebarEntry::Separator(_),