diff --git a/crates/language_tools/src/lsp_tool.rs b/crates/language_tools/src/lsp_tool.rs index 81cc38d33f..9e542f582a 100644 --- a/crates/language_tools/src/lsp_tool.rs +++ b/crates/language_tools/src/lsp_tool.rs @@ -735,8 +735,6 @@ impl LspTool { state.update(cx, |state, cx| state.fill_menu(menu, cx)) }); lsp_tool.lsp_menu = Some(menu.clone()); - // TODO kb will this work? - // what about the selections? lsp_tool.popover_menu_handle.refresh_menu( window, cx, diff --git a/crates/ui/Cargo.toml b/crates/ui/Cargo.toml index 625bdc62f5..c047291772 100644 --- a/crates/ui/Cargo.toml +++ b/crates/ui/Cargo.toml @@ -34,6 +34,9 @@ workspace-hack.workspace = true [target.'cfg(windows)'.dependencies] windows.workspace = true +[dev-dependencies] +gpui = { workspace = true, features = ["test-support"] } + [features] default = [] stories = ["dep:story"] diff --git a/crates/ui/src/components/context_menu.rs b/crates/ui/src/components/context_menu.rs index 075cf7a7d7..873b7b9e63 100644 --- a/crates/ui/src/components/context_menu.rs +++ b/crates/ui/src/components/context_menu.rs @@ -690,20 +690,15 @@ impl ContextMenu { cx: &mut Context, ) { if let Some(ix) = self.selected_index { - if ix == 0 { - self.handle_select_last(&SelectLast, window, cx); - } else { - for (ix, item) in self.items.iter().enumerate().take(ix).rev() { - if item.is_selectable() { - self.select_index(ix, window, cx); - cx.notify(); - break; - } + for (ix, item) in self.items.iter().enumerate().take(ix).rev() { + if item.is_selectable() { + self.select_index(ix, window, cx); + cx.notify(); + return; } } - } else { - self.handle_select_last(&SelectLast, window, cx); } + self.handle_select_last(&SelectLast, window, cx); } fn select_index( @@ -1171,3 +1166,75 @@ impl Render for ContextMenu { }))) } } + +#[cfg(test)] +mod tests { + use gpui::TestAppContext; + + use super::*; + + #[gpui::test] + fn can_navigate_back_over_headers(cx: &mut TestAppContext) { + let cx = cx.add_empty_window(); + let context_menu = cx.update(|window, cx| { + ContextMenu::build(window, cx, |menu, _, _| { + menu.header("First header") + .separator() + .entry("First entry", None, |_, _| {}) + .separator() + .separator() + .entry("Last entry", None, |_, _| {}) + }) + }); + + context_menu.update_in(cx, |context_menu, window, cx| { + assert_eq!( + None, context_menu.selected_index, + "No selection is in the menu initially" + ); + + context_menu.select_first(&SelectFirst, window, cx); + assert_eq!( + Some(2), + context_menu.selected_index, + "Should select first selectable entry, skipping the header and the separator" + ); + + context_menu.select_next(&SelectNext, window, cx); + assert_eq!( + Some(5), + context_menu.selected_index, + "Should select next selectable entry, skipping 2 separators along the way" + ); + + context_menu.select_next(&SelectNext, window, cx); + assert_eq!( + Some(2), + context_menu.selected_index, + "Should wrap around to first selectable entry" + ); + }); + + context_menu.update_in(cx, |context_menu, window, cx| { + assert_eq!( + Some(2), + context_menu.selected_index, + "Should start from the first selectable entry" + ); + + context_menu.select_previous(&SelectPrevious, window, cx); + assert_eq!( + Some(5), + context_menu.selected_index, + "Should wrap around to previous selectable entry (last)" + ); + + context_menu.select_previous(&SelectPrevious, window, cx); + assert_eq!( + Some(2), + context_menu.selected_index, + "Should go back to previous selectable entry (first)" + ); + }); + } +}