Fix AbstractSingleSelect selection and state updates (#10796)

all/src/main/templates/release-notes.html

@@ -111,6 +111,7 @@
         <li>Date range limits in <tt>AbstractDateFieldState</tt> are now <tt>String</tt>s instead of <tt>Date</tt>s, some client-side method signatures were changed</li>
         <li><tt>BrowserResizeListener</tt> is only called once resizing ends.</li>
         <li><tt>ItemClickEvent</tt> for <tt>Grid</tt> now takes an additional row index parameter.</li>
+        <li><tt>AbstractSingleSelect</tt> functionality has been partially re-written. Parts of the <tt>protected</tt> API regarding selection has been removed</li>
 
         <h2>For incompatible or behavior-altering changes in 8.3, please see <a href="https://vaadin.com/download/release/8.3/8.3.0/release-notes.html#incompatible">8.3 release notes</a></h2>
         

server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java

@@ -28,6 +28,7 @@ import org.jsoup.nodes.Element;
 import com.vaadin.data.HasValue;
 import com.vaadin.data.SelectionModel.Single;
 import com.vaadin.data.provider.DataCommunicator;
+import com.vaadin.data.provider.DataGenerator;
 import com.vaadin.event.selection.SingleSelectionEvent;
 import com.vaadin.event.selection.SingleSelectionListener;
 import com.vaadin.shared.Registration;
@@ -37,6 +38,7 @@ import com.vaadin.ui.declarative.DesignContext;
 import com.vaadin.ui.declarative.DesignException;
 
 import elemental.json.Json;
+import elemental.json.JsonObject;
 
 /**
  * An abstract base class for listing components that only support single
@@ -54,6 +56,8 @@ import elemental.json.Json;
 public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
         implements SingleSelect<T> {
 
+    private T selectedItem = null;
+
     /**
      * Creates a new {@code AbstractListing} with a default data communicator.
      * <p>
@@ -101,7 +105,7 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
      *         otherwise
      */
     public Optional<T> getSelectedItem() {
-        return Optional.ofNullable(keyToItem(getSelectedKey()));
+        return Optional.ofNullable(selectedItem);
     }
 
     /**
@@ -112,7 +116,7 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
      *            the item to select or {@code null} to clear selection
      */
     public void setSelectedItem(T item) {
-        setSelectedFromServer(item);
+        setSelectedItem(item, false);
     }
 
     /**
@@ -187,108 +191,6 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
         return super.isReadOnly();
     }
 
-    /**
-     * Returns the communication key of the selected item or {@code null} if no
-     * item is selected.
-     *
-     * @return the key of the selected item if any, {@code null} otherwise.
-     */
-    protected String getSelectedKey() {
-        return getState(false).selectedItemKey;
-    }
-
-    /**
-     * Sets the selected item based on the given communication key. If the key
-     * is {@code null}, clears the current selection if any.
-     *
-     * @param key
-     *            the key of the selected item or {@code null} to clear
-     *            selection
-     */
-    protected void doSetSelectedKey(String key) {
-        getState().selectedItemKey = key;
-    }
-
-    /**
-     * Sets the selection based on a client request. Does nothing if the select
-     * component is {@linkplain #isReadOnly()} or if the selection would not
-     * change. Otherwise updates the selection and fires a selection change
-     * event with {@code isUserOriginated == true}.
-     *
-     * @param key
-     *            the key of the item to select or {@code null} to clear
-     *            selection
-     */
-    protected void setSelectedFromClient(String key) {
-        if (isReadOnly()) {
-            return;
-        }
-        if (isKeySelected(key)) {
-            return;
-        }
-
-        T oldSelection = getSelectedItem().orElse(getEmptyValue());
-        doSetSelectedKey(key);
-
-        // Set diffstate to something that will always send selection to client
-        updateDiffstate("selectedItemKey", Json.createObject());
-
-        fireEvent(new SingleSelectionEvent<>(AbstractSingleSelect.this,
-                oldSelection, true));
-    }
-
-    /**
-     * Sets the selection based on server API call. Does nothing if the
-     * selection would not change; otherwise updates the selection and fires a
-     * selection change event with {@code isUserOriginated == false}.
-     *
-     * @param item
-     *            the item to select or {@code null} to clear selection
-     */
-    protected void setSelectedFromServer(T item) {
-        // TODO creates a key if item not in data provider
-        String key = itemToKey(item);
-
-        if (isKeySelected(key) || isSelected(item)) {
-            return;
-        }
-
-        T oldSelection = getSelectedItem().orElse(getEmptyValue());
-        doSetSelectedKey(key);
-
-        fireEvent(new SingleSelectionEvent<>(AbstractSingleSelect.this,
-                oldSelection, false));
-    }
-
-    /**
-     * Returns whether the given key maps to the currently selected item.
-     *
-     * @param key
-     *            the key to test or {@code null} to test whether nothing is
-     *            selected
-     * @return {@code true} if the key equals the key of the currently selected
-     *         item (or {@code null} if no selection), {@code false} otherwise.
-     */
-    protected boolean isKeySelected(String key) {
-        return Objects.equals(key, getSelectedKey());
-    }
-
-    /**
-     * Returns the communication key assigned to the given item.
-     *
-     * @param item
-     *            the item whose key to return
-     * @return the assigned key
-     */
-    protected String itemToKey(T item) {
-        if (item == null) {
-            return null;
-        } else {
-            // TODO creates a key if item not in data provider
-            return getDataCommunicator().getKeyMapper().key(item);
-        }
-    }
-
     /**
      * Returns the item that the given key is assigned to, or {@code null} if
      * there is no such item.
@@ -309,7 +211,16 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
      * @return {@code true} if the item is selected, {@code false} otherwise
      */
     public boolean isSelected(T item) {
-        return Objects.equals(getValue(), item);
+        if (Objects.equals(selectedItem, item)) {
+            return true;
+        }
+
+        if (item == null || selectedItem == null) {
+            return false;
+        }
+
+        return Objects.equals(getDataProvider().getId(selectedItem),
+                getDataProvider().getId(item));
     }
 
     @Override
@@ -379,16 +290,91 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T>
 
             @Override
             public void select(String key) {
-                setSelectedFromClient(key);
+                setSelectedItem(keyToItem(key), true);
             }
 
             @Override
             public void deselect(String key) {
-                if (isKeySelected(key)) {
-                    setSelectedFromClient(null);
+                T item = keyToItem(key);
+                if (isSelected(item)) {
+                    setSelectedItem(null, true);
+                }
+            }
+        });
+        addDataGenerator(new DataGenerator<T>() {
+
+            @Override
+            public void generateData(T item, JsonObject jsonObject) {
+                if (isSelected(item)) {
+                    // Deferred update of state.
+                    updateSelectedItemState(item);
+                }
+            }
+
+            @Override
+            public void refreshData(T item) {
+                if (isSelected(item)) {
+                    selectedItem = item;
+                    // Invalidate old data
+                    updateSelectedItemState(null);
                 }
             }
         });
     }
 
