]> source.dussan.org Git - vaadin-framework.git/commitdiff
Refactor server-side editor row to work in real(er) cases (#13334)
authorLeif Åstrand <leif@vaadin.com>
Fri, 12 Dec 2014 13:21:55 +0000 (15:21 +0200)
committerLeif Åstrand <leif@vaadin.com>
Fri, 12 Dec 2014 13:21:55 +0000 (15:21 +0200)
- 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

server/src/com/vaadin/data/fieldgroup/FieldGroup.java
server/src/com/vaadin/ui/Grid.java
server/tests/src/com/vaadin/data/fieldgroup/FieldGroupTests.java
server/tests/src/com/vaadin/tests/server/component/grid/EditorRowTests.java
uitest/src/com/vaadin/tests/components/grid/EditorRowUI.java [new file with mode: 0644]
uitest/src/com/vaadin/tests/components/grid/EditorRowUITest.java [new file with mode: 0644]
uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java

index c89b94ace96ff83fb535b1e5adfadf5eccb3f036..7e96e9e882a4dc816887dbe424d9a5ce2f2c9df4 100644 (file)
@@ -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();
             }
index 50ca047c64c6c28b5b67b6c4fa3e2307c05c1c6b..e50bfad1a830402028acaacdc81f826790fec03a 100644 (file)
@@ -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;
@@ -158,6 +157,22 @@ import elemental.json.JsonValue;
 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}.
@@ -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);
-    }
-
 }
index eb8f21c839de089c58161c27475e5eae9f3b5f04..0e7d682e395ccf36c9228f1c8eead0d6964498f0 100644 (file)
@@ -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()));
+    }
 }
index d521423187b09eb9774ceeabbc4abe43aa00475c..6252b6b5684222b2a2b77c93ad95ae2d569fe059 100644 (file)
@@ -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() {
diff --git a/uitest/src/com/vaadin/tests/components/grid/EditorRowUI.java b/uitest/src/com/vaadin/tests/components/grid/EditorRowUI.java
new file mode 100644 (file)
index 0000000..3891583
--- /dev/null
@@ -0,0 +1,49 @@
+/*
+ * Copyright 2000-2014 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.components.grid;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.tests.util.PersonContainer;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.PasswordField;
+import com.vaadin.ui.TextField;
+
+public class EditorRowUI extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        PersonContainer container = PersonContainer.createWithTestData();
+
+        Grid grid = new Grid(container);
+
+        // Don't use address since there's no converter
+        grid.removeColumn("address");
+
+        grid.setEditorRowEnabled(true);
+
+        grid.setEditorRowField("firstName", new PasswordField());
+
+        TextField lastNameField = (TextField) grid
+                .getEditorRowField("lastName");
+        lastNameField.setMaxLength(50);
+
+        grid.getEditorRowField("phoneNumber").setReadOnly(true);
+
+        addComponent(grid);
+    }
+
+}
diff --git a/uitest/src/com/vaadin/tests/components/grid/EditorRowUITest.java b/uitest/src/com/vaadin/tests/components/grid/EditorRowUITest.java
new file mode 100644 (file)
index 0000000..0d4de50
--- /dev/null
@@ -0,0 +1,65 @@
+/*
+ * Copyright 2000-2014 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.components.grid;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+import org.openqa.selenium.Keys;
+import org.openqa.selenium.interactions.Actions;
+
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.testbench.elements.GridElement.GridCellElement;
+import com.vaadin.testbench.elements.NotificationElement;
+import com.vaadin.testbench.elements.PasswordFieldElement;
+import com.vaadin.tests.annotations.TestCategory;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+@TestCategory("grid")
+public class EditorRowUITest extends MultiBrowserTest {
+
+    @Test
+    public void testEditorRow() {
+        setDebug(true);
+        openTestURL();
+
+        assertFalse("Sanity check",
+                isElementPresent(PasswordFieldElement.class));
+
+        openEditorRow(5);
+        assertFalse("Remove this when grid is fixed",
+                isElementPresent(PasswordFieldElement.class));
+        new Actions(getDriver()).sendKeys(Keys.ESCAPE).perform();
+
+        openEditorRow(10);
+
+        assertTrue("Edtor row should be opened with a password field",
+                isElementPresent(PasswordFieldElement.class));
+
+        assertFalse("Notification was present",
+                isElementPresent(NotificationElement.class));
+    }
+
+    private void openEditorRow(int rowIndex) {
+        GridElement grid = $(GridElement.class).first();
+
+        GridCellElement cell = grid.getCell(rowIndex, 1);
+
+        new Actions(driver).moveToElement(cell).doubleClick().build().perform();
+    }
+
+}
index e61edb4e65f0d6036564d9289c392acb6c88a263..a5c93edad02d6fbabac8207f68e8cec54f9edc90 100644 (file)
@@ -180,7 +180,7 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
 
         grid.setSelectionMode(SelectionMode.NONE);
 
-        grid.setPropertyEditable(getColumnProperty(3), false);
+        grid.getEditorRowField(getColumnProperty(3)).setReadOnly(true);
 
         createGridActions();