Refactor server-side editor row to work in real(er) cases (#13334)

- Don't configure fields for properties without columns
- Rename "bind" to "set" to match the getter
- Make it possible to get a field before editing has started
- Remove setting of editable since the field can be configured directly
- Remove error handler concept since we have not tests and no use cases
- Unbind and detach field when removing a column

Change-Id: I38ead4e680cdbab2525fe08ff09ed045255e2d4f

4 files changed, 114 insertions, 218 deletions

diff --git a/server/src/com/vaadin/data/fieldgroup/FieldGroup.java b/server/src/com/vaadin/data/fieldgroup/FieldGroup.java
index c89b94ace9..7e96e9e882 100644
--- a/server/src/com/vaadin/data/fieldgroup/FieldGroup.java
+++ b/server/src/com/vaadin/data/fieldgroup/FieldGroup.java
@@ -341,7 +341,8 @@ public class FieldGroup implements Serializable {
                     .getWrappedProperty();
 
         }
-        if (fieldDataSource == getItemProperty(propertyId)) {
+        if (getItemDataSource() != null
+                && fieldDataSource == getItemProperty(propertyId)) {
             if (null != wrapper) {
                 wrapper.detachFromProperty();
             }
diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java
index 50ca047c64..e50bfad1a8 100644
--- a/server/src/com/vaadin/ui/Grid.java
+++ b/server/src/com/vaadin/ui/Grid.java
@@ -64,7 +64,6 @@ import com.vaadin.event.SortOrderChangeEvent.SortOrderChangeListener;
 import com.vaadin.event.SortOrderChangeEvent.SortOrderChangeNotifier;
 import com.vaadin.server.AbstractClientConnector;
 import com.vaadin.server.AbstractExtension;
-import com.vaadin.server.ErrorHandler;
 import com.vaadin.server.ErrorMessage;
 import com.vaadin.server.JsonCodec;
 import com.vaadin.server.KeyMapper;
@@ -159,6 +158,22 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         SortOrderChangeNotifier, SelectiveRenderer {
 
     /**
+     * Custom field group that allows finding property types before an item has
+     * been bound.
+     */
+    private final class CustomFieldGroup extends FieldGroup {
+        @Override
+        protected Class<?> getPropertyType(Object propertyId)
+                throws BindException {
+            if (getItemDataSource() == null) {
+                return datasource.getType(propertyId);
+            } else {
+                return super.getPropertyType(propertyId);
+            }
+        }
+    }
+
+    /**
      * Selection modes representing built-in {@link SelectionModel
      * SelectionModels} that come bundled with {@link Grid}.
      * <p>
@@ -2095,9 +2110,7 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
     private final Footer footer = new Footer(this);
 
     private Object editedItemId = null;
-    private FieldGroup editorRowFieldGroup = new FieldGroup();
-    private HashSet<Object> uneditableProperties = new HashSet<Object>();
-    private ErrorHandler editorRowErrorHandler;
+    private FieldGroup editorRowFieldGroup = new CustomFieldGroup();
 
     private CellStyleGenerator cellStyleGenerator;
 
@@ -2323,12 +2336,8 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
             }
 
             private void handleError(Exception e) {
-                ErrorHandler handler = getEditorRowErrorHandler();
-                if (handler == null) {
-                    handler = com.vaadin.server.ErrorEvent
-                            .findErrorHandler(Grid.this);
-                }
-                handler.error(new ConnectorErrorEvent(Grid.this, e));
+                com.vaadin.server.ErrorEvent.findErrorHandler(Grid.this).error(
+                        new ConnectorErrorEvent(Grid.this, e));
             }
         });
     }
@@ -2675,6 +2684,7 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
      *            The property id of column to be removed
      */
     public void removeColumn(Object propertyId) {
+        setEditorRowField(propertyId, null);
         header.removeColumn(propertyId);
         footer.removeColumn(propertyId);
         Column column = columns.remove(propertyId);
@@ -3865,71 +3875,47 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         return editedItemId != null;
     }
 
-    /**
-     * Sets a property editable or not.
-     * <p>
-     * In order for a user to edit a particular value with a Field, it needs to
-     * be both non-readonly and editable.
-     * <p>
-     * The difference between read-only and uneditable is that the read-only
-     * state is propagated back into the property, while the editable property
-     * is internal metadata for the editor row.
-     * 
-     * @param propertyId
-     *            the id of the property to set as editable state
-     * @param editable
-     *            whether or not {@code propertyId} chould be editable
-     */
-    public void setPropertyEditable(Object propertyId, boolean editable) {
-        checkPropertyExists(propertyId);
-        if (getEditorRowField(propertyId) != null) {
-            getEditorRowField(propertyId).setReadOnly(!editable);
-        }
-        if (editable) {
-            uneditableProperties.remove(propertyId);
-        } else {
-            uneditableProperties.add(propertyId);
-        }
-    }
-
-    /**
-     * Checks whether a property is editable through the editor row.
-     * <p>
-     * This only checks whether the property is configured as uneditable in the
-     * editor row. The property's or field's readonly status will ultimately
-     * decide whether the value can be edited or not.
-     * 
-     * @param propertyId
-     *            the id of the property to check for editable status
-     * @return <code>true</code> iff the property is editable according to this
-     *         editor row
-     */
-    public boolean isPropertyEditable(Object propertyId) {
-        checkPropertyExists(propertyId);
-        return !uneditableProperties.contains(propertyId);
-    }
-
-    private void checkPropertyExists(Object propertyId) {
-        if (!getContainerDataSource().getContainerPropertyIds().contains(
-                propertyId)) {
-            throw new IllegalArgumentException("Property with id " + propertyId
-                    + " is not in the current Container");
+    private void checkColumnExists(Object propertyId) {
+        if (getColumn(propertyId) == null) {
+            throw new IllegalArgumentException(
+                    "There is no column with the property id " + propertyId);
         }
     }
 
     /**
-     * Gets the field component that represents a property in the editor row. If
-     * the property is not yet bound to a field, null is returned.
+     * Gets the field component that represents a property in the editor row.
      * <p>
      * When {@link #editItem(Object) editItem} is called, fields are
      * automatically created and bound for any unbound properties.
+     * <p>
+     * Getting a field before the editor row has been opened depends on special
+     * support from the {@link FieldGroup} in use. Using this method with a
+     * user-provided <code>FieldGroup</code> might cause {@link BindException}
+     * to be thrown.
      * 
      * @param propertyId
      *            the property id of the property for which to find the field
-     * @return the bound field or null if not bound
+     * @return the bound field, never null
+     * 
+     * @throws IllegalArgumentException
+     *             if there is no column for the provided property id
+     * @throws BindException
+     *             if no field has been configured and there is a problem
+     *             building or binding
      */
     public Field<?> getEditorRowField(Object propertyId) {
-        return editorRowFieldGroup.getField(propertyId);
+        checkColumnExists(propertyId);
+
+        Field<?> editor = editorRowFieldGroup.getField(propertyId);
+        if (editor == null) {
+            editor = editorRowFieldGroup.buildAndBind(propertyId);
+        }
+
+        if (editor.getParent() != Grid.this) {
+            assert editor.getParent() == null;
+            editor.setParent(this);
+        }
+        return editor;
     }
 
     /**
@@ -3964,49 +3950,41 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         editorRowFieldGroup.setItemDataSource(item);
         editedItemId = itemId;
 
-        for (Object propertyId : item.getItemPropertyIds()) {
+        for (Column column : getColumns()) {
+            Object propertyId = column.getColumnProperty();
 
-            final Field<?> editor;
-            if (editorRowFieldGroup.getUnboundPropertyIds()
-                    .contains(propertyId)) {
-                editor = editorRowFieldGroup.buildAndBind(propertyId);
-            } else {
-                editor = editorRowFieldGroup.getField(propertyId);
-            }
+            Field<?> editor = getEditorRowField(propertyId);
 
             getColumn(propertyId).getState().editorConnector = editor;
-
-            if (editor != null) {
-                editor.setReadOnly(!isPropertyEditable(propertyId));
-
-                if (editor.getParent() != Grid.this) {
-                    assert editor.getParent() == null;
-                    editor.setParent(Grid.this);
-                }
-            }
         }
     }
 
     /**
-     * Binds the field with the given propertyId from the current item. If an
-     * item has not been set then the binding is postponed until the item is set
-     * using {@link #editItem(Object)}.
+     * Binds the field to the given propertyId. If an item has not been set,
+     * then the binding is postponed until the item is set using
+     * {@link #editItem(Object)}.
      * <p>
-     * This method also adds validators when applicable.
-     * <p>
-     * <em>Note:</em> This is a pass-through call to the backing field group.
+     * Setting the field to <code>null</code> clears any previously set field,
+     * causing a new field to be created the next time the editor row is opened.
      * 
      * @param field
      *            The field to bind
      * @param propertyId
-     *            The propertyId to bind to the field
-     * @throws BindException
-     *             If the property id is already bound to another field by this
-     *             field binder
+     *            The propertyId to bind the field to
      */
-    public void bindEditorRowField(Object propertyId, Field<?> field)
-            throws BindException {
-        editorRowFieldGroup.bind(field, propertyId);
+    public void setEditorRowField(Object propertyId, Field<?> field) {
+        checkColumnExists(propertyId);
+
+        Field<?> oldField = editorRowFieldGroup.getField(propertyId);
+        if (oldField != null) {
+            editorRowFieldGroup.unbind(oldField);
+            oldField.setParent(null);
+        }
+
+        if (field != null) {
+            field.setParent(this);
+            editorRowFieldGroup.bind(field, propertyId);
+        }
     }
 
     /**
@@ -4051,23 +4029,30 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         }
 
         editedItemId = null;
-        editorRowFieldGroup = new FieldGroup();
-        uneditableProperties = new HashSet<Object>();
+        editorRowFieldGroup = new CustomFieldGroup();
     }
 
     /**
      * Gets a collection of all fields bound to the editor row of this grid.
      * <p>
-     * All non-editable fields (either readonly or uneditable) are in read-only
-     * mode.
-     * <p>
      * When {@link #editItem(Object) editItem} is called, fields are
      * automatically created and bound to any unbound properties.
      * 
      * @return a collection of all the fields bound to this editor row
      */
     Collection<Field<?>> getEditorRowFields() {
-        return editorRowFieldGroup.getFields();
+        Collection<Field<?>> fields = editorRowFieldGroup.getFields();
+        assert allAttached(fields);
+        return fields;
+    }
+
+    private boolean allAttached(Collection<? extends Component> components) {
+        for (Component component : components) {
+            if (component.getParent() != this) {
+                return false;
+            }
+        }
+        return true;
     }
 
     /**
@@ -4082,54 +4067,4 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
     public void setEditorRowFieldFactory(FieldGroupFieldFactory fieldFactory) {
         editorRowFieldGroup.setFieldFactory(fieldFactory);
     }
-
-    /**
-     * Returns the error handler of this editor row.
-     * 
-     * @return the error handler or null if there is no dedicated error handler
-     * 
-     * @see #setEditorRowErrorHandler(ErrorHandler)
-     * @see ClientConnector#getErrorHandler()
-     */
-    public ErrorHandler getEditorRowErrorHandler() {
-        return editorRowErrorHandler;
-    }
-
-    /**
-     * Sets the error handler for this editor row. The error handler is invoked
-     * for exceptions thrown while processing client requests; specifically when
-     * {@link #saveEditorRow()} triggered by the client throws a
-     * CommitException. If the error handler is not set, one is looked up via
-     * Grid.
-     * 
-     * @param errorHandler
-     *            the error handler to use
-     * 
-     * @see ClientConnector#setErrorHandler(ErrorHandler)
-     * @see ErrorEvent#findErrorHandler(ClientConnector)
-     */
-    public void setEditorRowErrorHandler(ErrorHandler errorHandler) {
-        editorRowErrorHandler = errorHandler;
-    }
-
-    /**
-     * Builds a field using the given caption and binds it to the given property
-     * id using the field binder. Ensures the new field is of the given type.
-     * <p>
-     * <em>Note:</em> This is a pass-through call to the backing field group.
-     * 
-     * @param propertyId
-     *            The property id to bind to. Must be present in the field
-     *            finder
-     * @param fieldType
-     *            The type of field that we want to create
-     * @throws BindException
-     *             If the field could not be created
-     * @return The created and bound field. Can be any type of {@link Field} .
-     */
-    public <T extends Field<?>> T buildAndBind(Object propertyId,
-            Class<T> fieldType) throws BindException {
-        return editorRowFieldGroup.buildAndBind(null, propertyId, fieldType);
-    }
-
 }
diff --git a/server/tests/src/com/vaadin/data/fieldgroup/FieldGroupTests.java b/server/tests/src/com/vaadin/data/fieldgroup/FieldGroupTests.java
index eb8f21c839..0e7d682e39 100644
--- a/server/tests/src/com/vaadin/data/fieldgroup/FieldGroupTests.java
+++ b/server/tests/src/com/vaadin/data/fieldgroup/FieldGroupTests.java
@@ -2,6 +2,7 @@ package com.vaadin.data.fieldgroup;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.core.Is.is;
+import static org.hamcrest.core.IsNull.nullValue;
 import static org.mockito.Mockito.mock;
 
 import org.junit.Before;
@@ -37,4 +38,11 @@ public class FieldGroupTests {
     public void cannotBindNullField() {
         sut.bind(null, "foobar");
     }
+
+    public void canUnbindWithoutItem() {
+        sut.bind(field, "foobar");
+
+        sut.unbind(field);
+        assertThat(sut.getField("foobar"), is(nullValue()));
+    }
 }
diff --git a/server/tests/src/com/vaadin/tests/server/component/grid/EditorRowTests.java b/server/tests/src/com/vaadin/tests/server/component/grid/EditorRowTests.java
index d521423187..6252b6b568 100644
--- a/server/tests/src/com/vaadin/tests/server/component/grid/EditorRowTests.java
+++ b/server/tests/src/com/vaadin/tests/server/component/grid/EditorRowTests.java
@@ -116,18 +116,6 @@ public class EditorRowTests {
         assertFalse(oldFieldGroup == grid.getEditorRowFieldGroup());
     }
 
-    @Test
-    public void propertyUneditable() throws Exception {
-        assertTrue(grid.isPropertyEditable(PROPERTY_NAME));
-        grid.setPropertyEditable(PROPERTY_NAME, false);
-        assertFalse(grid.isPropertyEditable(PROPERTY_NAME));
-    }
-
-    @Test(expected = IllegalArgumentException.class)
-    public void nonexistentPropertyUneditable() throws Exception {
-        grid.setPropertyEditable(new Object(), false);
-    }
-
     @Test(expected = IllegalStateException.class)
     public void disabledEditItem() throws Exception {
         grid.editItem(ITEM_ID);
@@ -155,63 +143,13 @@ public class EditorRowTests {
     @Test
     public void getFieldWithoutItem() throws Exception {
         grid.setEditorRowEnabled(true);
-        assertNull(grid.getEditorRowField(PROPERTY_NAME));
-    }
-
-    @Test
-    public void getFieldAfterReSettingFieldAsEditable() throws Exception {
-        startEdit();
-
-        grid.setPropertyEditable(PROPERTY_NAME, false);
-        grid.setPropertyEditable(PROPERTY_NAME, true);
         assertNotNull(grid.getEditorRowField(PROPERTY_NAME));
     }
 
     @Test
-    public void isEditable() {
-        assertTrue(grid.isPropertyEditable(PROPERTY_NAME));
-    }
-
-    @Test
-    public void isUneditable() {
-        grid.setPropertyEditable(PROPERTY_NAME, false);
-        assertFalse(grid.isPropertyEditable(PROPERTY_NAME));
-    }
-
-    @Test
-    public void isEditableAgain() {
-        grid.setPropertyEditable(PROPERTY_NAME, false);
-        grid.setPropertyEditable(PROPERTY_NAME, true);
-        assertTrue(grid.isPropertyEditable(PROPERTY_NAME));
-    }
-
-    @Test
-    public void isUneditableAgain() {
-        grid.setPropertyEditable(PROPERTY_NAME, false);
-        grid.setPropertyEditable(PROPERTY_NAME, true);
-        grid.setPropertyEditable(PROPERTY_NAME, false);
-        assertFalse(grid.isPropertyEditable(PROPERTY_NAME));
-    }
-
-    @Test(expected = IllegalArgumentException.class)
-    public void isNonexistentEditable() {
-        grid.isPropertyEditable(new Object());
-    }
-
-    @Test(expected = IllegalArgumentException.class)
-    public void setNonexistentUneditable() {
-        grid.setPropertyEditable(new Object(), false);
-    }
-
-    @Test(expected = IllegalArgumentException.class)
-    public void setNonexistentEditable() {
-        grid.setPropertyEditable(new Object(), true);
-    }
-
-    @Test
     public void customBinding() {
         TextField textField = new TextField();
-        grid.bindEditorRowField(PROPERTY_NAME, textField);
+        grid.setEditorRowField(PROPERTY_NAME, textField);
 
         startEdit();
 
@@ -242,12 +180,26 @@ public class EditorRowTests {
     }
 
     @Test
-    public void fieldIsReadonlyWhenPropertyIsNotEditable() {
-        startEdit();
-
-        grid.setPropertyEditable(PROPERTY_NAME, false);
+    public void columnRemoved() {
         Field<?> field = grid.getEditorRowField(PROPERTY_NAME);
-        assertTrue(field.isReadOnly());
+
+        assertSame("field should be attached to grid.", grid, field.getParent());
+
+        grid.removeColumn(PROPERTY_NAME);
+
+        assertNull("field should be detached from grid.", field.getParent());
+    }
+
+    @Test
+    public void setFieldAgain() {
+        TextField field = new TextField();
+        grid.setEditorRowField(PROPERTY_NAME, field);
+
+        field = new TextField();
+        grid.setEditorRowField(PROPERTY_NAME, field);
+
+        assertSame("new field should be used.", field,
+                grid.getEditorRowField(PROPERTY_NAME));
     }
 
     private void startEdit() {