Feature proposal: Add action closure to MenuItem

[Korne127]

Hey, first of all, thanks for the project. I think it's really great to be able to easily create a cross platform menu bar, and I like the way this crate specifically works.
I especially think the way that you can create a menu recursively (like how it's done in the examples) is really nice:

fn create_menu() {
    let menu = Menu::with_items(&[&Submenu::with_items(
        "",
        true,
        &[
            &MenuItem::new(
                "Item 1",
                true,
                Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyA)),
            ),
            &MenuItem::new(
                "Item 2",
                true,
                Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyB)),
            ),
            &MenuItem::new(
                "Item 3",
                true,
                Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyC)),
            ),
        ],
    )
    .unwrap()])
    .unwrap();
    menu.init_for_nsapp();
}

However, I noticed one thing. As it's necessary to process the menu events (otherwise the menu would be pretty useless), you need to create all menu items explicitly as variables to get their ids, so that you can process the events like this:

if let Ok(event) = MenuEvent::receiver().try_recv() {
    match event.id {
        _ if event.id == item_1.id() => {
            println!("Clicked on item 1");
        },
        _ if event.id == item_2.id() => {
            println!("Clicked on item 2");
        },
        _ if event.id == item_3.id() => {
            println!("Clicked on item 3");
        },
        _ => {}
    }
}

However, this destroys the benefit of the upper structure.
Instead of being able to write a recursive block like above, you have to do it like this:

let item_1 = MenuItem::new(
    "Item 1",
    true,
    Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyA)),
);
let item_2 = MenuItem::new(
    "Item 2",
    true,
    Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyB)),
);
let item_3 = MenuItem::new(
    "Item 3",
    true,
    Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyC)),
);

let menu =
    Menu::with_items(&[&Submenu::with_items("", true, &[&item_1, &item_2, &item3]).unwrap()])
        .unwrap();

This means that the menu definition has to be split up between the (sub-)menus and the menu items, essentially not allowing to use this great feature of defining menus recursively in-line. That can make it harder to intuitively see the structure of nested menus and make the code less comprehensible.
This system also means that the menu definition and the menu logic are separated. This makes it easier to introduce errors, e.g. by changing the structure or logic in one place and forgetting it in the other, and less easy to oversee.

Therefore, I'd suggest a feature that fixes this and allows for a full menu definition in one place: Including an FnMut action closure directly in the MenuItem.
This would allow creating a menu like this:

fn create_menu() {
    let menu = Menu::with_items(&[&Submenu::with_items(
        "",
        true,
        &[
            &MenuItem::new(
                "Item 1",
                true,
                Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyA)),
                Box::new(|| println!("Clicked on item 1")),
            ),
            &MenuItem::new(
                "Item 2",
                true,
                Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyB)),
                Box::new(|| println!("Clicked on item 2")),
            ),
            &MenuItem::new(
                "Item 3",
                true,
                Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyC)),
                Box::new(|| println!("Clicked on item 3")),
            ),
        ],
    )
    .unwrap()])
    .unwrap();
    menu.init_for_nsapp();
}

with all the advantages I mentioned earlier.
Of course you could also link to a real function for more advanced actions or different actions that overlap.

---

The only other reason why someone needs to create a menu item before integrating it in the menu is to keep it for the future, e.g. when you want to disable some menu items in specific situations. This is not as common as the actions (as they are basically necessary for every menu item), but it's still important to be able to do it.
I propose a second constructor, MenuItem::new_with_handle to solve this (and not require a prior menu item definition for this use case).

If you want to be able to access the MenuItem after the menu creation, you could use the constructor like this:

let mut item_1_handle: MenuItem;
let menu = Menu::with_items(&[&Submenu::with_items(
    "",
    true,
    &[MenuItem::new_with_handle(
        "Item 1",
        true,
        Some(Accelerator::new(Some(Modifiers::SUPER), Code::KeyA)),
        Box::new(|| println!("Clicked on item 1")),
        &mut item_1_handle
    ), …],
)
.unwrap()])
.unwrap();
menu.init_for_nsapp();

This would create the menu and save the created menu item in item_1_handle. This way, it allows to change the item in the future without having to separate it from the menu structure.

(To avoid the muts, you could also create a new MenuItemHandle struct that uses internal mutability and a function to expose the MenuItem, but I don't think that's necessary (although I wouldn't be opposed to that either)).

---

What do you think about these proposals? If you agree to these changes, I'd be very happy to open a PR implementing it. I think this would improve the way we can create menus with muda and allow us to use more of the potential this crate already has.

[amrbashir]

Thanks for the detailed proposal.

I didn't implement this action closure before because I didn't want to force users to share their app state through an Arc<Mutex> or Rc<RefCell>, and events seemed like a good way around this by allowing the user to process menu events in the same place as his app state.

Another reason was, users may expect this callback closure to have access to the menu item that is being triggered in case they want to modify it, like changing the icon or something. This can get complicated really fast as all menu items are basically a wrapper around Rc<RefCell> and we don't want to have a double mutable borrow or making clones that live in a closure which could prevent the Drop implementation from running.

I don't mind having this closure callbacks but ofc without removing the events, so if you want to work on this, please do, we appreciate it.

[Korne127]

Thank you for the quick and friendly answer :D
And thanks for the explanation; I understand it.
I'll work on it and try to implement it in a nice (optional) way.
I think the second proposal also helps when you want to keep using the event system as it allows you to recursively define a menu in-line and still have access to all individual items (and their ids), so that's an improvement no matter what you use to process menu events.