Refactor GridConnector to remove RowKeyHelper (#13334)

Change-Id: I4c2fec5b46b15eea456d4b0347b2a686943ad113

3 files changed, 81 insertions, 108 deletions

diff --git a/client/src/com/vaadin/client/data/RpcDataSourceConnector.java b/client/src/com/vaadin/client/data/RpcDataSourceConnector.java
index 0bee9dc34d..55c37185e0 100644
--- a/client/src/com/vaadin/client/data/RpcDataSourceConnector.java
+++ b/client/src/com/vaadin/client/data/RpcDataSourceConnector.java
@@ -66,7 +66,9 @@ public class RpcDataSourceConnector extends AbstractExtensionConnector {
         }
 
         public RowHandle<JSONObject> getHandleByKey(Object key) {
-            return new RowHandleImpl(null, key);
+            JSONObject row = new JSONObject();
+            row.put(GridState.JSONKEY_ROWKEY, new JSONString((String) key));
+            return new RowHandleImpl(row, key);
         }
     }
 
diff --git a/client/src/com/vaadin/client/ui/grid/GridConnector.java b/client/src/com/vaadin/client/ui/grid/GridConnector.java
index 3629cdd7e8..7582bee0bf 100644
--- a/client/src/com/vaadin/client/ui/grid/GridConnector.java
+++ b/client/src/com/vaadin/client/ui/grid/GridConnector.java
@@ -18,7 +18,6 @@ package com.vaadin.client.ui.grid;
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -74,88 +73,6 @@ import com.vaadin.shared.ui.grid.SortDirection;
 @Connect(com.vaadin.ui.components.grid.Grid.class)
 public class GridConnector extends AbstractComponentConnector {
 
-    /*
-     * TODO: henrik paul (4.7.2014)
-     * 
-     * This class should optimally not be needed. We should be able to use the
-     * keys in the state as the primary source of selection, and "simply" diff
-     * things once the state changes (we can't rebuild the selection pins from
-     * scratch, since we might lose some data that's currently out of view).
-     * 
-     * I was unable to remove this class with little effort, so it may remain as
-     * a todo for now.
-     */
-    private class RowKeyHelper {
-        private LinkedHashSet<String> selectedKeys = new LinkedHashSet<String>();
-
-        public LinkedHashSet<String> getSelectedKeys() {
-            return selectedKeys;
-        }
-
-        public void add(Collection<JSONObject> rows) {
-            for (JSONObject row : rows) {
-                add(row);
-            }
-        }
-
-        private void add(JSONObject row) {
-            selectedKeys.add((String) dataSource.getRowKey(row));
-        }
-
-        public void remove(Collection<JSONObject> rows) {
-            for (JSONObject row : rows) {
-                remove(row);
-            }
-        }
-
-        private void remove(JSONObject row) {
-            selectedKeys.remove(dataSource.getRowKey(row));
-        }
-
-        public void updateFromState() {
-            boolean changed = false;
-
-            List<String> stateKeys = getState().selectedKeys;
-
-            // find new selections
-            for (String key : stateKeys) {
-                if (!selectedKeys.contains(key)) {
-                    changed = true;
-                    selectByHandle(dataSource.getHandleByKey(key));
-                }
-            }
-
-            // find new deselections
-            for (String key : selectedKeys) {
-                if (!stateKeys.contains(key)) {
-                    changed = true;
-                    deselectByHandle(dataSource.getHandleByKey(key));
-                }
-            }
-
-            /*
-             * A defensive copy in case the collection in the state is mutated
-             * instead of re-assigned.
-             */
-            selectedKeys = new LinkedHashSet<String>(stateKeys);
-
-            /*
-             * We need to fire this event so that Grid is able to re-render the
-             * selection changes (if applicable).
-             * 
-             * add/remove methods will be called from the
-             * internalSelectionChangeHandler, so they shouldn't be called here.
-             */
-            if (changed) {
-                // At least for now there's no way to send the selected and/or
-                // deselected row data. Some data is only stored as keys
-                getWidget().fireEvent(
-                        new SelectionChangeEvent<JSONObject>(getWidget(),
-                                (List<JSONObject>) null, null));
-            }
-        }
-    }
-
     /**
      * Custom implementation of the custom grid column using a JSONObject to
      * represent the cell value and String as a column type.
@@ -211,21 +128,38 @@ public class GridConnector extends AbstractComponentConnector {
      * Maps a generated column id to a grid column instance
      */
     private Map<String, CustomGridColumn> columnIdToColumn = new HashMap<String, CustomGridColumn>();
+
     private AbstractRowHandleSelectionModel<JSONObject> selectionModel = createSelectionModel(SharedSelectionMode.NONE);
-    private RpcDataSource dataSource;
+    private Set<String> selectedKeys = new LinkedHashSet<String>();
 
-    private final RowKeyHelper rowKeyHelper = new RowKeyHelper();
+    /**
+     * updateFromState is set to true when {@link #updateSelectionFromState()}
+     * makes changes to selection. This flag tells the
+     * {@code internalSelectionChangeHandler} to not send same data straight
+     * back to server. Said listener sets it back to false when handling that
+     * event.
+     */
+    private boolean updatedFromState = false;
+
+    private RpcDataSource dataSource;
 
     private SelectionChangeHandler<JSONObject> internalSelectionChangeHandler = new SelectionChangeHandler<JSONObject>() {
         @Override
         public void onSelectionChange(SelectionChangeEvent<JSONObject> event) {
-            rowKeyHelper.remove(event.getRemoved());
-            rowKeyHelper.add(event.getAdded());
+            if (!updatedFromState) {
+                for (JSONObject row : event.getRemoved()) {
+                    selectedKeys.remove(dataSource.getRowKey(row));
+                }
 
-            // TODO change this to diff based. (henrik paul 24.6.2014)
-            List<String> selectedKeys = new ArrayList<String>(
-                    rowKeyHelper.getSelectedKeys());
-            getRpcProxy(GridServerRpc.class).selectionChange(selectedKeys);
+                for (JSONObject row : event.getAdded()) {
+                    selectedKeys.add((String) dataSource.getRowKey(row));
+                }
+
+                getRpcProxy(GridServerRpc.class).selectionChange(
+                        new ArrayList<String>(selectedKeys));
+            } else {
+                updatedFromState = false;
+            }
         }
     };
 
@@ -340,10 +274,6 @@ public class GridConnector extends AbstractComponentConnector {
                 getWidget().setLastFrozenColumn(null);
             }
         }
