Remove selection column from single selection model (#13334)

This patch makes some changes to selection model logic and refactors the
space selection handling from MultiSelectionRenderer to its own class.

Change-Id: I5c4a7d68531a8b4f38991719ae54c9f87119e9a4

7 files changed, 197 insertions, 85 deletions

diff --git a/client/src/com/vaadin/client/ui/grid/Grid.java b/client/src/com/vaadin/client/ui/grid/Grid.java
index db7b69ccb1..353c6ff97c 100644
--- a/client/src/com/vaadin/client/ui/grid/Grid.java
+++ b/client/src/com/vaadin/client/ui/grid/Grid.java
@@ -3786,6 +3786,11 @@ public class Grid<T> extends ResizeComposite implements
         }
 
         if (this.selectColumnRenderer != null) {
+            if (this.selectColumnRenderer instanceof ComplexRenderer) {
+                // End of Life for the old selection column renderer.
+                ((ComplexRenderer<?>) this.selectColumnRenderer).destroy();
+            }
+
             // Clear field so frozen column logic in the remove method knows
             // what to do
             GridColumn<?, T> colToRemove = selectionColumn;
@@ -3828,9 +3833,9 @@ public class Grid<T> extends ResizeComposite implements
             throw new IllegalArgumentException("Selection model can't be null");
         }
 
