Add lazy loading #131

pekam · 2018-10-02T09:45:11Z

Items are loaded lazily one page at a time as the user scrolls down the overlay
Added ItemFilter API for defining custom server-side filtering function
- This is different from V8 CaptionFilter, to allow filtering based on other properties than just the caption/label. It is useful especially when using more properties in the renderer.
When the size of the data set is less than pageSize, and the user has not defined a server-side filter function, the filtering will happen in the client-side, providing smoother user experience and reducing network traffic
Server-side filtering is debounced by 500ms to avoid sending requests and triggering server-side filtering all the time as the user types into the field
Default pageSize is 50
Breaking changes:
- setFilteredItems and getFilteredItems API removed
  - Use ItemFilter instead to implement custom filtering
- setNullRepresentation and getNullRepresentation API removed
  - DataProvider doesn't allow null items in the data
  - It makes null value ambiguous: Should setValue(null) clear the value or the select the null item?
- Calling setValue with an item which is not included in the data set doesn't throw an exception anymore
  - With large data sets, or a callback data provider connected to a database, it's not feasible to figure out whether an item is included

Resolves tickets #72 #17

This change is

Now all the tests are passing

…a provider

vaadin-bot · 2018-10-11T07:14:24Z

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java

+     * 
+     * @return the data provider used by this ComboBox
+     */
+    public DataProvider<T, ?> getDataProvider() {


Remove usage of generic wildcard type.

vaadin-bot · 2018-10-11T07:14:24Z

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java


+    private void initConnector() {


Move this method into the anonymous class declared at line 170.

vaadin-bot · 2018-10-11T07:14:25Z

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java

@@ -107,39 +135,107 @@ public void remove() {
        }
    }

-    private class RefreshJob implements SerializableConsumer<UI> {
+    private final class UpdateQueue implements Update {
+        private List<Runnable> queue = new ArrayList<>();


Make "queue" transient or serializable.

vaadin-bot · 2018-10-11T07:14:26Z

SonarQube analysis reported 4 issues

2 critical
2 minor

Watch the comments in this conversation to review them.

vaadin-bot · 2018-10-11T07:14:26Z

src/main/java/com/vaadin/flow/component/combobox/GeneratedVaadinComboBox.java

 import com.vaadin.flow.component.Tag;
 import com.vaadin.flow.component.dependency.HtmlImport;
+import com.vaadin.flow.component.HasStyle;
+import com.vaadin.flow.component.HasValue;


Remove this unused import 'com.vaadin.flow.component.HasValue'.

Fixes a bug: Otherwise when using server-side filtering, when you change the filter from empty to something else and back to empty within the debounce timeout, the overlay remains empty as the DataCommunicator thinks it doesn't need to send data.

gilberto-torrezan

Reviewed 3 of 18 files at r1, 17 of 22 files at r2, 3 of 3 files at r3.
Reviewable status: 20 of 23 files reviewed, 16 unresolved discussions (waiting on @gilberto-torrezan, @vaadin-bot, and @pekam)

a discussion (no related file):
There's a potential mismatch between the client-side filtering and the server-side one, if the browser has a different locale than the server (see the example with the i letter in Turkish). It doesn't need to be taken care of right now, but it's something to consider for next versions.

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 209 at r3 (raw file):

    };

    private Boolean forceServerSideFiltering;

I got that you wanted to have 3 states in a single variable, but the null case is hard to understand (and thus, maintain). I suggest using two booleans instead, or an enum with 3 values, with clear names on what each one represents.

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 231 at r3 (raw file):

        setItemValuePath("key");
        setItemIdPath("key");
        setPageSize(pageSize);

In Grid we had to support a public setPageSize method, because when using the component with @Id, only the empty constructor is invoked. You probably need to do that here as well, or mark somehow to be implemented in a future alpha.

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 306 at r3 (raw file):

        if (value != null && getKeyMapper().has(value)) {
            value = getKeyMapper().get(getKeyMapper().key(value));

You can optimize this to only call the getKeyMapper once.

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 312 at r3 (raw file):

        // item is not yet loaded
        JsonObject json = Json.createObject();
        json.put("key", getKeyMapper().key(value));

If the value is null, something crazy would happen here.... or would it?

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 846 at r3 (raw file):

        }
        runBeforeClientResponse(ui -> ui.getPage().executeJavaScript(
                "if($0.$connector) $0.$connector.reset();", getElement()));

Is there a situation where the $connector is undefined for the combo-box instrumented by the server?

src/main/resources/META-INF/resources/frontend/comboBoxConnector.js, line 68 at r3 (raw file):

      }

      const firstPage = index / comboBox.pageSize;

It's not nice to have two variables with the same name in different scopes. Weird things can happen when the code gets refactored. I suggest changing the name of this one to something else.

src/main/resources/META-INF/resources/frontend/comboBoxConnector.js, line 102 at r3 (raw file):

// Should be done by clearCache()

And why is not? Now that you control client and server-side, those things should work together.

if it's not possible to do it now for whatever reason, create a TODO here with the ticket number.

src/test/java/com/vaadin/flow/component/combobox/ComboBoxElementUpdated.java, line 25 at r3 (raw file):

 * @author Vaadin Ltd
 */
public class ComboBoxElementUpdated extends ComboBoxElement {

Why not updating the ComboBoxElement itself?

gilberto-torrezan

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @gilberto-torrezan, @vaadin-bot, and @pekam)

pekam

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @gilberto-torrezan, @vaadin-bot, and @pekam)

