Filter type support in Data View interfaces #8874

mshabarov · 2020-08-20T17:21:49Z

Part of https://github.com/vaadin/vaadin-combo-box-flow/issues/355

HasDataView and HasLazyDataView now contains F filter generic type in their declarations.
AbstractDataView requires a query supplier instead of just creating a Query with no filter and sorting. Components implies to provide such a query populated with their internal filter or sorting. Basically it delegates to DataCommunicator::buildQuery. For Select and CheckBoxGroup it will be new Query() like previously, because those components do not have them.
setItems(InMemoryDataProvider) is now supplemented with a filter converter, because it's impossible to just delegate it to generic setItems because of filter type in declaration. Old setItems(InMemoryDataProvider) methods proposed to be removed.

Grid vaadin/vaadin-grid-flow#1100
ComboBox vaadin/vaadin-combo-box-flow#380

Select TBD
CheckBoxGroup TBD

pleku · 2020-08-21T06:30:11Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasDataView.java

+     *            InMemoryDataProvider to use, not <code>null</code>
+     * @param filterConverter a function which converts a component's
+     *                        internal filter into a predicate to be
+     *                        applied to the all items of data provider


"...into a predicate applied to the data provider"

As for now this method looks as a candidate for being only in ComboBox, it will be fixed in ComboBox accordingly.

pleku · 2020-08-21T06:39:09Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasDataView.java