+    /**
+     * This method updates the internal selection state of the server-side of
+     * {@code AbstractSingleSelect}.
+     *
+     * @param value
+     *            the value that should be selected
+     * @param userOriginated
+     *            {@code true} if selection was done by user, {@code false} if
+     *            not
+     *
+     * @since
+     */
+    protected void setSelectedItem(T value, boolean userOriginated) {
+        if (isSelected(value)) {
+            return;
+        }
+
+        // Update selection
+        T oldValue = selectedItem;
+        selectedItem = value;
+
+        // Re-generate selected item data
+        if (oldValue != null) {
+            getDataCommunicator().refresh(oldValue);
+        }
+        if (value != null) {
+            getDataCommunicator().refresh(value);
+        }
+
+        // Deselection can be handled immediately
+        updateSelectedItemState(value);
+
+        // Update diffstate to make sure null can be selected later.
+        updateDiffstate("selectedItemKey", Json.createObject());
+
+        fireEvent(new SingleSelectionEvent<>(AbstractSingleSelect.this,
+                oldValue, userOriginated));
+    }
+
+    /**
+     * This method updates the shared selection state of the
+     * {@code AbstractSingleSelect}.
+     *
+     * @param value
+     *            the value that is selected; may be {@code null}
+     *
+     * @since
+     */
+    protected void updateSelectedItemState(T value) {
+        // FIXME: If selecting a value that does not exist, this will leave and
+        // extra object in the key mapper that will not be dropped any time.
+        getState().selectedItemKey = value != null
+                ? getDataCommunicator().getKeyMapper().key(value)
+                : null;
+    }
 }

