[BUG] Panic when opening empty menubar subtree

[DvvCz]

Describe the bug
When creating a menubar with an empty subtree, panics when you select it in the menu.
Dealing with this as I want to create a dynamic menubar to select a file, although maybe this isn't the best approach.

To Reproduce

let mut siv = cursive::default();
siv.menubar().add_subtree("foo", Tree::new());
siv.add_global_callback(Key::Esc, |s| s.select_menubar());
siv.run();
// Then press escape, go to foo and press enter.

Expected behavior
Expected to not panic

Environment

• OS: Linux (Nobara 39)
• Backend used: ncurses
• Current locale: en_US
• Cursive version: 0.20.0
• Nightly rust in debug build

Additional context
Assuming index OOB error "len is 0 but index is 0":

cursive/cursive-core/src/views/menu_popup.rs

Line 331 in 0a95c82

let item = &s.menu.children[i];

As panic points to around there (line 335 specifically).

[gyscos]

Thanks for the report!

Indeed, not panicking sounds like a better behavior :)

[correabuscar]

Tentative fix would be:

diff --git a/cursive-core/src/views/menu_popup.rs b/cursive-core/src/views/menu_popup.rs
index d3050ba..9fe5698 100644
--- a/cursive-core/src/views/menu_popup.rs
+++ b/cursive-core/src/views/menu_popup.rs
@@ -130,7 +130,11 @@ impl MenuPopup {
             } else if cycle {
                 // Only cycle once to prevent endless loop
                 cycle = false;
-                self.focus = self.menu.children.len() - 1;
+                let len = self.menu.children.len();
+                if len == 0 {
+                    break;
+                }
+                self.focus = len - 1;
             } else {
                 break;
             }
@@ -143,12 +147,16 @@ impl MenuPopup {
 
     fn scroll_down(&mut self, mut n: usize, mut cycle: bool) {
         while n > 0 {
-            if self.focus + 1 < self.menu.children.len() {
+            let len = self.menu.children.len();
+            if self.focus + 1 < len {
                 self.focus += 1;
             } else if cycle {
                 // Only cycle once to prevent endless loop
                 cycle = false;
                 self.focus = 0;
+                if len == 0 {
+                    break;
+                }
             } else {
                 // Stop if we're at the bottom.
                 break;
@@ -236,13 +244,19 @@ impl MenuPopup {
             Event::Key(Key::Home) => self.focus = 0,
             Event::Key(Key::End) => self.focus = self.menu.children.len().saturating_sub(1),
 
-            Event::Key(Key::Right) if self.menu.children[self.focus].is_subtree() => {
+            Event::Key(Key::Right)
+                if (self.focus < self.menu.children.len()
+                    && self.menu.children[self.focus].is_subtree()) =>
+            {
                 return match self.menu.children[self.focus] {
                     menu::Item::Subtree { ref tree, .. } => self.make_subtree_cb(tree),
                     _ => unreachable!("Child is a subtree"),
                 };
             }
-            Event::Key(Key::Enter) if self.menu.children[self.focus].is_enabled() => {
+            Event::Key(Key::Enter)
+                if (self.focus < self.menu.children.len()
+                    && self.menu.children[self.focus].is_enabled()) =>
+            {
                 return self.submit();
             }
             Event::Mouse {
@@ -263,7 +277,8 @@ impl MenuPopup {
                 event: MouseEvent::Release(MouseButton::Left),
                 position,
                 offset,
-            } if self.menu.children[self.focus].is_enabled()
+            } if (self.focus < self.menu.children.len()
+                && self.menu.children[self.focus].is_enabled())
                 && position
                     .checked_sub(offset)
                     .map(|position| position.y == self.focus)
@@ -320,7 +335,13 @@ impl View for MenuPopup {
         scroll::draw_box_frame(
             self,
             printer,
-            |s, y| s.menu.children[y].is_delimiter(),
+            |s, y| {
+                if s.menu.children.len() > y {
+                    s.menu.children[y].is_delimiter()
+                } else {
+                    false
+                }
+            },
             |_s, _x| false,
         );
 

But maybe this isn't the proper way to do it. Like, some indirection(via some new function) might be wanted instead.