Allow user to specify id for menu item #933

Open
vaadin-bot opened this Issue Dec 17, 2009 · 8 comments

Comments

Projects
None yet
1 participant
Collaborator

vaadin-bot commented Dec 17, 2009

Originally by bootlaces


Hi,

I have a menu bar with lots of menu items. I wish to attach only one command object to each menu item and then in the body of menuSelected(final MenuItem selectedItem) to switch on either an index or an arbitrary value I pass in, i.e.,

private static final ADD = 1;
private static final DELETE = 2;

MenuItem main = addItem("Items", null, null);
main.addItem(ADD, "Add Item", null, command);
main.addItem(DELETE, "Delete Item", null, command);
...
...

command = new Command() {

@override
public void menuSelected(final MenuItem selectedItem) {

switch(selectedItem.getIndex()) {
case ADD:
doSomethingWonderful();
break;
case DELETE:
doSomethingNotSoWonderful();
break;
}
}
}

This would be a great help. Otherwise, I have to do a string comparision, and since my application is I18N aware, it means doing a ResourceBundle lookup each time for the string...slooooow.

Thanks

-=bootlaces=-


Imported from https://dev.vaadin.com/ issue #3873

Collaborator

vaadin-bot commented Dec 17, 2009

Originally by bootlaces


Oops, forgot to put in WikiFormatting:

MenuItem main = addItem("Items", null, null); 
main.addItem(ADD, "Add Item", null, command); 
main.addItem(DELETE, "Delete Item", null, command); 
... 
...

command = new Command() {

    @Override 
    public void menuSelected(final MenuItem selectedItem) {
        switch(selectedItem.getIndex()) {
           case ADD:
              doSomethingWonderful(); break;
           case DELETE:
              doSomethingNotSoWonderful(); break;

        }
     }
}

Collaborator

vaadin-bot commented Dec 18, 2009

Originally by bootlaces


Added in a patch.

Collaborator

vaadin-bot commented Dec 18, 2009

Originally by bootlaces


Attachment added: menuItem.patch (15.3 KiB)

Collaborator

vaadin-bot commented Jan 14, 2010

Originally by @Artur-


Isn't the very same thing accomplishable (easier) by doing

MenuItem main = addItem("Items", null, null); 
main.addItem(ADD, "Add Item", null, new Command() {add();});
main.addItem(DELETE, "Delete Item", null, new Command() {delete();}); 

Why do you need the integers?

Collaborator

vaadin-bot commented Jan 14, 2010

Originally by @Artur-


Additionally, every MenuItem has already an id, which is an integer although it is generated automatically: first item is 1, second is 2 etc.

Collaborator

vaadin-bot commented Jan 14, 2010

Originally by @hezamu


You should consider Artur's approac, but FWIW I use the anonymous inner class pattern to do it this way:

rootItem.addItem("foo", new Command() {
   public void menuSelected(MenuItem selectedItem) {
      // Do some stuff when user selects the "foo" menu
   }
});

That way every command logic is separated from the others, and there is no need for a crufty and potentially large switch structure.

If there is a API change for this I propose something similar to get/setData() in AbstractComponent that'd be more flexible than just integers. Note that I've had a need for something like it, and had to use a somewhat kludgy Map<MenuItem, Object> to associate MenuItems to stuff instead.

Collaborator

vaadin-bot commented Jan 14, 2010

Originally by bootlaces


Hi,

The reason why it's an idea to use ints, is to have only one command object instead of having many. The current approach requires either new command objects for every menu item, or as Henri says to use anonymous inner classes. This means a lot of code duplication and possible introduction of bugs.

The problem with anonymous inner classes, is that with any reasonably large menu structure, you are talking about a HUGE amount of inner class code, plus duplication! Having lots of menuSelected etc... can become quite cumbersome.

Additionally, in response to the other point, they may be initially assigned 1, 2, 3 etc.., but it'll fall apart if you addItemBefore an existing menu item...the mappings will quickly become out of sync.

-=bootlaces=-

Collaborator

vaadin-bot commented Sep 30, 2010

Originally by @Artur-


Updated summary. IMO the correct solution would be to allow the user to specify an id (Object) which is used externally and the generated id would only be used internally. This is symmetric with how containers etc work. For the API it should mean that getId() returns Object instead of int.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment