sidebar: Refactor Sidebar API to avoid map call will segmentation fault on release. (#628)

Close #471

```
    Finished `release` profile [optimized] target(s) in 9.99s
     Running `target/release/examples/sidebar`
[1]    30606 segmentation fault  cargo run --example sidebar --release
```

This may cause by used `map` break by maybe Rust compiler bug, I
detected that if I remove a `map`, then the crash will gone.

So, this changes to refactor the Sidebar API to avoid use `map`.

crates/story/src/sidebar_story.rs

@@ -10,7 +10,8 @@ use gpui_component::{
     h_flex,
     popup_menu::PopupMenuExt,
     sidebar::{
-        Sidebar, SidebarFooter, SidebarGroup, SidebarHeader, SidebarMenu, SidebarToggleButton,
+        Sidebar, SidebarFooter, SidebarGroup, SidebarHeader, SidebarMenu, SidebarMenuItem,
+        SidebarToggleButton,
     },
     switch::Switch,
     v_flex, white, ActiveTheme, Collapsible, Icon, IconName, Side,
@@ -197,6 +198,7 @@ impl Focusable for SidebarStory {
         self.focus_handle.clone()
     }
 }
+
 impl Render for SidebarStory {
     fn render(
         &mut self,
@@ -305,47 +307,51 @@ impl Render for SidebarStory {
                                 )
                             }),
                     )
