Improve caching when expanding nodes in hierarchical data

[tsuoanttila]

Fixes #8790

---

This change is 

[hesara]

Reviewed 2 of 4 files at r1.
Review status: 2 of 4 files reviewed at latest revision, 3 unresolved discussions.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 187 at r1 (raw file):

     * This is the cache portion that was moved due to rows being added. When
     * the client receives more data from the server, it will check if this
     * cache now fits into the continuous range of rows. If yes, it will added

"it will added"?
Also, perhaps the whole comment could perhaps be more explicit about the lifetime of this map etc. and the name "invalidCache" might give the wrong impression about what this is about.

---

uitest/src/test/java/com/vaadin/tests/components/treegrid/TreeGridExpandDataRequestTest.java, line 12 at r1 (raw file):

import com.vaadin.tests.tb3.SingleBrowserTest;

@RunLocally(Browser.PHANTOMJS)

remove RunLocally

---

uitest/src/test/java/com/vaadin/tests/components/treegrid/TreeGridExpandDataRequestTest.java, line 50 at r1 (raw file):

        grid.expandWithClick(0);
        Assert.assertFalse(
                "Log should not contain request for children of '0 | 1'.",

pipe character not what the UI puts there (uses semicolon) so this test would always return false

---

Comments from Reviewable

[hesara]

Review status: 2 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 570 at r1 (raw file):

        if (invalidCache.containsKey(potentialCache.getStart() - 1)) {
            // Invalid cache is contains a key it should not.

"is contains"? Also, not clear to me why this is a case where "invalidCache" is invalid

---

Comments from Reviewable

[hesara]

Review status: 2 of 4 files reviewed at latest revision, 4 unresolved discussions.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 192 at r2 (raw file):

     * <p>
     * Subsequent calls to row data insertions clears the map to avoid potential
     * issues with weird ordering of data updates.

could still add mention about null vs. empty map, and a mention that the keys of this map have already been adjusted for the insertion

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 714 at r2 (raw file):

            Range invalid = splitAt[1];

            if (!invalid.isEmpty() && invalidatedRows == null) {

what if invalidatedRows is an empty map at this point?

---

server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java, line 386 at r2 (raw file):

        getClientRpc().insertRows(expandedRowIndex + 1, expandedNodeSize);
        // TODO optimize by sending "just enough" of the expanded items directly
        doPushRows(Range.withLength(expandedRowIndex + 1, expandedNodeSize));

(not reseting pushRows - I assume this is intentional, but is this always correct?)

---

Comments from Reviewable

[tsuoanttila]

Review status: 1 of 4 files reviewed at latest revision, 5 unresolved discussions.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 714 at r2 (raw file):

Previously, hesara (Henri Sara) wrote…

what if invalidatedRows is an empty map at this point?

That basically means that instead of ARDS getting the data push it got more row changes. Technically, we could still attempt to track the invalidated row indices, but in my opinion that's just too risky.

The promise is simple "if you add and immediately push the new rows, we're going to deal with it". Any other case goes through cache coverage check and request to the server.

---

server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java, line 386 at r2 (raw file):

Previously, hesara (Henri Sara) wrote…

(not reseting pushRows - I assume this is intentional, but is this always correct?)

this is only about pushing expanded rows now (and this has a potential issue with changing the columns of grid in expand listener), any other requests will be handled as normally.

---

uitest/src/test/java/com/vaadin/tests/components/treegrid/TreeGridExpandDataRequestTest.java, line 50 at r1 (raw file):

Previously, hesara (Henri Sara) wrote…

pipe character not what the UI puts there (uses semicolon) so this test would always return false

The pipe in this case comes from the item, not the UI logging output (I did run the tests before applying the client-side change to make sure that this did indeed fail before). I used semicolon in logging on purpose to make clearer.

---

Comments from Reviewable

[hesara]

Reviewed 1 of 4 files at r1, 1 of 1 files at r3.
Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 714 at r2 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…

That basically means that instead of ARDS getting the data push it got more row changes. Technically, we could still attempt to track the invalidated row indices, but in my opinion that's just too risky.

The promise is simple "if you add and immediately push the new rows, we're going to deal with it". Any other case goes through cache coverage check and request to the server.

this could be made clear to the next reader in the form of comments

---

Comments from Reviewable

[tsuoanttila]

Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 192 at r2 (raw file):

Previously, hesara (Henri Sara) wrote…

could still add mention about null vs. empty map, and a mention that the keys of this map have already been adjusted for the insertion

Done.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 714 at r2 (raw file):

Previously, hesara (Henri Sara) wrote…

this could be made clear to the next reader in the form of comments

Done.

---

Comments from Reviewable

[hesara]

Review status: 3 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java, line 602 at r4 (raw file):

            invalidatedRows = null;
        } finally {
            // Update cachce range and clean up

"cache"

---

uitest/src/test/java/com/vaadin/tests/applicationcontext/CloseSessionTest.java, line 9 at r4 (raw file):

import com.vaadin.testbench.elements.ButtonElement;
import com.vaadin.testbench.elements.NotificationElement;
import com.vaadin.testbench.elements.UIElement;

(good but unrelated changes - should probably be in a separate PR)

---

Comments from Reviewable

[hesara]

Reviewed 4 of 4 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable