diff options
7 files changed, 323 insertions, 149 deletions
diff --git a/all/src/main/templates/release-notes.html b/all/src/main/templates/release-notes.html index 88518e5644..c9d6957e99 100644 --- a/all/src/main/templates/release-notes.html +++ b/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> diff --git a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java index 541480ebd6..a4dad34830 100644 --- a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java +++ b/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); } /** @@ -188,108 +192,6 @@ public abstract class AbstractSingleSelect<T> extends AbstractListing<T> } /** - * 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; + } } diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java index 107e503256..494871bb2c 100644 --- a/server/src/main/java/com/vaadin/ui/ComboBox.java +++ b/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 diff --git a/server/src/test/java/com/vaadin/ui/ComboBoxTest.java b/server/src/test/java/com/vaadin/ui/ComboBoxTest.java index ea431ec614..74a194e96e 100644 --- a/server/src/test/java/com/vaadin/ui/ComboBoxTest.java +++ b/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"); diff --git a/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxCaptionAndIconUpdate.java b/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxCaptionAndIconUpdate.java new file mode 100644 index 0000000000..7e1e200a71 --- /dev/null +++ b/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; + } + +} diff --git a/uitest/src/main/java/com/vaadin/tests/data/DummyData.java b/uitest/src/main/java/com/vaadin/tests/data/DummyData.java index b4ecfd38c7..8842aca1de 100644 --- a/uitest/src/main/java/com/vaadin/tests/data/DummyData.java +++ b/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); @@ -59,22 +57,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())))); diff --git a/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxCaptionAndIconUpdateTest.java b/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxCaptionAndIconUpdateTest.java new file mode 100644 index 0000000000..5ef0fdf0fa --- /dev/null +++ b/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(); + } +} |