-        if (selectColumnRenderer != null
-                && selectColumnRenderer instanceof ComplexRenderer) {
-            ((ComplexRenderer<?>) selectColumnRenderer).destroy();
+        if (this.selectionModel != null) {
+            // Detach selection model from Grid.
+            this.selectionModel.setGrid(null);
         }
 
         this.selectionModel = selectionModel;
diff --git a/client/src/com/vaadin/client/ui/grid/selection/MultiSelectionRenderer.java b/client/src/com/vaadin/client/ui/grid/selection/MultiSelectionRenderer.java
index 01be4ea43f..2b5e3b79a4 100644
--- a/client/src/com/vaadin/client/ui/grid/selection/MultiSelectionRenderer.java
+++ b/client/src/com/vaadin/client/ui/grid/selection/MultiSelectionRenderer.java
@@ -27,7 +27,6 @@ import com.google.gwt.dom.client.InputElement;
 import com.google.gwt.dom.client.NativeEvent;
 import com.google.gwt.dom.client.TableElement;
 import com.google.gwt.dom.client.TableSectionElement;
-import com.google.gwt.event.dom.client.KeyCodes;
 import com.google.gwt.event.shared.HandlerRegistration;
 import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.Event;
@@ -35,17 +34,10 @@ import com.google.gwt.user.client.Event.NativePreviewEvent;
 import com.google.gwt.user.client.Event.NativePreviewHandler;
 import com.vaadin.client.Util;
 import com.vaadin.client.ui.grid.Cell;
-import com.vaadin.client.ui.grid.DataAvailableEvent;
-import com.vaadin.client.ui.grid.DataAvailableHandler;
 import com.vaadin.client.ui.grid.FlyweightCell;
 import com.vaadin.client.ui.grid.Grid;
-import com.vaadin.client.ui.grid.events.BodyKeyDownHandler;
-import com.vaadin.client.ui.grid.events.BodyKeyUpHandler;
-import com.vaadin.client.ui.grid.events.GridKeyDownEvent;
-import com.vaadin.client.ui.grid.events.GridKeyUpEvent;
 import com.vaadin.client.ui.grid.renderers.ComplexRenderer;
 import com.vaadin.client.ui.grid.selection.SelectionModel.Multi.Batched;
-import com.vaadin.shared.ui.grid.ScrollDestination;
 
 /* This class will probably not survive the final merge of all selection functionality. */
 public class MultiSelectionRenderer<T> extends ComplexRenderer<Boolean> {
@@ -540,76 +532,19 @@ public class MultiSelectionRenderer<T> extends ComplexRenderer<Boolean> {
         }
     }
 
-    private class SpaceKeyDownSelectHandler implements BodyKeyDownHandler {
-
-        private HandlerRegistration scrollHandler = null;
-        private boolean spaceDown = false;
-
-        @Override
-        public void onKeyDown(GridKeyDownEvent event) {
-            if (event.getNativeKeyCode() != KeyCodes.KEY_SPACE || spaceDown) {
-                return;
-            }
-
-            // Prevent space page scrolling
-            event.getNativeEvent().preventDefault();
-
-            spaceDown = true;
-            Cell focused = event.getFocusedCell();
-            final int rowIndex = focused.getRow();
-
-            if (scrollHandler != null) {
-                scrollHandler.removeHandler();
-                scrollHandler = null;
-            }
-
-            scrollHandler = grid
-                    .addDataAvailableHandler(new DataAvailableHandler() {
-
-                        @Override
-                        public void onDataAvailable(
-                                DataAvailableEvent dataAvailableEvent) {
-                            if (dataAvailableEvent.getAvailableRows().contains(
-                                    rowIndex)) {
-                                setSelected(rowIndex, !isSelected(rowIndex));
-                                scrollHandler.removeHandler();
-                                scrollHandler = null;
-                            }
-                        }
-                    });
-            grid.scrollToRow(rowIndex, ScrollDestination.ANY);
-        }
-
-    }
-
     private static final String LOGICAL_ROW_PROPERTY_INT = "vEscalatorLogicalRow";
 
     private final Grid<T> grid;
     private HandlerRegistration nativePreviewHandlerRegistration;
-    private final SpaceKeyDownSelectHandler handler = new SpaceKeyDownSelectHandler();
-    private HandlerRegistration spaceDownRegistration;
-    private HandlerRegistration spaceUpRegistration;
 
     private final AutoScrollHandler autoScrollHandler = new AutoScrollHandler();
 
     public MultiSelectionRenderer(final Grid<T> grid) {
         this.grid = grid;
-        spaceDownRegistration = grid.addBodyKeyDownHandler(handler);
-        spaceUpRegistration = grid.addBodyKeyUpHandler(new BodyKeyUpHandler() {
-
-            @Override
-            public void onKeyUp(GridKeyUpEvent event) {
-                if (event.getNativeKeyCode() == KeyCodes.KEY_SPACE) {
-                    handler.spaceDown = false;
-                }
-            }
-        });
     }
 
     @Override
     public void destroy() {
-        spaceDownRegistration.removeHandler();
-        spaceUpRegistration.removeHandler();
         if (nativePreviewHandlerRegistration != null) {
             removeNativeHandler();
         }
diff --git a/client/src/com/vaadin/client/ui/grid/selection/SelectionModel.java b/client/src/com/vaadin/client/ui/grid/selection/SelectionModel.java
index 55336ce3da..9f81f8f5f2 100644
--- a/client/src/com/vaadin/client/ui/grid/selection/SelectionModel.java
+++ b/client/src/com/vaadin/client/ui/grid/selection/SelectionModel.java
@@ -60,7 +60,8 @@ public interface SelectionModel<T> {
      * internally by Grid.
      * 
      * @param grid
-     *            a {@link Grid} instance
+     *            a {@link Grid} instance; <code>null</code> when removing from
+     *            Grid
      */
     public void setGrid(Grid<T> grid);
 
diff --git a/client/src/com/vaadin/client/ui/grid/selection/SelectionModelMulti.java b/client/src/com/vaadin/client/ui/grid/selection/SelectionModelMulti.java
index 33b5354570..177e3bdca8 100644
--- a/client/src/com/vaadin/client/ui/grid/selection/SelectionModelMulti.java
+++ b/client/src/com/vaadin/client/ui/grid/selection/SelectionModelMulti.java
@@ -43,6 +43,9 @@ public class SelectionModelMulti<T> extends AbstractRowHandleSelectionModel<T>
     private final LinkedHashSet<RowHandle<T>> selectionBatch = new LinkedHashSet<RowHandle<T>>();
     private final LinkedHashSet<RowHandle<T>> deselectionBatch = new LinkedHashSet<RowHandle<T>>();
 
+    /* Event handling for selection with space key */
+    private SpaceSelectHandler<T> spaceSelectHandler;
+
     public SelectionModelMulti() {
         grid = null;
         renderer = null;
@@ -61,18 +64,24 @@ public class SelectionModelMulti<T> extends AbstractRowHandleSelectionModel<T>
 
     @Override
     public void setGrid(Grid<T> grid) {
-        if (grid == null) {
-            throw new IllegalArgumentException("Grid cannot be null");
+        if (this.grid != null && grid != null) {
+            // Trying to replace grid
+            throw new IllegalStateException(
+                    "Selection model is already attached to a grid. "
+                            + "Remove the selection model first from "
+                            + "the grid and then add it.");
         }
 
-        if (this.grid == null) {
-            this.grid = grid;
+        this.grid = grid;
+        if (this.grid != null) {
+            spaceSelectHandler = new SpaceSelectHandler<T>(grid);
+            this.renderer = new MultiSelectionRenderer<T>(grid);
         } else {
-            throw new IllegalStateException(
-                    "Grid reference cannot be reassigned");
+            spaceSelectHandler.removeHandler();
+            spaceSelectHandler = null;
+            this.renderer = null;
         }
 
-        this.renderer = new MultiSelectionRenderer<T>(grid);
     }
 
     @Override
diff --git a/client/src/com/vaadin/client/ui/grid/selection/SelectionModelSingle.java b/client/src/com/vaadin/client/ui/grid/selection/SelectionModelSingle.java
index d63b371c4d..2c8b6cd391 100644
--- a/client/src/com/vaadin/client/ui/grid/selection/SelectionModelSingle.java
+++ b/client/src/com/vaadin/client/ui/grid/selection/SelectionModelSingle.java
@@ -33,7 +33,8 @@ public class SelectionModelSingle<T> extends AbstractRowHandleSelectionModel<T>
 
     private Grid<T> grid;
     private RowHandle<T> selectedRow;
-    private Renderer<Boolean> renderer;
+    /** Event handling for selection with space key */
+    private SpaceSelectHandler<T> spaceSelectHandler;
 
     @Override
     public boolean isSelected(T row) {
@@ -43,22 +44,27 @@ public class SelectionModelSingle<T> extends AbstractRowHandleSelectionModel<T>
 
     @Override
     public Renderer<Boolean> getSelectionColumnRenderer() {
-        return renderer;
+        // No Selection column renderer for single selection
+        return null;
     }
 
     @Override
     public void setGrid(Grid<T> grid) {
-        if (grid == null) {
-            throw new IllegalArgumentException("Grid cannot be null");
+        if (this.grid != null && grid != null) {
+            // Trying to replace grid
+            throw new IllegalStateException(
+                    "Selection model is already attached to a grid. "
+                            + "Remove the selection model first from "
+                            + "the grid and then add it.");
         }
 
-        if (this.grid == null) {
-            this.grid = grid;
+        this.grid = grid;
+        if (this.grid != null) {
+            spaceSelectHandler = new SpaceSelectHandler<T>(grid);
         } else {
-            throw new IllegalStateException(
-                    "Grid reference cannot be reassigned");
+            spaceSelectHandler.removeHandler();
+            spaceSelectHandler = null;
         }
-        renderer = new MultiSelectionRenderer<T>(grid);
     }
 
     @Override
diff --git a/client/src/com/vaadin/client/ui/grid/selection/SpaceSelectHandler.java b/client/src/com/vaadin/client/ui/grid/selection/SpaceSelectHandler.java
new file mode 100644
index 0000000000..c92ebacfd1
--- /dev/null
+++ b/client/src/com/vaadin/client/ui/grid/selection/SpaceSelectHandler.java
@@ -0,0 +1,126 @@
+/*
+ * 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.client.ui.grid.selection;
+
+import com.google.gwt.event.dom.client.KeyCodes;
+import com.google.gwt.event.shared.HandlerRegistration;
+import com.vaadin.client.ui.grid.Cell;
+import com.vaadin.client.ui.grid.DataAvailableEvent;
+import com.vaadin.client.ui.grid.DataAvailableHandler;
+import com.vaadin.client.ui.grid.Grid;
+import com.vaadin.client.ui.grid.events.BodyKeyDownHandler;
+import com.vaadin.client.ui.grid.events.BodyKeyUpHandler;
+import com.vaadin.client.ui.grid.events.GridKeyDownEvent;
+import com.vaadin.client.ui.grid.events.GridKeyUpEvent;
+import com.vaadin.shared.ui.grid.ScrollDestination;
+
+/**
+ * Generic class to perform selections when pressing space key.
+ * 
+ * @since
+ * @author Vaadin Ltd
+ * @param <T>
+ *            row data type
+ */
+public class SpaceSelectHandler<T> {
+
+    /**
+     * Handler for space key down events in Grid Body
+     */
+    private class SpaceKeyDownHandler implements BodyKeyDownHandler {
+        private HandlerRegistration scrollHandler = null;
+
+        @Override
+        public void onKeyDown(GridKeyDownEvent event) {
+            if (event.getNativeKeyCode() != KeyCodes.KEY_SPACE || spaceDown) {
+                return;
+            }
+
+            // Prevent space page scrolling
+            event.getNativeEvent().preventDefault();
+
+            spaceDown = true;
+            Cell focused = event.getFocusedCell();
+            final int rowIndex = focused.getRow();
+
+            if (scrollHandler != null) {
+                scrollHandler.removeHandler();
+                scrollHandler = null;
+            }
+
+            scrollHandler = grid
+                    .addDataAvailableHandler(new DataAvailableHandler() {
+
+                        @Override
+                        public void onDataAvailable(
+                                DataAvailableEvent dataAvailableEvent) {
+                            if (dataAvailableEvent.getAvailableRows().contains(
+                                    rowIndex)) {
+                                setSelected(grid, rowIndex);
+                                scrollHandler.removeHandler();
+                                scrollHandler = null;
+                            }
+                        }
+                    });
+            grid.scrollToRow(rowIndex, ScrollDestination.ANY);
+        }
+
+        protected void setSelected(Grid<T> grid, int rowIndex) {
+            T row = grid.getDataSource().getRow(rowIndex);
+
+            if (grid.isSelected(row)) {
+                grid.deselect(row);
+            } else {
+                grid.select(row);
+            }
+        }
+    }
+
+    private boolean spaceDown = false;
+    private Grid<T> grid;
+    private HandlerRegistration spaceUpHandler;
+    private HandlerRegistration spaceDownHandler;
+
+    /**
+     * Constructor for SpaceSelectHandler. This constructor will add all
+     * necessary handlers for selecting rows with space.
+     * 
+     * @param grid
+     *            grid to attach to
+     */
+    public SpaceSelectHandler(Grid<T> grid) {
+        this.grid = grid;
+        spaceDownHandler = grid
+                .addBodyKeyDownHandler(new SpaceKeyDownHandler());
+        spaceUpHandler = grid.addBodyKeyUpHandler(new BodyKeyUpHandler() {
+
+            @Override
+            public void onKeyUp(GridKeyUpEvent event) {
+                if (event.getNativeKeyCode() == KeyCodes.KEY_SPACE) {
+                    spaceDown = false;
+                }
+            }
+        });
+    }
+
+    /**
+     * Clean up function for removing all now obsolete handlers.
+     */
+    public void removeHandler() {
+        spaceDownHandler.removeHandler();
+        spaceUpHandler.removeHandler();
+    }
+}
\ No newline at end of file
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridSelectionTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridSelectionTest.java
index 8a76acb60b..a242ad1dd1 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridSelectionTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridSelectionTest.java
@@ -150,7 +150,31 @@ public class GridSelectionTest extends GridBasicFeaturesTest {
 
         assertTrue("Grid row 3 was not selected with space key.", grid
                 .getRow(3).isSelected());
+    }
 
+    @Test
+    public void testKeyboardWithSingleSelection() {
+        openTestURL();
+        setSelectionModelSingle();
+
+        GridElement grid = getGridElement();
+        grid.getCell(3, 1).click();
+        new Actions(getDriver()).sendKeys(Keys.SPACE).perform();
+
+        assertTrue("Grid row 3 was not selected with space key.", grid
+                .getRow(3).isSelected());
+
+        new Actions(getDriver()).sendKeys(Keys.SPACE).perform();
+
+        assertTrue("Grid row 3 was not deselected with space key.", !grid
+                .getRow(3).isSelected());
+
+        grid.scrollToRow(500);
+
+        new Actions(getDriver()).sendKeys(Keys.SPACE).perform();
+
+        assertTrue("Grid row 3 was not selected with space key.", grid
+                .getRow(3).isSelected());
     }
 
     @Test
@@ -194,6 +218,12 @@ public class GridSelectionTest extends GridBasicFeaturesTest {
                 "Check box shouldn't have been in header for Single Selection Model",
                 header.isElementPresent(By.tagName("input")));
 
+        // Single selection model shouldn't have selection column to begin with
+        assertFalse(
+                "Selection columnn shouldn't have been in grid for Single Selection Model",
+                getGridElement().getCell(0, 1).isElementPresent(
+                        By.tagName("input")));
+
         setSelectionModelNone();
         header = getGridElement().getHeaderCell(0, 0);
         assertFalse(