Remove Column visibility setting from server side (#13334)

Change-Id: I2ef97672dd2085e9178f44fce39ead40bd84713c

7 files changed, 52 insertions, 105 deletions

diff --git a/server/src/com/vaadin/data/RpcDataProviderExtension.java b/server/src/com/vaadin/data/RpcDataProviderExtension.java
index d607879aa5..6b35ab7f88 100644
--- a/server/src/com/vaadin/data/RpcDataProviderExtension.java
+++ b/server/src/com/vaadin/data/RpcDataProviderExtension.java
@@ -722,12 +722,16 @@ public class RpcDataProviderExtension extends AbstractExtension {
         for (Object propertyId : propertyIds) {
             Column column = grid.getColumn(propertyId);
 
-            Object propertyValue = item.getItemProperty(propertyId).getValue();
-            JsonValue encodedValue = encodeValue(propertyValue,
-                    column.getRenderer(), column.getConverter(),
-                    grid.getLocale());
-
-            rowData.put(columnKeys.key(propertyId), encodedValue);
+            // TODO: Optimize this with Grid.getColumns() 04.12.2014 -Teemu
+            if (column != null) {
+                Object propertyValue = item.getItemProperty(propertyId)
+                        .getValue();
+                JsonValue encodedValue = encodeValue(propertyValue,
+                        column.getRenderer(), column.getConverter(),
+                        grid.getLocale());
+
+                rowData.put(columnKeys.key(propertyId), encodedValue);
+            }
         }
 
         final JsonObject rowObject = Json.createObject();
diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java
index ef4f48c9c4..1453178cf6 100644
--- a/server/src/com/vaadin/ui/Grid.java
+++ b/server/src/com/vaadin/ui/Grid.java
@@ -1100,36 +1100,6 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         }
 
         /**
-         * Is this column visible in the grid. By default all columns are
-         * visible.
-         * 
-         * @return <code>true</code> if the column is visible
-         * @throws IllegalStateException
-         *             if the column is no longer attached to any grid
-         */
-        public boolean isVisible() throws IllegalStateException {
-            checkColumnIsAttached();
-            return state.visible;
-        }
-
-        /**
-         * Set the visibility of this column
-         * 
-         * @param visible
-         *            is the column visible
-         * @return the column itself
-         * 
-         * @throws IllegalStateException
-         *             if the column is no longer attached to any grid
-         */
-        public Column setVisible(boolean visible) throws IllegalStateException {
-            checkColumnIsAttached();
-            state.visible = visible;
-            grid.markAsDirty();
-            return this;
-        }
-
-        /**
          * Checks if column is attached and throws an
          * {@link IllegalStateException} if it is not
          * 
@@ -1919,6 +1889,7 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
             }
             for (Object columnId : removedColumns) {
                 removeColumn(columnId);
+                columnKeys.remove(columnId);
             }
             datasourceExtension.propertiesRemoved(removedColumns);
 
@@ -2248,6 +2219,7 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
             removeExtension(datasourceExtension);
         }
 
+        columnKeys.removeAll();
         datasource = container;
 
         /*
@@ -2529,7 +2501,6 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         Column column = columns.remove(propertyId);
         getState().columnOrder.remove(columnKeys.key(propertyId));
         getState().columns.remove(column.getState());
-        columnKeys.remove(propertyId);
         removeExtension(column.getRenderer());
     }
 
@@ -3537,7 +3508,7 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         Header header = getHeader();
         for (int i = 0; i < header.getRowCount(); ++i) {
             HeaderRow row = header.getRow(i);
-            for (Object propId : datasource.getContainerPropertyIds()) {
+            for (Object propId : columns.keySet()) {
                 HeaderCell cell = row.getCell(propId);
                 if (cell.getCellState().type == GridStaticCellType.WIDGET) {
                     componentList.add(cell.getComponent());
@@ -3548,7 +3519,7 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         Footer footer = getFooter();
         for (int i = 0; i < footer.getRowCount(); ++i) {
             FooterRow row = footer.getRow(i);
-            for (Object propId : datasource.getContainerPropertyIds()) {
+            for (Object propId : columns.keySet()) {
                 FooterCell cell = row.getCell(propId);
                 if (cell.getCellState().type == GridStaticCellType.WIDGET) {
                     componentList.add(cell.getComponent());
diff --git a/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnBuildTest.java b/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnAddingAndRemovingTest.java
index e36b6b4d0f..f401fba1e3 100644
--- a/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnBuildTest.java
+++ b/server/tests/src/com/vaadin/tests/server/component/grid/GridColumnAddingAndRemovingTest.java
@@ -27,7 +27,7 @@ import com.vaadin.data.Property;
 import com.vaadin.data.util.IndexedContainer;
 import com.vaadin.ui.Grid;
 
-public class GridColumnBuildTest {
+public class GridColumnAddingAndRemovingTest {
 
     Grid grid = new Grid();
     Container.Indexed container;
diff --git a/server/tests/src/com/vaadin/tests/server/component/grid/GridColumns.java b/server/tests/src/com/vaadin/tests/server/component/grid/GridColumns.java
index 0f8887e9ce..f18eeb42c4 100644
--- a/server/tests/src/com/vaadin/tests/server/component/grid/GridColumns.java
+++ b/server/tests/src/com/vaadin/tests/server/component/grid/GridColumns.java
@@ -97,14 +97,6 @@ public class GridColumns {
         assertEquals(column.getHeaderCaption(), grid.getDefaultHeaderRow()
                 .getCell("column1").getText());
 
-        column.setVisible(false);
-        assertFalse(column.isVisible());
-        assertFalse(getColumnState("column1").visible);
-
-        column.setVisible(true);
-        assertTrue(column.isVisible());
-        assertTrue(getColumnState("column1").visible);
-
         column.setWidth(100);
         assertEquals(100, column.getWidth());
         assertEquals(column.getWidth(), getColumnState("column1").width);
@@ -121,7 +113,8 @@ public class GridColumns {
     }
 
     @Test
-    public void testRemovingColumn() throws Exception {
+    public void testRemovingColumnByRemovingPropertyFromContainer()
+            throws Exception {
 
         Column column = grid.getColumn("column1");
         assertNotNull(column);
@@ -138,13 +131,6 @@ public class GridColumns {
         }
 
         try {
-            column.setVisible(false);
-            fail("Succeeded in modifying a detached column");
-        } catch (IllegalStateException ise) {
-            // Detached state should throw exception
-        }
-
-        try {
             column.setWidth(123);
             fail("Succeeded in modifying a detached column");
         } catch (IllegalStateException ise) {
@@ -156,7 +142,7 @@ public class GridColumns {
     }
 
     @Test
-    public void testAddingColumn() throws Exception {
+    public void testAddingColumnByAddingPropertyToContainer() throws Exception {
         grid.getContainerDataSource().addContainerProperty("columnX",
                 String.class, "");
         Column column = grid.getColumn("columnX");
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridColspans.java b/uitest/src/com/vaadin/tests/components/grid/GridColspans.java
index 602f17a15f..4c23c3bcab 100644
--- a/uitest/src/com/vaadin/tests/components/grid/GridColspans.java
+++ b/uitest/src/com/vaadin/tests/components/grid/GridColspans.java
@@ -24,7 +24,6 @@ import com.vaadin.ui.Button;
 import com.vaadin.ui.Button.ClickEvent;
 import com.vaadin.ui.Grid;
 import com.vaadin.ui.Grid.FooterRow;
-import com.vaadin.ui.Grid.Column;
 import com.vaadin.ui.Grid.HeaderRow;
 import com.vaadin.ui.components.grid.renderers.NumberRenderer;
 
@@ -70,8 +69,11 @@ public class GridColspans extends AbstractTestUI {
 
                     @Override
                     public void buttonClick(ClickEvent event) {
-                        Column column = grid.getColumn("firstName");
-                        column.setVisible(!column.isVisible());
+                        if (grid.getColumn("firstName") != null) {
+                            grid.removeColumn("firstName");
+                        } else {
+                            grid.addColumn("firstName");
+                        }
                     }
                 }));
 
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 ccbb85da08..2e0ac5a5cf 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java
@@ -466,27 +466,17 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
         for (int c = 0; c < COLUMNS; c++) {
             createCategory(getColumnProperty(c), "Columns");
 
-            createBooleanAction("Visible", getColumnProperty(c), true,
-                    new Command<Grid, Boolean>() {
-
-                        @Override
-                        public void execute(Grid grid, Boolean value,
-                                Object columnIndex) {
-                            Object propertyId = getColumnProperty((Integer) columnIndex);
-                            Column column = grid.getColumn(propertyId);
-                            column.setVisible(!column.isVisible());
-                        }
-                    }, c);
-
-            createClickAction("Remove", getColumnProperty(c),
+            createClickAction("Add / Remove", getColumnProperty(c),
                     new Command<Grid, String>() {
 
                         @Override
                         public void execute(Grid grid, String value, Object data) {
-                            grid.getContainerDataSource()
-                                    .removeContainerProperty(
-                                            getColumnProperty((Integer) data));
-                            removeCategory("Column " + data);
+                            String columnProperty = getColumnProperty((Integer) data);
+                            if (grid.getColumn(columnProperty) == null) {
+                                grid.addColumn(columnProperty);
+                            } else {
+                                grid.removeColumn(columnProperty);
+                            }
                         }
                     }, null, c);
 
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java
index 795755fc3d..9f1d61124e 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridStructureTest.java
@@ -40,32 +40,12 @@ import com.vaadin.tests.components.grid.basicfeatures.GridBasicFeaturesTest;
 public class GridStructureTest extends GridBasicFeaturesTest {
 
     @Test
-    public void testHidingColumn() throws Exception {
-        openTestURL();
-
-        // Column 0 should be visible
-        List<TestBenchElement> cells = getGridHeaderRowCells();
-        assertEquals("column 0", cells.get(0).getText().toLowerCase());
-
-        // Hide column 0
-        selectMenuPath("Component", "Columns", "Column 0", "Visible");
-
-        // Column 1 should now be the first cell
-        cells = getGridHeaderRowCells();
-
-        /*
-         * Reindeer has a CSS text transformation that changes the casing so
-         * that we can't rely on it being what we set
-         */
-        assertEquals("column 1", cells.get(0).getText().toLowerCase());
-    }
-
-    @Test
-    public void testHidingAllColumns() {
+    public void testRemovingAllColumns() {
         setDebug(true);
         openTestURL();
         for (int i = 0; i < GridBasicFeatures.COLUMNS; ++i) {
-            selectMenuPath("Component", "Columns", "Column " + i, "Visible");
+            selectMenuPath("Component", "Columns", "Column " + i,
+                    "Add / Remove");
             assertFalse(isElementPresent(NotificationElement.class));
         }
 
@@ -74,6 +54,20 @@ public class GridStructureTest extends GridBasicFeaturesTest {
     }
 
     @Test
+    public void testRemoveAndAddColumn() {
+        setDebug(true);
+        openTestURL();
+
+        assertEquals("Column 0", getGridElement().getHeaderCell(0, 0).getText());
+        selectMenuPath("Component", "Columns", "Column 0", "Add / Remove");
+        assertEquals("Column 1", getGridElement().getHeaderCell(0, 0).getText());
+        selectMenuPath("Component", "Columns", "Column 0", "Add / Remove");
+        // Column 0 is appended to the end of grid
+        assertEquals("Column 0", getGridElement().getHeaderCell(0, 11)
+                .getText());
+    }
+
+    @Test
     public void testRemovingColumn() throws Exception {
         openTestURL();
 
@@ -82,7 +76,7 @@ public class GridStructureTest extends GridBasicFeaturesTest {
         assertEquals("column 0", cells.get(0).getText().toLowerCase());
 
         // Hide column 0
-        selectMenuPath("Component", "Columns", "Column 0", "Remove");
+        selectMenuPath("Component", "Columns", "Column 0", "Add / Remove");
 
         // Column 1 should now be the first cell
         cells = getGridHeaderRowCells();
@@ -94,9 +88,9 @@ public class GridStructureTest extends GridBasicFeaturesTest {
         openTestURL();
 
         // Remove columns 2,3,4
-        selectMenuPath("Component", "Columns", "Column 2", "Remove");
-        selectMenuPath("Component", "Columns", "Column 3", "Remove");
-        selectMenuPath("Component", "Columns", "Column 4", "Remove");
+        selectMenuPath("Component", "Columns", "Column 2", "Add / Remove");
+        selectMenuPath("Component", "Columns", "Column 3", "Add / Remove");
+        selectMenuPath("Component", "Columns", "Column 4", "Add / Remove");
 
         // Scroll so new data is lazy loaded
         scrollGridVerticallyTo(1000);
@@ -300,7 +294,7 @@ public class GridStructureTest extends GridBasicFeaturesTest {
         String columnName = "Column " + (GridBasicFeatures.COLUMNS - 1);
         assertTrue(columnName + " was not present in DOM",
                 isElementPresent(By.xpath("//th[text()='" + columnName + "']")));
-        selectMenuPath("Component", "Columns", columnName, "Remove");
+        selectMenuPath("Component", "Columns", columnName, "Add / Remove");
         assertFalse(isElementPresent(NotificationElement.class));
         assertFalse(columnName + " was still present in DOM",
                 isElementPresent(By.xpath("//th[text()='" + columnName + "']")));