-
-        if (stateChangeEvent.hasPropertyChanged("selectedKeys")) {
-            rowKeyHelper.updateFromState();
-        }
     }
 
     private void updateSectionFromState(GridStaticSection<?> section,
@@ -511,13 +441,54 @@ public class GridConnector extends AbstractComponentConnector {
         if (!model.getClass().equals(selectionModel.getClass())) {
             selectionModel = model;
             getWidget().setSelectionModel(model);
-            rowKeyHelper.selectedKeys.clear();
+            selectedKeys.clear();
+        }
+    }
+
+    @OnStateChange("selectedKeys")
+    private void updateSelectionFromState() {
+        boolean changed = false;
+
+        List<String> stateKeys = getState().selectedKeys;
+
+        // find new deselections
+        for (String key : selectedKeys) {
+            if (!stateKeys.contains(key)) {
+                changed = true;
+                deselectByHandle(dataSource.getHandleByKey(key));
+            }
+        }
+
+        // find new selections
+        for (String key : stateKeys) {
+            if (!selectedKeys.contains(key)) {
+                changed = true;
+                selectByHandle(dataSource.getHandleByKey(key));
+            }
         }
 
+        /*
+         * A defensive copy in case the collection in the state is mutated
+         * instead of re-assigned.
+         */
+        selectedKeys = new LinkedHashSet<String>(stateKeys);
+
+        /*
+         * We need to fire this event so that Grid is able to re-render the
+         * selection changes (if applicable).
+         */
+        if (changed) {
+            // At least for now there's no way to send the selected and/or
+            // deselected row data. Some data is only stored as keys
+            updatedFromState = true;
+            getWidget().fireEvent(
+                    new SelectionChangeEvent<JSONObject>(getWidget(),
+                            (List<JSONObject>) null, null));
+        }
     }
 
     @OnStateChange({ "sortColumns", "sortDirs" })
-    void onSortStateChange() {
+    private void onSortStateChange() {
         List<SortOrder> sortOrder = new ArrayList<SortOrder>();
 
         String[] sortColumns = getState().sortColumns;
diff --git a/server/src/com/vaadin/ui/components/grid/Grid.java b/server/src/com/vaadin/ui/components/grid/Grid.java
index f5846c0148..514a0496e2 100644
--- a/server/src/com/vaadin/ui/components/grid/Grid.java
+++ b/server/src/com/vaadin/ui/components/grid/Grid.java
@@ -286,7 +286,7 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier {
                 SetView<Object> removedItemIds = Sets.difference(oldSelection,
                         newSelection);
 
-                if (!addedItemIds.isEmpty()) {
+                if (!removedItemIds.isEmpty()) {
                     /*
                      * Since these changes come from the client, we want to
                      * modify the selection model and get that event fired to
@@ -296,16 +296,16 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier {
                      */
                     ignoreSelectionClientSync++;
 
-                    if (addedItemIds.size() == 1) {
-                        select(addedItemIds.iterator().next());
+                    if (removedItemIds.size() == 1) {
+                        deselect(removedItemIds.iterator().next());
                     } else {
-                        assert getSelectionModel() instanceof SelectionModel.Multi : "Got multiple selections, but the selection model is not a SelectionModel.Multi";
+                        assert getSelectionModel() instanceof SelectionModel.Multi : "Got multiple deselections, but the selection model is not a SelectionModel.Multi";
                         ((SelectionModel.Multi) getSelectionModel())
-                                .select(addedItemIds);
+                                .deselect(removedItemIds);
                     }
                 }
 
-                if (!removedItemIds.isEmpty()) {
+                if (!addedItemIds.isEmpty()) {
                     /*
                      * Since these changes come from the client, we want to
                      * modify the selection model and get that event fired to
@@ -315,12 +315,12 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier {
                      */
                     ignoreSelectionClientSync++;
 
-                    if (removedItemIds.size() == 1) {
-                        deselect(removedItemIds.iterator().next());
+                    if (addedItemIds.size() == 1) {
+                        select(addedItemIds.iterator().next());
                     } else {
-                        assert getSelectionModel() instanceof SelectionModel.Multi : "Got multiple deselections, but the selection model is not a SelectionModel.Multi";
+                        assert getSelectionModel() instanceof SelectionModel.Multi : "Got multiple selections, but the selection model is not a SelectionModel.Multi";
                         ((SelectionModel.Multi) getSelectionModel())
-                                .deselect(removedItemIds);
+                                .select(addedItemIds);
                     }
                 }
             }