server/src/main/java/com/vaadin/ui/ComboBox.java

@@ -680,9 +680,6 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
     public void setItemCaptionGenerator(
             ItemCaptionGenerator<T> itemCaptionGenerator) {
         super.setItemCaptionGenerator(itemCaptionGenerator);
-        if (getSelectedItem().isPresent()) {
-            updateSelectedItemCaption();
-        }
     }
 
     /**
@@ -724,10 +721,6 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
     @Override
     public void setItemIconGenerator(IconGenerator<T> itemIconGenerator) {
         super.setItemIconGenerator(itemIconGenerator);
-
-        if (getSelectedItem().isPresent()) {
-            updateSelectedItemIcon();
-        }
     }
 
     @Override
@@ -819,25 +812,23 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
     }
 
     @Override
-    protected void doSetSelectedKey(String key) {
-        super.doSetSelectedKey(key);
+    protected void updateSelectedItemState(T value) {
+        super.updateSelectedItemState(value);
 
-        updateSelectedItemCaption();
-        updateSelectedItemIcon();
+        updateSelectedItemCaption(value);
+        updateSelectedItemIcon(value);
     }
 
-    private void updateSelectedItemCaption() {
+    private void updateSelectedItemCaption(T value) {
         String selectedCaption = null;
-        T value = keyToItem(getSelectedKey());
         if (value != null) {
             selectedCaption = getItemCaptionGenerator().apply(value);
         }
         getState().selectedItemCaption = selectedCaption;
     }
 
-    private void updateSelectedItemIcon() {
+    private void updateSelectedItemIcon(T value) {
         String selectedItemIcon = null;
-        T value = keyToItem(getSelectedKey());
         if (value != null) {
             Resource icon = getItemIconGenerator().apply(value);
             if (icon != null) {
@@ -859,7 +850,8 @@ public class ComboBox<T> extends AbstractSingleSelect<T>
     public void attach() {
         super.attach();
 
-        updateSelectedItemIcon();
+        // Update icon for ConnectorResource
+        updateSelectedItemIcon(getValue());
     }
 
     @Override

server/src/test/java/com/vaadin/ui/ComboBoxTest.java

@@ -1,5 +1,10 @@
 package com.vaadin.ui;
 
+import static org.junit.Assert.assertArrayEquals;
+
+import java.util.ArrayList;
+import java.util.List;
+
 import org.junit.Test;
 
 import com.vaadin.server.ServerRpcManager;
@@ -8,6 +13,8 @@ import com.vaadin.tests.util.MockUI;
 
 public class ComboBoxTest {
 
+    private List<String> eventValues = new ArrayList<>();
+
     @Test
     public void testResetValue() {
         ComboBox<String> comboBox = new ComboBox<>();
@@ -15,10 +22,15 @@ public class ComboBoxTest {
 
         // Reset value whenever it changes (in a real case, this listener would
         // do something with the selected value before discarding it)
-        comboBox.addValueChangeListener(event -> comboBox.setValue(null));
+        comboBox.addValueChangeListener(event -> {
+            eventValues.add(event.getValue());
+            comboBox.setValue(null);
+        });
 
         // "Attach" the component and initialize diffstate
         new MockUI().setContent(comboBox);
+        // Generate initial data
+        comboBox.getDataCommunicator().beforeClientResponse(true);
         ComponentTest.syncToClient(comboBox);
 
         // Emulate selection of "one"
@@ -27,6 +39,9 @@ public class ComboBoxTest {
         ServerRpcManager.getRpcProxy(comboBox, SelectionServerRpc.class)
                 .select(oneKey);
 
+        assertArrayEquals("Unexpected values from selection events",
+                new Object[] { "one", null }, eventValues.toArray());
+
         ComponentTest.assertEncodedStateProperties(comboBox,
                 "Selection change done by the listener should be sent to the client",
                 "selectedItemKey");

uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxCaptionAndIconUpdate.java

@@ -0,0 +1,102 @@
+package com.vaadin.tests.components.combobox;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.server.ClassResource;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Button.ClickListener;
+import com.vaadin.ui.ComboBox;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+public class ComboBoxCaptionAndIconUpdate extends AbstractTestUI {
+
+    public static class Commit {
+        private final long id;
+        private String message;
+        private ClassResource icon;
+
+        Commit(long id, String message, ClassResource icon) {
+            this.id = id;
+            this.message = message;
+            this.icon = icon;
+        }
+
+        public long getId() {
+            return id;
+        }
+
+        public String getMessage() {
+            return message;
+        }
+
+        public void setMessage(String message) {
+            this.message = message;
+        }
+
+        public ClassResource getIcon() {
+            return icon;
+        }
+
+        public void setIcon(ClassResource icon) {
+            this.icon = icon;
+        }
+    }
+
+    List<Commit> backend = new ArrayList<>();
+
+    private final ClassResource M_RESOURCE = new ClassResource(
+            "/com/vaadin/tests/m.gif");
+    private final ClassResource FI_RESOURCE = new ClassResource(
+            "/com/vaadin/tests/integration/fi.gif");
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        ComboBox<Commit> comboBox = new ComboBox<Commit>();
+
+        backend = Stream.of(1, 2)
+                .map(id -> new Commit(id, "Commit ID " + id, M_RESOURCE))
+                .collect(Collectors.toList());
+        comboBox.setItems(backend);
+        comboBox.setValue(backend.get(0));
+
+        comboBox.setItemIconGenerator(i -> FI_RESOURCE);
+        comboBox.setItemCaptionGenerator(i -> "Commit " + i.getId());
+        comboBox.setWidth("300px");
+
+        addComponent(comboBox);
+        addComponent(createButton("Set Icon Generator", "icon",
+                e -> comboBox.setItemIconGenerator(Commit::getIcon)));
+        addComponent(createButton("Set Caption Generator", "caption",
+                e -> comboBox.setItemCaptionGenerator(Commit::getMessage)));
+        addComponent(createButton("Edit Message", "editMsg", e -> {
+            Commit item = backend.get(0);
+            item.setMessage("Edited message");
+            comboBox.getDataProvider().refreshItem(item);
+        }));
+        addComponent(createButton("Edit Icon", "editIcon", e -> {
+            Commit item = backend.get(0);
+            item.setIcon(FI_RESOURCE);
+            comboBox.getDataProvider().refreshItem(item);
+        }));
+        addComponent(createButton("Edit Message and Icon", "editAll", e -> {
+            Commit item = backend.get(0);
+            item.setMessage("Edited message and icon");
+            item.setIcon(FI_RESOURCE);
+            comboBox.getDataProvider().refreshItem(item);
+        }));
+    }
+
+    private Button createButton(String caption, String id,
+            ClickListener listener) {
+        Button button = new Button(caption, listener);
+        button.setId(id);
+        return button;
+    }
+
+}

uitest/src/main/java/com/vaadin/tests/data/DummyData.java

@@ -47,8 +47,6 @@ public class DummyData extends AbstractTestUIWithLog {
     public static class DummyComponent extends AbstractSingleSelect<String>
             implements HasDataProvider<String> {
 
-        private String selected;
-
         private DummyComponent() {
             addDataGenerator((str, json) -> {
                 json.put(DataCommunicatorConstants.DATA, str);
@@ -58,22 +56,6 @@ public class DummyData extends AbstractTestUIWithLog {
             });
         }
 
-        @Override
-        public Optional<String> getSelectedItem() {
-            return Optional.ofNullable(selected);
-        }
-
-        @Override
-        public void setValue(String item) {
-            if (selected != null) {
-                getDataCommunicator().refresh(selected);
-            }
-            selected = item;
-            if (selected != null) {
-                getDataCommunicator().refresh(selected);
-            }
-        }
-
         @Override
         public DataProvider<String, ?> getDataProvider() {
             return internalGetDataProvider();
@@ -98,12 +80,10 @@ public class DummyData extends AbstractTestUIWithLog {
 
         HorizontalLayout controls = new HorizontalLayout();
         addComponent(controls);
-        controls.addComponent(
-                new Button("Select Foo 20",
-                        event -> dummy.setValue("Foo " + 20)));
-        controls.addComponent(new Button("Reset data provider",
-                event -> dummy
-                        .setDataProvider(new LoggingDataProvider(items))));
+        controls.addComponent(new Button("Select Foo 20",
+                event -> dummy.setValue("Foo " + 20)));
+        controls.addComponent(new Button("Reset data provider", event -> dummy
+                .setDataProvider(new LoggingDataProvider(items))));
         controls.addComponent(
                 new Button("Remove all data", event -> dummy.setDataProvider(
                         new LoggingDataProvider(Collections.emptyList()))));

uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxCaptionAndIconUpdateTest.java

@@ -0,0 +1,98 @@
+package com.vaadin.tests.components.combobox;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+import org.openqa.selenium.By;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.ComboBoxElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class ComboBoxCaptionAndIconUpdateTest extends SingleBrowserTest {
+
+    @Test
+    public void testInitialData() {
+        openTestURL();
+
+        assertDisplayValues("fi.gif", "Commit 1");
+    }
+
+    @Test
+    public void testChangeIconProvider() {
+        openTestURL();
+        changeIconGenerator();
+
+        assertDisplayValues("m.gif", "Commit 1");
+    }
+
+    @Test
+    public void testChangeCaptionProvider() {
+        openTestURL();
+        changeCaptionGenerator();
+
+        assertDisplayValues("fi.gif", "Commit ID 1");
+    }
+
+    @Test
+    public void testItemAndCaptionProvider() {
+        openTestURL();
+        changeCaptionGenerator();
+        changeIconGenerator();
+
+        assertDisplayValues("m.gif", "Commit ID 1");
+    }
+
+    @Test
+    public void testEditCaption() {
+        openTestURL();
+        changeIconGenerator();
+        changeCaptionGenerator();
+
+        clickButton("editMsg");
+        assertDisplayValues("m.gif", "Edited message");
+    }
+
+    @Test
+    public void testEditIcon() {
+        openTestURL();
+        changeIconGenerator();
+        changeCaptionGenerator();
+
+        clickButton("editIcon");
+        assertDisplayValues("fi.gif", "Commit ID 1");
+    }
+
+    @Test
+    public void testEditIconAndCaption() {
+        openTestURL();
+        changeIconGenerator();
+        changeCaptionGenerator();
+
+        clickButton("editAll");
+        assertDisplayValues("fi.gif", "Edited message and icon");
+
+    }
+
+    private void assertDisplayValues(String iconName, String caption) {
+        ComboBoxElement comboBox = $(ComboBoxElement.class).first();
+        String iconURL = comboBox.findElement(By.tagName("img"))
+                .getAttribute("src");
+        assertTrue("Icon URL did not end with " + iconName,
+                iconURL.endsWith(iconName));
+        assertEquals("Caption did not match", caption, comboBox.getValue());
+    }
+
+    private void changeIconGenerator() {
+        clickButton("icon");
+    }
+
+    private void changeCaptionGenerator() {
+        clickButton("caption");
+    }
+
+    private void clickButton(String id) {
+        $(ButtonElement.class).id(id).click();
+    }
+}