diff options
6 files changed, 161 insertions, 60 deletions
diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java index 2763c7f788..7c0dba107c 100644 --- a/server/src/main/java/com/vaadin/data/Binder.java +++ b/server/src/main/java/com/vaadin/data/Binder.java @@ -30,6 +30,7 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import com.vaadin.data.HasValue.ValueChangeEvent; import com.vaadin.data.util.converter.Converter; import com.vaadin.data.util.converter.StringToIntegerConverter; import com.vaadin.data.util.converter.ValueContext; @@ -525,8 +526,10 @@ public class Binder<BEAN> implements Serializable { this.getter = getter; this.setter = setter; + onValueChange = getField() + .addValueChangeListener(this::handleFieldValueChange); getBinder().bindings.add(this); - getBinder().getBean().ifPresent(this::bind); + getBinder().getBean().ifPresent(this::initFieldValue); getBinder().fireStatusChangeEvent(false); } @@ -655,12 +658,6 @@ public class Binder<BEAN> implements Serializable { return l; } - private void bind(BEAN bean) { - setFieldValue(bean); - onValueChange = getField() - .addValueChangeListener(e -> handleFieldValueChange(bean)); - } - @Override public ValidationStatus<TARGET> validate() { ValidationStatus<TARGET> status = doValidation(); @@ -675,9 +672,9 @@ public class Binder<BEAN> implements Serializable { * Returns the field value run through all converters and validators, * but doesn't pass the {@link ValidationStatus} to any status handler. * - * @return the conversation result + * @return the result of the conversion */ - private Result<TARGET> doConversation() { + private Result<TARGET> doConversion() { FIELDVALUE fieldValue = field.getValue(); return converterValidatorChain.convertToModel(fieldValue, createValueContext()); @@ -698,7 +695,7 @@ public class Binder<BEAN> implements Serializable { * @return the validation status */ private ValidationStatus<TARGET> doValidation() { - return toValidationStatus(doConversation()); + return toValidationStatus(doConversion()); } /** @@ -714,20 +711,24 @@ public class Binder<BEAN> implements Serializable { return new ValueContext(findLocale()); } - private void reset() { - onValueChange.remove(); - } - /** * Sets the field value by invoking the getter function on the given - * bean. + * bean. The default listener attached to the field will be removed for + * the duration of this update. * * @param bean * the bean to fetch the property value from */ - private void setFieldValue(BEAN bean) { + private void initFieldValue(BEAN bean) { assert bean != null; - getField().setValue(convertDataToFieldType(bean)); + assert onValueChange != null; + onValueChange.remove(); + try { + getField().setValue(convertDataToFieldType(bean)); + } finally { + onValueChange = getField() + .addValueChangeListener(this::handleFieldValueChange); + } } private FIELDVALUE convertDataToFieldType(BEAN bean) { @@ -741,21 +742,29 @@ public class Binder<BEAN> implements Serializable { * @param bean * the new value */ - private void handleFieldValueChange(BEAN bean) { + private void handleFieldValueChange( + ValueChangeEvent<FIELDVALUE> event) { getBinder().setHasChanges(true); - // store field value if valid - ValidationStatus<TARGET> fieldValidationStatus = writeFieldValue( - bean); - List<ValidationResult> binderValidationResults; - // if all field level validations pass, run bean level validation - if (!getBinder().bindings.stream().map(BindingImpl::doValidation) - .anyMatch(ValidationStatus::isError)) { - binderValidationResults = getBinder().validateBean(bean); + List<ValidationResult> binderValidationResults = Collections + .emptyList(); + ValidationStatus<TARGET> fieldValidationStatus; + if (getBinder().getBean().isPresent()) { + BEAN bean = getBinder().getBean().get(); + fieldValidationStatus = writeFieldValue(bean); + if (!getBinder().bindings.stream() + .map(BindingImpl::doValidation) + .anyMatch(ValidationStatus::isError)) { + binderValidationResults = getBinder().validateBean(bean); + if (!binderValidationResults.stream() + .anyMatch(ValidationResult::isError)) { + getBinder().setHasChanges(false); + } + } } else { - binderValidationResults = Collections.emptyList(); + fieldValidationStatus = doValidation(); } BinderValidationStatus<BEAN> status = new BinderValidationStatus<>( - binder, Arrays.asList(fieldValidationStatus), + getBinder(), Arrays.asList(fieldValidationStatus), binderValidationResults); getBinder().getValidationStatusHandler().accept(status); getBinder().fireStatusChangeEvent(status.hasErrors()); @@ -771,7 +780,7 @@ public class Binder<BEAN> implements Serializable { private ValidationStatus<TARGET> writeFieldValue(BEAN bean) { assert bean != null; - Result<TARGET> result = doConversation(); + Result<TARGET> result = doConversion(); if (setter != null) { result.ifOk(value -> setter.accept(bean, value)); } @@ -1008,7 +1017,7 @@ public class Binder<BEAN> implements Serializable { } else { doRemoveBean(false); this.bean = bean; - bindings.forEach(b -> b.bind(bean)); + bindings.forEach(b -> b.initFieldValue(bean)); // if there has been field value change listeners that trigger // validation, need to make sure the validation errors are cleared getValidationStatusHandler().accept( @@ -1045,7 +1054,7 @@ public class Binder<BEAN> implements Serializable { public void readBean(BEAN bean) { Objects.requireNonNull(bean, "bean cannot be null"); setHasChanges(false); - bindings.forEach(binding -> binding.setFieldValue(bean)); + bindings.forEach(binding -> binding.initFieldValue(bean)); getValidationStatusHandler() .accept(BinderValidationStatus.createUnresolvedStatus(this)); @@ -1530,12 +1539,43 @@ public class Binder<BEAN> implements Serializable { /** * Check whether any of the bound fields' values have changed since last - * explicit call to {@link #setBean(Object)}, {@link #writeBean(Object)} or - * {@link #readBean(Object)}. Unsuccessful write operations will not affect - * this value. + * explicit call to {@link #setBean(Object)}, {@link #readBean(Object)}, + * {@link #removeBean()}, {@link #writeBean(Object)} or + * {@link #writeBeanIfValid(Object)}. Unsuccessful write operations will not + * affect this value. Return values for each case are compiled into the + * following table: + * + * <p> + * + * <table> + * <tr> + * <td></td> + * <td>After readBean, setBean or removeBean</td> + * <td>After valid user changes</td> + * <td>After invalid user changes</td> + * <td>After successful writeBean or writeBeanIfValid</td> + * <td>After unsuccessful writeBean or writeBeanIfValid</td> + * </tr> + * <tr> + * <td>A bean is currently bound</td> + * <td>{@code false}</td> + * <td>{@code false}</td> + * <td>{@code true}</td> + * <td>{@code false}</td> + * <td>no change</td> + * </tr> + * <tr> + * <td>No bean is currently bound</td> + * <td>{@code false}</td> + * <td>{@code true}</td> + * <td>{@code true}</td> + * <td>{@code false}</td> + * <td>no change</td> + * </tr> + * </table> * * @return whether any bound field's value has changed since last call to - * setBean, writeBean or readBean + * setBean, readBean, writeBean or writeBeanIfValid */ public boolean hasChanges() { return hasChanges; @@ -1557,7 +1597,6 @@ public class Binder<BEAN> implements Serializable { setHasChanges(false); if (bean != null) { bean = null; - bindings.forEach(BindingImpl::reset); } getValidationStatusHandler() .accept(BinderValidationStatus.createUnresolvedStatus(this)); diff --git a/server/src/test/java/com/vaadin/data/BinderBookOfVaadinTest.java b/server/src/test/java/com/vaadin/data/BinderBookOfVaadinTest.java index f7ab22a121..a621549b83 100644 --- a/server/src/test/java/com/vaadin/data/BinderBookOfVaadinTest.java +++ b/server/src/test/java/com/vaadin/data/BinderBookOfVaadinTest.java @@ -23,7 +23,6 @@ import java.util.stream.Collectors; import org.junit.Assert; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import com.vaadin.data.Binder.Binding; @@ -713,7 +712,6 @@ public class BinderBookOfVaadinTest { } @Test - @Ignore public void statusChangeListener_binderIsNotBound() { Button saveButton = new Button(); Button resetButton = new Button(); @@ -803,8 +801,6 @@ public class BinderBookOfVaadinTest { field.setValue("a"); // there are valid changes - Assert.assertTrue(saveButton.isEnabled()); - Assert.assertTrue(resetButton.isEnabled()); verifyEventIsFired(eventIsFired); field.setValue(""); diff --git a/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java b/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java index c567f6e5df..b3d6d62c0b 100644 --- a/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java +++ b/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java @@ -387,19 +387,42 @@ public class BinderConverterValidatorTest binder.setBean(item); assertFalse(binder.hasChanges()); + // Bound binder + valid user changes: hasChanges == false nameField.setValue("foo"); - assertTrue(binder.hasChanges()); - binder.readBean(item); assertFalse(binder.hasChanges()); nameField.setValue("bar"); binder.writeBeanIfValid(new Person()); assertFalse(binder.hasChanges()); - nameField.setValue("baz"); - binder.writeBean(new Person()); + // Bound binder + invalid user changes: hasChanges() == true + nameField.setValue(""); + binder.writeBeanIfValid(new Person()); + assertTrue(binder.hasChanges()); + + // Read bean resets hasChanges + binder.readBean(item); assertFalse(binder.hasChanges()); + // Removing a bound bean resets hasChanges + nameField.setValue(""); + assertTrue(binder.hasChanges()); + binder.removeBean(); + assertFalse(binder.hasChanges()); + + // Unbound binder + valid user changes: hasChanges() == true + nameField.setValue("foo"); + assertTrue(binder.hasChanges()); + + // successful writeBean resets hasChanges to false + binder.writeBeanIfValid(new Person()); + assertFalse(binder.hasChanges()); + + // Unbound binder + invalid user changes: hasChanges() == true + nameField.setValue(""); + assertTrue(binder.hasChanges()); + + // unsuccessful writeBean doesn't affect hasChanges nameField.setValue(""); binder.writeBeanIfValid(new Person()); assertTrue(binder.hasChanges()); diff --git a/server/src/test/java/com/vaadin/data/BinderStatusChangeTest.java b/server/src/test/java/com/vaadin/data/BinderStatusChangeTest.java index b394bd8d72..1e03a22fd7 100644 --- a/server/src/test/java/com/vaadin/data/BinderStatusChangeTest.java +++ b/server/src/test/java/com/vaadin/data/BinderStatusChangeTest.java @@ -143,7 +143,7 @@ public class BinderStatusChangeTest } @Test - public void load_hasBindings_singleEventOnLoad() { + public void readBean_hasBindings_singleEventOnLoad() { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.addStatusChangeListener(this::statusChanged); @@ -153,7 +153,7 @@ public class BinderStatusChangeTest } @Test - public void load_hasSeveralBindings_singleEventOnLoad() { + public void readBean_hasSeveralBindings_singleEventOnLoad() { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.forField(ageField) @@ -166,7 +166,7 @@ public class BinderStatusChangeTest } @Test - public void load_hasNoBindings_singleEvent() { + public void readBean_hasNoBindings_singleEvent() { binder.addStatusChangeListener(this::statusChanged); Assert.assertNull(event.get()); binder.readBean(item); @@ -174,7 +174,8 @@ public class BinderStatusChangeTest } @Test - public void save_hasNoBindings_singleEvent() throws ValidationException { + public void writeBean_hasNoBindings_singleEvent() + throws ValidationException { binder.addStatusChangeListener(this::statusChanged); Assert.assertNull(event.get()); binder.writeBean(item); @@ -182,7 +183,7 @@ public class BinderStatusChangeTest } @Test - public void saveIfValid_hasNoBindings_singleEvent() { + public void writeBeanIfValid_hasNoBindings_singleEvent() { binder.addStatusChangeListener(this::statusChanged); Assert.assertNull(event.get()); binder.writeBeanIfValid(item); @@ -190,7 +191,7 @@ public class BinderStatusChangeTest } @Test - public void save_hasBindings_singleEvent() throws ValidationException { + public void writeBean_hasBindings_singleEvent() throws ValidationException { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.readBean(item); @@ -202,7 +203,7 @@ public class BinderStatusChangeTest } @Test - public void save_hasSeveralBindings_singleEvent() + public void writeBean_hasSeveralBindings_singleEvent() throws ValidationException { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); @@ -218,7 +219,7 @@ public class BinderStatusChangeTest } @Test - public void saveIfValid_hasBindings_singleEvent() { + public void writeBeanIfValid_hasBindings_singleEvent() { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.readBean(item); @@ -230,7 +231,7 @@ public class BinderStatusChangeTest } @Test - public void saveIfValid_hasSeveralBindings_singleEvent() { + public void writeBeanIfValid_hasSeveralBindings_singleEvent() { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.forField(ageField) @@ -245,7 +246,7 @@ public class BinderStatusChangeTest } @Test - public void saveInvalidValue_hasBindings_singleEvent() { + public void writeBeanInvalidValue_hasBindings_singleEvent() { binder.forField(nameField).withValidator(name -> false, "") .bind(Person::getFirstName, Person::setFirstName); binder.readBean(item); @@ -260,7 +261,7 @@ public class BinderStatusChangeTest } @Test - public void saveIfValid_invalidValueAndBinderHasBindings_singleEvent() { + public void writeBeanIfValid_invalidValueAndBinderHasBindings_singleEvent() { binder.forField(nameField).withValidator(name -> false, "") .bind(Person::getFirstName, Person::setFirstName); binder.readBean(item); @@ -272,7 +273,7 @@ public class BinderStatusChangeTest } @Test - public void saveIfValid_invalidValueAndBinderHasSeveralBindings_singleEvent() { + public void writeBeanIfValid_invalidValueAndBinderHasSeveralBindings_singleEvent() { binder.forField(nameField).withValidator(name -> false, "") .bind(Person::getFirstName, Person::setFirstName); binder.forField(ageField) @@ -287,7 +288,7 @@ public class BinderStatusChangeTest } @Test - public void saveInvalidBean_hasBindings_singleEvent() { + public void writeBeanInvalidBean_hasBindings_singleEvent() { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.readBean(item); @@ -303,7 +304,7 @@ public class BinderStatusChangeTest } @Test - public void saveIfValid_invalidBeanAndBinderHasBindings_singleEvent() { + public void writeBeanIfValid_invalidBeanAndBinderHasBindings_singleEvent() { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.readBean(item); @@ -316,7 +317,7 @@ public class BinderStatusChangeTest } @Test - public void saveValidBean_hasBindings_singleEvent() + public void writeValidBean_hasBindings_singleEvent() throws ValidationException { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); @@ -330,7 +331,7 @@ public class BinderStatusChangeTest } @Test - public void saveIfValid_validBeanAndBinderHasBindings_singleEvent() { + public void writeBeanIfValid_validBeanAndBinderHasBindings_singleEvent() { binder.forField(nameField).bind(Person::getFirstName, Person::setFirstName); binder.readBean(item); diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java index 22af7d2272..0ad86b9f19 100644 --- a/server/src/test/java/com/vaadin/data/BinderTest.java +++ b/server/src/test/java/com/vaadin/data/BinderTest.java @@ -385,4 +385,45 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> { Assert.assertNull(textField.getErrorMessage()); Assert.assertTrue(textField.isRequiredIndicatorVisible()); } + + @Test + public void validationStatusHandler_onlyRunForChangedField() { + TextField firstNameField = new TextField(); + TextField lastNameField = new TextField(); + + AtomicInteger invokes = new AtomicInteger(); + + binder.forField(firstNameField) + .withValidator(new NotEmptyValidator<>("")) + .withValidationStatusHandler( + validationStatus -> invokes.addAndGet(1)) + .bind(Person::getFirstName, Person::setFirstName); + binder.forField(lastNameField) + .withValidator(new NotEmptyValidator<>("")) + .bind(Person::getLastName, Person::setLastName); + + binder.setBean(item); + // setting the bean causes 2: + Assert.assertEquals(2, invokes.get()); + + lastNameField.setValue(""); + Assert.assertEquals(2, invokes.get()); + + firstNameField.setValue(""); + Assert.assertEquals(3, invokes.get()); + + binder.removeBean(); + Person person = new Person(); + person.setFirstName("a"); + person.setLastName("a"); + binder.readBean(person); + // reading from a bean causes 2: + Assert.assertEquals(5, invokes.get()); + + lastNameField.setValue(""); + Assert.assertEquals(5, invokes.get()); + + firstNameField.setValue(""); + Assert.assertEquals(6, invokes.get()); + } }
\ No newline at end of file diff --git a/server/src/test/java/com/vaadin/data/BinderValidationStatusTest.java b/server/src/test/java/com/vaadin/data/BinderValidationStatusTest.java index b884d85990..6cef908aef 100644 --- a/server/src/test/java/com/vaadin/data/BinderValidationStatusTest.java +++ b/server/src/test/java/com/vaadin/data/BinderValidationStatusTest.java @@ -67,6 +67,7 @@ public class BinderValidationStatusTest Assert.assertEquals(EMPTY_ERROR_MESSAGE, evt.getMessage().get()); Assert.assertEquals(nameField, evt.getField()); + statusCapture.set(null); nameField.setValue("foo"); statusCapture.set(null); |