Fixes property change events after first render (#13334)

Change-Id: Ief2e1763c7764009e9244ae4334ccacacc3bb205

6 files changed, 132 insertions, 38 deletions

diff --git a/client/src/com/vaadin/client/ui/grid/FlyweightCell.java b/client/src/com/vaadin/client/ui/grid/FlyweightCell.java
index 30cc4fc79e..adcca1b630 100644
--- a/client/src/com/vaadin/client/ui/grid/FlyweightCell.java
+++ b/client/src/com/vaadin/client/ui/grid/FlyweightCell.java
@@ -102,6 +102,10 @@ public class FlyweightCell {
         if (iterator.areCellsAttached()) {
             final TableCellElement e = row.getElement().getCells()
                     .getItem(column);
+
+            assert e != null : "Cell " + column + " for logical row "
+                    + row.getRow() + " doesn't exist in the DOM!";
+
             e.setPropertyInt(COLSPAN_ATTR, 1);
             e.getStyle().setWidth(row.getColumnWidth(column), Unit.PX);
             e.getStyle().clearDisplay();
diff --git a/client/src/com/vaadin/client/ui/grid/Grid.java b/client/src/com/vaadin/client/ui/grid/Grid.java
index 6e752bc989..748f5c69be 100644
--- a/client/src/com/vaadin/client/ui/grid/Grid.java
+++ b/client/src/com/vaadin/client/ui/grid/Grid.java
@@ -1849,8 +1849,6 @@ public class Grid<T> extends ResizeComposite implements
         ((AbstractGridColumn<?, T>) column).setGrid(null);
 
         columns.remove(columnIndex);
-
-        refreshBody();
     }
 
     /**
diff --git a/client/src/com/vaadin/client/ui/grid/GridConnector.java b/client/src/com/vaadin/client/ui/grid/GridConnector.java
index 39c7e2cde4..f719b5979e 100644
--- a/client/src/com/vaadin/client/ui/grid/GridConnector.java
+++ b/client/src/com/vaadin/client/ui/grid/GridConnector.java
@@ -27,6 +27,8 @@ import java.util.Map;
 import java.util.Set;
 import java.util.logging.Logger;
 
+import com.google.gwt.core.client.Scheduler;
+import com.google.gwt.core.client.Scheduler.ScheduledCommand;
 import com.google.gwt.json.client.JSONObject;
 import com.google.gwt.json.client.JSONValue;
 import com.google.gwt.user.client.ui.Widget;
@@ -106,6 +108,13 @@ public class GridConnector extends AbstractHasComponentsConnector implements
                     + rowData;
 
             final JSONValue columnValue = rowDataObject.get(id);
+
+            /*
+             * note, Java "null" is different from JSONValue "null" (i.e.
+             * JSONNull).
+             */
+            assert columnValue != null : "Could not find data for column with id "
+                    + id;
             return rendererConnector.decode(columnValue);
         }
 
@@ -363,53 +372,76 @@ public class GridConnector extends AbstractHasComponentsConnector implements
     }
 
     @Override
