Fix selecting logic of SingleSelectionModel API (#13334)

Change-Id: Ib48e586cb791669efc4aa98442a31c1dda42e1aa

server/src/com/vaadin/ui/Grid.java

@@ -2575,7 +2575,10 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
     // keep this javadoc in sync with SelectionModel.Single.deselect
     public boolean deselect(Object itemId) throws IllegalStateException {
         if (selectionModel instanceof SelectionModel.Single) {
-            return ((SelectionModel.Single) selectionModel).deselect(itemId);
+            if (isSelected(itemId)) {
+                return ((SelectionModel.Single) selectionModel).select(null);
+            }
+            return false;
         } else if (selectionModel instanceof SelectionModel.Multi) {
             return ((SelectionModel.Multi) selectionModel).deselect(itemId);
         } else {

server/src/com/vaadin/ui/components/grid/selection/SelectionModel.java

@@ -167,40 +167,26 @@ public interface SelectionModel extends Serializable {
      * do in the server-side and client-side APIs.
      */
     public interface Single extends SelectionModel {
+
         /**
          * Marks an item as selected.
          * 
          * @param itemIds
-         *            the itemId to mark as selected
+         *            the itemId to mark as selected; <code>null</code> for
+         *            deselect
          * @return <code>true</code> if the selection state changed.
          *         <code>false</code> if the itemId already was selected
          * @throws IllegalStateException
          *             if the selection was illegal. One such reason might be
-         *             that the implementation already had an item selected, and
-         *             that needs to be explicitly deselected before
-         *             re-selecting something
+         *             that the given id was null, indicating a deselect, but
+         *             implementation doesn't allow deselecting. re-selecting
+         *             something
          * @throws IllegalArgumentException
          *             if given itemId does not exist in the container of Grid
-         * @see #deselect(Object)
          */
         boolean select(Object itemId) throws IllegalStateException,
                 IllegalArgumentException;
 
-        /**
-         * Marks an item as deselected.
-         * 
-         * @param itemId
-         *            the itemId to remove from being selected
-         * @return <code>true</code> if the selection state changed.
-         *         <code>false</code> if the itemId already was selected
-         * @throws IllegalStateException
-         *             if the deselection was illegal. One such reason might be
-         *             that the implementation enforces that an item is always
-         *             selected
-         * @see #select(Object)
-         */
-        boolean deselect(Object itemId) throws IllegalStateException;
-
         /**
          * Gets the item id of the currently selected item.
          * 

server/src/com/vaadin/ui/components/grid/selection/SingleSelectionModel.java

@@ -28,6 +28,10 @@ public class SingleSelectionModel extends AbstractSelectionModel implements
         SelectionModel.Single {
     @Override
     public boolean select(final Object itemId) {
+        if (itemId == null) {
+            return deselect(getSelectedRow());
+        }
+
         checkItemIdExists(itemId);
 
         final Object selectedRow = getSelectedRow();
@@ -47,8 +51,7 @@ public class SingleSelectionModel extends AbstractSelectionModel implements
         return modified;
     }
 
-    @Override
-    public boolean deselect(final Object itemId) {
+    private boolean deselect(final Object itemId) {
         return deselectInternal(itemId, true);
     }
 

server/tests/src/com/vaadin/tests/server/component/grid/GridSelection.java

@@ -31,11 +31,6 @@ import com.vaadin.ui.components.grid.selection.SelectionChangeEvent;
 import com.vaadin.ui.components.grid.selection.SelectionChangeListener;
 import com.vaadin.ui.components.grid.selection.SelectionModel;
 
-/**
- * 
- * @since
- * @author Vaadin Ltd
- */
 public class GridSelection {
 
     private static class MockSelectionChangeListener implements

server/tests/src/com/vaadin/tests/server/component/grid/SingleSelectionModelTest.java

@@ -0,0 +1,154 @@
+/*
+ * 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.server.component.grid;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.vaadin.data.Container;
+import com.vaadin.data.util.IndexedContainer;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.Grid.SelectionMode;
+import com.vaadin.ui.components.grid.selection.SelectionChangeEvent;
+import com.vaadin.ui.components.grid.selection.SelectionChangeListener;
+import com.vaadin.ui.components.grid.selection.SingleSelectionModel;
+
+public class SingleSelectionModelTest {
+
+    private Object itemId1Present = "itemId1Present";
+    private Object itemId2Present = "itemId2Present";
+
+    private Object itemIdNotPresent = "itemIdNotPresent";
+    private Container.Indexed dataSource;
+    private SingleSelectionModel model;
+    private Grid grid;
+
+    private boolean expectingEvent = false;
+
+    @Before
+    public void setUp() {
+        dataSource = createDataSource();
+        grid = new Grid(dataSource);
+        grid.setSelectionMode(SelectionMode.SINGLE);
+        model = (SingleSelectionModel) grid.getSelectionModel();
+    }
+
+    @After
+    public void tearDown() {
+        Assert.assertFalse("Some expected event did not happen.",
+                expectingEvent);
+    }
+
+    private IndexedContainer createDataSource() {
+        final IndexedContainer container = new IndexedContainer();
+        container.addItem(itemId1Present);
+        container.addItem(itemId2Present);
+        for (int i = 2; i < 10; i++) {
+            container.addItem(new Object());
+        }
+
+        return container;
+    }
+
+    @Test
+    public void testSelectAndDeselctRow() throws Throwable {
+        try {
+            expectEvent(itemId1Present, null);
+            model.select(itemId1Present);
+            expectEvent(null, itemId1Present);
+            model.select(null);
+        } catch (Exception e) {
+            throw e.getCause();
+        }
+    }
+
+    @Test
+    public void testSelectAndChangeSelectedRow() throws Throwable {
+        try {
+            expectEvent(itemId1Present, null);
+            model.select(itemId1Present);
+            expectEvent(itemId2Present, itemId1Present);
+            model.select(itemId2Present);
+        } catch (Exception e) {
+            throw e.getCause();
+        }
+    }
+
+    @Test
+    public void testRemovingSelectedRowAndThenDeselecting() throws Throwable {
+        try {
+            expectEvent(itemId2Present, null);
+            model.select(itemId2Present);
+            dataSource.removeItem(itemId2Present);
+            expectEvent(null, itemId2Present);
+            model.select(null);
+        } catch (Exception e) {
+            throw e.getCause();
+        }
+    }
+
+    @Test
+    public void testSelectAndReSelectRow() throws Throwable {
+        try {
+            expectEvent(itemId1Present, null);
+            model.select(itemId1Present);
+            expectEvent(null, null);
+            // This is no-op. Nothing should happen.
+            model.select(itemId1Present);
+        } catch (Exception e) {
+            throw e.getCause();
+        }
+        Assert.assertTrue("Should still wait for event", expectingEvent);
+        expectingEvent = false;
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testSelectNonExistentRow() {
+        model.select(itemIdNotPresent);
+    }
+
+    private void expectEvent(final Object selected, final Object deselected) {
+        expectingEvent = true;
+        grid.addSelectionChangeListener(new SelectionChangeListener() {
+
+            @Override
+            public void selectionChange(SelectionChangeEvent event) {
+                if (selected != null) {
+                    Assert.assertTrue(
+                            "Selection did not contain expected item", event
+                                    .getAdded().contains(selected));
+                } else {
+                    Assert.assertTrue("Unexpected selection", event.getAdded()
+                            .isEmpty());
+                }
+
+                if (deselected != null) {
+                    Assert.assertTrue(
+                            "DeSelection did not contain expected item", event
+                                    .getRemoved().contains(deselected));
+                } else {
+                    Assert.assertTrue("Unexpected selection", event
+                            .getRemoved().isEmpty());
+                }
+
+                grid.removeSelectionChangeListener(this);
+                expectingEvent = false;
+            }
+        });
+    }
+}