From: Leif Åstrand Date: Fri, 12 Dec 2014 13:21:55 +0000 (+0200) Subject: Refactor server-side editor row to work in real(er) cases (#13334) X-Git-Tag: 7.4.0.beta1~9^2~26 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=b7c01560877c3d1006422a84abb59e18ce357fad;p=vaadin-framework.git 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 --- 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; @@ -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 uneditableProperties = new HashSet(); - 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. - *

- * In order for a user to edit a particular value with a Field, it needs to - * be both non-readonly and editable. - *

- * 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. - *

- * 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 true 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. *

* When {@link #editItem(Object) editItem} is called, fields are * automatically created and bound for any unbound properties. + *

+ * 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 FieldGroup 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)}. *

- * This method also adds validators when applicable. - *

- * Note: This is a pass-through call to the backing field group. + * Setting the field to null 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(); + editorRowFieldGroup = new CustomFieldGroup(); } /** * Gets a collection of all fields bound to the editor row of this grid. *

- * All non-editable fields (either readonly or uneditable) are in read-only - * mode. - *

* 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> getEditorRowFields() { - return editorRowFieldGroup.getFields(); + Collection> fields = editorRowFieldGroup.getFields(); + assert allAttached(fields); + return fields; + } + + private boolean allAttached(Collection 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. - *

- * Note: 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 buildAndBind(Object propertyId, - Class 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() { 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 index 0000000000..3891583098 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/grid/EditorRowUI.java @@ -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 index 0000000000..0d4de5074c --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/grid/EditorRowUITest.java @@ -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(); + } + +} diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java index e61edb4e65..a5c93edad0 100644 --- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java +++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java @@ -180,7 +180,7 @@ public class GridBasicFeatures extends AbstractComponentTest { grid.setSelectionMode(SelectionMode.NONE); - grid.setPropertyEditable(getColumnProperty(3), false); + grid.getEditorRowField(getColumnProperty(3)).setReadOnly(true); createGridActions();