-    public void onStateChanged(StateChangeEvent stateChangeEvent) {
+    public void onStateChanged(final StateChangeEvent stateChangeEvent) {
         super.onStateChanged(stateChangeEvent);
 
-        // Column updates
-        if (stateChangeEvent.hasPropertyChanged("columns")) {
+        /*
+         * The operations in here have been made deferred.
+         * 
+         * The row data needed to react to column changes comes in the RPC
+         * calls. Since state is always updated before RPCs are called, we need
+         * to be sure that RPC is called before Grid reacts to state changes.
+         * 
+         * Note that there are still some methods annotated with @OnStateChange
+         * that aren't deferred. That's okay, though.
+         */
 
-            // Remove old columns
-            purgeRemovedColumns();
+        Scheduler.get().scheduleDeferred(new ScheduledCommand() {
+            @Override
+            public void execute() {
+                // Column updates
+                if (stateChangeEvent.hasPropertyChanged("columns")) {
+
+                    // Remove old columns
+                    purgeRemovedColumns();
+
+                    // Add new columns
+                    for (GridColumnState state : getState().columns) {
+                        if (!columnIdToColumn.containsKey(state.id)) {
+                            addColumnFromStateChangeEvent(state);
+                        }
+                        updateColumnFromState(columnIdToColumn.get(state.id),
+                                state);
+                    }
+                }
 
-            // Add new columns
-            for (GridColumnState state : getState().columns) {
-                if (!columnIdToColumn.containsKey(state.id)) {
-                    addColumnFromStateChangeEvent(state);
+                if (stateChangeEvent.hasPropertyChanged("columnOrder")) {
+                    if (orderNeedsUpdate(getState().columnOrder)) {
+                        updateColumnOrderFromState(getState().columnOrder);
+                    }
                 }
-                updateColumnFromState(columnIdToColumn.get(state.id), state);
-            }
-        }
 
-        if (stateChangeEvent.hasPropertyChanged("columnOrder")) {
-            if (orderNeedsUpdate(getState().columnOrder)) {
-                updateColumnOrderFromState(getState().columnOrder);
-            }
-        }
+                if (stateChangeEvent.hasPropertyChanged("header")) {
+                    updateSectionFromState(getWidget().getHeader(),
+                            getState().header);
+                }
 
-        if (stateChangeEvent.hasPropertyChanged("header")) {
-            updateSectionFromState(getWidget().getHeader(), getState().header);
-        }
+                if (stateChangeEvent.hasPropertyChanged("footer")) {
+                    updateSectionFromState(getWidget().getFooter(),
+                            getState().footer);
+                }
 
-        if (stateChangeEvent.hasPropertyChanged("footer")) {
-            updateSectionFromState(getWidget().getFooter(), getState().footer);
-        }
+                if (stateChangeEvent.hasPropertyChanged("lastFrozenColumnId")) {
+                    String frozenColId = getState().lastFrozenColumnId;
+                    if (frozenColId != null) {
+                        CustomGridColumn column = columnIdToColumn
+                                .get(frozenColId);
+                        assert column != null : "Column to be frozen could not be found (id:"
+                                + frozenColId + ")";
+                        getWidget().setLastFrozenColumn(column);
+                    } else {
+                        getWidget().setLastFrozenColumn(null);
+                    }
+                }
+
+                if (stateChangeEvent.hasPropertyChanged("editorRowEnabled")) {
+                    getWidget().getEditorRow().setEnabled(
+                            getState().editorRowEnabled);
+                }
 
-        if (stateChangeEvent.hasPropertyChanged("lastFrozenColumnId")) {
-            String frozenColId = getState().lastFrozenColumnId;
-            if (frozenColId != null) {
-                CustomGridColumn column = columnIdToColumn.get(frozenColId);
-                assert column != null : "Column to be frozen could not be found (id:"
-                        + frozenColId + ")";
-                getWidget().setLastFrozenColumn(column);
-            } else {
-                getWidget().setLastFrozenColumn(null);
             }
-        }
+        });
 
-        if (stateChangeEvent.hasPropertyChanged("editorRowEnabled")) {
-            getWidget().getEditorRow().setEnabled(getState().editorRowEnabled);
-        }
     }
 
     private void updateColumnOrderFromState(List<String> stateColumnOrder) {
diff --git a/server/src/com/vaadin/data/RpcDataProviderExtension.java b/server/src/com/vaadin/data/RpcDataProviderExtension.java
index 8e98316d00..9744ed3b92 100644
--- a/server/src/com/vaadin/data/RpcDataProviderExtension.java
+++ b/server/src/com/vaadin/data/RpcDataProviderExtension.java
@@ -453,6 +453,10 @@ public class RpcDataProviderExtension extends AbstractExtension {
          *            the property ids that have been added to the container
          */
         public void propertiesAdded(Collection<Object> addedPropertyIds) {
+            if (addedPropertyIds.isEmpty()) {
+                return;
+            }
+
             for (int i = activeRange.getStart(); i < activeRange.getEnd(); i++) {
                 final Object itemId = container.getIdByIndex(i);
                 final Item item = container.getItem(itemId);
@@ -468,6 +472,8 @@ public class RpcDataProviderExtension extends AbstractExtension {
                                 .addValueChangeListener(listener);
                     }
                 }
+
+                updateRowData(i);
             }
         }
 
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 b6da2f53ee..cac82c2a3c 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java
@@ -67,6 +67,7 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
 
     private int columnGroupRows = 0;
     private IndexedContainer ds;
+    private Grid grid;
 
     @Override
     @SuppressWarnings("unchecked")
@@ -186,6 +187,8 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
 
         createColumnActions();
 
+        createPropertyActions();
+
         createHeaderActions();
 
         createFooterActions();
@@ -218,6 +221,7 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
                     }
                 }, null);
 
+        this.grid = grid;
         return grid;
     }
 
@@ -569,6 +573,28 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
         return "Column " + c;
     }
 
+    protected void createPropertyActions() {
+        createCategory("Properties", null);
+
+        createBooleanAction("Prepend property", "Properties", false,
+                new Command<Grid, Boolean>() {
+                    private final Object propertyId = new Object();
+
+                    @Override
+                    public void execute(Grid c, Boolean enable, Object data) {
+                        if (enable.booleanValue()) {
+                            ds.addContainerProperty(propertyId, String.class,
+                                    "property value");
+                            grid.getColumn(propertyId).setHeaderCaption(
+                                    "new property");
+                            grid.setColumnOrder(propertyId);
+                        } else {
+                            ds.removeContainerProperty(propertyId);
+                        }
+                    }
+                }, null);
+    }
+
     protected void createRowActions() {
         createCategory("Body rows", null);
 
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 34b09d18d1..190feba2db 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
@@ -19,6 +19,7 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.core.IsNot.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -303,6 +304,33 @@ public class GridStructureTest extends GridBasicFeaturesTest {
         }
     }
 
+    @Test
+    public void testAddingProperty() {
+        setDebug(true);
+        openTestURL();
+
+        assertNotEquals("property value", getGridElement().getCell(0, 0)
+                .getText());
+        selectMenuPath("Component", "Properties", "Prepend property");
+        assertEquals("property value", getGridElement().getCell(0, 0).getText());
+    }
+
+    @Test
+    public void testRemovingAddedProperty() {
+        openTestURL();
+
+        assertEquals("(0, 0)", getGridElement().getCell(0, 0).getText());
+        assertNotEquals("property value", getGridElement().getCell(0, 0)
+                .getText());
+
+        selectMenuPath("Component", "Properties", "Prepend property");
+        selectMenuPath("Component", "Properties", "Prepend property");
+
+        assertNotEquals("property value", getGridElement().getCell(0, 0)
+                .getText());
+        assertEquals("(0, 0)", getGridElement().getCell(0, 0).getText());
+    }
+
     private boolean verticalScrollbarIsPresent() {
         return "scroll".equals(getGridVerticalScrollbar().getCssValue(
                 "overflow-y"));