a discussion (no related file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

There's a potential mismatch between the client-side filtering and the server-side one, if the browser has a different locale than the server (see the example with the i letter in Turkish). It doesn't need to be taken care of right now, but it's something to consider for next versions.

Yes. The reason why I left it out from the client-side is that the web component itself doesn't care about locales with the default filtering.

Maybe I should not use locale in the server either, to make it match with the client filtering, or what do you think?

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 306 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

        if (value != null && getKeyMapper().has(value)) {
            value = getKeyMapper().get(getKeyMapper().key(value));

You can optimize this to only call the getKeyMapper once.

Done.

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 312 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

If the value is null, something crazy would happen here.... or would it?

We have at least ClearValueIT testing that you can use setValue(null)

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 846 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

Is there a situation where the $connector is undefined for the combo-box instrumented by the server?

When the component is first attached, this is executed before connector's initLazy.
Of course this call is not required at all at that point, but it comes from setDataProvider and setRenderer.

src/main/resources/META-INF/resources/frontend/comboBoxConnector.js, line 68 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

It's not nice to have two variables with the same name in different scopes. Weird things can happen when the code gets refactored. I suggest changing the name of this one to something else.

Nice catch, I didn't notice at all.
Done.

src/main/resources/META-INF/resources/frontend/comboBoxConnector.js, line 102 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

// Should be done by clearCache()

And why is not? Now that you control client and server-side, those things should work together.

if it's not possible to do it now for whatever reason, create a TODO here with the ticket number.

This is a bug which I found in the web component, and it was already fixed but not released yet.
I asked Vladimir and he promised to release a new alpha soon.

gilberto-torrezan

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gilberto-torrezan, @vaadin-bot, and @pekam)

a discussion (no related file):

Previously, pekam (Pekka Maanpää) wrote…

Yes. The reason why I left it out from the client-side is that the web component itself doesn't care about locales with the default filtering.

Maybe I should not use locale in the server either, to make it match with the client filtering, or what do you think?

The locale support is yet a different beast on all of this. IMO there should be a way to get the locale used by the browser and send it to the server, and then the server would use that as the default, but let's no design this here. I suggest creating a ticket for it.

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 846 at r3 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

When the component is first attached, this is executed before connector's initLazy.
Of course this call is not required at all at that point, but it comes from setDataProvider and setRenderer.

I suggest adding that as a comment in the code, for maintenability purposes.

pekam

Reviewable status: 19 of 23 files reviewed, 11 unresolved discussions (waiting on @gilberto-torrezan, @vaadin-bot, and @pekam)

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 231 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

In Grid we had to support a public setPageSize method, because when using the component with @Id, only the empty constructor is invoked. You probably need to do that here as well, or mark somehow to be implemented in a future alpha.

Done.

…ined

pekam

Reviewable status: 19 of 23 files reviewed, 10 unresolved discussions (waiting on @gilberto-torrezan, @vaadin-bot, and @pekam)

a discussion (no related file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

The locale support is yet a different beast on all of this. IMO there should be a way to get the locale used by the browser and send it to the server, and then the server would use that as the default, but let's no design this here. I suggest creating a ticket for it.

https://github.com/vaadin/vaadin-combo-box-flow/issues/138

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 209 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

I got that you wanted to have 3 states in a single variable, but the null case is hard to understand (and thus, maintain). I suggest using two booleans instead, or an enum with 3 values, with clear names on what each one represents.

Done.
I couldn't come up with any nicer-sounding enum, and I think using two booleans would be too messy.
If you have better naming suggestions, feel free to share them.

src/main/java/com/vaadin/flow/component/combobox/ComboBox.java, line 846 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

I suggest adding that as a comment in the code, for maintenability purposes.

Done.

gilberto-torrezan

Reviewed 4 of 4 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @vaadin-bot and @pekam)

pekam

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @vaadin-bot and @pekam)

src/test/java/com/vaadin/flow/component/combobox/ComboBoxElementUpdated.java, line 25 at r3 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

Why not updating the ComboBoxElement itself?

Because it is still in a different repository and I don't want to wait for a new release of it.

I will create a ticket for TB once this is in, so I can link to this file.

gilberto-torrezan

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @vaadin-bot)

pekam

Dismissed @vaadin-bot from 4 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved

pekam added 30 commits September 20, 2018 14:09

wip lazy-loading

