Refreshing a full subtree of a node in TreeGrid (hierarchical data providers)

[wfischlein]

This is the flow part for fixing https://vaadin.com/pro/support#3047

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[caalador]

CLA is not signed.

Reviewed 5 of 5 files at r1.
Reviewable status: 9 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @wfischlein)

---

flow-data/src/main/java/com/vaadin/flow/data/provider/AbstractDataProvider.java, line 63 at r1 (raw file):

, false

This doesn't need adding as the default is false.

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataChangeEvent.java, line 44 at r1 (raw file):

deep

Should it be refreshAll, fullRefreshor other descriptive name asdeep` doesn't tell that much of what it will do. (name should be updated to all instances)

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataChangeEvent.java, line 86 at r1 (raw file):

        }

        /**

Missing javadoc.

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataCommunicator.java, line 396 at r1 (raw file):

handleDataRefreshEvent((DataRefreshEvent<T>) event);

Not needed and if wanting to clear it I would suggest to pick out the item as a variable e.g.

T item = ((DataRefreshEvent<T>) event).getItem();
refresh(item);

or

DataRefreshEvent<T> refreshEvent = (DataRefreshEvent<T>) event;
refresh(refreshEvent.getItem());

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataProvider.java, line 123 at r1 (raw file):

     *            whether or not to refresh child items
     */
    default void refreshItem(T item, boolean deep) {

Shouldn't this be the other way around that refreshItem(item) is the default that calls the boolean version which is required to implement?

Also do we need a default as AbstractDataProvider implements these methods?

---

flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java, line 164 at r1 (raw file):

    }

    protected void handleDataRefreshEvent(DataChangeEvent.DataRefreshEvent<T> event) {

add @Override

---

flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java, line 171 at r1 (raw file):

setParentRequestedRange(0, 50, item);

Why 50?
shouldn't it be setParentRequestedRange(0, countChildItems(item), item) to refresh all even if over 50?

[caalador]

Also a test for refershing with and without deep would be needed.

Reviewable status: 9 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @wfischlein)

[wfischlein]

Reviewable status: 9 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @wfischlein)

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataChangeEvent.java, line 44 at r1 (raw file):

Previously, caalador wrote…

deep

Should it be refreshAll, fullRefreshor other descriptive name asdeep` doesn't tell that much of what it will do. (name should be updated to all instances)

renamed to 'refreshChildren' because 'refreshAll' is another method that refreshes the whole dataprovider and therefore could be misinterpreted, fullRefresh imho isn't more descriptive than deep whereas 'refreshChildren' describes exactly what it does.

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataCommunicator.java, line 396 at r1 (raw file):

Previously, caalador wrote…

handleDataRefreshEvent((DataRefreshEvent<T>) event);

Not needed and if wanting to clear it I would suggest to pick out the item as a variable e.g.

T item = ((DataRefreshEvent<T>) event).getItem();
refresh(item);

or

DataRefreshEvent<T> refreshEvent = (DataRefreshEvent<T>) event;
refresh(refreshEvent.getItem());

It is needed to enable 'com.vaadin.flow.data.provider.hierarchy.HierarchicalDataCommunicator' to deal with the 'deep' case. What do you suggest: to parameterise 'refresh'?

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataProvider.java, line 123 at r1 (raw file):

Previously, caalador wrote…

Shouldn't this be the other way around that refreshItem(item) is the default that calls the boolean version which is required to implement?

Also do we need a default as AbstractDataProvider implements these methods?

The goal was to

• have only minimal changes to the API
• by default do what the former behavior was
  And guess yes, we need the default as I can not guarantee that the AbstractDataProvider will be the only implementation of the interface. If it is supposed to be the only implementation that would bring up the question about the purpose of the interface. We should then throw away the interface

[caalador]

Reviewed 4 of 4 files at r2.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained

---

flow-data/src/main/java/com/vaadin/flow/data/provider/DataCommunicator.java, line 396 at r1 (raw file):

Previously, wfischlein (Wolfgang Fischlein) wrote…

It is needed to enable 'com.vaadin.flow.data.provider.hierarchy.HierarchicalDataCommunicator' to deal with the 'deep' case. What do you suggest: to parameterise 'refresh'?

No this is fine.

[caalador]

CLA is not signed. You need to sign it else we can not merge the patch.