From 7973103ab0d043abd00bcc1141a4fa5446c24c38 Mon Sep 17 00:00:00 2001 From: Onur Idrisoglu <33827141+onuridrisoglu@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:41:55 +0300 Subject: [PATCH] feat:Improve Binder change detection (#19488) Fixes #19260 * Introduced binder change detection for readBean case * Added equality predicate to Binding level and deprecated the old handleFieldValueChange method * Fixed unit test error * Added default implementation for the new withEqualityPredicate method * Added missing line from the deprecated method * 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 * fixed format issues * Add global setting and default comparison * Add tests --------- Co-authored-by: Teppo Kurki Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com> --- .../com/vaadin/flow/data/binder/Binder.java | 132 +++++++++++++++++- .../vaadin/flow/data/binder/BinderTest.java | 104 ++++++++++++++ 2 files changed, 235 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 bfd710832c4..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 @@ -55,6 +55,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; @@ -321,6 +322,18 @@ default BindingValidationStatus validate() { * changes, otherwise {@literal false}. */ boolean hasChanges(); + + /** + * 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(); } /** @@ -951,6 +964,26 @@ BindingBuilder asRequired( */ public BindingBuilder asRequired( Validator customRequiredValidator); + + /** + * 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. + *

+ * + * @param equalityPredicate + * the predicate to use for equality comparison + * @return this {@code BindingBuilder}, for method chaining + */ + public default BindingBuilder withEqualityPredicate( + SerializableBiPredicate equalityPredicate) { + return this; + } } /** @@ -986,6 +1019,13 @@ protected static class BindingBuilderImpl 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 + */ + private SerializableBiPredicate equalityPredicate = null; + /** * Creates a new binding builder associated with the given field. * Initializes the builder with the given converter chain and status @@ -1204,6 +1244,15 @@ 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 * possibility to disable (reset) default null representation converter. @@ -1318,6 +1367,10 @@ protected static class BindingImpl private Boolean defaultValidatorEnabled; + private SerializableBiPredicate equalityPredicate; + + private TARGET initialValue; + public BindingImpl(BindingBuilderImpl builder, ValueProvider getter, Setter setter) { @@ -1329,6 +1382,8 @@ public BindingImpl(BindingBuilderImpl builder, defaultValidatorEnabled = builder.defaultValidatorEnabled; + equalityPredicate = builder.equalityPredicate; + onValueChange = getField().addValueChangeListener( event -> handleFieldValueChange(event)); @@ -1516,6 +1571,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); getBinder().fireEvent(event); } } @@ -1573,6 +1632,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 @@ -1704,6 +1764,36 @@ public boolean hasChanges() throws IllegalStateException { return this.binder.hasChanges(this); } + + @Override + public SerializableBiPredicate getEqualityPredicate() { + return equalityPredicate; + } + + /** + * 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 + * {@code changeBindings} + */ + private void removeFromChangedBindingsIfReverted( + SerializableConsumer> removeBindingAction) { + if (binder.isChangeDetectionEnabled() + || equalityPredicate != null) { + doConversion().ifOk(convertedValue -> { + SerializableBiPredicate effectivePredicate = equalityPredicate == null + ? Objects::equals + : equalityPredicate; + if (effectivePredicate.test(initialValue, convertedValue)) { + removeBindingAction.accept(this); + } + }); + } + } } /** @@ -1824,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 @@ -3902,10 +3994,17 @@ protected void removeBindingInternal(Binding binding) { if (bindings.remove(binding)) { boundProperties.entrySet() .removeIf(entry -> entry.getValue().equals(binding)); - changedBindings.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. * @@ -3963,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}, 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 156c8d34648..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 @@ -526,6 +526,110 @@ public void update_bound_propertyIsUpdated() throws ValidationException { Assert.assertEquals(0, updatedPerson.getAge()); } + @Test + public void update_to_initial_value_removes_binding_from_changedBindings_with_set_predicates() + 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)) + .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); + 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_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);