9a72c02

Fix most issues and add missing methods

23e774d

Switch to using the feature branch of the wc

7995634

Clean unused functions from connector

09def6b

Fix item label generator

96b980b

Add ctor for custom pageSize

9d1865c

Use dp for setitems

1200010

Some cleanup + javadocs

958409e

Change from LabelFilter to ItemFilter

27da422

Improve javadocs for setItems()

30be1aa

Fix label generator null handling

529d628

Fix test by executing beforeClientResponse

9ce89d0

Fix unit tests

e2260a4

set items instead of dataprovider to fix tests

01d389f

Render max once in a roundtrip

ddc7d01

Fix id in test

68a086f

Reset items when changing label generator

f543996

Now all the tests are passing

Improve javadocs

1eb8a40

Re-add/fix filteredItems API

3e38368

Add IT for lazily loading items and using component renderer with dat…

4d4f765

…a provider

Add test for value change

8667ac5

Add test for setValue

53cad1a

Add test for custom pageSize

65bdb69

Update web component branch

b563ee5

Add bean combobox to lazyloading view

ae0a051

Fix selection when item not yet loaded

1beffc7

Add test for initial value

792cf86

Add tests for changing renderer, labelgenerator and dataprovider

d69ecac

Avoid adding multiple filter listeners

c557381

Remove connector update function

f6cd146

pekam added 4 commits October 10, 2018 18:37

Debounce server-side filtering

e13cd24

Fix tests after debouncing

6df69b4

Merge branch 'master' into lazy-loading

6e48b42

Try to make LazyLoadingIT more stable by not scrolling too far

cd55854

vaadin-bot reviewed Oct 11, 2018

View reviewed changes

pekam added 2 commits October 11, 2018 10:39

Test that increasing pageSize affects client-side filtering

25c0a28

gilberto-torrezan suggested changes Oct 12, 2018

View reviewed changes

gilberto-torrezan reviewed Oct 12, 2018

View reviewed changes

pekam added 3 commits October 12, 2018 13:53

Rename duplicate js variable

a9a9968

Improve comment on clearCache workaround

c7d8f0e

Call getKeyMapper only once in setValue

bb01b5a

pekam commented Oct 12, 2018

View reviewed changes

gilberto-torrezan suggested changes Oct 12, 2018

View reviewed changes

Add setter and getter for pageSize

3def766

pekam commented Oct 12, 2018

View reviewed changes

pekam added 2 commits October 12, 2018 15:41

Add comment for resetting the connector

f92e51b

Use enum instead of Boolean for checking if server-side filter is def…

9530198

…ined

pekam commented Oct 12, 2018

View reviewed changes

gilberto-torrezan approved these changes Oct 12, 2018

View reviewed changes

pekam commented Oct 12, 2018

View reviewed changes

gilberto-torrezan approved these changes Oct 12, 2018

View reviewed changes

pekam commented Oct 12, 2018

View reviewed changes

pekam merged commit c17fcfe into master Oct 12, 2018

pekam deleted the lazy-loading branch October 12, 2018 13:47

pekam added this to the 2.0.0.alpha1 milestone Oct 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lazy loading #131

Add lazy loading #131

pekam commented Oct 2, 2018 •

edited

vaadin-bot Oct 11, 2018

vaadin-bot Oct 11, 2018 •

edited

vaadin-bot Oct 11, 2018

vaadin-bot commented Oct 11, 2018

vaadin-bot Oct 11, 2018

gilberto-torrezan left a comment

gilberto-torrezan left a comment

pekam left a comment

gilberto-torrezan left a comment

pekam left a comment

pekam left a comment

gilberto-torrezan left a comment

pekam left a comment

gilberto-torrezan left a comment

pekam left a comment

Add lazy loading #131

Add lazy loading #131

Conversation

pekam commented Oct 2, 2018 • edited

vaadin-bot Oct 11, 2018

Choose a reason for hiding this comment

vaadin-bot Oct 11, 2018 • edited

Choose a reason for hiding this comment

vaadin-bot Oct 11, 2018

Choose a reason for hiding this comment

vaadin-bot commented Oct 11, 2018

vaadin-bot Oct 11, 2018

Choose a reason for hiding this comment

gilberto-torrezan left a comment

Choose a reason for hiding this comment

gilberto-torrezan left a comment

Choose a reason for hiding this comment

pekam left a comment

Choose a reason for hiding this comment

gilberto-torrezan left a comment

Choose a reason for hiding this comment

pekam left a comment

Choose a reason for hiding this comment

pekam left a comment

Choose a reason for hiding this comment

gilberto-torrezan left a comment

Choose a reason for hiding this comment

pekam left a comment

Choose a reason for hiding this comment

gilberto-torrezan left a comment

Choose a reason for hiding this comment

pekam left a comment

Choose a reason for hiding this comment

pekam commented Oct 2, 2018 •

edited

vaadin-bot Oct 11, 2018 •

edited