ListSelect, multiselect mode, bad values not removed on select

I am not sure if this is a bug in ListSelect or not, but I have run into when an object has a set of values (originally set in the ListSelect), but one or more of those original values is no longer valid (contains a value no longer in the list of choices), but even when I select a new value in the ListSelect, it won’t just return that selected item, but that item plus any and all values were there before (hidden selected values).

By this I mean, for example, that my ListSelect with multiselect accepts, for example, planet names from the choices: Venus, Mars, Earth.

So the user chooses Earth. Earth is highlighted in the list as expected.

But then because of some other change (or user permission), the planet names allowed now are: Saturn, Neptune, Uranus. So Earth is no longer valid, but when the ListSelect is generated, the HashSet value contains Earth still as that was the value stored in the DB.

This results in a ListSelect in which nothing appears to be selected, which would be fine, except that Earth is actually a value still in the set. There are “hidden” choices essentially.

Then, when I click in the new ListSelect and choose Saturn, the ListSelect’s value is a Set with Earth AND Saturn, not just Saturn which is the only visibly selected value. If I selected Saturn and Neptune, the value set would be Earth, Saturn and Neptune, even though there is no Earth selected.

It seems that the ListSelect is not returning just the values currently selected, but it’s somehow doing add/removes based on user action rather than setting the values to all of the currently selected values. This results in “no longer valid values” still be in the set.

Is that by spec or is this a bug for multi-select? I have found this is not an issue for a ListSelect that only allows one choice, or a regular NativeSelect, etc. that only supports a single value, in which whatever the user clicks in the select box becomes the new value.

EDITED TO ADD:
I am using a BeanItemContainer, which complicates things since the get/set values occur automatically, so I cannot easily create a reduced set based on the values the UI has decided are allowed in the ListSelect. If I called the listSelect.setValue(myHashSet) I could do this, but that’s not an option with BeanItems.

After more testing, I think I see what is happening:

  1. In an HTML multi-select SELECT box, it is impossible to set values that do not exist as options.

  2. In an HTML mult-select SELECT box, when the value is transmitted, it sends ALL values currently selected. Again, this makes the value impossible to be different.

But I think the ListSelect is trying to be “efficient” and may only be sending ADD or REMOVE type requests to the select box as users SELECT and UNSELECT them. This allows any bad values to remain in the set even if they are not selected in the UI component.

Hope this helps…

It seems like a bug to me too.

Look in the Vaadin bug tracker if there is not already a bug for this.
If not, create a new one. But the chances to get it fixed quickly would dramatically increase if you could come up with a reduced test case and attach it to the ticket.

And add a link here to the ticket for the other users who will find this thread first.

Here’s a code example (this is easy to reproduce fortunately):


public class TestApplication extends Application {

	@Override
	public void init() {
		Window mainWindow = new Window("Test Application", new VerticalLayout());
		mainWindow.setSizeFull();
		setMainWindow(mainWindow);
		
		// Note that the initial value will not be one of the "allowed" choices.
		HashSet<String> initialValues = new HashSet<String>();
		initialValues.add("MoonNotInList");
		
		final ListSelect select = new ListSelect("Test");
		select.setImmediate(true);
		select.setMultiSelect(true);
		select.addItem("Earth");
		select.addItem("Mars");
		select.addItem("Venus");
		select.setValue(initialValues);
		select.addListener(new Property.ValueChangeListener() {

			@Override
			public void valueChange(ValueChangeEvent event) { 
				getMainWindow().showNotification("ValueChangeEvent: " + event.getProperty(), Notification.TYPE_ERROR_MESSAGE);
			}
			
		});
		
		mainWindow.addComponent(select);
	}
}

You will note that as you click, the original “MoonNotInList” will remain in the list no matter what you select.

EDIT TO COMMENT:
In this example, of course, it would be easy to ensure the initial values are valid. But if this were in a Form using BeanItemContainer, the bean’s get/set methods are unrelated to the UI code that creates the ListSelect where the allowable values are set.

Created
ticket 8349

For now, I’ve created this subclass that seems to do the trick (so far, but not really tested fully), by checking if the value being set is in our allowable list, and only create a new value set if something had to be removed:

public class ListSelectValid extends ListSelect {

	public ListSelectValid(String caption) {
		super(caption);
    }
    
	@Override
    protected void setValue(Object newValue, boolean repaintIsNotNeeded) throws Property.ReadOnlyException, Property.ConversionException {
		if ( newValue == null ) {
			super.setValue(newValue,repaintIsNotNeeded);
		} else {
			if ( isMultiSelect() && newValue instanceof Set ) {
				Set<?> newValueSet = (Set<?>)newValue;
	        	HashSet<?> updatedItemIds = null;
	    		for( Object newValueItemId : newValueSet ) {
	    			boolean found = false;
	    	    	for( Object itemId : getItemIds() ) {
	    	    		if ( (itemId == null && newValueItemId == null) || itemId.equals(newValueItemId) ) {
	    	    			found = true;
	    	    			break;
	    	    		}
	    	    	}
	    	    	if ( ! found ) {
	    	    		if ( updatedItemIds == null ) {
	    	    			updatedItemIds = new HashSet<Object>(newValueSet);
	    	    		}
	    	    		updatedItemIds.remove(newValueItemId);
	    	    	}
	    		}
				if ( updatedItemIds != null ) {
					super.setValue(updatedItemIds,repaintIsNotNeeded);
				} else {
					super.setValue(newValue,repaintIsNotNeeded);
				}
			} else {
		    	for( Object itemId : getItemIds() ) {
		    		if ( itemId.equals(newValue) ) {
		    			super.setValue(newValue,repaintIsNotNeeded);
		    			break;
		    		}
		    	}
			}
		}
	}
}