Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat:Improve Binder change detection #19488

Merged
merged 13 commits into from
Aug 23, 2024
Merged
132 changes: 131 additions & 1 deletion flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -321,6 +322,18 @@ default BindingValidationStatus<TARGET> validate() {
* changes, otherwise {@literal false}.
*/
boolean hasChanges();

/**
* Used in comparison of the current value of a field with its initial
* value.
* <p>
* Once set, the value of the field that binding uses will be compared
* with the initial value for hasChanged.
* </p>
*
* @return the predicate to use for equality comparison
*/
SerializableBiPredicate<TARGET, TARGET> getEqualityPredicate();
}

/**
Expand Down Expand Up @@ -951,6 +964,26 @@ BindingBuilder<BEAN, TARGET> asRequired(
*/
public BindingBuilder<BEAN, TARGET> asRequired(
Validator<TARGET> customRequiredValidator);

/**
* Sets the {@code equalityPredicate} used to compare the current value
* of a field with its initial value.
* <p>
* 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.
* </p>
*
* @param equalityPredicate
* the predicate to use for equality comparison
* @return this {@code BindingBuilder}, for method chaining
*/
public default BindingBuilder<BEAN, TARGET> withEqualityPredicate(
SerializableBiPredicate<TARGET, TARGET> equalityPredicate) {
return this;
}
}

/**
Expand Down Expand Up @@ -986,6 +1019,13 @@ protected static class BindingBuilderImpl<BEAN, FIELDVALUE, TARGET>

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<TARGET, TARGET> equalityPredicate = null;

/**
* Creates a new binding builder associated with the given field.
* Initializes the builder with the given converter chain and status
Expand Down Expand Up @@ -1204,6 +1244,15 @@ public BindingBuilder<BEAN, TARGET> asRequired(
});
}

@Override
public BindingBuilder<BEAN, TARGET> withEqualityPredicate(
SerializableBiPredicate<TARGET, TARGET> 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.
Expand Down Expand Up @@ -1318,6 +1367,10 @@ protected static class BindingImpl<BEAN, FIELDVALUE, TARGET>

private Boolean defaultValidatorEnabled;

private SerializableBiPredicate<TARGET, TARGET> equalityPredicate;

private TARGET initialValue;

public BindingImpl(BindingBuilderImpl<BEAN, FIELDVALUE, TARGET> builder,
ValueProvider<BEAN, TARGET> getter,
Setter<BEAN, TARGET> setter) {
Expand All @@ -1329,6 +1382,8 @@ public BindingImpl(BindingBuilderImpl<BEAN, FIELDVALUE, TARGET> builder,

defaultValidatorEnabled = builder.defaultValidatorEnabled;

equalityPredicate = builder.equalityPredicate;

onValueChange = getField().addValueChangeListener(
event -> handleFieldValueChange(event));

Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1704,6 +1764,36 @@ public boolean hasChanges() throws IllegalStateException {

return this.binder.hasChanges(this);
}

@Override
public SerializableBiPredicate<TARGET, TARGET> 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<Binding<BEAN, TARGET>> removeBindingAction) {
if (binder.isChangeDetectionEnabled()
|| equalityPredicate != null) {
doConversion().ifOk(convertedValue -> {
SerializableBiPredicate<TARGET, TARGET> effectivePredicate = equalityPredicate == null
? Objects::equals
: equalityPredicate;
if (effectivePredicate.test(initialValue, convertedValue)) {
removeBindingAction.accept(this);
}
});
}
}
}

/**
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -3902,10 +3994,17 @@ protected void removeBindingInternal(Binding<BEAN, ?> 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<BEAN, ?> binding) {
changedBindings.remove(binding);
}

/**
* Finds and removes the Binding for the given property name.
*
Expand Down Expand Up @@ -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},
Expand Down
104 changes: 104 additions & 0 deletions flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<Person> binder = new Binder<>();
Binding<Person, String> nameBinding = binder.forField(nameField)
.withEqualityPredicate(
(oldVal, newVal) -> Objects.equals(oldVal, newVal))
.bind(Person::getFirstName, Person::setFirstName);
Binding<Person, Integer> 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<Person> binder = new Binder<>();
binder.setChangeDetectionEnabled(true);
Binding<Person, String> nameBinding = binder.forField(nameField)
.bind(Person::getFirstName, Person::setFirstName);
Binding<Person, Integer> 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<Person> binder = new Binder<>();
Binding<Person, String> nameBinding = binder.forField(nameField)
.bind(Person::getFirstName, Person::setFirstName);
Binding<Person, Integer> 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);
Expand Down
Loading