From e397ea01e5ddb0c977561c008b84c6ed7c0ef706 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Leif=20=C3=85strand?= Date: Wed, 25 Jan 2017 09:44:16 +0200 Subject: [PATCH] Refactor how DataCommunicator passes filters to its DataProvider (#8309) The immediate benefit of this change is that ComboBox doesn't have to do any wrapping when a ListDataProvider is set. A secondary benefit is that a bunch of redundant type parameters and unsafe casts can be removed. This is one of many steps towards #8245 --- .../data/HasFilterableDataProvider.java | 25 ++++++-- .../data/provider/DataCommunicator.java | 56 +++++++++++------- .../java/com/vaadin/ui/AbstractListing.java | 17 ++++-- .../com/vaadin/ui/AbstractSingleSelect.java | 8 +-- .../src/main/java/com/vaadin/ui/ComboBox.java | 54 ++++++++++------- .../grid/MultiSelectionModelImpl.java | 2 +- .../data/provider/DataCommunicatorTest.java | 8 +-- .../provider/ReplaceDataProviderTest.java | 8 +-- .../combobox/ComboBoxFilteringTest.java | 59 ++++++++++++++++--- 9 files changed, 162 insertions(+), 75 deletions(-) diff --git a/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java b/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java index 03e2e30379..4d2964b2e5 100644 --- a/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java +++ b/server/src/main/java/com/vaadin/data/HasFilterableDataProvider.java @@ -16,6 +16,7 @@ package com.vaadin.data; import com.vaadin.data.provider.DataProvider; +import com.vaadin.server.SerializableFunction; /** * A generic interface for listing components that use a filterable data @@ -39,16 +40,32 @@ public interface HasFilterableDataProvider extends HasItems { /** * Returns the source of data items used by this listing. * - * @return the data provider, not null + * @return the data provider, not null */ - public DataProvider getDataProvider(); + public DataProvider getDataProvider(); /** * Sets the data provider for this listing. The data provider is queried for * displayed items as needed. * * @param dataProvider - * the data provider, not null + * the data provider, not null */ - public void setDataProvider(DataProvider dataProvider); + public default void setDataProvider(DataProvider dataProvider) { + setDataProvider(dataProvider, SerializableFunction.identity()); + } + + /** + * Sets the data provider and filter converter for this listing. The data + * provider is queried for displayed items as needed. + * + * @param dataProvider + * the data provider, not null + * @param filterConverter + * a function that converts filter values produced by this + * listing into filter values expected by the provided data + * provider, not null + */ + public void setDataProvider(DataProvider dataProvider, + SerializableFunction filterConverter); } diff --git a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java index 7b909b831d..021623555e 100644 --- a/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java +++ b/server/src/main/java/com/vaadin/data/provider/DataCommunicator.java @@ -29,6 +29,7 @@ import java.util.stream.Stream; import com.vaadin.server.AbstractExtension; import com.vaadin.server.KeyMapper; +import com.vaadin.server.SerializableConsumer; import com.vaadin.shared.Range; import com.vaadin.shared.Registration; import com.vaadin.shared.data.DataCommunicatorClientRpc; @@ -48,12 +49,10 @@ import elemental.json.JsonObject; * * @param * the bean type - * @param - * the filter type * * @since 8.0 */ -public class DataCommunicator extends AbstractExtension { +public class DataCommunicator extends AbstractExtension { private Registration dataProviderUpdateRegistration; @@ -190,7 +189,7 @@ public class DataCommunicator extends AbstractExtension { private final ActiveDataHandler handler = new ActiveDataHandler(); /** Empty default data provider */ - private DataProvider dataProvider = new CallbackDataProvider<>( + private DataProvider dataProvider = new CallbackDataProvider<>( q -> Stream.empty(), q -> 0); private final DataKeyMapper keyMapper; @@ -199,7 +198,7 @@ public class DataCommunicator extends AbstractExtension { private int minPushSize = 40; private Range pushRows = Range.withLength(0, minPushSize); - private F filter; + private Object filter; private Comparator inMemorySorting; private final List backEndSorting = new ArrayList<>(); private final DataCommunicatorClientRpc rpc; @@ -236,7 +235,8 @@ public class DataCommunicator extends AbstractExtension { } if (initial || reset) { - int dataProviderSize = getDataProvider().size(new Query<>(filter)); + @SuppressWarnings({ "rawtypes", "unchecked" }) + int dataProviderSize = getDataProvider().size(new Query(filter)); rpc.reset(dataProviderSize); } @@ -244,7 +244,8 @@ public class DataCommunicator extends AbstractExtension { int offset = pushRows.getStart(); int limit = pushRows.length(); - Stream rowsToPush = getDataProvider().fetch(new Query<>(offset, + @SuppressWarnings({ "rawtypes", "unchecked" }) + Stream rowsToPush = getDataProvider().fetch(new Query(offset, limit, backEndSorting, inMemorySorting, filter)); pushData(offset, rowsToPush); @@ -398,17 +399,6 @@ public class DataCommunicator extends AbstractExtension { updatedData.add(data); } - /** - * Sets the filter to use. - * - * @param filter - * the filter - */ - public void setFilter(F filter) { - this.filter = filter; - reset(); - } - /** * Sets the {@link Comparator} to use with in-memory sorting. * @@ -459,19 +449,31 @@ public class DataCommunicator extends AbstractExtension { * * @return the data provider */ - public DataProvider getDataProvider() { + public DataProvider getDataProvider() { return dataProvider; } /** * Sets the current data provider for this DataCommunicator. + *

+ * The returned consumer can be used to set some other filter value that + * should be included in queries sent to the data provider. It is only valid + * until another data provider is set. * * @param dataProvider - * the data provider to set, not null + * the data provider to set, not null + * @param initialFilter + * the initial filter value to use, or null to not + * use any initial filter value + * @return a consumer that accepts a new filter value to use */ - public void setDataProvider(DataProvider dataProvider) { + public SerializableConsumer setDataProvider( + DataProvider dataProvider, F initialFilter) { Objects.requireNonNull(dataProvider, "data provider cannot be null"); + + filter = initialFilter; this.dataProvider = dataProvider; + detachDataProviderListener(); dropAllData(); /* @@ -491,6 +493,18 @@ public class DataCommunicator extends AbstractExtension { attachDataProviderListener(); } reset(); + + return filter -> { + if (this.dataProvider != dataProvider) { + throw new IllegalStateException( + "Filter slot is no longer valid after data provider has been changed"); + } + + if (!Objects.equals(this.filter, filter)) { + this.filter = filter; + reset(); + } + }; } /** diff --git a/server/src/main/java/com/vaadin/ui/AbstractListing.java b/server/src/main/java/com/vaadin/ui/AbstractListing.java index c3328683a1..dfb8ac103e 100644 --- a/server/src/main/java/com/vaadin/ui/AbstractListing.java +++ b/server/src/main/java/com/vaadin/ui/AbstractListing.java @@ -29,6 +29,7 @@ import com.vaadin.data.provider.DataProvider; import com.vaadin.data.provider.Query; import com.vaadin.server.AbstractExtension; import com.vaadin.server.Resource; +import com.vaadin.server.SerializableConsumer; import com.vaadin.shared.extension.abstractlisting.AbstractListingExtensionState; import com.vaadin.shared.ui.abstractlisting.AbstractListingState; import com.vaadin.ui.Component.Focusable; @@ -132,7 +133,7 @@ public abstract class AbstractListing extends AbstractComponent } } - private final DataCommunicator dataCommunicator; + private final DataCommunicator dataCommunicator; /** * Creates a new {@code AbstractListing} with a default data communicator. @@ -154,7 +155,7 @@ public abstract class AbstractListing extends AbstractComponent * @param dataCommunicator * the data communicator to use, not null */ - protected AbstractListing(DataCommunicator dataCommunicator) { + protected AbstractListing(DataCommunicator dataCommunicator) { Objects.requireNonNull(dataCommunicator, "dataCommunicator cannot be null"); @@ -162,10 +163,14 @@ public abstract class AbstractListing extends AbstractComponent addExtension(dataCommunicator); } - @SuppressWarnings({ "rawtypes", "unchecked" }) protected void internalSetDataProvider(DataProvider dataProvider) { - // cast needed by compiler - getDataCommunicator().setDataProvider((DataProvider) dataProvider); + internalSetDataProvider(dataProvider, null); + } + + protected SerializableConsumer internalSetDataProvider( + DataProvider dataProvider, F initialFilter) { + return getDataCommunicator().setDataProvider(dataProvider, + initialFilter); } protected DataProvider internalGetDataProvider() { @@ -257,7 +262,7 @@ public abstract class AbstractListing extends AbstractComponent * * @return the data communicator, not null */ - public DataCommunicator getDataCommunicator() { + public DataCommunicator getDataCommunicator() { return dataCommunicator; } diff --git a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java index 224d6823e7..2b0e6b7d01 100644 --- a/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java +++ b/server/src/main/java/com/vaadin/ui/AbstractSingleSelect.java @@ -74,7 +74,7 @@ public abstract class AbstractSingleSelect extends AbstractListing * @param dataCommunicator * the data communicator to use, not null */ - protected AbstractSingleSelect(DataCommunicator dataCommunicator) { + protected AbstractSingleSelect(DataCommunicator dataCommunicator) { super(dataCommunicator); init(); } @@ -152,9 +152,9 @@ public abstract class AbstractSingleSelect extends AbstractListing @Override public Registration addValueChangeListener( HasValue.ValueChangeListener listener) { - return addSelectionListener(event -> listener.valueChange( - new ValueChangeEvent<>(this, event.getOldValue(), - event.isUserOriginated()))); + return addSelectionListener( + event -> listener.valueChange(new ValueChangeEvent<>(this, + event.getOldValue(), event.isUserOriginated()))); } @Override diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java index d08f269fa3..515c581510 100644 --- a/server/src/main/java/com/vaadin/ui/ComboBox.java +++ b/server/src/main/java/com/vaadin/ui/ComboBox.java @@ -43,6 +43,7 @@ import com.vaadin.server.Resource; import com.vaadin.server.ResourceReference; import com.vaadin.server.SerializableBiPredicate; import com.vaadin.server.SerializableConsumer; +import com.vaadin.server.SerializableFunction; import com.vaadin.shared.Registration; import com.vaadin.shared.data.DataCommunicatorConstants; import com.vaadin.shared.ui.combobox.ComboBoxConstants; @@ -125,7 +126,8 @@ public class ComboBox extends AbstractSingleSelect @Override public void setFilter(String filterText) { - getDataCommunicator().setFilter(filterText); + currentFilterText = filterText; + filterSlot.accept(filterText); } }; @@ -136,13 +138,19 @@ public class ComboBox extends AbstractSingleSelect private StyleGenerator itemStyleGenerator = item -> null; + private String currentFilterText; + + private SerializableConsumer filterSlot = filter -> { + // Just ignore when neither setDataProvider nor setItems has been called + }; + /** * Constructs an empty combo box without a caption. The content of the combo * box can be set with {@link #setDataProvider(DataProvider)} or * {@link #setItems(Collection)} */ public ComboBox() { - super(new DataCommunicator() { + super(new DataCommunicator() { @Override protected DataKeyMapper createKeyMapper() { return new KeyMapper() { @@ -294,11 +302,6 @@ public class ComboBox extends AbstractSingleSelect * Sets a list data provider with an item caption filter as the data * provider of this combo box. The caption filter is used to compare the * displayed caption of each item to the filter text entered by the user. - *

- * Note that this is a shorthand that calls - * {@link #setDataProvider(DataProvider)} with a wrapper of the provided - * list data provider. This means that {@link #getDataProvider()} will - * return the wrapper instead of the original list data provider. * * @param captionFilter * filter to check if an item is shown when user typed some text @@ -313,9 +316,8 @@ public class ComboBox extends AbstractSingleSelect // Must do getItemCaptionGenerator() for each operation since it might // not be the same as when this method was invoked - DataProvider provider = listDataProvider.filteringBy( - item -> getItemCaptionGenerator().apply(item), captionFilter); - setDataProvider(provider); + setDataProvider(listDataProvider, filterText -> item -> captionFilter + .test(getItemCaptionGenerator().apply(item), filterText)); } /** @@ -702,22 +704,30 @@ public class ComboBox extends AbstractSingleSelect } @Override - @SuppressWarnings("unchecked") - public DataProvider getDataProvider() { - return (DataProvider) internalGetDataProvider(); + public DataProvider getDataProvider() { + return internalGetDataProvider(); } @Override - public void setDataProvider(DataProvider dataProvider) { - internalSetDataProvider(dataProvider); - } + public void setDataProvider(DataProvider dataProvider, + SerializableFunction filterConverter) { + Objects.requireNonNull(dataProvider, "dataProvider cannot be null"); + Objects.requireNonNull(filterConverter, + "filterConverter cannot be null"); + + SerializableFunction convertOrNull = filterText -> { + if (filterText == null) { + return null; + } - @Override - @SuppressWarnings("unchecked") - public DataCommunicator getDataCommunicator() { - // Not actually an unsafe cast. DataCommunicator is final and set by - // ComboBox. - return (DataCommunicator) super.getDataCommunicator(); + return filterConverter.apply(filterText); + }; + + SerializableConsumer providerFilterSlot = internalSetDataProvider( + dataProvider, convertOrNull.apply(currentFilterText)); + + filterSlot = filter -> providerFilterSlot + .accept(convertOrNull.apply(filter)); } @Override 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 89e7374bc6..551918ef85 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 @@ -436,7 +436,7 @@ public class MultiSelectionModelImpl extends AbstractSelectionModel set.addAll(addedItems); // refresh method is NOOP for items that are not present client side - DataCommunicator dataCommunicator = getGrid() + DataCommunicator dataCommunicator = getGrid() .getDataCommunicator(); removedItems.forEach(dataCommunicator::refresh); addedItems.forEach(dataCommunicator::refresh); diff --git a/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java b/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java index f49dbf1f88..50a3ebc337 100644 --- a/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java +++ b/server/src/test/java/com/vaadin/data/provider/DataCommunicatorTest.java @@ -22,7 +22,6 @@ import org.junit.Test; import org.mockito.Mockito; import com.vaadin.server.MockVaadinSession; -import com.vaadin.server.SerializablePredicate; import com.vaadin.server.VaadinRequest; import com.vaadin.server.VaadinService; import com.vaadin.server.VaadinSession; @@ -81,8 +80,7 @@ public class DataCommunicatorTest { } - private static class TestDataCommunicator - extends DataCommunicator> { + private static class TestDataCommunicator extends DataCommunicator { protected void extend(UI ui) { super.extend(ui); } @@ -100,7 +98,7 @@ public class DataCommunicatorTest { TestDataCommunicator communicator = new TestDataCommunicator(); TestDataProvider dataProvider = new TestDataProvider(); - communicator.setDataProvider(dataProvider); + communicator.setDataProvider(dataProvider, null); Assert.assertFalse(dataProvider.isListenerAdded()); @@ -118,7 +116,7 @@ public class DataCommunicatorTest { TestDataCommunicator communicator = new TestDataCommunicator(); TestDataProvider dataProvider = new TestDataProvider(); - communicator.setDataProvider(dataProvider); + communicator.setDataProvider(dataProvider, null); communicator.extend(ui); diff --git a/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java b/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java index 0510022e5e..f64d6b27c5 100644 --- a/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java +++ b/server/src/test/java/com/vaadin/data/provider/ReplaceDataProviderTest.java @@ -11,8 +11,6 @@ import java.util.stream.IntStream; import org.junit.Test; -import com.vaadin.server.SerializablePredicate; - public class ReplaceDataProviderTest { public static class BeanWithEquals extends Bean { @@ -73,14 +71,14 @@ public class ReplaceDataProviderTest { private void doTest(IntFunction beanConstructor) { - DataCommunicator> dataCommunicator = new DataCommunicator<>(); + DataCommunicator dataCommunicator = new DataCommunicator<>(); List beans1 = createCollection(beanConstructor); ListDataProvider dataProvider = new ListDataProvider<>( beans1); - dataCommunicator.setDataProvider(dataProvider); + dataCommunicator.setDataProvider(dataProvider, null); dataCommunicator.pushData(1, beans1.stream()); SOME_BEAN bean1_17 = beans1.get(17); @@ -91,7 +89,7 @@ public class ReplaceDataProviderTest { List beans2 = createCollection(beanConstructor); dataProvider = new ListDataProvider<>(beans2); - dataCommunicator.setDataProvider(dataProvider); + dataCommunicator.setDataProvider(dataProvider, null); dataCommunicator.pushData(1, beans2.stream()); SOME_BEAN bean2_17 = beans2.get(17); diff --git a/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java b/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java index e8c8d3dd6e..159af4ad0b 100644 --- a/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java +++ b/server/src/test/java/com/vaadin/tests/server/component/combobox/ComboBoxFilteringTest.java @@ -15,6 +15,7 @@ */ package com.vaadin.tests.server.component.combobox; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.stream.Collectors; @@ -24,13 +25,16 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import com.vaadin.data.provider.DataCommunicator; import com.vaadin.data.provider.DataProvider; import com.vaadin.data.provider.ListDataProvider; -import com.vaadin.data.provider.Query; +import com.vaadin.server.ClientMethodInvocation; +import com.vaadin.shared.ui.combobox.ComboBoxServerRpc; import com.vaadin.tests.data.bean.Address; import com.vaadin.tests.data.bean.Person; import com.vaadin.tests.data.bean.Sex; import com.vaadin.ui.ComboBox; +import com.vaadin.ui.ComponentTest; /** * Test for ComboBox data providers and filtering. @@ -178,21 +182,62 @@ public class ComboBoxFilteringTest { checkFiltering("t", "Engel", 2, 1); } + @Test + public void filterEmptyComboBox() { + // Testing that filtering doesn't cause problems in the edge case where + // neither setDataProvider nor setItems has been called + checkFiltering("foo", "bar", 0, 0); + } + + @Test + public void setListDataProvider_notWrapped() { + ListDataProvider provider = new ListDataProvider<>( + Collections.emptyList()); + + comboBox.setDataProvider(provider); + + Assert.assertSame(provider, comboBox.getDataProvider()); + } + + @Test + public void setItems_hasListDataProvider() { + comboBox.setItems(); + + Assert.assertEquals(ListDataProvider.class, + comboBox.getDataProvider().getClass()); + } + private void checkFiltering(String filterText, String nonMatchingFilterText, int totalMatches, int matchingResults) { - DataProvider dataProvider = comboBox.getDataProvider(); - Assert.assertEquals( "ComboBox filtered out results with no filter applied", - totalMatches, dataProvider.size(new Query<>())); + totalMatches, comboBoxSizeWithFilter(null)); Assert.assertEquals( "ComboBox filtered out results with empty filter string", - totalMatches, dataProvider.size(new Query<>(""))); + totalMatches, comboBoxSizeWithFilter("")); Assert.assertEquals("ComboBox filtered out wrong number of results", - matchingResults, dataProvider.size(new Query<>(filterText))); + matchingResults, comboBoxSizeWithFilter(filterText)); Assert.assertEquals( "ComboBox should have no results with a non-matching filter", 0, - dataProvider.size(new Query<>(nonMatchingFilterText))); + comboBoxSizeWithFilter(nonMatchingFilterText)); + } + + private int comboBoxSizeWithFilter(String filter) { + DataCommunicator dataCommunicator = comboBox + .getDataCommunicator(); + + // Discard any currently pending RPC calls + dataCommunicator.retrievePendingRpcCalls(); + + ComponentTest.getRpcProxy(comboBox, ComboBoxServerRpc.class) + .setFilter(filter); + dataCommunicator.beforeClientResponse(true); + + ClientMethodInvocation resetInvocation = dataCommunicator + .retrievePendingRpcCalls().get(0); + assert resetInvocation.getMethodName().equals("reset"); + + return ((Integer) resetInvocation.getParameters()[0]).intValue(); } private List getPersonCollection() { -- 2.39.5