Wrap back around in context menu properly (#34112)

Kirill Bulatov created 4 months ago

When navigating back in the context menu, it was not possible to get
past first element, if it was not selectable.
The other way around works, hence the fix.

Release Notes:

- N/A

Change summary

crates/language_tools/src/lsp_tool.rs    |  2 
crates/ui/Cargo.toml                     |  3 
crates/ui/src/components/context_menu.rs | 89 ++++++++++++++++++++++---
3 files changed, 81 insertions(+), 13 deletions(-)

Detailed changes

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,

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"]

crates/ui/src/components/context_menu.rs

@@ -690,20 +690,15 @@ impl ContextMenu {
         cx: &mut Context<Self>,
     ) {
         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)"
+            );
+        });
+    }
+}