+
+    /**
+     * Sets an in-memory data provider for the component to use, taking into
+     * account only in-memory filtering from data provider.


I don't know if the filtering here is that important, since the setItems(DataProvider) neither mentions anything about it.
Maybe it would be OK to clarify in a later sentence that if the component has inbuilt filtering, then it the other method with filter converter should be used instead.

It will be mentioned in combobox that using "no converter" method overload throws, and ask to use the "with converter" method overload.

pleku · 2020-08-21T06:42:27Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasDataView.java

+    // TODO: probably we should just remove this methods, because it has a
+    //  significant drawback: it ignores the component's filter quietly, even
+    //  though it described in javadoc, so the developer can easily make a
+    //  mistake here.


Since ComboBox is still the only component with inbuilt filtering, it might be OK even still to just force the components to implement this method always (not have default implementation at all) and then make CB throw in the implementation, and point to the method with the filter converter instead, which would only be in CB.

I cannot say at the moment which is better or worse option.

Alright, I like this idea, because: 1. The implementation in custom components/addons might be not only in delegating to setItems(DataProvider<T, F> dataProvider) 2. Component's author will be responsible for handling the input data provider in implementation and he will decide whether ignore the filter or not.

pleku · 2020-08-21T06:42:30Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasDataView.java

-     * Sets an in-memory data provider for the component to use.
+     * Sets an in-memory data provider for the component to use, taking into
+     * account both in-memory filtering from data provider and component
+     * specific internal filter.


Would be better to start why one should use this method instead of explaining how it works:
"Sets an in-memory data provider to use with a component that has internal filtering, like {@code ComboBox}. For components without inbuilt filtering use ...... instead."

Will be done on ComboBox side

pleku · 2020-08-21T06:44:42Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasLazyDataView.java

@@ -45,12 +48,11 @@
     *
     * @param fetchCallback
     *            function that returns a stream of items from the backend based
-     *            on the offset and limit provided by the query object
+     *            on the filter, offset and limit provided by the query object


Since filter is basically optional, I would maybe mention that one last as "...and an optional filter"

pleku · 2020-08-21T07:26:16Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasLazyDataView.java

        extends Serializable {

    /**
     * Supply items lazily with a callback from a backend. The component will
     * automatically fetch more items and adjust its size until the backend runs
     * out of items. Usage example:
     * <p>
-     * {@code component.setItems(query -> orderService.getOrders(query.getOffset(), query.getLimit());}
+     * {@code component.setItems(query -> orderService.getOrders(
+     *                  query.getFilter, query.getOffset(), query.getLimit());}


I would move filter to be last since it is not valid for components which don't provide a filter.
Maybe even better to leave it out completely (keep it as it was) and add
"Usage example without component provided filter:"

pleku · 2020-08-21T07:27:47Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasLazyDataView.java

-     *                    query -> orderService.getSize());}
+     *                    query -> orderService.getOrders(query.getFilter,
+     *                              query.getOffset, query.getLimit()),
+     *                    query -> orderService.getSize(query.getFilter));}


Again, maybe for simplicity the filter could be kept out of this and then another comment could follow the example stating that "If the component supports filtering, it can be fetched with query.getFilter()."

pleku · 2020-08-21T07:28:04Z

flow-data/src/main/java/com/vaadin/flow/data/provider/HasLazyDataView.java

-     *                    query -> orderService.getOrders(query.getOffset, query.getLimit()),
-     *                    query -> orderService.getSize());}
+     *                    query -> orderService.getOrders(query.getFilter,
+     *                              query.getOffset, query.getLimit()),


Missing () x 2

Obsolete, because getFilter has been removed from example. Done

pleku · 2020-08-21T07:31:50Z

flow-data/src/main/java/com/vaadin/flow/data/provider/AbstractDataView.java

@@ -37,6 +38,7 @@
    protected static final String NULL_IDENTIFIER_ERROR_MESSAGE = "Identity provider should not return null";

    protected SerializableSupplier<? extends DataProvider<T, ?>> dataProviderSupplier;
+    protected SerializableBiFunction<Integer, Integer, Query<T, ?>> querySupplier;


Why is this needed as a property instead of just keeping the buildQuery method abstract ? Is it going to change on the fly ?

I would also keep expectations clearer by having this as an abstract method only.

I did it so initially, but there is an obstacle:

The goal of this method is to provide a query with component's actual filtering and sorting filled up. Making this method abstract means we will have to create a anonymous class just to put the values there:

public ComboBoxDataView<T> getGenericDataView() { return new AbstractDataView(dataCommunicator, this) { @Override public Query buildQuery(int offset, int limit) { return new Query(offset, limit, sortOrder, inMemorySorting, lastFilter); } } }

I don't want go this way, but create a regular class implementation instead.

Sorry, I might be wrong, but it needs a link to component's filter and sorting anyway, through the supplier, or through component's public method.

Ok. I think if combo box is the only component that needs this type of callback, then it can have a data view implementation which get those from the component when it constructs the data view for itself. For other purposes, this feels a bit of an antipattern that makes everything else overly complex. That is at least my feeling.

Hmm, then I could just revert my changes and just include this kind of supplier in ComboBoxDataView and ComboBoxListDataView only. It would be much simpler, thanks!

…er to combobox

pleku · 2020-08-21T12:41:03Z

flow-data/src/main/java/com/vaadin/flow/data/provider/AbstractListDataView.java

            Component component) {
-        super(dataProviderSupplier, querySupplier, component);
+        super(dataProviderSupplier, component);
    }

    @Override


Just noting here that both Grid and ComboBox need to have the getItems() method overridden for DataView and ListDataView due to sorting (grid) and filtering (cb). LazyDataView is safe because it uses data communicator

Yes, GridDataView and GridListDataView already override getItems and delegate to dataCommunicator.buildQuery(0, Integer.MAX_VALUE), will check and do the same for CB

vaadin-bot · 2020-08-21T15:02:42Z

SonarQube analysis reported 2 issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

AbstractListDataView.java#L85: Cast one of the operands of this addition operation to a "long".
AbstractListDataView.java#L94: Cast one of the operands of this subtraction operation to a "long".

Moves setItemCountCallback to Grid and ComboBox. Related to vaadin/vaadin-combo-box-flow#355

Filter type support in Data View interfaces

1534369

mshabarov added this to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) via automation Aug 20, 2020

mshabarov mentioned this pull request Aug 20, 2020

Filter support in Data View API vaadin/vaadin-grid-flow#1100

Merged

vaadin-bot added the +1.0.0 label Aug 20, 2020

pleku suggested changes Aug 21, 2020

View reviewed changes

Remove default implementations from HasDataView and move query suppli…

22a8cb0

…er to combobox

pleku previously approved these changes Aug 21, 2020

View reviewed changes

Move setItemCountCallback to Grid and ComboBox

9bf7cc1

mshabarov dismissed pleku’s stale review via 9bf7cc1 August 21, 2020 14:07

Remove Unused import

78f444f

mshabarov marked this pull request as ready for review August 24, 2020 07:13

pleku approved these changes Aug 24, 2020

View reviewed changes

pleku merged commit 91c7e18 into master Aug 24, 2020

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Aug 24, 2020

pleku deleted the 8646-add-filter-to-lazydataview branch August 24, 2020 07:32

pleku pushed a commit that referenced this pull request Aug 24, 2020

Filter type support in HasDataView interfaces (#8874)

043dd61

Moves setItemCountCallback to Grid and ComboBox. Related to vaadin/vaadin-combo-box-flow#355

pleku added target/4.0 labels Aug 24, 2020

This was referenced Aug 24, 2020

Filter support in Data View API vaadin/vaadin-select-flow#89

Merged

Filter support in Data View API vaadin/vaadin-checkbox-flow#162

Merged

pleku pushed a commit that referenced this pull request Aug 24, 2020

Filter type support in HasDataView interfaces (#8874)

554b1b4

Moves setItemCountCallback to Grid and ComboBox. Related to vaadin/vaadin-combo-box-flow#355

jhult mentioned this pull request Aug 26, 2020

HasDataView and HasLazyDataView now require filter generic type in their declarations mvysny/vaadin-on-kotlin#57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter type support in Data View interfaces #8874

Filter type support in Data View interfaces #8874

mshabarov commented Aug 20, 2020 •

edited

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

caalador Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020

pleku Aug 21, 2020

mshabarov Aug 21, 2020 •

edited

vaadin-bot commented Aug 21, 2020

Filter type support in Data View interfaces #8874

Filter type support in Data View interfaces #8874

Conversation

mshabarov commented Aug 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshabarov Aug 21, 2020 • edited

Choose a reason for hiding this comment

vaadin-bot commented Aug 21, 2020

mshabarov commented Aug 20, 2020 •

edited

mshabarov Aug 21, 2020 •

edited