From 4dc3f49029355dd9a0958f43d36d74da7300c39f Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Mon, 1 Dec 2014 14:04:14 +0200 Subject: [PATCH] Fix addColumn to function correctly with non-default container (#13334) Change-Id: I57b3e819e4709187139cd52ac8f437252fcc738b --- server/src/com/vaadin/ui/Grid.java | 83 +++++++++++++++---- .../component/grid/GridColumnBuildTest.java | 33 ++++++++ .../server/component/grid/sort/SortTest.java | 5 ++ 3 files changed, 107 insertions(+), 14 deletions(-) diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java index 23193e230b..c307de84a5 100644 --- a/server/src/com/vaadin/ui/Grid.java +++ b/server/src/com/vaadin/ui/Grid.java @@ -1919,6 +1919,16 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier, private EditorRow editorRow; + /** + * true if Grid is using the internal IndexedContainer created + * in Grid() constructor, or false if the user has set their + * own Container. + * + * @see #setContainerDataSource() + * @see #Grid() + */ + private boolean defaultContainer = true; + private static final Method SELECTION_CHANGE_METHOD = ReflectTools .findMethod(SelectionChangeListener.class, "selectionChange", SelectionChangeEvent.class); @@ -1931,7 +1941,8 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier, * Creates a new Grid with a new {@link IndexedContainer} as the datasource. */ public Grid() { - this(new IndexedContainer()); + internalSetContainerDataSource(new IndexedContainer()); + initGrid(); } /** @@ -1942,7 +1953,13 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier, */ public Grid(final Container.Indexed datasource) { setContainerDataSource(datasource); + initGrid(); + } + /** + * Grid initial setup + */ + private void initGrid() { setSelectionMode(SelectionMode.MULTI); addSelectionChangeListener(new SelectionChangeListener() { @Override @@ -2153,7 +2170,11 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier, * if the data source is null */ public void setContainerDataSource(Container.Indexed container) { + defaultContainer = false; + internalSetContainerDataSource(container); + } + private void internalSetContainerDataSource(Container.Indexed container) { if (container == null) { throw new IllegalArgumentException( "Cannot set the datasource to null"); @@ -2248,6 +2269,18 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier, propertyId)); } } + } else { + Collection properties = datasource.getContainerPropertyIds(); + for (Object property : columns.keySet()) { + if (!properties.contains(property)) { + throw new IllegalStateException( + "Found at least one column in Grid that does not exist in the given container: " + + property + + " with the header \"" + + getColumn(property).getHeaderCaption() + + "\""); + } + } } } @@ -2272,38 +2305,60 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier, } /** - * Used internally by the {@link Grid} to get a {@link Column} by - * referencing its generated state id. Also used by {@link Column} to verify - * if it has been detached from the {@link Grid}. Adds a new Column to Grid. - * Also adds the property to container with data type String, if property - * for column does not exist in it. Default value for the new property is an - * empty String. + * Adds a new Column to Grid. Also adds the property to container with data + * type String, if property for column does not exist in it. Default value + * for the new property is an empty String. + *

+ * Note that adding a new property is only done for the default container + * that Grid sets up with the default constructor. * * @param propertyId * the property id of the new column * @return the new column + * + * @throws IllegalStateException + * if column for given property already exists in this grid */ - public Column addColumn(Object propertyId) { - addColumnProperty(propertyId, String.class, ""); + + public Column addColumn(Object propertyId) throws IllegalStateException { + if (datasource.getContainerPropertyIds().contains(propertyId) + && !columns.containsKey(propertyId)) { + appendColumn(propertyId); + } else { + addColumnProperty(propertyId, String.class, ""); + } return getColumn(propertyId); } /** - * Adds a new Column to Grid. Also adds the property to container with given - * Number data type, if property for column does not exist already in it. - * Default value for the new property is 0. + * Adds a new Column to Grid. This function makes sure that the property + * with the given id and data type exists in the container. If property does + * not exists, it will be created. Default value for the new property is 0. + *

+ * Note that adding a new property is only done for the default container + * that Grid sets up with the default constructor. * * @param propertyId * the property id of the new column * @return the new column + * + * @throws IllegalStateException + * if column for given property already exists in this grid or + * property already exists in the container with wrong type */ - public Column addColumn(Object propertyId, Class type) { + public Column addColumn(Object propertyId, Class type) + throws IllegalStateException { addColumnProperty(propertyId, type, 0); return getColumn(propertyId); } protected void addColumnProperty(Object propertyId, Class type, - Object defaultValue) { + Object defaultValue) throws IllegalStateException { + if (!defaultContainer) { + throw new IllegalStateException( + "Container for this Grid is not a default container from Grid() constructor"); + } + if (!columns.containsKey(propertyId)) { if (!datasource.getContainerPropertyIds().contains(propertyId)) { datasource.addContainerProperty(propertyId, type, defaultValue); diff --git a/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnBuildTest.java b/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnBuildTest.java index ca11ffe885..31b918f51e 100644 --- a/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnBuildTest.java +++ b/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnBuildTest.java @@ -16,12 +16,15 @@ package com.vaadin.tests.server.component.grid; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import org.junit.Before; import org.junit.Test; import com.vaadin.data.Container; import com.vaadin.data.Property; +import com.vaadin.data.util.IndexedContainer; import com.vaadin.ui.Grid; public class GridColumnBuildTest { @@ -80,4 +83,34 @@ public class GridColumnBuildTest { grid.removeColumn("foo"); grid.addColumn("foo", Integer.class); } + + @Test(expected = IllegalStateException.class) + public void testAddColumnToNonDefaultContainer() { + grid.setContainerDataSource(new IndexedContainer()); + grid.addColumn("foo"); + } + + @Test + public void testAddColumnForExistingProperty() { + grid.addColumn("bar"); + IndexedContainer container2 = new IndexedContainer(); + container2.addContainerProperty("foo", Integer.class, 0); + container2.addContainerProperty("bar", String.class, ""); + grid.setContainerDataSource(container2); + assertNull("Grid should not have a column for property foo", + grid.getColumn("foo")); + grid.removeAllColumns(); + grid.addColumn("foo"); + assertNotNull("Grid should now have a column for property foo", + grid.getColumn("foo")); + assertNull("Grid should not have a column for property bar anymore", + grid.getColumn("bar")); + } + + @Test(expected = IllegalStateException.class) + public void testAddIncompatibleColumnProperty() { + grid.addColumn("bar"); + grid.removeAllColumns(); + grid.addColumn("bar", Integer.class); + } } diff --git a/server/tests/src/com/vaadin/tests/server/component/grid/sort/SortTest.java b/server/tests/src/com/vaadin/tests/server/component/grid/sort/SortTest.java index 343cad36c4..5c74728437 100644 --- a/server/tests/src/com/vaadin/tests/server/component/grid/sort/SortTest.java +++ b/server/tests/src/com/vaadin/tests/server/component/grid/sort/SortTest.java @@ -167,6 +167,9 @@ public class SortTest { @Test public void testChangeContainerAfterSorting() { + class Person { + } + container.expectedSort(new Object[] { "foo", "bar", "baz" }, new SortDirection[] { SortDirection.ASCENDING, SortDirection.ASCENDING, SortDirection.DESCENDING }); @@ -179,7 +182,9 @@ public class SortTest { SortDirection.DESCENDING)); container = new DummySortingIndexedContainer(); + container.addContainerProperty("foo", Person.class, null); container.addContainerProperty("baz", String.class, ""); + container.addContainerProperty("bar", Person.class, null); container.expectedSort(new Object[] { "baz" }, new SortDirection[] { SortDirection.DESCENDING }); grid.setContainerDataSource(container); -- 2.39.5