-                    .child(SidebarGroup::new("Platform").child(SidebarMenu::new().map(
+                    .child(
-                        |mut menu| {
+                        SidebarGroup::new("Platform").child(SidebarMenu::new().children({
+                            let mut items = Vec::with_capacity(groups[0].len());
                             for item in groups[0].iter() {
                                 let item = *item;
-                                menu = menu.submenu(
+                                items.push(
-                                    item.label(),
+                                    SidebarMenuItem::new(item.label())
-                                    Some(item.icon().into()),
+                                        .icon(item.icon().into())
-                                    self.active_item == item,
+                                        .active(self.active_item == item)
-                                    |mut submenu| {
+                                        .children({
-                                        for subitem in item.items() {
+                                            let mut sub_items =
-                                            submenu = submenu.menu(
+                                                Vec::with_capacity(item.items().len());
-                                                subitem.label(),
+                                            for sub_item in item.items() {
-                                                None,
+                                                sub_items.push(
-                                                self.active_subitem == Some(subitem),
+                                                    SidebarMenuItem::new(sub_item.label())
-                                                cx.listener(subitem.handler(&item)),
+                                                        .active(
-                                            );
+                                                            self.active_subitem == Some(sub_item),
-                                        }
+                                                        )
-                                        submenu
+                                                        .on_click(
-                                    },
+                                                            cx.listener(sub_item.handler(&item)),
-                                    cx.listener(move |this, _, _, cx| {
+                                                        ),
-                                        this.active_item = item;
+                                                );
-                                        cx.notify();
+                                            }
-                                    }),
+                                            sub_items
+                                        })
+                                        .on_click(cx.listener(item.handler())),
                                 );
                             }
-                            menu
+                            items
-                        },
+                        })),
-                    )))
+                    )
-                    .child(SidebarGroup::new("Projects").child(SidebarMenu::new().map(
+                    .child(
-                        |mut menu| {
+                        SidebarGroup::new("Projects").child(SidebarMenu::new().children({
+                            let mut items = Vec::with_capacity(groups[1].len());
                             for item in groups[1].iter() {
-                                menu = menu.menu(
+                                items.push(
-                                    item.label(),
+                                    SidebarMenuItem::new(item.label())
-                                    Some(item.icon().into()),
+                                        .icon(item.icon().into())
-                                    self.active_item == *item,
+                                        .active(self.active_item == *item)
-                                    cx.listener(item.handler()),
+                                        .on_click(cx.listener(item.handler())),
                                 );
                             }
-                            menu
+                            items
-                        },
+                        })),
-                    ))),
+                    ),
             )
             .child(
                 v_flex()

crates/ui/src/sidebar/menu.rs

@@ -20,41 +20,16 @@ impl SidebarMenu {
         }
     }
 
-    pub fn menu(
+    pub fn child(mut self, child: impl Into<SidebarMenuItem>) -> Self {
-        mut self,
+        self.items.push(child.into());
-        label: impl Into<SharedString>,
-        icon: Option<Icon>,
-        active: bool,
-        handler: impl Fn(&ClickEvent, &mut Window, &mut App) + 'static,
-    ) -> Self {
-        self.items.push(SidebarMenuItem::Item {
-            icon,
-            label: label.into(),
-            handler: Rc::new(handler),
-            active,
-            collapsed: self.collapsed,
-        });
         self
     }
 
-    pub fn submenu(
+    pub fn children(
         mut self,
-        label: impl Into<SharedString>,
+        children: impl IntoIterator<Item = impl Into<SidebarMenuItem>>,
-        icon: Option<Icon>,
-        open: bool,
-        items: impl FnOnce(SidebarMenu) -> Self,
-        handler: impl Fn(&ClickEvent, &mut Window, &mut App) + 'static,
     ) -> Self {
-        let menu = SidebarMenu::new();
+        self.items = children.into_iter().map(Into::into).collect();
-        let menu = items(menu);
-        self.items.push(SidebarMenuItem::Submenu {
-            icon,
-            label: label.into(),
-            items: menu.items,
-            open,
-            collapsed: self.collapsed,
-            handler: Rc::new(handler),
-        });
         self
     }
 }
@@ -70,93 +45,84 @@ impl Collapsible for SidebarMenu {
 }
 impl RenderOnce for SidebarMenu {
     fn render(self, _window: &mut Window, _cx: &mut App) -> impl IntoElement {
-        v_flex()
+        v_flex().gap_2().children(self.items)
-            .gap_2()
-            .children(self.items.into_iter().map(|mut item| {
-                match &mut item {
-                    SidebarMenuItem::Item { collapsed, .. } => *collapsed = self.collapsed,
-                    SidebarMenuItem::Submenu { collapsed, .. } => *collapsed = self.collapsed,
-                }
-                item
-            }))
     }
 }
 
 /// A sidebar menu item
 #[derive(IntoElement)]
-enum SidebarMenuItem {
+pub struct SidebarMenuItem {
-    Item {
+    icon: Option<Icon>,
-        icon: Option<Icon>,
+    label: SharedString,
-        label: SharedString,
+    handler: Rc<dyn Fn(&ClickEvent, &mut Window, &mut App)>,
-        handler: Rc<dyn Fn(&ClickEvent, &mut Window, &mut App)>,
+    active: bool,
-        active: bool,
+    collapsed: bool,
-        collapsed: bool,
+    children: Vec<Self>,
-    },
-    Submenu {
-        icon: Option<Icon>,
-        label: SharedString,
-        handler: Rc<dyn Fn(&ClickEvent, &mut Window, &mut App)>,
-        items: Vec<SidebarMenuItem>,
-        open: bool,
-        collapsed: bool,
-    },
 }
 
 impl SidebarMenuItem {
+    /// Create a new SidebarMenuItem with a label
+    pub fn new(label: impl Into<SharedString>) -> Self {
+        Self {
+            icon: None,
+            label: label.into(),
+            handler: Rc::new(|_, _, _| {}),
+            active: false,
+            collapsed: false,
+            children: Vec::new(),
+        }
+    }
+
+    /// Set the icon for the menu item
+    pub fn icon(mut self, icon: Icon) -> Self {
+        self.icon = Some(icon);
+        self
+    }
+
+    /// Set the active state of the menu item
+    pub fn active(mut self, active: bool) -> Self {
+        self.active = active;
+        self
+    }
+
+    /// Add a click handler to the menu item
+    pub fn on_click(
+        mut self,
+        handler: impl Fn(&ClickEvent, &mut Window, &mut App) + 'static,
+    ) -> Self {
+        self.handler = Rc::new(handler);
+        self
+    }
+
+    /// Set the collapsed state of the menu item
+    pub fn collapsed(mut self, collapsed: bool) -> Self {
+        self.collapsed = collapsed;
+        self
+    }
+
+    pub fn children(mut self, children: impl IntoIterator<Item = impl Into<Self>>) -> Self {
+        self.children = children.into_iter().map(Into::into).collect();
+        self
+    }
+
     fn is_submenu(&self) -> bool {
-        matches!(self, SidebarMenuItem::Submenu { .. })
+        self.children.len() > 0
-    }
-
-    fn icon(&self) -> Option<Icon> {
-        match self {
-            SidebarMenuItem::Item { icon, .. } => icon.clone(),
-            SidebarMenuItem::Submenu { icon, .. } => icon.clone(),
-        }
-    }
-
-    fn label(&self) -> SharedString {
-        match self {
-            SidebarMenuItem::Item { label, .. } => label.clone(),
-            SidebarMenuItem::Submenu { label, .. } => label.clone(),
-        }
-    }
-
-    fn is_active(&self) -> bool {
-        match self {
-            SidebarMenuItem::Item { active, .. } => *active,
-            SidebarMenuItem::Submenu { .. } => false,
-        }
     }
 
     fn is_open(&self) -> bool {
-        match self {
+        if self.is_submenu() {
-            SidebarMenuItem::Item { .. } => false,
+            self.active
-            SidebarMenuItem::Submenu { open, items, .. } => {
+        } else {
-                *open || items.iter().any(|item| item.is_active())
+            false
-            }
         }
     }
 
-    fn is_collapsed(&self) -> bool {
+    fn render_menu_item(&self, _: &Window, cx: &App) -> impl IntoElement {
-        match self {
+        let handler = self.handler.clone();
-            SidebarMenuItem::Item { collapsed, .. } => *collapsed,
+        let is_collapsed = self.collapsed;
-            SidebarMenuItem::Submenu { collapsed, .. } => *collapsed,
+        let is_active = self.active;
-        }
+        let is_open = self.is_open();
-    }
+        let is_submenu = self.is_submenu();
-
-    fn render_menu_item(
-        &self,
-        is_submenu: bool,
-        is_active: bool,
-        is_open: bool,
-        _: &Window,
-        cx: &App,
-    ) -> impl IntoElement {
-        let handler = match &self {
-            SidebarMenuItem::Item { handler, .. } => Some(handler.clone()),
-            SidebarMenuItem::Submenu { handler, .. } => Some(handler.clone()),
-        };
-        let is_collapsed = self.is_collapsed();
 
         h_flex()
             .id("sidebar-menu-item")
@@ -172,18 +138,18 @@ impl SidebarMenuItem {
                 this.bg(cx.theme().sidebar_accent)
                     .text_color(cx.theme().sidebar_accent_foreground)
             })
-            .when(is_active, |this| {
+            .when(is_active && !is_submenu, |this| {
                 this.font_medium()
                     .bg(cx.theme().sidebar_accent)
                     .text_color(cx.theme().sidebar_accent_foreground)
             })
-            .when_some(self.icon(), |this, icon| this.child(icon.size_4()))
+            .when_some(self.icon.clone(), |this, icon| this.child(icon.size_4()))
             .when(is_collapsed, |this| {
                 this.justify_center().size_7().mx_auto()
             })
             .when(!is_collapsed, |this| {
                 this.h_7()
-                    .child(div().flex_1().child(self.label()))
+                    .child(div().flex_1().child(self.label.clone()))
                     .when(is_submenu, |this| {
                         this.child(
                             Icon::new(IconName::ChevronRight)
@@ -192,43 +158,29 @@ impl SidebarMenuItem {
                         )
                     })
             })
-            .when_some(handler, |this, handler| {
+            .on_click(move |ev, window, cx| handler(ev, window, cx))
-                this.on_click(move |ev, window, cx| handler(ev, window, cx))
-            })
     }
 }
 
 impl RenderOnce for SidebarMenuItem {
     fn render(self, window: &mut Window, cx: &mut App) -> impl IntoElement {
         let is_submenu = self.is_submenu();
-        let is_active = self.is_active();
         let is_open = self.is_open();
 
         div()
             .w_full()
-            .child(self.render_menu_item(is_submenu, is_active, is_open, window, cx))
+            .child(self.render_menu_item(window, cx))
-            .when(is_open, |this| {
+            .when(is_submenu && is_open, |this| {
-                this.map(|this| match self {
+                this.child(
-                    SidebarMenuItem::Submenu {
+                    v_flex()
-                        items, collapsed, ..
+                        .border_l_1()
-                    } => {
+                        .border_color(cx.theme().sidebar_border)
-                        if collapsed {
+                        .gap_1()
-                            this
+                        .mx_3p5()
-                        } else {
+                        .px_2p5()
-                            this.child(
+                        .py_0p5()
-                                v_flex()
+                        .children(self.children),
-                                    .border_l_1()
+                )
-                                    .border_color(cx.theme().sidebar_border)
-                                    .gap_1()
-                                    .mx_3p5()
-                                    .px_2p5()
-                                    .py_0p5()
-                                    .children(items),
-                            )
-                        }
-                    }
-                    _ => this,
-                })
             })
     }
 }