Disregard all client side changes to MapProperty if there are server updates

[denis-anisimov]

Fix for #2460 and #3312

---

This change is 

[SomeoneToIgnore]

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

---

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 129 at r1 (raw file):

     */
    public void setValue(Object value) {
        // mark as server update is in progress

This particular comment is rather redundant, imo.

---

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 153 at r1 (raw file):

            // mark as server update is in progress
            isServerUpdate = true;
            updateValue(null, false);

So, we call updateValue in two places and set isServerUpdate = true; in same two places, right before the method.
Would it make sense to put the isServerUpdate = true; into the method?
We can also rename it to state that we blocking all other updates.

---

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 161 at r1 (raw file):

    private void doSetValue(Object value) {
        if (hasValue && Objects.equals(value, this.value)) {

This looks like the same case as in syncToServer method when we set isServerUpdate = false; if new value equals to old value.
Should we do the same here also?

If my previous comment about putting isServerUpdate = true; into updateValue will be done, then this is not needed.

---

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/MultiplePropsMutation.html, line 29 at r1 (raw file):

            static get is() { return 'multiple-props-mutation' }
        
	        static get properties(){

Tabs again.

---

Comments from Reviewable

[denis-anisimov]

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

---

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 129 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

This particular comment is rather redundant, imo.

Done.

---

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 153 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

So, we call updateValue in two places and set isServerUpdate = true; in same two places, right before the method.
Would it make sense to put the isServerUpdate = true; into the method?
We can also rename it to state that we blocking all other updates.

into the method

you mean updateValue method ?

No.
syncToServer delegates its call to doSetValue which also calls the updateValue method.
And this way flag should not be set.
Otherwise the whole idea won't work at all.

---

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 161 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

This looks like the same case as in syncToServer method when we set isServerUpdate = false; if new value equals to old value.
Should we do the same here also?

If my previous comment about putting isServerUpdate = true; into updateValue will be done, then this is not needed.

There are two different aspects of checking for the same value:

• this one checks whether the value needs to be updated at all and event needs to be sent if the update is required. This allows to avoid endless loop of update: e.g. client side updates the value , the value is updated and event is sent, then the event causes updates in client side which may again request set value (exactly happens with notify properties).
• the check inside syncToServer is another side of the same story: if the property change on the client side fires an event then syncToServer will be called. It may happen event when we are changing the property value from the server side: client side property is updated as a result of the event from setValue , then client fires an event (for synchronized property) and then syncToServer is called with the same value which has been set from the server. Normally it should be ignored and this is expected (again: preventing the endless loop). But I use this is as an indicator that now server side update is applied and the flag can be reset.

I will update comments inside syncToServer.

---

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/MultiplePropsMutation.html, line 29 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Tabs again.

Done.

---

Comments from Reviewable

[SomeoneToIgnore]

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 161 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

There are two different aspects of checking for the same value:

• this one checks whether the value needs to be updated at all and event needs to be sent if the update is required. This allows to avoid endless loop of update: e.g. client side updates the value , the value is updated and event is sent, then the event causes updates in client side which may again request set value (exactly happens with notify properties).
• the check inside syncToServer is another side of the same story: if the property change on the client side fires an event then syncToServer will be called. It may happen event when we are changing the property value from the server side: client side property is updated as a result of the event from setValue , then client fires an event (for synchronized property) and then syncToServer is called with the same value which has been set from the server. Normally it should be ignored and this is expected (again: preventing the endless loop). But I use this is as an indicator that now server side update is applied and the flag can be reset.

I will update comments inside syncToServer.

Oh, ok, thanks for the explanation.

---

Comments from Reviewable

[SomeoneToIgnore]

flow-client/src/main/java/com/vaadin/client/flow/nodefeature/MapProperty.java, line 153 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

into the method

you mean updateValue method ?

No.
syncToServer delegates its call to doSetValue which also calls the updateValue method.
And this way flag should not be set.
Otherwise the whole idea won't work at all.

Ok.

---

Comments from Reviewable

[SomeoneToIgnore]

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/MultiplePropsMutation.html, line 29 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Done.

Was not done btw, but whatever.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/MultiplePropsMutation.html, line 29 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Was not done btw, but whatever.

Weird, I've reformatted the code...

---

Comments from Reviewable