Fix WidgetRenderer column cells not correctly init on change (#19086)

When changing to a widget renderer with an existing column, the widget
renderer expects that the cells would be initialized to have a correct
widget for it. Because of original design where you could not change
renderers, this was not taken into account and cells did not get
reinitialized when changing the renderer.

This patch showed another underlying detach problem from removing a
widget renderer and destroying complex renderers. These both are also
addressed to make this bug possible to test correctly.

Patch includes a client-side test that verifies the integrity of the
renderer state in different stages of its lifecycle.

Change-Id: I67330e5d07c95047cb69040e8355a17dc8a96f08

7 files changed, 414 insertions, 2 deletions

diff --git a/client/src/com/vaadin/client/connectors/MultiSelectionModelConnector.java b/client/src/com/vaadin/client/connectors/MultiSelectionModelConnector.java
index 04c56a5b44..5d00619995 100644
--- a/client/src/com/vaadin/client/connectors/MultiSelectionModelConnector.java
+++ b/client/src/com/vaadin/client/connectors/MultiSelectionModelConnector.java
@@ -128,7 +128,6 @@ public class MultiSelectionModelConnector extends
                             }
                         });
             } else if (renderer != null) {
-                renderer.destroy();
                 selectAll.removeHandler();
                 dataAvailable.removeHandler();
                 renderer = null;
diff --git a/client/src/com/vaadin/client/widgets/Grid.java b/client/src/com/vaadin/client/widgets/Grid.java
index f96ee69010..806bc6a220 100644
--- a/client/src/com/vaadin/client/widgets/Grid.java
+++ b/client/src/com/vaadin/client/widgets/Grid.java
@@ -4791,8 +4791,36 @@ public class Grid<T> extends ResizeComposite implements
             }
 
             if (renderer != bodyRenderer) {
+                // Variables used to restore removed column.
+                boolean columnRemoved = false;
+                double widthInConfiguration = 0.0d;
+                ColumnConfiguration conf = null;
+                int index = 0;
+
+                if (grid != null
+                        && (bodyRenderer instanceof WidgetRenderer || renderer instanceof WidgetRenderer)) {
+                    // Column needs to be recreated.
+                    index = grid.getColumns().indexOf(this);
+                    conf = grid.escalator.getColumnConfiguration();
+                    widthInConfiguration = conf.getColumnWidth(index);
+
+                    conf.removeColumns(index, 1);
+                    columnRemoved = true;
+                }
+
+                // Complex renderers need to be destroyed.
+                if (bodyRenderer instanceof ComplexRenderer) {
+                    ((ComplexRenderer) bodyRenderer).destroy();
+                }
+
                 bodyRenderer = renderer;
 
+                if (columnRemoved) {
+                    // Restore the column.
+                    conf.insertColumns(index, 1);
+                    conf.setColumnWidth(index, widthInConfiguration);
+                }
+
                 if (grid != null) {
                     grid.refreshBody();
                 }
@@ -5486,7 +5514,7 @@ public class Grid<T> extends ResizeComposite implements
                 if (renderer instanceof WidgetRenderer) {
                     try {
                         Widget w = WidgetUtil.findWidget(cell.getElement()
-                                .getFirstChildElement(), Widget.class);
+                                .getFirstChildElement(), null);
                         if (w != null) {
 
                             // Logical detach
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridRendererChange.java b/uitest/src/com/vaadin/tests/components/grid/GridRendererChange.java
new file mode 100644
index 0000000000..180ed0ded9
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/grid/GridRendererChange.java
@@ -0,0 +1,68 @@
+/*
+ * 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.ui.Button;
+import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.HorizontalLayout;
+import com.vaadin.ui.renderers.ButtonRenderer;
+import com.vaadin.ui.renderers.HtmlRenderer;
+import com.vaadin.ui.renderers.TextRenderer;
+
+public class GridRendererChange extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+
+        final Grid grid = new Grid();
+        grid.setColumns("num", "foo");
+        grid.getColumn("num").setRenderer(new ButtonRenderer());
+
+        for (int i = 0; i < 1000; i++) {
+            grid.addRow(String.format("<b>line %d</b>", i), "" + i);
+        }
+
+        Button button = new Button("Set ButtonRenderer",
+                new Button.ClickListener() {
+                    @Override
+                    public void buttonClick(ClickEvent event) {
+                        grid.getColumn("num").setRenderer(new ButtonRenderer());
+                    }
+                });
+
+        Button buttonHtml = new Button("Set HTMLRenderer",
+                new Button.ClickListener() {
+                    @Override
+                    public void buttonClick(ClickEvent event) {
+                        grid.getColumn("num").setRenderer(new HtmlRenderer());
+                    }
+                });
+
+        Button buttonText = new Button("Set TextRenderer",
+                new Button.ClickListener() {
+                    @Override
+                    public void buttonClick(ClickEvent event) {
+                        grid.getColumn("num").setRenderer(new TextRenderer());
+                    }
+                });
+
+        addComponent(new HorizontalLayout(button, buttonHtml, buttonText));
+        addComponent(grid);
+    }
+}
\ No newline at end of file
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridRendererChangeTest.java b/uitest/src/com/vaadin/tests/components/grid/GridRendererChangeTest.java
new file mode 100644
index 0000000000..a8d7fb06be
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/grid/GridRendererChangeTest.java
@@ -0,0 +1,59 @@
+/*
+ * 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.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Test;
+import org.openqa.selenium.By;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.testbench.elements.GridElement.GridCellElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class GridRendererChangeTest extends MultiBrowserTest {
+
+    @Test
+    public void testChangeRenderer() {
+        setDebug(true);
+        openTestURL();
+
+        GridCellElement cell = $(GridElement.class).first().getCell(0, 0);
+        assertTrue("No button in the first cell.",
+                cell.isElementPresent(By.tagName("button")));
+        int width = cell.getSize().getWidth();
+
+        List<ButtonElement> buttons = $(ButtonElement.class).all();
+        Collections.reverse(buttons);
+
+        // Order: TextRenderer, HTMLRenderer, ButtonRenderer
+        for (ButtonElement button : buttons) {
+            button.click();
+            assertNoErrorNotifications();
+            cell = $(GridElement.class).first().getCell(0, 0);
+            assertEquals("Cell size changed", width, cell.getSize().getWidth());
+        }
+
+        assertTrue("No button in the first cell.",
+                cell.isElementPresent(By.tagName("button")));
+    }
+
+}
\ No newline at end of file
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridWidgetRendererChange.java b/uitest/src/com/vaadin/tests/components/grid/GridWidgetRendererChange.java
new file mode 100644
index 0000000000..8d9847b428
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/grid/GridWidgetRendererChange.java
@@ -0,0 +1,33 @@
+/*
+ * 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.annotations.Widgetset;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.widgetset.TestingWidgetSet;
+import com.vaadin.tests.widgetset.client.grid.GridRendererChangeWidget;
+import com.vaadin.tests.widgetset.server.TestWidgetComponent;
+import com.vaadin.ui.UI;
+
+@Widgetset(TestingWidgetSet.NAME)
+public class GridWidgetRendererChange extends UI {
+
+    @Override
+    protected void init(VaadinRequest request) {
+        setContent(new TestWidgetComponent(GridRendererChangeWidget.class));
+    }
+
+}
\ No newline at end of file
diff --git a/uitest/src/com/vaadin/tests/components/grid/GridWidgetRendererChangeTest.java b/uitest/src/com/vaadin/tests/components/grid/GridWidgetRendererChangeTest.java
new file mode 100644
index 0000000000..5098a0e7d8
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/grid/GridWidgetRendererChangeTest.java
@@ -0,0 +1,63 @@
+/*
+ * 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 org.junit.Test;
+import org.openqa.selenium.WebElement;
+
+import com.vaadin.testbench.By;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class GridWidgetRendererChangeTest extends SingleBrowserTest {
+
+    @Test
+    public void testChangeWidgetRenderer() {
+        setDebug(true);
+        openTestURL();
+
+        selectMenuPath("Component", "Change first renderer");
+
+        assertNoErrorNotifications();
+
+        selectMenuPath("Component", "Change first renderer");
+
+        assertNoErrorNotifications();
+
+        // First renderer OK
+
+        selectMenuPath("Component", "Change second renderer");
+
+        assertNoErrorNotifications();
+
+        selectMenuPath("Component", "Change second renderer");
+
+        assertNoErrorNotifications();
+
+    }
+
+    @Override
+    protected void selectMenu(String menuCaption) {
+        // GWT menu does not need to be clicked.
+        selectMenu(menuCaption, false);
+    }
+
+    @Override
+    protected WebElement getMenuElement(String menuCaption) {
+        return getDriver().findElement(
+                By.xpath("//td[text() = '" + menuCaption + "']"));
+    }
+
+}
diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/GridRendererChangeWidget.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/GridRendererChangeWidget.java
new file mode 100644
index 0000000000..d35580b3c6
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/GridRendererChangeWidget.java
@@ -0,0 +1,162 @@
+/*
+ * 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.widgetset.client.grid;
+
+import com.google.gwt.core.client.Scheduler.ScheduledCommand;
+import com.google.gwt.user.client.ui.Button;
+import com.vaadin.client.renderers.ButtonRenderer;
+import com.vaadin.client.renderers.TextRenderer;
+import com.vaadin.client.widget.grid.RendererCellReference;
+import com.vaadin.client.widget.grid.datasources.ListDataSource;
+import com.vaadin.client.widgets.Grid;
+import com.vaadin.client.widgets.Grid.Column;
+
+public class GridRendererChangeWidget extends
+        PureGWTTestApplication<Grid<String[]>> {
+
+    public class MyButtonRenderer extends ButtonRenderer {
+
+        private final Button widget = new Button();
+
+        private boolean hasInit = false;
+        private boolean hasBeenDestroyed = false;
+        private boolean wasAttached = false;
+
+        @Override
+        public void init(RendererCellReference cell) {
+            if (hasInit || hasBeenDestroyed) {
+                throw new RuntimeException("Init in an unexpected state.");
+            }
+            super.init(cell);
+
+            hasInit = true;
+        }
+
+        @Override
+        public Button createWidget() {
+            return widget;
+        }
+
+        @Override
+        public void render(RendererCellReference cell, String text,
+                Button button) {
+            if (!hasInit || hasBeenDestroyed) {
+                throw new RuntimeException("Render in an unexpected state.");
+            }
+            if (button != widget) {
+                throw new RuntimeException("Unexpected button instance");
+            }
+            if (button.getParent() != getTestedWidget()) {
+                throw new RuntimeException("Button not attached!");
+            }
+
+            super.render(cell, text, button);
+
+            wasAttached = true;
+        }
+
+        @Override
+        public void destroy() {
+            if (!hasInit || !wasAttached) {
+                throw new RuntimeException("Destroy in an unexpected state");
+            }
+
+            super.destroy();
+
+            hasBeenDestroyed = true;
+        }
+
+        public void verify() {
+            if (!hasInit) {
+                throw new RuntimeException("Failed. Not initialized");
+            } else if (!wasAttached) {
+                throw new RuntimeException("Failed. Not attached");
+            } else if (widget.getParent() != null) {
+                throw new RuntimeException("Failed. Not detached");
+            } else if (!hasBeenDestroyed) {
+                throw new RuntimeException("Failed. Not destroyed");
+            }
+        }
+    }
+
+    public GridRendererChangeWidget() {
+        super(new Grid<String[]>());
+        String[] strArr = new String[] { "foo", "bar" };
+        ListDataSource<String[]> ds = new ListDataSource<String[]>(strArr);
+        final Grid<String[]> grid = getTestedWidget();
+        grid.setDataSource(ds);
+        final Column<String, String[]> first = new Column<String, String[]>() {
+
+            @Override
+            public String getValue(String[] row) {
+                return row[0];
+            }
+        };
+        grid.addColumn(first).setHeaderCaption("First")
+                .setRenderer(new MyButtonRenderer());
+        final Column<String, String[]> second = new Column<String, String[]>() {
+
+            @Override
+            public String getValue(String[] row) {
+                return row[1];
+            }
+        };
+        grid.addColumn(second).setHeaderCaption("Second")
+                .setRenderer(new MyButtonRenderer());
+
+        addMenuCommand("Change first renderer", new ScheduledCommand() {
+
+            boolean isButton = true;
+
+            @Override
+            public void execute() {
+                if (isButton) {
+                    final MyButtonRenderer r = (MyButtonRenderer) first
+                            .getRenderer();
+                    first.setRenderer(new TextRenderer());
+                    r.verify();
+                } else {
+                    first.setRenderer(new MyButtonRenderer());
+                }
+                isButton = !isButton;
+            }
+
+        }, "Component");
+        addMenuCommand("Change second renderer", new ScheduledCommand() {
+
+            boolean isButton = true;
+
+            @Override
+            public void execute() {
+                if (isButton) {
+                    MyButtonRenderer r = (MyButtonRenderer) second
+                            .getRenderer();
+                    second.setRenderer(new TextRenderer());
+                    r.verify();
+                } else {
+                    second.setRenderer(new MyButtonRenderer());
+                }
+                isButton = !isButton;
+            }
+
+        }, "Component");
+
+        addNorth(grid, 600);
+
+        grid.getElement().getStyle().setZIndex(0);
+    }
+
+}