Allow changing renderers after column creation (#8841)

Closes #8250

8 files changed, 296 insertions, 26 deletions

diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
index d619b50b5d..149ef4c1f9 100644
--- a/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
@@ -73,7 +73,6 @@ public class ColumnConnector extends AbstractExtensionConnector {
                 return null;
             }
         };
-        column.setRenderer(getRendererConnector().getRenderer());
         getParent().addColumn(column, getState().internalId);
     }
 
@@ -92,6 +91,11 @@ public class ColumnConnector extends AbstractExtensionConnector {
         column.setSortable(getState().sortable);
     }
 
+    @OnStateChange("renderer")
+    void updateRenderer() {
+        column.setRenderer(getRendererConnector().getRenderer());
+    }
+
     @OnStateChange("hidingToggleCaption")
     void updateHidingToggleCaption() {
         column.setHidingToggleCaption(getState().hidingToggleCaption);
diff --git a/client/src/main/java/com/vaadin/client/connectors/treegrid/TreeGridConnector.java b/client/src/main/java/com/vaadin/client/connectors/treegrid/TreeGridConnector.java
index 7f11683bbe..0cccb6cd6f 100644
--- a/client/src/main/java/com/vaadin/client/connectors/treegrid/TreeGridConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/treegrid/TreeGridConnector.java
@@ -18,12 +18,13 @@ package com.vaadin.client.connectors.treegrid;
 import java.util.Collection;
 import java.util.logging.Logger;
 
+import com.google.gwt.core.client.Scheduler;
 import com.google.gwt.dom.client.BrowserEvents;
 import com.google.gwt.dom.client.Element;
 import com.google.gwt.event.dom.client.KeyCodes;
 import com.google.gwt.user.client.Event;
 import com.google.web.bindery.event.shared.HandlerRegistration;
-import com.vaadin.client.communication.StateChangeEvent;
+import com.vaadin.client.annotations.OnStateChange;
 import com.vaadin.client.connectors.grid.GridConnector;
 import com.vaadin.client.renderers.ClickableRenderer;
 import com.vaadin.client.renderers.HierarchyRenderer;
@@ -43,7 +44,7 @@ import elemental.json.JsonObject;
 
 /**
  * A connector class for the TreeGrid component.
- * 
+ *
  * @author Vaadin Ltd
  * @since 8.1
  */
@@ -67,13 +68,18 @@ public class TreeGridConnector extends GridConnector {
         return (TreeGridState) super.getState();
     }
 
-    @Override
-    public void onStateChanged(StateChangeEvent stateChangeEvent) {
-        super.onStateChanged(stateChangeEvent);
-
-        if (stateChangeEvent.hasPropertyChanged("hierarchyColumnId")
-                || stateChangeEvent.hasPropertyChanged("columns")) {
-
+    /**
+     * This method has been scheduled finally to avoid possible race conditions
+     * between state change handling for the Grid and its columns. The renderer
+     * of the column is set in a state change handler, and might not be
+     * available when this method is executed.
+     * <p>
+     * TODO: This might need some clean up if we decide to allow setting a new
+     * renderer for hierarchy columns.
+     */
+    @OnStateChange("hierarchyColumnId")
+    void updateHierarchyColumn() {
+        Scheduler.get().scheduleFinally(() -> {
             // Id of old hierarchy column
             String oldHierarchyColumnId = this.hierarchyColumnId;
 
@@ -111,7 +117,7 @@ public class TreeGridConnector extends GridConnector {
                 Logger.getLogger(TreeGridConnector.class.getName()).warning(
                         "Couldn't find column: " + newHierarchyColumnId);
             }
-        }
+        });
     }
 
     private HierarchyRenderer getHierarchyRenderer() {
@@ -159,18 +165,21 @@ public class TreeGridConnector extends GridConnector {
     }
 
     private native void replaceCellFocusEventHandler(Grid<?> grid,
-            GridEventHandler<?> eventHandler)/*-{
+            GridEventHandler<?> eventHandler)
+    /*-{
         var browserEventHandlers = grid.@com.vaadin.client.widgets.Grid::browserEventHandlers;
-        
+    
         // FocusEventHandler is initially 5th in the list of browser event handlers
         browserEventHandlers.@java.util.List::set(*)(5, eventHandler);
     }-*/;
 
-    private native void replaceClickEvent(Grid<?> grid, GridClickEvent event)/*-{
+    private native void replaceClickEvent(Grid<?> grid, GridClickEvent event)
+    /*-{
         grid.@com.vaadin.client.widgets.Grid::clickEvent = event;
     }-*/;
 
-    private native EventCellReference<?> getEventCell(Grid<?> grid)/*-{
+    private native EventCellReference<?> getEventCell(Grid<?> grid)
+    /*-{
         return grid.@com.vaadin.client.widgets.Grid::eventCell;
     }-*/;
 
@@ -213,13 +222,15 @@ public class TreeGridConnector extends GridConnector {
             }
         }
 
-        private native Collection<String> getNavigationEvents(Grid<?> grid)/*-{
-           return grid.@com.vaadin.client.widgets.Grid::cellFocusHandler
-           .@com.vaadin.client.widgets.Grid.CellFocusHandler::getNavigationEvents()();
+        private native Collection<String> getNavigationEvents(Grid<?> grid)
+        /*-{
+            return grid.@com.vaadin.client.widgets.Grid::cellFocusHandler
+            .@com.vaadin.client.widgets.Grid.CellFocusHandler::getNavigationEvents()();
         }-*/;
 
         private native void handleNavigationEvent(Grid<?> grid,
-                Grid.GridEvent<JsonObject> event)/*-{
+                Grid.GridEvent<JsonObject> event)
+        /*-{
             grid.@com.vaadin.client.widgets.Grid::cellFocusHandler
             .@com.vaadin.client.widgets.Grid.CellFocusHandler::handleNavigationEvent(*)(
             event.@com.vaadin.client.widgets.Grid.GridEvent::getDomEvent()(),
diff --git a/documentation/components/components-grid.asciidoc b/documentation/components/components-grid.asciidoc
index ed66c45d2f..223be80e6f 100644
--- a/documentation/components/components-grid.asciidoc
+++ b/documentation/components/components-grid.asciidoc
@@ -403,9 +403,23 @@ You set the column renderer in the [classname]#Grid.Column# object as follows:
 [source, java]
 ----
 // the type of birthYear is a number
-Column<Integer> bornColumn = grid.addColumn(Person:getBirthYear,
+Column<Person, Integer> bornColumn = grid.addColumn(Person:getBirthYear,
       new NumberRenderer("born in %d AD"));
 ----
+
+Changing the renderer during runtime is also possible, but for type safety
+you should store the column reference with data types for doing this.
+When you change the renderer, the content of Grid is refreshed.
+
+[source, java]
+----
+Column<Person, Integer> ageColumn = grid.addColumn(Person::getBirthYear);
+// The default renderer is TextRenderer
+addComponent(new Button("Change renderer", 
+	clickEvent -> ageColumn.setRenderer(new NumberRenderer())
+));
+----
+
 The following renderers are available, as defined in the server-side
 [package]#com.vaadin.ui.renderers# package:
 
diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java
index cfd0ff2006..19222d8c85 100644
--- a/server/src/main/java/com/vaadin/ui/Grid.java
+++ b/server/src/main/java/com/vaadin/ui/Grid.java
@@ -73,6 +73,7 @@ import com.vaadin.server.SerializableFunction;
 import com.vaadin.server.SerializableSupplier;
 import com.vaadin.server.Setter;
 import com.vaadin.server.VaadinServiceClassLoaderUtil;
+import com.vaadin.shared.Connector;
 import com.vaadin.shared.MouseEventDetails;
 import com.vaadin.shared.Registration;
 import com.vaadin.shared.data.DataCommunicatorConstants;
@@ -1814,6 +1815,34 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
         }
 
         /**
+         * Sets the Renderer for this Column. Setting the renderer will cause
+         * all currently available row data to be recreated and sent to the
+         * client.
+         *
+         * @param renderer
+         *            the new renderer
+         * @return this column
+         */
+        public Column<T, V> setRenderer(Renderer<? super V> renderer) {
+            Objects.requireNonNull(renderer, "Renderer can't be null");
+
+            // Remove old renderer
+            Connector oldRenderer = getState().renderer;
+            if (oldRenderer != null && oldRenderer instanceof Extension) {
+                removeExtension((Extension) oldRenderer);
+            }
+
+            // Set new renderer
+            getState().renderer = renderer;
+            addExtension(renderer);
+
+            // Trigger redraw
+            getParent().getDataCommunicator().reset();
+
+            return this;
+        }
+
+        /**
          * Gets the grid that this column belongs to.
          *
          * @return the grid that this column belongs to, or <code>null</code> if
@@ -2341,11 +2370,25 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
     public <V> Column<T, V> addColumn(ValueProvider<T, V> valueProvider,
             AbstractRenderer<? super T, ? super V> renderer) {
         String generatedIdentifier = getGeneratedIdentifier();
-        Column<T, V> column = new Column<>(valueProvider, renderer);
+        Column<T, V> column = createColumn(valueProvider, renderer);
         addColumn(generatedIdentifier, column);
         return column;
     }
 
+    /**
+     * Creates a column instance from a value provider and a renderer.
+     *
+     * @param valueProvider
+     *            the value provider
+     * @param renderer
+     *            the renderer
+     * @return a new column instance
+     */
+    protected <V> Column<T, V> createColumn(ValueProvider<T, V> valueProvider,
+            AbstractRenderer<? super T, ? super V> renderer) {
+        return new Column<>(valueProvider, renderer);
+    }
+
     private void addColumn(String identifier, Column<T, ?> column) {
         if (getColumns().contains(column)) {
             return;
diff --git a/server/src/main/java/com/vaadin/ui/TreeGrid.java b/server/src/main/java/com/vaadin/ui/TreeGrid.java
index f438df4e8f..46e6c99b59 100644
--- a/server/src/main/java/com/vaadin/ui/TreeGrid.java
+++ b/server/src/main/java/com/vaadin/ui/TreeGrid.java
@@ -19,21 +19,24 @@ import java.util.Collection;
 import java.util.Objects;
 import java.util.stream.Stream;
 
+import com.vaadin.data.ValueProvider;
 import com.vaadin.data.provider.DataProvider;
 import com.vaadin.data.provider.HierarchicalDataProvider;
 import com.vaadin.shared.ui.treegrid.NodeCollapseRpc;
 import com.vaadin.shared.ui.treegrid.TreeGridCommunicationConstants;
 import com.vaadin.shared.ui.treegrid.TreeGridState;
+import com.vaadin.ui.renderers.AbstractRenderer;
+import com.vaadin.ui.renderers.Renderer;
 
 import elemental.json.Json;
 import elemental.json.JsonObject;
 
 /**
  * A grid component for displaying hierarchical tabular data.
- * 
+ *
  * @author Vaadin Ltd
  * @since 8.1
- * 
+ *
  * @param <T>
  *            the grid bean type
  */
@@ -106,9 +109,12 @@ public class TreeGrid<T> extends Grid<T> {
      * <p>
      * Setting a hierarchy column by calling this method also sets the column to
      * be visible and not hidable.
-     * 
+     * <p>
+     * <strong>Note:</strong> Changing the Renderer of the hierarchy column is
+     * not supported.
+     *
      * @see Column#setId(String)
-     * 
+     *
      * @param id
      *            id of the column to use for displaying hierarchy
      */
@@ -137,7 +143,7 @@ public class TreeGrid<T> extends Grid<T> {
      * expanded, it will be collapsed.
      * <p>
      * Toggling expansion on a leaf item in the hierarchy will have no effect.
-     * 
+     *
      * @param item
      *            the item to toggle expansion for
      */
@@ -161,4 +167,24 @@ public class TreeGrid<T> extends Grid<T> {
         }
         return (HierarchicalDataProvider<T, ?>) dataProvider;
     }
+
+    @Override
+    protected <V> Column<T, V> createColumn(ValueProvider<T, V> valueProvider,
+            AbstractRenderer<? super T, ? super V> renderer) {
+        return new Column<T, V>(valueProvider, renderer) {
+
+            @Override
+            public com.vaadin.ui.Grid.Column<T, V> setRenderer(
+                    Renderer<? super V> renderer) {
+                // Disallow changing renderer for the hierarchy column
+                if (getInternalIdForColumn(this).equals(
+                        TreeGrid.this.getState(false).hierarchyColumnId)) {
+                    throw new IllegalStateException(
+                            "Changing the renderer of the hierarchy column is not allowed.");
+                }
+
+                return super.setRenderer(renderer);
+            }
+        };
+    }
 }
diff --git a/server/src/test/java/com/vaadin/tests/components/treegrid/TreeGridColumnTest.java b/server/src/test/java/com/vaadin/tests/components/treegrid/TreeGridColumnTest.java
new file mode 100644
index 0000000000..a928a48a08
--- /dev/null
+++ b/server/src/test/java/com/vaadin/tests/components/treegrid/TreeGridColumnTest.java
@@ -0,0 +1,19 @@
+package com.vaadin.tests.components.treegrid;
+
+import org.junit.Test;
+
+import com.vaadin.ui.TreeGrid;
+import com.vaadin.ui.renderers.TextRenderer;
+
+public class TreeGridColumnTest {
+
+    private TreeGrid<String> treeGrid = new TreeGrid<>();
+
+    @Test(expected = IllegalStateException.class)
+    public void testChangeRendererOfHierarchyColumn() {
+        treeGrid.addColumn(Object::toString).setId("foo");
+        treeGrid.setHierarchyColumn("foo");
+        // This should not be allowed.
+        treeGrid.getColumn("foo").setRenderer(new TextRenderer());
+    }
+}
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridRendererSwitch.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRendererSwitch.java
new file mode 100644
index 0000000000..b73383d22b
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRendererSwitch.java
@@ -0,0 +1,53 @@
+package com.vaadin.tests.components.grid;
+
+import java.util.stream.IntStream;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.Grid.Column;
+import com.vaadin.ui.Notification;
+import com.vaadin.ui.renderers.ButtonRenderer;
+import com.vaadin.ui.renderers.TextRenderer;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+public class GridRendererSwitch extends AbstractTestUI {
+
+    private boolean textRenderer = true;
+    private boolean reverse = false;
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        Grid<Integer> grid = new Grid<>();
+        Column<Integer, String> column = grid.addColumn(i -> "Foo " + i)
+                .setCaption("Foo");
+        Column<Integer, String> secondColumn = grid.addColumn(i -> "Bar " + i)
+                .setCaption("Bar");
+
+        addComponent(grid);
+        addComponent(new Button("Switch", e -> {
+            if (textRenderer) {
+                ButtonRenderer<Integer> renderer = new ButtonRenderer<>();
+                renderer.addClickListener(event -> Notification
+                        .show("Click on row: " + event.getItem()));
+                column.setRenderer(renderer);
+            } else {
+                column.setRenderer(new TextRenderer());
+            }
+            textRenderer = !textRenderer;
+        }));
+        addComponent(new Button("Reverse", e -> {
+            if (reverse) {
+                grid.setColumnOrder(column, secondColumn);
+            } else {
+                grid.setColumnOrder(secondColumn, column);
+            }
+            reverse = !reverse;
+        }));
+
+        grid.setItems(IntStream.range(0, 10).boxed());
+    }
+
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridRendererSwitchTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRendererSwitchTest.java
new file mode 100644
index 0000000000..1fd775b58a
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRendererSwitchTest.java
@@ -0,0 +1,100 @@
+package com.vaadin.tests.components.grid;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.vaadin.testbench.By;
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.testbench.elements.NotificationElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class GridRendererSwitchTest extends SingleBrowserTest {
+
+    @Test
+    public void testSwitchRenderer() {
+        setDebug(true);
+        openTestURL();
+
+        GridElement grid = $(GridElement.class).first();
+        Assert.assertEquals("Unexpected content in first grid cell", "Foo 0",
+                grid.getCell(0, 0).getAttribute("innerHTML"));
+        ButtonElement button = $(ButtonElement.class).first();
+        button.click();
+        Assert.assertFalse("No button in cell", grid.getCell(0, 0)
+                .findElements(By.tagName("button")).isEmpty());
+        grid.getCell(0, 0).findElement(By.tagName("button")).click();
+        Assert.assertTrue("Notification not shown",
+                isElementPresent(NotificationElement.class));
+        button.click();
+        Assert.assertEquals("Cell should be back to text content.", "Foo 0",
+                grid.getCell(0, 0).getAttribute("innerHTML"));
+
+        assertNoErrorNotifications();
+    }
+
+    @Test
+    public void testSwitchRendererReorderColumns() {
+        setDebug(true);
+        openTestURL();
+
+        GridElement grid = $(GridElement.class).first();
+        Assert.assertEquals("Unexpected content in first grid cell", "Foo 0",
+                grid.getCell(0, 0).getAttribute("innerHTML"));
+        ButtonElement button = $(ButtonElement.class).caption("Switch").first();
+        button.click();
+        ButtonElement reverse = $(ButtonElement.class).caption("Reverse")
+                .first();
+        reverse.click();
+        Assert.assertEquals(
+                "Unexpected content in first grid cell after reorder", "Bar 0",
+                grid.getCell(0, 0).getAttribute("innerHTML"));
+
+        Assert.assertFalse("No button in cell after reversing order", grid
+                .getCell(0, 1).findElements(By.tagName("button")).isEmpty());
+        grid.getCell(0, 1).findElement(By.tagName("button")).click();
+        Assert.assertTrue("Notification not shown",
+                isElementPresent(NotificationElement.class));
+        reverse.click();
+        Assert.assertFalse("No button in cell after restoring original order",
+                grid.getCell(0, 0).findElements(By.tagName("button"))
+                        .isEmpty());
+
+        assertNoErrorNotifications();
+    }
+
+    @Test
+    public void testReorderColumnsSwitchRenderer() {
+        setDebug(true);
+        openTestURL();
+
+        GridElement grid = $(GridElement.class).first();
+        Assert.assertEquals("Unexpected content in first grid cell", "Foo 0",
+                grid.getCell(0, 0).getAttribute("innerHTML"));
+        ButtonElement reverse = $(ButtonElement.class).caption("Reverse")
+                .first();
+
+        reverse.click();
+        Assert.assertEquals(
+                "Unexpected content in first grid cell after reorder", "Bar 0",
+                grid.getCell(0, 0).getAttribute("innerHTML"));
+
+        ButtonElement button = $(ButtonElement.class).caption("Switch").first();
+        button.click();
+
+        Assert.assertFalse(
+                "No button in cell after reversing order and changing renderer",
+                grid.getCell(0, 1).findElements(By.tagName("button"))
+                        .isEmpty());
+        grid.getCell(0, 1).findElement(By.tagName("button")).click();
+        Assert.assertTrue("Notification not shown",
+                isElementPresent(NotificationElement.class));
+
+        button.click();
+        Assert.assertEquals("Cell should be back to text content.", "Foo 0",
+                grid.getCell(0, 1).getAttribute("innerHTML"));
+
+        assertNoErrorNotifications();
+    }
+
+}