João Soares created
Closes #50627
Before you mark this PR as ready for review, make sure that you have:
- [x] Added a solid test coverage and/or screenshots from doing manual
testing
- [x] Done a self-review taking into account security and performance
aspects
- [ ] Aligned any UI changes with the [UI
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
### Videos:
Before:
https://drive.google.com/file/d/1PahGhfx-wq9cyNqqMlctb9Np7M1Meo71/view?usp=sharing
After:
https://drive.google.com/file/d/135W6MQ9hKBurw5Z7YpHQQhoQwkh1NvUb/view?usp=sharing
Release Notes:
- Fixed clicking on non-selectable picker entries (e.g. section headers)
confirming the currently selected item.Cargo.lock | 4
crates/agent_ui/src/agent_configuration/tool_picker.rs | 7
crates/agent_ui/src/config_options.rs | 7
crates/agent_ui/src/language_model_selector.rs | 7
crates/agent_ui/src/model_selector.rs | 7
crates/agent_ui/src/profile_selector.rs | 7
crates/picker/Cargo.toml | 4
crates/picker/src/picker.rs | 177 +++++++++++
crates/recent_projects/src/recent_projects.rs | 7
crates/rules_library/src/rules_library.rs | 2
crates/sidebar/src/sidebar.rs | 7
11 files changed, 185 insertions(+), 51 deletions(-)
@@ -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",
@@ -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<Picker<Self>>,
- ) -> bool {
+ fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context<Picker<Self>>) -> bool {
let item = &self.filtered_items[ix];
match item {
PickerItem::Tool { .. } => true,
@@ -493,12 +493,7 @@ impl PickerDelegate for ConfigOptionPickerDelegate {
cx.notify();
}
- fn can_select(
- &mut self,
- ix: usize,
- _window: &mut Window,
- _cx: &mut Context<Picker<Self>>,
- ) -> bool {
+ fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context<Picker<Self>>) -> bool {
match self.filtered_entries.get(ix) {
Some(ConfigOptionPickerEntry::Option(_)) => true,
Some(ConfigOptionPickerEntry::Separator(_)) | None => false,
@@ -455,12 +455,7 @@ impl PickerDelegate for LanguageModelPickerDelegate {
cx.notify();
}
- fn can_select(
- &mut self,
- ix: usize,
- _window: &mut Window,
- _cx: &mut Context<Picker<Self>>,
- ) -> bool {
+ fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context<Picker<Self>>) -> bool {
match self.filtered_entries.get(ix) {
Some(LanguageModelPickerEntry::Model(_)) => true,
Some(LanguageModelPickerEntry::Separator(_)) | None => false,
@@ -212,12 +212,7 @@ impl PickerDelegate for ModelPickerDelegate {
cx.notify();
}
- fn can_select(
- &mut self,
- ix: usize,
- _window: &mut Window,
- _cx: &mut Context<Picker<Self>>,
- ) -> bool {
+ fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context<Picker<Self>>) -> bool {
match self.filtered_entries.get(ix) {
Some(ModelPickerEntry::Model(_, _)) => true,
Some(ModelPickerEntry::Separator(_)) | None => false,
@@ -443,12 +443,7 @@ impl PickerDelegate for ProfilePickerDelegate {
cx.notify();
}
- fn can_select(
- &mut self,
- ix: usize,
- _window: &mut Window,
- _cx: &mut Context<Picker<Self>>,
- ) -> bool {
+ fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context<Picker<Self>>) -> bool {
match self.filtered_entries.get(ix) {
Some(ProfilePickerEntry::Profile(_)) => true,
Some(ProfilePickerEntry::Header(_)) | None => false,
@@ -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
@@ -114,7 +114,7 @@ pub trait PickerDelegate: Sized + 'static {
None
}
fn can_select(
- &mut self,
+ &self,
_ix: usize,
_window: &mut Window,
_cx: &mut Context<Picker<Self>>,
@@ -619,6 +619,9 @@ impl<D: PickerDelegate> Picker<D> {
) {
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<D: PickerDelegate> Picker<D> {
ix: usize,
) -> impl IntoElement + use<D> {
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<D: PickerDelegate> Picker<D> {
}
}
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use gpui::TestAppContext;
+ use std::cell::Cell;
+
+ struct TestDelegate {
+ items: Vec<bool>,
+ selected_index: usize,
+ confirmed_index: Rc<Cell<Option<usize>>>,
+ }
+
+ impl TestDelegate {
+ fn new(items: Vec<bool>) -> 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<Picker<Self>>,
+ ) {
+ self.selected_index = ix;
+ }
+
+ fn can_select(
+ &self,
+ ix: usize,
+ _window: &mut Window,
+ _cx: &mut Context<Picker<Self>>,
+ ) -> bool {
+ self.items.get(ix).copied().unwrap_or(false)
+ }
+
+ fn placeholder_text(&self, _window: &mut Window, _cx: &mut App) -> Arc<str> {
+ "Test".into()
+ }
+
+ fn update_matches(
+ &mut self,
+ _query: String,
+ _window: &mut Window,
+ _cx: &mut Context<Picker<Self>>,
+ ) -> Task<()> {
+ Task::ready(())
+ }
+
+ fn confirm(
+ &mut self,
+ _secondary: bool,
+ _window: &mut Window,
+ _cx: &mut Context<Picker<Self>>,
+ ) {
+ self.confirmed_index.set(Some(self.selected_index));
+ }
+
+ fn dismissed(&mut self, _window: &mut Window, _cx: &mut Context<Picker<Self>>) {}
+
+ fn render_match(
+ &self,
+ ix: usize,
+ selected: bool,
+ _window: &mut Window,
+ _cx: &mut Context<Picker<Self>>,
+ ) -> Option<Self::ListItem> {
+ 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<D: PickerDelegate> EventEmitter<DismissEvent> for Picker<D> {}
impl<D: PickerDelegate> ModalView for Picker<D> {}
@@ -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<Picker<Self>>,
- ) -> bool {
+ fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context<Picker<Self>>) -> bool {
matches!(
self.filtered_entries.get(ix),
Some(ProjectPickerEntry::OpenFolder { .. } | ProjectPickerEntry::RecentProject(_))
@@ -222,7 +222,7 @@ impl PickerDelegate for RulePickerDelegate {
cx.notify();
}
- fn can_select(&mut self, ix: usize, _: &mut Window, _: &mut Context<Picker<Self>>) -> bool {
+ fn can_select(&self, ix: usize, _: &mut Window, _: &mut Context<Picker<Self>>) -> bool {
match self.filtered_entries.get(ix) {
Some(RulePickerEntry::Rule(_)) => true,
Some(RulePickerEntry::Header(_)) | Some(RulePickerEntry::Separator) | None => false,
@@ -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<Picker<Self>>,
- ) -> bool {
+ fn can_select(&self, ix: usize, _window: &mut Window, _cx: &mut Context<Picker<Self>>) -> bool {
match self.matches.get(ix) {
Some(SidebarMatch {
entry: SidebarEntry::Separator(_),