Fix field value change event handling for unbound binder

Unbound binder now correctly sets hasChanges and sends binder
status change events. In addition, hasChanges no longer returns
true in the case where a bean is bound and changes to the bean
were valid.

Change-Id: Ia0a0915c2a205461a2a4b1bfd393413520f863eb

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);