menu: Add item and export PopupMenuItem with builder methods to PopupMenu. (#1445)

Now we can be easy to add a menu item without define `action`, just use
`on_click` callback.

## Break Changes

- Removed complex methods: `menu_element_with_check_and_disabled`,
`menu_element_with_icon_and_disabled`, you can use `item` and
`PopupMenuItem` instead.

crates/story/src/menu_story.rs

@@ -3,8 +3,12 @@ use gpui::{
     ParentElement as _, Render, SharedString, Styled as _, Window, actions, div, px,
 };
 use gpui_component::{
-    ActiveTheme as _, IconName, button::Button, context_menu::ContextMenuExt, h_flex,
+    ActiveTheme as _, IconName,
-    popup_menu::PopupMenuExt as _, v_flex,
+    button::Button,
+    context_menu::ContextMenuExt,
+    h_flex,
+    popup_menu::{PopupMenuExt as _, PopupMenuItem},
+    v_flex,
 };
 use serde::Deserialize;
 
@@ -104,6 +108,7 @@ impl MenuStory {
 impl Render for MenuStory {
     fn render(&mut self, _: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
         let checked = self.checked;
+        let view = cx.entity();
 
         v_flex()
             .key_context(CONTEXT)
@@ -124,6 +129,14 @@ impl Render for MenuStory {
                             .label("Edit")
                             .popup_menu(move |this, window, cx| {
                                 this.link("About", "https://github.com/longbridge/gpui-component")
+                                    .separator()
+                                    .item(PopupMenuItem::new("Handle Click").on_click(
+                                        window.listener_for(&view, |this, _, _, cx| {
+                                            this.message =
+                                                "You have clicked Handle Click".to_string();
+                                            cx.notify();
+                                        }),
+                                    ))
                                     .separator()
                                     .menu("Copy", Box::new(Copy))
                                     .menu("Cut", Box::new(Cut))
@@ -133,14 +146,23 @@ impl Render for MenuStory {
                                     .separator()
                                     .menu_with_icon("Search", IconName::Search, Box::new(SearchAll))
                                     .separator()
-                                    .menu_element(Box::new(Info(0)), |_, cx| {
+                                    .item(
-                                        v_flex().child("Custom Element").child(
+                                        PopupMenuItem::element(|_, cx| {
-                                            div()
+                                            v_flex().child("Custom Element").child(
-                                                .text_xs()
+                                                div()
-                                                .text_color(cx.theme().muted_foreground)
+                                                    .text_xs()
-                                                .child("THis is sub-title"),
+                                                    .text_color(cx.theme().muted_foreground)
-                                        )
+                                                    .child("This is sub-title"),
-                                    })
+                                            )
+                                        })
+                                        .on_click(
+                                            window.listener_for(&view, |this, _, _, cx| {
+                                                this.message = "You have clicked on custom element"
+                                                    .to_string();
+                                                cx.notify();
+                                            }),
+                                        ),
+                                    )
                                     .menu_element_with_check(checked, Box::new(Info(0)), |_, cx| {
                                         h_flex().gap_1().child("Custom Element").child(
                                             div()

crates/ui/src/menu/popup_menu.rs

@@ -13,7 +13,7 @@ use gpui::{
     InteractiveElement, IntoElement, KeyBinding, ParentElement, Pixels, Render, ScrollHandle,
     SharedString, StatefulInteractiveElement, Styled, WeakEntity, Window,
 };
-use gpui::{Half, MouseDownEvent, OwnedMenuItem, Subscription};
+use gpui::{ClickEvent, Half, MouseDownEvent, OwnedMenuItem, Subscription};
 use std::rc::Rc;
 
 const CONTEXT: &str = "PopupMenu";
@@ -59,9 +59,12 @@ pub trait PopupMenuExt: Styled + Selectable + InteractiveElement + IntoElement +
 }
 impl PopupMenuExt for Button {}
 
-pub(crate) enum PopupMenuItem {
+/// An menu item in a popup menu.
+pub enum PopupMenuItem {
     Separator,
+    /// A non-interactive label item.
     Label(SharedString),
+    /// A standard menu item.
     Item {
         icon: Option<Icon>,
         label: SharedString,
@@ -69,15 +72,19 @@ pub(crate) enum PopupMenuItem {
         is_link: bool,
         action: Option<Box<dyn Action>>,
         // For link item
-        handler: Option<Rc<dyn Fn(&mut Window, &mut App)>>,
+        handler: Option<Rc<dyn Fn(&ClickEvent, &mut Window, &mut App)>>,
     },
+    /// A menu item with custom element render.
     ElementItem {
         icon: Option<Icon>,
         disabled: bool,
-        action: Box<dyn Action>,
+        action: Option<Box<dyn Action>>,
         render: Box<dyn Fn(&mut Window, &mut App) -> AnyElement + 'static>,
-        handler: Option<Rc<dyn Fn(&mut Window, &mut App)>>,
+        handler: Option<Rc<dyn Fn(&ClickEvent, &mut Window, &mut App)>>,
     },
+    /// A submenu item that opens another popup menu.
+    ///
+    /// NOTE: This is only supported when the parent menu is not `scrollable`.
     Submenu {
         icon: Option<Icon>,
         label: SharedString,
@@ -86,7 +93,147 @@ pub(crate) enum PopupMenuItem {
     },
 }
 
+impl FluentBuilder for PopupMenuItem {}
 impl PopupMenuItem {
+    /// Create a new menu item with the given label.
+    #[inline]
+    pub fn new(label: impl Into<SharedString>) -> Self {
+        PopupMenuItem::Item {
+            icon: None,
+            label: label.into(),
+            disabled: false,
+            action: None,
+            is_link: false,
+            handler: None,
+        }
+    }
+
+    /// Create a new menu item with custom element render.
+    #[inline]
+    pub fn element<F, E>(builder: F) -> Self
+    where
+        F: Fn(&mut Window, &mut App) -> E + 'static,
+        E: IntoElement,
+    {
+        PopupMenuItem::ElementItem {
+            icon: None,
+            disabled: false,
+            action: None,
+            render: Box::new(move |window, cx| builder(window, cx).into_any_element()),
+            handler: None,
+        }
+    }
+
+    /// Create a new submenu item that opens another popup menu.
+    #[inline]
+    pub fn submenu(label: impl Into<SharedString>, menu: Entity<PopupMenu>) -> Self {
+        PopupMenuItem::Submenu {
+            icon: None,
+            label: label.into(),
+            disabled: false,
+            menu,
+        }
+    }
+
+    /// Create a separator menu item.
+    #[inline]
+    pub fn separator() -> Self {
+        PopupMenuItem::Separator
+    }
+
+    /// Creates a label menu item.
+    #[inline]
+    pub fn label(label: impl Into<SharedString>) -> Self {
+        PopupMenuItem::Label(label.into())
+    }
+
+    /// Set the icon for the menu item.
+    ///
+    /// Only works for [`PopupMenuItem::Item`], [`PopupMenuItem::ElementItem`] and [`PopupMenuItem::Submenu`].
+    pub fn icon(mut self, icon: impl Into<Icon>) -> Self {
+        match &mut self {
+            PopupMenuItem::Item { icon: i, .. } => {
+                *i = Some(icon.into());
+            }
+            PopupMenuItem::ElementItem { icon: i, .. } => {
+                *i = Some(icon.into());
+            }
+            PopupMenuItem::Submenu { icon: i, .. } => {
+                *i = Some(icon.into());
+            }
+            _ => {}
+        }
+        self
+    }
+
+    /// Set the action for the menu item.
+    ///
+    /// Only works for [`PopupMenuItem::Item`] and [`PopupMenuItem::ElementItem`].
+    pub fn action(mut self, action: Box<dyn Action>) -> Self {
+        match &mut self {
+            PopupMenuItem::Item { action: a, .. } => {
+                *a = Some(action);
+            }
+            PopupMenuItem::ElementItem { action: a, .. } => {
+                *a = Some(action);
+            }
+            _ => {}
+        }
+        self
+    }
+
+    /// Set the disabled state for the menu item.
+    ///
+    /// Only works for [`PopupMenuItem::Item`], [`PopupMenuItem::ElementItem`] and [`PopupMenuItem::Submenu`].
+    pub fn disabled(mut self, disabled: bool) -> Self {
+        match &mut self {
+            PopupMenuItem::Item { disabled: d, .. } => {
+                *d = disabled;
+            }
+            PopupMenuItem::ElementItem { disabled: d, .. } => {
+                *d = disabled;
+            }
+            PopupMenuItem::Submenu { disabled: d, .. } => {
+                *d = disabled;
+            }
+            _ => {}
+        }
+        self
+    }
+
+    /// Add a click handler for the menu item.
+    ///
+    /// Only works for [`PopupMenuItem::Item`] and [`PopupMenuItem::ElementItem`].
+    pub fn on_click<F>(mut self, handler: F) -> Self
+    where
+        F: Fn(&ClickEvent, &mut Window, &mut App) + 'static,
+    {
+        match &mut self {
+            PopupMenuItem::Item { handler: h, .. } => {
+                *h = Some(Rc::new(handler));
+            }
+            PopupMenuItem::ElementItem { handler: h, .. } => {
+                *h = Some(Rc::new(handler));
+            }
+            _ => {}
+        }
+        self
+    }
+
+    /// Create a link menu item.
+    #[inline]
+    pub fn link(label: impl Into<SharedString>, href: impl Into<String>) -> Self {
+        let href = href.into();
+        PopupMenuItem::Item {
+            icon: None,
+            label: label.into(),
+            disabled: false,
+            action: None,
+            is_link: true,
+            handler: Some(Rc::new(move |_, _, cx| cx.open_url(&href))),
+        }
+    }
+
     #[inline]
     fn is_clickable(&self) -> bool {
         !matches!(self, PopupMenuItem::Separator)
@@ -238,7 +385,7 @@ impl PopupMenu {
 
     /// Add label
     pub fn label(mut self, label: impl Into<SharedString>) -> Self {
-        self.menu_items.push(PopupMenuItem::Label(label.into()));
+        self.menu_items.push(PopupMenuItem::label(label.into()));
         self
     }
 
@@ -255,14 +402,8 @@ impl PopupMenu {
         disabled: bool,
     ) -> Self {
         let href = href.into();
-        self.menu_items.push(PopupMenuItem::Item {
+        self.menu_items
-            icon: None,
+            .push(PopupMenuItem::link(label, href).disabled(disabled));
-            label: label.into(),
-            disabled,
-            action: None,
-            is_link: true,
-            handler: Some(Rc::new(move |_, cx| cx.open_url(&href))),
-        });
         self
     }
 
@@ -277,7 +418,7 @@ impl PopupMenu {
     }
 
     /// Add Menu to open link with icon and disabled state
-    pub fn link_with_icon_and_disabled(
+    fn link_with_icon_and_disabled(
         mut self,
         label: impl Into<SharedString>,
         icon: impl Into<Icon>,
@@ -285,14 +426,11 @@ impl PopupMenu {
         disabled: bool,
     ) -> Self {
         let href = href.into();
-        self.menu_items.push(PopupMenuItem::Item {
+        self.menu_items.push(
-            icon: Some(icon.into()),
+            PopupMenuItem::link(label, href)
-            label: label.into(),
+                .icon(icon)
-            disabled,
+                .disabled(disabled),
-            action: None,
+        );
-            is_link: true,
-            handler: Some(Rc::new(move |_, cx| cx.open_url(&href))),
-        });
         self
     }
 
@@ -382,29 +520,6 @@ impl PopupMenu {
         self.menu_element_with_icon_and_disabled(icon, action, false, builder)
     }
 
-    /// Add Menu Item with custom element render with icon and disabled state
-    pub fn menu_element_with_icon_and_disabled<F, E>(
-        mut self,
-        icon: impl Into<Icon>,
-        action: Box<dyn Action>,
-        disabled: bool,
-        builder: F,
-    ) -> Self
-    where
-        F: Fn(&mut Window, &mut App) -> E + 'static,
-        E: IntoElement,
-    {
-        self.menu_items.push(PopupMenuItem::ElementItem {
-            render: Box::new(move |window, cx| builder(window, cx).into_any_element()),
-            action,
-            icon: Some(icon.into()),
-            disabled,
-            handler: None,
-        });
-        self.has_icon = true;
-        self
-    }
-
     /// Add Menu Item with custom element render with check state
     pub fn menu_element_with_check<F, E>(
         self,
@@ -419,8 +534,30 @@ impl PopupMenu {
         self.menu_element_with_check_and_disabled(checked, action, false, builder)
     }
 
+    /// Add Menu Item with custom element render with icon and disabled state
+    fn menu_element_with_icon_and_disabled<F, E>(
+        mut self,
+        icon: impl Into<Icon>,
+        action: Box<dyn Action>,
+        disabled: bool,
+        builder: F,
+    ) -> Self
+    where
+        F: Fn(&mut Window, &mut App) -> E + 'static,
+        E: IntoElement,
+    {
+        self.menu_items.push(
+            PopupMenuItem::element(builder)
+                .action(action)
+                .icon(icon)
+                .disabled(disabled),
+        );
+        self.has_icon = true;
+        self
+    }
+
     /// Add Menu Item with custom element render with check state and disabled state
-    pub fn menu_element_with_check_and_disabled<F, E>(
+    fn menu_element_with_check_and_disabled<F, E>(
         mut self,
         checked: bool,
         action: Box<dyn Action>,
@@ -431,30 +568,13 @@ impl PopupMenu {
         F: Fn(&mut Window, &mut App) -> E + 'static,
         E: IntoElement,
     {
-        if checked {
+        self.menu_items.push(
-            self.menu_items.push(PopupMenuItem::ElementItem {
+            PopupMenuItem::element(builder)
-                render: Box::new(move |window, cx| builder(window, cx).into_any_element()),
+                .action(action)
-                action,
+                .when(checked, |item| item.icon(IconName::Check))
-                handler: None,
+                .disabled(disabled),
-                icon: Some(IconName::Check.into()),
+        );
-                disabled,
+        self.has_icon = self.has_icon || checked;
-            });
-            self.has_icon = true;
-        } else {
-            self.menu_items.push(PopupMenuItem::ElementItem {
-                render: Box::new(move |window, cx| builder(window, cx).into_any_element()),
-                action,
-                handler: None,
-                icon: None,
-                disabled,
-            });
-        }
-        self
-    }
-
-    /// Use small size, the menu item will have smaller height.
-    pub(crate) fn small(mut self) -> Self {
-        self.size = Size::Small;
         self
     }
 
@@ -468,7 +588,7 @@ impl PopupMenu {
             return self;
         }
 
-        self.menu_items.push(PopupMenuItem::Separator);
+        self.menu_items.push(PopupMenuItem::separator());
         self
     }
 
@@ -483,36 +603,11 @@ impl PopupMenu {
         self.submenu_with_icon(None, label, window, cx, f)
     }
 
-    /// Add a Submenu item with disabled state
-    pub fn submenu_with_disabled(
-        self,
-        label: impl Into<SharedString>,
-        disabled: bool,
-        window: &mut Window,
-        cx: &mut Context<Self>,
-        f: impl Fn(PopupMenu, &mut Window, &mut Context<PopupMenu>) -> PopupMenu + 'static,
-    ) -> Self {
-        self.submenu_with_icon_with_disabled(None, label, disabled, window, cx, f)
-    }
-
     /// Add a Submenu item with icon
     pub fn submenu_with_icon(
-        self,
-        icon: Option<Icon>,
-        label: impl Into<SharedString>,
-        window: &mut Window,
-        cx: &mut Context<Self>,
-        f: impl Fn(PopupMenu, &mut Window, &mut Context<PopupMenu>) -> PopupMenu + 'static,
-    ) -> Self {
-        self.submenu_with_icon_with_disabled(icon, label, false, window, cx, f)
-    }
-
-    /// Add a Submenu item with icon and disabled state
-    pub fn submenu_with_icon_with_disabled(
         mut self,
         icon: Option<Icon>,
         label: impl Into<SharedString>,
-        disabled: bool,
         window: &mut Window,
         cx: &mut Context<Self>,
         f: impl Fn(PopupMenu, &mut Window, &mut Context<PopupMenu>) -> PopupMenu + 'static,
@@ -523,12 +618,21 @@ impl PopupMenu {
             view.parent_menu = Some(parent_menu);
         });
 
-        self.menu_items.push(PopupMenuItem::Submenu {
+        self.menu_items.push(
-            icon,
+            PopupMenuItem::submenu(label, submenu).when_some(icon, |this, icon| this.icon(icon)),
-            label: label.into(),
+        );
-            menu: submenu,
+        self
-            disabled,
+    }
-        });
+
+    /// Add menu item.
+    pub fn item(mut self, item: impl Into<PopupMenuItem>) -> Self {
+        self.menu_items.push(item.into());
+        self
+    }
+
+    /// Use small size, the menu item will have smaller height.
+    pub(crate) fn small(mut self) -> Self {
+        self.size = Size::Small;
         self
     }
 
@@ -543,14 +647,12 @@ impl PopupMenu {
             self.has_icon = true;
         }
 
-        self.menu_items.push(PopupMenuItem::Item {
+        self.menu_items.push(
-            icon,
+            PopupMenuItem::new(label)
-            label: label.into(),
+                .when_some(icon, |item, icon| item.icon(icon))
-            disabled,
+                .disabled(disabled)
-            action: Some(action.boxed_clone()),
+                .action(action),
-            is_link: false,
+        );
-            handler: None,
-        });
         self
     }
 
@@ -627,7 +729,7 @@ impl PopupMenu {
                         handler, action, ..
                     }) => {
                         if let Some(handler) = handler {
-                            handler(window, cx);
+                            handler(&ClickEvent::default(), window, cx);
                         } else if let Some(action) = action.as_ref() {
                             self.dispatch_confirm_action(action, window, cx);
                         }
@@ -638,8 +740,8 @@ impl PopupMenu {
                         handler, action, ..
                     }) => {
                         if let Some(handler) = handler {
-                            handler(window, cx);
+                            handler(&ClickEvent::default(), window, cx);
-                        } else {
+                        } else if let Some(action) = action.as_ref() {
                             self.dispatch_confirm_action(action, window, cx);
                         }
                         self.dismiss(&Cancel, window, cx)

docs/docs/components/popup_menu.md

@@ -244,6 +244,10 @@ menu.menu_element(Box::new(CustomAction), |window, cx| {
 
 ### Scrollable Menus
 
+:::warning
+When you have enabled `scrollable()` on a menu, avoid using submenus within it, as this can lead to usability issues.
+:::
+
 For menus with many items, enable scrolling:
 
 ```rust
@@ -290,59 +294,9 @@ menu.action_context(focus_handle)
 
 ## API Reference
 
-### PopupMenu
+- [PopupMenu]
-
+- [context_menu]
-| Method                     | Description                                   |
+- [PopupMenuItem]
-| -------------------------- | --------------------------------------------- |
-| `build(window, cx, f)`     | Create a new popup menu with builder function |
-| `action_context(handle)`   | Set focus handle for action dispatch          |
-| `min_w(width)`             | Set minimum width                             |
-| `max_w(width)`             | Set maximum width                             |
-| `max_h(height)`            | Set maximum height                            |
-| `scrollable()`             | Enable vertical scrolling                     |
-| `external_link_icon(bool)` | Show/hide external link icons                 |
-
-### Menu Items
-
-| Method                                        | Description                       |
-| --------------------------------------------- | --------------------------------- |
-| `menu(label, action)`                         | Add basic menu item               |
-| `menu_with_disabled(label, action, disabled)` | Add menu item with disabled state |
-| `menu_with_icon(label, icon, action)`         | Add menu item with icon           |
-| `menu_with_check(label, checked, action)`     | Add checkable menu item           |
-| `label(text)`                                 | Add non-interactive label         |
-| `separator()`                                 | Add visual separator              |
-| `link(label, url)`                            | Add link menu item                |
-| `link_with_icon(label, icon, url)`            | Add link with icon                |
-
-### Custom Elements
-
-| Method                                              | Description                         |
-| --------------------------------------------------- | ----------------------------------- |
-| `menu_element(action, builder)`                     | Add custom element menu item        |
-| `menu_element_with_icon(icon, action, builder)`     | Add custom element with icon        |
-| `menu_element_with_check(checked, action, builder)` | Add custom element with check state |
-
-### Submenus
-
-| Method                                                        | Description                     |
-| ------------------------------------------------------------- | ------------------------------- |
-| `submenu(label, window, cx, builder)`                         | Add submenu                     |
-| `submenu_with_icon(icon, label, window, cx, builder)`         | Add submenu with icon           |
-| `submenu_with_disabled(label, disabled, window, cx, builder)` | Add submenu with disabled state |
-
-### PopupMenuExt Trait
-
-| Method                                    | Description                                |
-| ----------------------------------------- | ------------------------------------------ |
-| `popup_menu(builder)`                     | Add popup menu to button (top-left anchor) |
-| `popup_menu_with_anchor(corner, builder)` | Add popup menu with custom anchor          |
-
-### ContextMenuExt Trait
-
-| Method                  | Description                 |
-| ----------------------- | --------------------------- |
-| `context_menu(builder)` | Add context menu to element |
 
 ## Examples
 
@@ -369,6 +323,30 @@ div()
     })
 ```
 
+### Add MenuItem without action
+
+Sometimes you may not like to define an action for a menu item, you just want add a `on_click` handler, in this case, the `item` and [PopupMenuItem] can help you:
+
+```rust
+use gpui_component::{popup_menu::PopupMenuItem, Button};
+
+Button::new("custom-item-menu")
+    .label("Options")
+    .popup_menu(|menu, window, cx| {
+        menu.item(
+            PopupMenuItem::new("Custom Action")
+                .disabled(false)
+                .icon(IconName::Star)
+                .on_click(|window, cx| {
+                    // Custom click handler logic
+                    println!("Custom Action Clicked!");
+                })
+        )
+        .separator()
+        .menu("Standard Action", Box::new(StandardAction))
+    })
+```
+
 ### Editor Menu with Shortcuts
 
 ```rust
@@ -460,3 +438,7 @@ Button::new("settings")
 6. **Progressive Disclosure**: Use submenus for complex hierarchies
 7. **Clear Labels**: Use descriptive, action-oriented labels
 8. **Reasonable Limits**: Use scrollable menus for more than 10-15 items
+
+[PopupMenu]: https://docs.rs/gpui-component/latest/gpui_component/menu/popup_menu/struct.PopupMenu.html
+[PopupMenuItem]: https://docs.rs/gpui-component/latest/gpui_component/menu/popup_menu/struct.PopupMenuItem.html
+[context_menu]: https://docs.rs/gpui-component/latest/gpui_component/menu/context_menu/trait.ContextMenuExt.html#method.context_menu