From 6fb2d1a98076c6fbb9ba014fab37d7bf7a6e6a62 Mon Sep 17 00:00:00 2001 From: Anastasia Smirnova Date: Thu, 22 Mar 2018 15:39:18 +0200 Subject: [PATCH] Correctly handle data providers with overriden getId (#10728) Fixes #10647 Fixes #10498 --- .../com/vaadin/ui/AbstractMultiSelect.java | 37 +++++- .../ui/AbstractMultiSelectUsingIdTest.java | 97 +++++++++++++++ .../CheckCheckBoxGroupWithId.java | 71 +++++++++++ .../TwinColSelectAndIdBasedDataProvider.java | 110 ++++++++++++++++++ .../CheckCheckBoxGroupWithIdTest.java | 26 +++++ ...inColSelectAndIdBasedDataProviderTest.java | 28 +++++ 6 files changed, 365 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/com/vaadin/ui/AbstractMultiSelectUsingIdTest.java create mode 100644 uitest/src/main/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithId.java create mode 100644 uitest/src/main/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProvider.java create mode 100644 uitest/src/test/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithIdTest.java create mode 100644 uitest/src/test/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProviderTest.java diff --git a/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java b/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java index fdc4f5d4a1..558545d653 100644 --- a/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java +++ b/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java @@ -323,16 +323,25 @@ public abstract class AbstractMultiSelect extends AbstractListing // if there are duplicates, some item is both added & removed, just // discard that and leave things as was before - addedItems.removeIf(item -> removedItems.remove(item)); + DataProvider dataProvider = internalGetDataProvider(); + addedItems.removeIf(item -> { + Object addedId = dataProvider.getId(item); + return removedItems.stream().map(dataProvider::getId) + .anyMatch(addedId::equals)? removedItems.remove(item):false; + }); - if (selection.containsAll(addedItems) - && Collections.disjoint(selection, removedItems)) { + if (isAllSelected(addedItems) && isNoneSelected(removedItems)) { return; } updateSelection(set -> { // order of add / remove does not matter since no duplicates - set.removeAll(removedItems); + set.removeIf(item -> { + Object itemId = dataProvider.getId(item); + + return removedItems.stream().map(dataProvider::getId) + .anyMatch(itemId::equals); + }); set.addAll(addedItems); }, userOriginated); } @@ -359,6 +368,26 @@ public abstract class AbstractMultiSelect extends AbstractListing } + private boolean isAllSelected(Collection items) { + for (T item : items) { + if (!isSelected(item)) { + return false; + } + } + + return true; + } + + private boolean isNoneSelected(Collection items) { + for (T item : items) { + if (isSelected(item)) { + return false; + } + } + + return true; + } + /** * Deselects the given item. If the item is not currently selected, does * nothing. diff --git a/server/src/test/java/com/vaadin/ui/AbstractMultiSelectUsingIdTest.java b/server/src/test/java/com/vaadin/ui/AbstractMultiSelectUsingIdTest.java new file mode 100644 index 0000000000..4f131ba23d --- /dev/null +++ b/server/src/test/java/com/vaadin/ui/AbstractMultiSelectUsingIdTest.java @@ -0,0 +1,97 @@ +/* + * Copyright 2000-2016 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.ui; + +import static org.junit.Assert.assertEquals; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import org.junit.Before; +import org.junit.Test; + +import com.vaadin.data.provider.ListDataProvider; + +public class AbstractMultiSelectUsingIdTest { + + public TwinColSelect selectToTest; + + public static class ItemWithId { + private int id; + + public ItemWithId() { + } + + public ItemWithId(int id) { + this.id = id; + } + + public int getId() { + return id; + } + + public void setId(int id) { + this.id = id; + } + } + + @Before + public void setUp() { + selectToTest = new TwinColSelect<>(); + List items = new ArrayList<>(); + items.add(new ItemWithId(3)); + items.add(new ItemWithId(2)); + items.add(new ItemWithId(1)); + items.add(new ItemWithId(8)); + items.add(new ItemWithId(7)); + items.add(new ItemWithId(4)); + items.add(new ItemWithId(6)); + ListDataProvider dataProvider = new ListDataProvider( + items) { + @Override + public Object getId(ItemWithId item) { + return item.getId(); + } + }; + selectToTest.setDataProvider(dataProvider); + + } + + @Test + public void selectTwiceSelectsOnce() { + selectToTest.select(new ItemWithId(1)); + assertSelectionOrder(1); + selectToTest.select(new ItemWithId(1)); + assertSelectionOrder(1); + } + + @Test + public void deselectWorks() { + selectToTest.select(new ItemWithId(1)); + selectToTest.deselect(new ItemWithId(1)); + assertSelectionOrder(); + } + + private void assertSelectionOrder(Integer... selectionOrder) { + List asList = Arrays.asList(selectionOrder); + assertEquals(asList, selectToTest.getSelectedItems().stream() + .map(ItemWithId::getId).collect(Collectors.toList())); + } + +} diff --git a/uitest/src/main/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithId.java b/uitest/src/main/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithId.java new file mode 100644 index 0000000000..408e786a91 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithId.java @@ -0,0 +1,71 @@ +package com.vaadin.tests.components.checkboxgroup; + +import com.vaadin.data.provider.DataProvider; +import com.vaadin.data.provider.ListDataProvider; +import com.vaadin.server.SerializablePredicate; +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUIWithLog; +import com.vaadin.ui.CheckBoxGroup; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.Objects; + +public class CheckCheckBoxGroupWithId extends AbstractTestUIWithLog { + + @Override + protected void setup(VaadinRequest request) { + DataProvider> dataProvider = new ListDataProvider( + Arrays.asList(new MyObject("Yellow", "real"), + new MyObject("Red", "real"))) { + @Override + public Object getId(MyObject item) { + return item.getName(); + } + }; + + CheckBoxGroup checkBoxGroup = new CheckBoxGroup<>(); + checkBoxGroup.setItemCaptionGenerator(MyObject::getName); + checkBoxGroup.setDataProvider(dataProvider); + checkBoxGroup.setValue( + new HashSet<>(Arrays.asList(new MyObject("Yellow", null)))); + + addComponent(checkBoxGroup); + addButton("Deselect", + event -> checkBoxGroup.deselect(new MyObject("Yellow", "XX"))); + } + + public static class MyObject { + private final String name; + private final String other; + + public MyObject(String name, String other) { + this.name = name; + this.other = other; + } + + public String getName() { + return name; + } + + public String getOther() { + return other; + } + + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + MyObject myObject = (MyObject) o; + return Objects.equals(name, myObject.name) + && Objects.equals(other, myObject.other); + } + + @Override + public int hashCode() { + return Objects.hash(name, other); + } + } +} diff --git a/uitest/src/main/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProvider.java b/uitest/src/main/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProvider.java new file mode 100644 index 0000000000..e6dce9a443 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProvider.java @@ -0,0 +1,110 @@ +/* + * Copyright 2000-2016 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.tests.components.twincolselect; + +import java.util.Arrays; +import java.util.Collections; +import java.util.Objects; + +import com.vaadin.annotations.Widgetset; +import com.vaadin.data.provider.ListDataProvider; +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUIWithLog; +import com.vaadin.ui.Button; +import com.vaadin.ui.TwinColSelect; + +@Widgetset("com.vaadin.DefaultWidgetSet") +public class TwinColSelectAndIdBasedDataProvider extends AbstractTestUIWithLog { + + public static class MyObject { + private int id; + private String name; + + public MyObject() { + + } + + public MyObject(int id, String name) { + this.id = id; + this.name = name; + } + + public int getId() { + return id; + } + + public void setId(int id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + @Override + public String toString() { + return "MyObject{" + "id=" + id + ", name='" + name + '\'' + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + MyObject myObject = (MyObject) o; + return id == myObject.id && Objects.equals(name, myObject.name); + } + + @Override + public int hashCode() { + return Objects.hash(id, name); + } + } + + @Override + protected void setup(VaadinRequest request) { + ListDataProvider dataProvider = new ListDataProvider( + Arrays.asList(new MyObject(0, "Zero"), + new MyObject(1, "One"))) { + + @Override + public Object getId(MyObject item) { + return item.getId(); + } + }; + + TwinColSelect twinColSelect = new TwinColSelect<>(); + twinColSelect.setDataProvider(dataProvider); + + twinColSelect.setValue(Collections.singleton(new MyObject(1, null))); + + twinColSelect.addValueChangeListener( + event1 -> log.log("value: " + event1.getValue())); + + addComponent(twinColSelect); + addComponent(new Button("Deselect id=1", e -> { + twinColSelect.deselect(new MyObject(1, "foo")); + })); + } + +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithIdTest.java b/uitest/src/test/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithIdTest.java new file mode 100644 index 0000000000..afcaae52f3 --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/checkboxgroup/CheckCheckBoxGroupWithIdTest.java @@ -0,0 +1,26 @@ +package com.vaadin.tests.components.checkboxgroup; + +import com.vaadin.testbench.elements.ButtonElement; +import com.vaadin.testbench.elements.CheckBoxGroupElement; +import com.vaadin.tests.tb3.MultiBrowserTest; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class CheckCheckBoxGroupWithIdTest extends MultiBrowserTest { + private CheckBoxGroupElement checkBoxGroup; + + @Before + public void setUp() { + openTestURL(); + checkBoxGroup = $(CheckBoxGroupElement.class).first(); + } + + @Test + public void TestSelection() { + assertEquals(checkBoxGroup.getValue().size(), 1); + $(ButtonElement.class).first().click(); + assertEquals(checkBoxGroup.getValue().size(), 0); + } +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProviderTest.java b/uitest/src/test/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProviderTest.java new file mode 100644 index 0000000000..6475f43718 --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/twincolselect/TwinColSelectAndIdBasedDataProviderTest.java @@ -0,0 +1,28 @@ +package com.vaadin.tests.components.twincolselect; + +import com.vaadin.testbench.elements.ButtonElement; +import com.vaadin.testbench.elements.TwinColSelectElement; +import com.vaadin.tests.tb3.MultiBrowserTest; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class TwinColSelectAndIdBasedDataProviderTest extends MultiBrowserTest { + + @Before + public void setUp() { + openTestURL(); + } + + @Test + public void TestSelection() { + assertEquals(getTwinColElement().getValues().size(), 1); + $(ButtonElement.class).first().click(); + assertEquals(getTwinColElement().getValues().size(), 0); + } + + private TwinColSelectElement getTwinColElement() { + return $(TwinColSelectElement.class).first(); + } +} -- 2.39.5