From 0860c1914940a8f7db7fb389bf742d7fedac47e0 Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Mon, 30 Jul 2018 12:48:32 +0300 Subject: [PATCH] Fix Grid MultiSelectionModel to always use getId (#11086) Fixes #11083 --- .../grid/MultiSelectionModelImpl.java | 50 +++++----- .../GridMultiSelectionModelProxyItemTest.java | 99 +++++++++++++++++++ .../grid/GridMultiSelectionModelTest.java | 4 +- 3 files changed, 128 insertions(+), 25 deletions(-) create mode 100644 server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelProxyItemTest.java diff --git a/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java b/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java index 3979a25a1a5..f51744c96e0 100644 --- a/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java +++ b/server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java @@ -15,23 +15,7 @@ */ package com.vaadin.ui.components.grid; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Objects; -import java.util.Set; -import java.util.function.Consumer; -import java.util.stream.Collectors; -import java.util.stream.Stream; - -import com.vaadin.data.provider.DataCommunicator; -import com.vaadin.data.provider.DataProvider; -import com.vaadin.data.provider.HierarchicalDataProvider; -import com.vaadin.data.provider.HierarchicalQuery; -import com.vaadin.data.provider.Query; +import com.vaadin.data.provider.*; import com.vaadin.event.selection.MultiSelectionEvent; import com.vaadin.event.selection.MultiSelectionListener; import com.vaadin.shared.Registration; @@ -39,6 +23,11 @@ import com.vaadin.shared.ui.grid.MultiSelectionModelState; import com.vaadin.ui.MultiSelect; +import java.util.*; +import java.util.function.Consumer; +import java.util.stream.Collectors; +import java.util.stream.Stream; + /** * Multiselection model for grid. *

@@ -429,12 +418,20 @@ protected void updateSelection(Set addedItems, Set removedItems, + " although user selection is disallowed"); } - // 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 = getGrid().getDataProvider(); - if (selection.containsAll(addedItems) - && Collections.disjoint(selection, removedItems)) { + addedItems.removeIf(item -> { + Object id = dataProvider.getId(item); + Optional toRemove = removedItems.stream() + .filter(i -> dataProvider.getId(i).equals(id)).findFirst(); + toRemove.ifPresent(i -> removedItems.remove(i)); + return toRemove.isPresent(); + }); + + if (addedItems.stream().map(dataProvider::getId) + .allMatch(this::selectionContainsId) + && removedItems.stream().map(dataProvider::getId) + .noneMatch(this::selectionContainsId)) { return; } @@ -446,8 +443,13 @@ protected void updateSelection(Set addedItems, Set removedItems, doUpdateSelection(set -> { // order of add / remove does not matter since no duplicates - set.removeAll(removedItems); - set.addAll(addedItems); + Set removedItemIds = removedItems.stream() + .map(dataProvider::getId).collect(Collectors.toSet()); + set.removeIf( + item -> removedItemIds.contains(dataProvider.getId(item))); + addedItems.stream().filter( + item -> !selectionContainsId(dataProvider.getId(item))) + .forEach(set::add); // refresh method is NOOP for items that are not present client side DataCommunicator dataCommunicator = getGrid() diff --git a/server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelProxyItemTest.java b/server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelProxyItemTest.java new file mode 100644 index 00000000000..53982fd288f --- /dev/null +++ b/server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelProxyItemTest.java @@ -0,0 +1,99 @@ +package com.vaadin.tests.components.grid; + +import com.liferay.portal.kernel.atom.AtomEntryContent; +import com.vaadin.data.provider.CallbackDataProvider; +import com.vaadin.ui.Grid; +import com.vaadin.ui.components.grid.MultiSelectionModel; +import org.junit.Before; +import org.junit.Test; + +import java.util.*; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +public class GridMultiSelectionModelProxyItemTest { + + private List data = IntStream.range(0, 100).boxed() + .map(i -> "String " + i).collect(Collectors.toList()); + private Grid> proxyGrid = new Grid<>(); + private MultiSelectionModel> model; + private AtomicReference>> selectionEvent = new AtomicReference<>(); + + @Before + public void setup() { + proxyGrid.setDataProvider(new CallbackDataProvider<>( + q -> data.stream().map(AtomicReference::new).skip(q.getOffset()) + .limit(q.getLimit()), + q -> data.size(), AtomicReference::get)); + model = (MultiSelectionModel>) proxyGrid + .setSelectionMode(Grid.SelectionMode.MULTI); + model.addSelectionListener(e -> { + selectionEvent.set(e.getAllSelectedItems()); + }); + } + + @Test + public void testSelectAllWithProxyDataProvider() { + model.selectAll(); + assertEquals("Item count mismatch on first select all", 100, + getSelectionEvent().size()); + model.deselect(model.getFirstSelectedItem().orElseThrow( + () -> new IllegalStateException("Items should be selected"))); + assertEquals("Item count mismatch on deselect", 99, + getSelectionEvent().size()); + model.selectAll(); + assertEquals("Item count mismatch on second select all", 100, + getSelectionEvent().size()); + } + + @Test + public void testUpdateSelectionWithDuplicateEntries() { + List selection = data.stream().filter(s -> s.contains("1")) + .collect(Collectors.toList()); + model.updateSelection(selection.stream().map(AtomicReference::new) + .collect(Collectors.toSet()), Collections.emptySet()); + assertEquals("Failure in initial selection", selection.size(), + getSelectionEvent().size()); + + String toRemove = model.getFirstSelectedItem().map(AtomicReference::get) + .orElseThrow(() -> new IllegalStateException( + "Items should be selected")); + model.updateSelection( + Stream.of(toRemove).map(AtomicReference::new) + .collect(Collectors.toSet()), + Stream.of(toRemove).map(AtomicReference::new) + .collect(Collectors.toSet())); + assertNull( + "Selection should not change when selecting and deselecting once", + selectionEvent.get()); + + Set> added = new LinkedHashSet<>(); + Set> removed = new LinkedHashSet<>(); + for (int i = 0; i < 20; ++i) { + added.add(new AtomicReference<>(toRemove)); + removed.add(new AtomicReference<>(toRemove)); + } + model.updateSelection(added, removed); + assertNull( + "Selection should not change when selecting and deselecting 20 times", + selectionEvent.get()); + + removed.add(new AtomicReference<>(toRemove)); + model.updateSelection(added, removed); + assertEquals("Item should have been deselected", selection.size() - 1, + getSelectionEvent().size()); + } + + private Set> getSelectionEvent() { + Optional>> eventOptional = Optional + .of(selectionEvent.get()); + selectionEvent.set(null); + return eventOptional.orElseThrow(() -> new IllegalStateException( + "Expected selection event never happened")); + } +} diff --git a/server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelTest.java b/server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelTest.java index 131dc2025bd..7d79cdaadca 100644 --- a/server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelTest.java +++ b/server/src/test/java/com/vaadin/tests/components/grid/GridMultiSelectionModelTest.java @@ -17,8 +17,11 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; import java.util.stream.IntStream; +import java.util.stream.Stream; +import com.vaadin.data.provider.CallbackDataProvider; import org.easymock.Capture; import org.junit.Before; import org.junit.Test; @@ -703,6 +706,5 @@ public void selectAllCheckboxVisible__lazyDataProvider() { assertFalse(model.isSelectAllCheckBoxVisible()); assertEquals(SelectAllCheckBoxVisibility.DEFAULT, model.getSelectAllCheckBoxVisibility()); - } }