From d2019455ef285202698b6a4a4aa771e528108ecf Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu Date: Fri, 31 May 2024 13:04:31 +0300 Subject: [PATCH 1/9] Introduced binder change detection for readBean case --- .../com/vaadin/flow/data/binder/Binder.java | 56 +++++++++++++++---- .../vaadin/flow/data/binder/BinderTest.java | 31 ++++++++++ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index bfd2891c730..3a4fb72b28a 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -1512,7 +1512,7 @@ private void handleFieldValueChange( if (binder != null) { // Inform binder of changes; if setBean: writeIfValid - getBinder().handleFieldValueChange(this); + getBinder().handleFieldValueChange(this, event); getBinder().fireEvent(event); } } @@ -1809,6 +1809,7 @@ void setIdentity() { private BindingExceptionHandler exceptionHandler = new DefaultBindingExceptionHandler(); + private Map, Object> bindingInitialValuesMap = new HashMap<>(); private Set> changedBindings = new LinkedHashSet<>(); private boolean validatorsDisabled = false; @@ -1918,8 +1919,9 @@ public static Binder withPropertySet( /** * Informs the Binder that a value in Binding was changed. * - * If {@link #readBean(Object)} was used, this method will only validate the - * changed binding and ignore state of other bindings. + * If {@link #readBean(Object)} was used, this method will check if the value is reverted + * to initial value, in this case the binding will be removed from the changed bindings. + * This method will also validate the changed binding and ignore state of other bindings. * * If {@link #setBean(Object)} was used, all pending changed bindings will * be validated and non-changed ones will be ignored. The changed value will @@ -1928,18 +1930,41 @@ public static Binder withPropertySet( * * @param binding * the binding whose value has been changed + * @param event + * the value change event */ - protected void handleFieldValueChange(Binding binding) { - changedBindings.add(binding); - + protected void handleFieldValueChange(Binding binding, ValueChangeEvent event) { if (getBean() == null) { + if (!bindingInitialValuesMap.containsKey(binding)) { + // If the field is changed the first time, keep the old value as the initial value + bindingInitialValuesMap.put(binding, event.getOldValue()); + changedBindings.add(binding); + } else if (isRevertedToInitialValue(binding, event.getValue())) { + changedBindings.remove(binding); + } binding.validate(); } else { + changedBindings.add(binding); doWriteIfValid(getBean(), changedBindings); } } + /** + * If {@link #readBean(Object)} was used, checks if the field value is + * reverted to the initial value in bean + * + * @param binding + * the binding whose value has been changed + * @param newValue + * the new value if the field + * @return true if the field value is reverted + */ + protected boolean isRevertedToInitialValue(Binding binding, Object newValue) { + return Objects.equals(bindingInitialValuesMap.get(binding), newValue); + } + + /** * Returns the bean that has been bound with {@link #bind}, or null if a * bean is not currently bound. * @@ -2307,7 +2332,7 @@ public void readBean(BEAN bean) { binding.initFieldValue(bean, false); } }); - changedBindings.clear(); + clearChangedBindings(); getValidationStatusHandler().statusChange( BinderValidationStatus.createUnresolvedStatus(this)); fireStatusChangeEvent(false); @@ -2561,14 +2586,14 @@ private BinderValidationStatus doWriteIfValid(BEAN bean, * Changes have been successfully saved. The set is only cleared * when the changes are stored in the currently set bean. */ - changedBindings.clear(); + clearChangedBindings(); } else if (getBean() == null) { /* * When using readBean and writeBean there is no knowledge of * which bean the changes come from or are stored in. Binder is * no longer "changed" when saved succesfully to any bean. */ - changedBindings.clear(); + clearChangedBindings(); } } @@ -2738,7 +2763,15 @@ private void clearFields() { if (hasChanges()) { fireStatusChangeEvent(false); } - changedBindings.clear(); + clearChangedBindings(); + } + + /** + * Clear changed bindings and initial values cache + */ + private void clearChangedBindings() { + bindingInitialValuesMap.clear(); + changedBindings.clear(); } /** @@ -3312,7 +3345,7 @@ public void setReadOnly(boolean readOnly) { } private void doRemoveBean(boolean fireStatusEvent) { - changedBindings.clear(); + clearChangedBindings(); if (bean != null) { bean = null; } @@ -3773,6 +3806,7 @@ protected void removeBindingInternal(Binding binding) { boundProperties.entrySet() .removeIf(entry -> entry.getValue().equals(binding)); changedBindings.remove(binding); + bindingInitialValuesMap.remove(binding); } } diff --git a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java index a51db02540a..8310369f2b8 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java @@ -525,6 +525,37 @@ public void update_bound_propertyIsUpdated() throws ValidationException { Assert.assertEquals(nameValue, updatedPerson.getFirstName()); Assert.assertEquals(0, updatedPerson.getAge()); } + + @Test + public void update_to_initial_value_removes_binding_from_changedBindings() throws ValidationException { + Person person = new Person(); + String initialName = "Foo"; + person.setFirstName(initialName); + person.setAge(20); + + Binder binder = new Binder<>(); + Binding nameBinding = binder.bind(nameField, Person::getFirstName, Person::setFirstName); + Binding ageBinding = binder.forField(ageField) + .withConverter(new StringToIntegerConverter("")) + .bind(Person::getAge, Person::setAge); + + binder.readBean(person); + nameField.setValue("Bar"); + + assertEquals(1, binder.getChangedBindings().size()); + assertTrue(binder.getChangedBindings().contains(nameBinding)); + + ageField.setValue("21"); + assertEquals(2, binder.getChangedBindings().size()); + + nameField.setValue(initialName); + + assertEquals(1, binder.getChangedBindings().size()); + assertTrue(binder.getChangedBindings().contains(ageBinding)); + + ageField.setValue("20"); + assertTrue(binder.getChangedBindings().isEmpty()); + } @Test public void save_bound_beanAsDraft() { From e48ac6bebd7b60c5e3e101322384ccd560338662 Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu Date: Mon, 17 Jun 2024 15:00:55 +0300 Subject: [PATCH 2/9] Added equality predicate to Binding level and deprecated the old handleFieldValueChange method --- .../com/vaadin/flow/data/binder/Binder.java | 64 ++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 30d245fe1ed..8ed78a6606c 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -54,6 +54,7 @@ import com.vaadin.flow.data.converter.StringToIntegerConverter; import com.vaadin.flow.data.validator.BeanValidator; import com.vaadin.flow.dom.Style; +import com.vaadin.flow.function.SerializableBiPredicate; import com.vaadin.flow.function.SerializableConsumer; import com.vaadin.flow.function.SerializableFunction; import com.vaadin.flow.function.SerializablePredicate; @@ -320,6 +321,8 @@ default BindingValidationStatus validate() { * changes, otherwise {@literal false}. */ boolean hasChanges(); + + SerializableBiPredicate getEqualityPredicate(); } /** @@ -949,6 +952,18 @@ BindingBuilder asRequired( */ public BindingBuilder asRequired( Validator customRequiredValidator); + + /** + * Sets the {@code equalityPredicate} used to compare the current value of a field with its initial value. + *

+ * The default implementation uses {@link Objects#equals(Object, Object)}, but a custom equality predicate + * can be provided for cases where the value type does not support the standard {@code equals} method. + *

+ * + * @param equalityPredicate the predicate to use for equality comparison + * @return this {@code BindingBuilder}, for method chaining + */ + public BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate); } /** @@ -983,6 +998,13 @@ protected static class BindingBuilderImpl private boolean asRequiredSet; private Boolean defaultValidatorEnabled; + + /** + * A predicate used to compare the current value of a field with its initial value. + * The default implementation is {@link Objects#equals(Object, Object)} + * but can be customized if the value type does not support equals + */ + private SerializableBiPredicate equalityPredicate = (val1, val2) -> Objects.equals(val1, val2); /** * Creates a new binding builder associated with the given field. @@ -1201,6 +1223,13 @@ public BindingBuilder asRequired( } }); } + + @Override + public BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { + Objects.requireNonNull(equalityPredicate, "equality predicate cannot be null"); + this.equalityPredicate = equalityPredicate; + return this; + } /** * Implements {@link #withConverter(Converter)} method with additional @@ -1315,6 +1344,8 @@ protected static class BindingImpl private Registration onValidationStatusChange; private Boolean defaultValidatorEnabled; + + private SerializableBiPredicate equalityPredicate; public BindingImpl(BindingBuilderImpl builder, ValueProvider getter, @@ -1326,6 +1357,8 @@ public BindingImpl(BindingBuilderImpl builder, converterValidatorChain = ((Converter) builder.converterValidatorChain); defaultValidatorEnabled = builder.defaultValidatorEnabled; + + equalityPredicate = builder.equalityPredicate; onValueChange = getField().addValueChangeListener( event -> handleFieldValueChange(event)); @@ -1702,6 +1735,11 @@ public boolean hasChanges() throws IllegalStateException { return this.binder.hasChanges(this); } + + @Override + public SerializableBiPredicate getEqualityPredicate() { + return equalityPredicate; + } } /** @@ -1917,6 +1955,30 @@ public static Binder withPropertySet( return new Binder<>(propertySet); } + /** + * Informs the Binder that a value in Binding was changed. + * + * If {@link #readBean(Object)} was used, this method will only validate the + * changed binding and ignore state of other bindings. + * + * If {@link #setBean(Object)} was used, all pending changed bindings will + * be validated and non-changed ones will be ignored. The changed value will + * be written to the bean immediately, assuming that Binder-level validators + * also pass. + * + * @param binding + * the binding whose value has been changed + * + * @Deprecated(since = "24.5", forRemoval = true) + */ + protected void handleFieldValueChange(Binding binding) { + if (getBean() == null) { + binding.validate(); + } else { + doWriteIfValid(getBean(), changedBindings); + } + } + /** * Informs the Binder that a value in Binding was changed. * @@ -1962,7 +2024,7 @@ protected void handleFieldValueChange(Binding binding, ValueChangeEvent * @return true if the field value is reverted */ protected boolean isRevertedToInitialValue(Binding binding, Object newValue) { - return Objects.equals(bindingInitialValuesMap.get(binding), newValue); + return binding.getEqualityPredicate().test(bindingInitialValuesMap.get(binding), newValue); } /** From 75a5fbaaee61dffed208681beb4c77ba494a3044 Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu Date: Mon, 17 Jun 2024 18:30:53 +0300 Subject: [PATCH 3/9] Fixed unit test error --- .../src/main/java/com/vaadin/flow/data/binder/Binder.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 8ed78a6606c..5c3a7b1a758 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -1968,9 +1968,8 @@ public static Binder withPropertySet( * * @param binding * the binding whose value has been changed - * - * @Deprecated(since = "24.5", forRemoval = true) */ + @Deprecated(since = "24.5", forRemoval = true) protected void handleFieldValueChange(Binding binding) { if (getBean() == null) { binding.validate(); From 75cfc938df1bf415dd8772be270a828c3998e516 Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu Date: Mon, 17 Jun 2024 18:41:03 +0300 Subject: [PATCH 4/9] Added default implementation for the new withEqualityPredicate method --- .../src/main/java/com/vaadin/flow/data/binder/Binder.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 5c3a7b1a758..c057de6fe5d 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -963,7 +963,9 @@ public BindingBuilder asRequired( * @param equalityPredicate the predicate to use for equality comparison * @return this {@code BindingBuilder}, for method chaining */ - public BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate); + public default BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { + return this; + } } /** From 267fad0b9094b49dd5c4b7970a1b1bb6d9f2d329 Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu Date: Mon, 17 Jun 2024 19:37:04 +0300 Subject: [PATCH 5/9] Added missing line from the deprecated method --- flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index c057de6fe5d..f41be45d201 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -1973,6 +1973,8 @@ public static Binder withPropertySet( */ @Deprecated(since = "24.5", forRemoval = true) protected void handleFieldValueChange(Binding binding) { + changedBindings.add(binding); + if (getBean() == null) { binding.validate(); } else { From 88f974b71c3248524d0c1c8e41249af09ef6dab1 Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu Date: Wed, 7 Aug 2024 12:45:15 +0300 Subject: [PATCH 6/9] Changes for review comments: - removed bindingInitialValuesMap from Binder, and kept the initial value in binding level instead - removed the default implementation for equalityPredicate and now this feature will only be active if an equalityPredicate is set --- .../com/vaadin/flow/data/binder/Binder.java | 131 ++++++++---------- .../vaadin/flow/data/binder/BinderTest.java | 5 +- 2 files changed, 59 insertions(+), 77 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index f41be45d201..61b34f3dd5d 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -322,7 +322,15 @@ default BindingValidationStatus validate() { */ boolean hasChanges(); - SerializableBiPredicate getEqualityPredicate(); + /** + * Used in comparison of the current value of a field with its initial value. + *

+ * Once set, the value of the field that binding uses will be compared with the initial value for hasChanged. + *

+ * + * @return the predicate to use for equality comparison + */ + SerializableBiPredicate getEqualityPredicate(); } /** @@ -956,14 +964,15 @@ public BindingBuilder asRequired( /** * Sets the {@code equalityPredicate} used to compare the current value of a field with its initial value. *

- * The default implementation uses {@link Objects#equals(Object, Object)}, but a custom equality predicate - * can be provided for cases where the value type does not support the standard {@code equals} method. + * By default it is {@literal null}, meaning the initial value comparison is not active. + * Once it is set, the value of the field will be compared with its initial value. + * If the value of the field is set back to its initial value, it will not be considered as having uncommitted changes. *

* * @param equalityPredicate the predicate to use for equality comparison * @return this {@code BindingBuilder}, for method chaining */ - public default BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { + public default BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { return this; } } @@ -1003,10 +1012,9 @@ protected static class BindingBuilderImpl /** * A predicate used to compare the current value of a field with its initial value. - * The default implementation is {@link Objects#equals(Object, Object)} - * but can be customized if the value type does not support equals + * By default it is {@literal null} meaning that the initial value comparison is not active */ - private SerializableBiPredicate equalityPredicate = (val1, val2) -> Objects.equals(val1, val2); + private SerializableBiPredicate equalityPredicate = null; /** * Creates a new binding builder associated with the given field. @@ -1227,7 +1235,7 @@ public BindingBuilder asRequired( } @Override - public BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { + public BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { Objects.requireNonNull(equalityPredicate, "equality predicate cannot be null"); this.equalityPredicate = equalityPredicate; return this; @@ -1347,7 +1355,9 @@ protected static class BindingImpl private Boolean defaultValidatorEnabled; - private SerializableBiPredicate equalityPredicate; + private SerializableBiPredicate equalityPredicate; + + private TARGET initialValue; public BindingImpl(BindingBuilderImpl builder, ValueProvider getter, @@ -1548,7 +1558,9 @@ private void handleFieldValueChange( if (binder != null) { // Inform binder of changes; if setBean: writeIfValid - getBinder().handleFieldValueChange(this, event); + getBinder().handleFieldValueChange(this); + // Compare the value with initial value, and remove the binder from changed bindings if reverted + removeFromChangedBindingsIfReverted(getBinder()::removeFromChangedBindings); getBinder().fireEvent(event); } } @@ -1606,6 +1618,7 @@ private void convertAndSetFieldValue(TARGET modelValue) { FIELDVALUE convertedValue = convertToFieldType(modelValue); try { field.setValue(convertedValue); + initialValue = modelValue; } catch (RuntimeException e) { /* * Add an additional hint to the exception for the typical @@ -1739,9 +1752,26 @@ public boolean hasChanges() throws IllegalStateException { } @Override - public SerializableBiPredicate getEqualityPredicate() { + public SerializableBiPredicate getEqualityPredicate() { return equalityPredicate; } + + /** + * if {@code equalityPredicate} is set, compares the new value of the field with its initial value, + * and removes the current binding from the {@code changeBindings} + * + * @param removeBindingAction + * the binding consumer that removes the binding from the {@code changeBindings} + */ + private void removeFromChangedBindingsIfReverted(SerializableConsumer> removeBindingAction) { + if (equalityPredicate != null) { + doConversion().ifOk(convertedValue -> { + if (equalityPredicate.test(initialValue, convertedValue)) { + removeBindingAction.accept(this); + } + }); + } + } } /** @@ -1850,7 +1880,6 @@ void setIdentity() { private BindingExceptionHandler exceptionHandler = new DefaultBindingExceptionHandler(); - private Map, Object> bindingInitialValuesMap = new HashMap<>(); private Set> changedBindings = new LinkedHashSet<>(); private boolean validatorsDisabled = false; @@ -1971,7 +2000,6 @@ public static Binder withPropertySet( * @param binding * the binding whose value has been changed */ - @Deprecated(since = "24.5", forRemoval = true) protected void handleFieldValueChange(Binding binding) { changedBindings.add(binding); @@ -1982,54 +2010,6 @@ protected void handleFieldValueChange(Binding binding) { } } - /** - * Informs the Binder that a value in Binding was changed. - * - * If {@link #readBean(Object)} was used, this method will check if the value is reverted - * to initial value, in this case the binding will be removed from the changed bindings. - * This method will also validate the changed binding and ignore state of other bindings. - * - * If {@link #setBean(Object)} was used, all pending changed bindings will - * be validated and non-changed ones will be ignored. The changed value will - * be written to the bean immediately, assuming that Binder-level validators - * also pass. - * - * @param binding - * the binding whose value has been changed - * @param event - * the value change event - */ - protected void handleFieldValueChange(Binding binding, ValueChangeEvent event) { - if (getBean() == null) { - if (!bindingInitialValuesMap.containsKey(binding)) { - // If the field is changed the first time, keep the old value as the initial value - bindingInitialValuesMap.put(binding, event.getOldValue()); - changedBindings.add(binding); - } else if (isRevertedToInitialValue(binding, event.getValue())) { - changedBindings.remove(binding); - } - binding.validate(); - } else { - changedBindings.add(binding); - doWriteIfValid(getBean(), changedBindings); - } - } - - - /** - * If {@link #readBean(Object)} was used, checks if the field value is - * reverted to the initial value in bean - * - * @param binding - * the binding whose value has been changed - * @param newValue - * the new value if the field - * @return true if the field value is reverted - */ - protected boolean isRevertedToInitialValue(Binding binding, Object newValue) { - return binding.getEqualityPredicate().test(bindingInitialValuesMap.get(binding), newValue); - } - /** * Returns the bean that has been bound with {@link #bind}, or null if a * bean is not currently bound. @@ -2398,7 +2378,7 @@ public void readBean(BEAN bean) { binding.initFieldValue(bean, false); } }); - clearChangedBindings(); + changedBindings.clear(); getValidationStatusHandler().statusChange( BinderValidationStatus.createUnresolvedStatus(this)); fireStatusChangeEvent(false); @@ -2652,14 +2632,14 @@ private BinderValidationStatus doWriteIfValid(BEAN bean, * Changes have been successfully saved. The set is only cleared * when the changes are stored in the currently set bean. */ - clearChangedBindings(); + changedBindings.clear(); } else if (getBean() == null) { /* * When using readBean and writeBean there is no knowledge of * which bean the changes come from or are stored in. Binder is * no longer "changed" when saved succesfully to any bean. */ - clearChangedBindings(); + changedBindings.clear(); } } @@ -2829,17 +2809,9 @@ private void clearFields() { if (hasChanges()) { fireStatusChangeEvent(false); } - clearChangedBindings(); + changedBindings.clear(); } - /** - * Clear changed bindings and initial values cache - */ - private void clearChangedBindings() { - bindingInitialValuesMap.clear(); - changedBindings.clear(); - } - /** * Validates the values of all bound fields and returns the validation * status. @@ -3411,7 +3383,7 @@ public void setReadOnly(boolean readOnly) { } private void doRemoveBean(boolean fireStatusEvent) { - clearChangedBindings(); + changedBindings.clear(); if (bean != null) { bean = null; } @@ -3871,10 +3843,17 @@ protected void removeBindingInternal(Binding binding) { if (bindings.remove(binding)) { boundProperties.entrySet() .removeIf(entry -> entry.getValue().equals(binding)); - changedBindings.remove(binding); - bindingInitialValuesMap.remove(binding); + removeFromChangedBindings(binding); } } + + + /** + * Removes (internally) the {@code Binding} from the changed bindings + */ + private void removeFromChangedBindings(Binding binding) { + changedBindings.remove(binding); + } /** * Finds and removes the Binding for the given property name. diff --git a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java index 8310369f2b8..b111b7f9f2f 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java @@ -534,9 +534,12 @@ public void update_to_initial_value_removes_binding_from_changedBindings() throw person.setAge(20); Binder binder = new Binder<>(); - Binding nameBinding = binder.bind(nameField, Person::getFirstName, Person::setFirstName); + Binding nameBinding = binder.forField(nameField) + .withEqualityPredicate((oldVal, newVal) -> Objects.equals(oldVal, newVal)) + .bind(Person::getFirstName, Person::setFirstName); Binding ageBinding = binder.forField(ageField) .withConverter(new StringToIntegerConverter("")) + .withEqualityPredicate((oldVal, newVal) -> Objects.equals(oldVal, newVal)) .bind(Person::getAge, Person::setAge); binder.readBean(person); From d6f7d558615a0712e6e5b600809a65a4c5f6a7c1 Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu Date: Wed, 7 Aug 2024 12:53:26 +0300 Subject: [PATCH 7/9] fixed format issues --- .../com/vaadin/flow/data/binder/Binder.java | 128 ++++++++++-------- .../vaadin/flow/data/binder/BinderTest.java | 55 ++++---- 2 files changed, 100 insertions(+), 83 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 4ac430166ea..707ebaf8444 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -321,13 +321,15 @@ default BindingValidationStatus validate() { * changes, otherwise {@literal false}. */ boolean hasChanges(); - + /** - * Used in comparison of the current value of a field with its initial value. + * Used in comparison of the current value of a field with its initial + * value. *

- * Once set, the value of the field that binding uses will be compared with the initial value for hasChanged. + * Once set, the value of the field that binding uses will be compared + * with the initial value for hasChanged. *

- * + * * @return the predicate to use for equality comparison */ SerializableBiPredicate getEqualityPredicate(); @@ -961,20 +963,25 @@ BindingBuilder asRequired( */ public BindingBuilder asRequired( Validator customRequiredValidator); - + /** - * Sets the {@code equalityPredicate} used to compare the current value of a field with its initial value. + * Sets the {@code equalityPredicate} used to compare the current value + * of a field with its initial value. *

- * By default it is {@literal null}, meaning the initial value comparison is not active. - * Once it is set, the value of the field will be compared with its initial value. - * If the value of the field is set back to its initial value, it will not be considered as having uncommitted changes. + * By default it is {@literal null}, meaning the initial value + * comparison is not active. Once it is set, the value of the field will + * be compared with its initial value. If the value of the field is set + * back to its initial value, it will not be considered as having + * uncommitted changes. *

- * - * @param equalityPredicate the predicate to use for equality comparison + * + * @param equalityPredicate + * the predicate to use for equality comparison * @return this {@code BindingBuilder}, for method chaining */ - public default BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { - return this; + public default BindingBuilder withEqualityPredicate( + SerializableBiPredicate equalityPredicate) { + return this; } } @@ -1010,10 +1017,11 @@ protected static class BindingBuilderImpl private boolean asRequiredSet; private Boolean defaultValidatorEnabled; - + /** - * A predicate used to compare the current value of a field with its initial value. - * By default it is {@literal null} meaning that the initial value comparison is not active + * A predicate used to compare the current value of a field with its + * initial value. By default it is {@literal null} meaning that the + * initial value comparison is not active */ private SerializableBiPredicate equalityPredicate = null; @@ -1234,12 +1242,14 @@ public BindingBuilder asRequired( } }); } - + @Override - public BindingBuilder withEqualityPredicate(SerializableBiPredicate equalityPredicate) { - Objects.requireNonNull(equalityPredicate, "equality predicate cannot be null"); - this.equalityPredicate = equalityPredicate; - return this; + public BindingBuilder withEqualityPredicate( + SerializableBiPredicate equalityPredicate) { + Objects.requireNonNull(equalityPredicate, + "equality predicate cannot be null"); + this.equalityPredicate = equalityPredicate; + return this; } /** @@ -1355,9 +1365,9 @@ protected static class BindingImpl private Registration onValidationStatusChange; private Boolean defaultValidatorEnabled; - + private SerializableBiPredicate equalityPredicate; - + private TARGET initialValue; public BindingImpl(BindingBuilderImpl builder, @@ -1370,7 +1380,7 @@ public BindingImpl(BindingBuilderImpl builder, converterValidatorChain = ((Converter) builder.converterValidatorChain); defaultValidatorEnabled = builder.defaultValidatorEnabled; - + equalityPredicate = builder.equalityPredicate; onValueChange = getField().addValueChangeListener( @@ -1560,8 +1570,10 @@ private void handleFieldValueChange( if (binder != null) { // Inform binder of changes; if setBean: writeIfValid getBinder().handleFieldValueChange(this); - // Compare the value with initial value, and remove the binder from changed bindings if reverted - removeFromChangedBindingsIfReverted(getBinder()::removeFromChangedBindings); + // Compare the value with initial value, and remove the binder + // from changed bindings if reverted + removeFromChangedBindingsIfReverted( + getBinder()::removeFromChangedBindings); getBinder().fireEvent(event); } } @@ -1752,27 +1764,30 @@ public boolean hasChanges() throws IllegalStateException { return this.binder.hasChanges(this); } - @Override - public SerializableBiPredicate getEqualityPredicate() { - return equalityPredicate; - } + @Override + public SerializableBiPredicate getEqualityPredicate() { + return equalityPredicate; + } - /** - * if {@code equalityPredicate} is set, compares the new value of the field with its initial value, - * and removes the current binding from the {@code changeBindings} - * - * @param removeBindingAction - * the binding consumer that removes the binding from the {@code changeBindings} - */ - private void removeFromChangedBindingsIfReverted(SerializableConsumer> removeBindingAction) { - if (equalityPredicate != null) { - doConversion().ifOk(convertedValue -> { - if (equalityPredicate.test(initialValue, convertedValue)) { - removeBindingAction.accept(this); - } - }); - } - } + /** + * if {@code equalityPredicate} is set, compares the new value of the + * field with its initial value, and removes the current binding from + * the {@code changeBindings} + * + * @param removeBindingAction + * the binding consumer that removes the binding from the + * {@code changeBindings} + */ + private void removeFromChangedBindingsIfReverted( + SerializableConsumer> removeBindingAction) { + if (equalityPredicate != null) { + doConversion().ifOk(convertedValue -> { + if (equalityPredicate.test(initialValue, convertedValue)) { + removeBindingAction.accept(this); + } + }); + } + } } /** @@ -2002,16 +2017,16 @@ public static Binder withPropertySet( * the binding whose value has been changed */ protected void handleFieldValueChange(Binding binding) { - changedBindings.add(binding); - + changedBindings.add(binding); + if (getBean() == null) { binding.validate(); } else { doWriteIfValid(getBean(), changedBindings); } } - - /** + + /** * Returns the bean that has been bound with {@link #bind}, or null if a * bean is not currently bound. * @@ -2633,14 +2648,14 @@ private BinderValidationStatus doWriteIfValid(BEAN bean, * Changes have been successfully saved. The set is only cleared * when the changes are stored in the currently set bean. */ - changedBindings.clear(); + changedBindings.clear(); } else if (getBean() == null) { /* * When using readBean and writeBean there is no knowledge of * which bean the changes come from or are stored in. Binder is * no longer "changed" when saved succesfully to any bean. */ - changedBindings.clear(); + changedBindings.clear(); } } @@ -2812,7 +2827,7 @@ private void clearFields() { } changedBindings.clear(); } - + /** * Validates the values of all bound fields and returns the validation * status. @@ -3384,7 +3399,7 @@ public void setReadOnly(boolean readOnly) { } private void doRemoveBean(boolean fireStatusEvent) { - changedBindings.clear(); + changedBindings.clear(); if (bean != null) { bean = null; } @@ -3847,13 +3862,12 @@ protected void removeBindingInternal(Binding binding) { removeFromChangedBindings(binding); } } - - + /** * Removes (internally) the {@code Binding} from the changed bindings */ private void removeFromChangedBindings(Binding binding) { - changedBindings.remove(binding); + changedBindings.remove(binding); } /** diff --git a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java index b111b7f9f2f..3ac11c741e6 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java @@ -525,39 +525,42 @@ public void update_bound_propertyIsUpdated() throws ValidationException { Assert.assertEquals(nameValue, updatedPerson.getFirstName()); Assert.assertEquals(0, updatedPerson.getAge()); } - - @Test - public void update_to_initial_value_removes_binding_from_changedBindings() throws ValidationException { - Person person = new Person(); - String initialName = "Foo"; - person.setFirstName(initialName); - person.setAge(20); - - Binder binder = new Binder<>(); + + @Test + public void update_to_initial_value_removes_binding_from_changedBindings() + throws ValidationException { + Person person = new Person(); + String initialName = "Foo"; + person.setFirstName(initialName); + person.setAge(20); + + Binder binder = new Binder<>(); Binding nameBinding = binder.forField(nameField) - .withEqualityPredicate((oldVal, newVal) -> Objects.equals(oldVal, newVal)) + .withEqualityPredicate( + (oldVal, newVal) -> Objects.equals(oldVal, newVal)) .bind(Person::getFirstName, Person::setFirstName); Binding ageBinding = binder.forField(ageField) .withConverter(new StringToIntegerConverter("")) - .withEqualityPredicate((oldVal, newVal) -> Objects.equals(oldVal, newVal)) + .withEqualityPredicate( + (oldVal, newVal) -> Objects.equals(oldVal, newVal)) .bind(Person::getAge, Person::setAge); - - binder.readBean(person); - nameField.setValue("Bar"); - - assertEquals(1, binder.getChangedBindings().size()); - assertTrue(binder.getChangedBindings().contains(nameBinding)); - - ageField.setValue("21"); - assertEquals(2, binder.getChangedBindings().size()); - + + binder.readBean(person); + nameField.setValue("Bar"); + + assertEquals(1, binder.getChangedBindings().size()); + assertTrue(binder.getChangedBindings().contains(nameBinding)); + + ageField.setValue("21"); + assertEquals(2, binder.getChangedBindings().size()); + nameField.setValue(initialName); - + assertEquals(1, binder.getChangedBindings().size()); - assertTrue(binder.getChangedBindings().contains(ageBinding)); - - ageField.setValue("20"); - assertTrue(binder.getChangedBindings().isEmpty()); + assertTrue(binder.getChangedBindings().contains(ageBinding)); + + ageField.setValue("20"); + assertTrue(binder.getChangedBindings().isEmpty()); } @Test From 158e743e9ebf01133062cd644ee4471d0d2250a4 Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Fri, 23 Aug 2024 11:44:56 +0300 Subject: [PATCH 8/9] Add global setting and default comparison --- .../com/vaadin/flow/data/binder/Binder.java | 48 +++++++++++++++++-- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 66c12bf78bc..6a861bfdb06 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -1771,9 +1771,10 @@ public SerializableBiPredicate getEqualityPredicate() { } /** - * if {@code equalityPredicate} is set, compares the new value of the - * field with its initial value, and removes the current binding from - * the {@code changeBindings} + * compares the new value of the field with its initial value, and + * removes the current binding from the {@code changeBindings}, but only + * if {@code equalityPredicate} is set, or + * {@link #isChangeDetectionEnabled()} returns true. * * @param removeBindingAction * the binding consumer that removes the binding from the @@ -1781,9 +1782,13 @@ public SerializableBiPredicate getEqualityPredicate() { */ private void removeFromChangedBindingsIfReverted( SerializableConsumer> removeBindingAction) { - if (equalityPredicate != null) { + if (binder.isChangeDetectionEnabled() + || equalityPredicate != null) { doConversion().ifOk(convertedValue -> { - if (equalityPredicate.test(initialValue, convertedValue)) { + SerializableBiPredicate effectivePredicate = equalityPredicate == null + ? Objects::equals + : equalityPredicate; + if (effectivePredicate.test(initialValue, convertedValue)) { removeBindingAction.accept(this); } }); @@ -1909,6 +1914,8 @@ void setIdentity() { private boolean defaultValidatorsEnabled = true; + private boolean changeDetectionEnabled = false; + /** * Creates a binder using a custom {@link PropertySet} implementation for * finding and resolving property names for @@ -4055,6 +4062,37 @@ public boolean isFieldsValidationStatusChangeListenerEnabled() { return fieldsValidationStatusChangeListenerEnabled; } + /** + * Sets change/revert detection enabled or disabled. When set to + * {@literal true}, any binding that is first changed and then reverted to + * its original value will be removed from the list of changed bindings. + * + * By default, {@link Objects#equals(Object, Object)} is used for value + * comparison, but it can be overridden on binding level using + * {@link BindingBuilder#withEqualityPredicate(SerializableBiPredicate)}. + * + * @param changeDetectionEnabled + * Boolean value + */ + public void setChangeDetectionEnabled(boolean changeDetectionEnabled) { + this.changeDetectionEnabled = changeDetectionEnabled; + } + + /** + * Returns if change/revert detection is enabled. When set to + * {@literal true}, any binding that is first changed and then reverted to + * its original value will be removed from the list of changed bindings. + * + * By default, {@link Objects#equals(Object, Object)} is used for value + * comparison, but it can be overridden on binding level using + * {@link BindingBuilder#withEqualityPredicate(SerializableBiPredicate)}. + * + * @return Boolean value + */ + public boolean isChangeDetectionEnabled() { + return changeDetectionEnabled; + } + /** * Sets a {@code handler} to customize the {@link RuntimeException} thrown * by delegates (like {@link Setter}, {@link ValueProvider}, From c79b79f94af16a593374a881f2a88a43a4a00c0d Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Fri, 23 Aug 2024 11:49:18 +0300 Subject: [PATCH 9/9] Add tests --- .../vaadin/flow/data/binder/BinderTest.java | 69 ++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java index de47fe91019..0c9b4c0d7cf 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java @@ -527,7 +527,7 @@ public void update_bound_propertyIsUpdated() throws ValidationException { } @Test - public void update_to_initial_value_removes_binding_from_changedBindings() + public void update_to_initial_value_removes_binding_from_changedBindings_with_set_predicates() throws ValidationException { Person person = new Person(); String initialName = "Foo"; @@ -563,6 +563,73 @@ public void update_to_initial_value_removes_binding_from_changedBindings() assertTrue(binder.getChangedBindings().isEmpty()); } + @Test + public void update_to_initial_value_removes_binding_from_changedBindings_with_default_predicates() + throws ValidationException { + Person person = new Person(); + String initialName = "Foo"; + person.setFirstName(initialName); + person.setAge(20); + + Binder binder = new Binder<>(); + binder.setChangeDetectionEnabled(true); + Binding nameBinding = binder.forField(nameField) + .bind(Person::getFirstName, Person::setFirstName); + Binding ageBinding = binder.forField(ageField) + .withConverter(new StringToIntegerConverter("")) + .bind(Person::getAge, Person::setAge); + + binder.readBean(person); + nameField.setValue("Bar"); + + assertEquals(1, binder.getChangedBindings().size()); + assertTrue(binder.getChangedBindings().contains(nameBinding)); + + ageField.setValue("21"); + assertEquals(2, binder.getChangedBindings().size()); + + nameField.setValue(initialName); + + assertEquals(1, binder.getChangedBindings().size()); + assertTrue(binder.getChangedBindings().contains(ageBinding)); + + ageField.setValue("20"); + assertTrue(binder.getChangedBindings().isEmpty()); + } + + @Test + public void update_to_initial_value_does_not_remove_binding_from_changedBindings_by_default() + throws ValidationException { + Person person = new Person(); + String initialName = "Foo"; + person.setFirstName(initialName); + person.setAge(20); + + Binder binder = new Binder<>(); + Binding nameBinding = binder.forField(nameField) + .bind(Person::getFirstName, Person::setFirstName); + Binding ageBinding = binder.forField(ageField) + .withConverter(new StringToIntegerConverter("")) + .bind(Person::getAge, Person::setAge); + + binder.readBean(person); + nameField.setValue("Bar"); + + assertEquals(1, binder.getChangedBindings().size()); + assertTrue(binder.getChangedBindings().contains(nameBinding)); + + ageField.setValue("21"); + assertEquals(2, binder.getChangedBindings().size()); + assertTrue(binder.getChangedBindings().contains(ageBinding)); + + nameField.setValue(initialName); + + assertEquals(2, binder.getChangedBindings().size()); + + ageField.setValue("20"); + assertEquals(2, binder.getChangedBindings().size()); + } + @Test public void save_bound_beanAsDraft() { do_test_save_bound_beanAsDraft(false);