Fix RpcDataSource to use RPC for row pins/unpins (#13334)

This patch removes the temprarilyPinnedRows workaround from
AbstractRemoteDataSource and refactors the whole feature to be part of
RpcDataSource where it should be.

Change-Id: Id55020dd11dda3dcf54dfe3c1b41af8e495c1c0c

client/src/com/vaadin/client/data/AbstractRemoteDataSource.java

@@ -76,32 +76,18 @@ public abstract class AbstractRemoteDataSource<T> implements DataSource<T> {
             }
         }
 
-        private boolean isPinned() {
+        protected boolean isPinned() {
             return pinnedRows.containsKey(key);
         }
 
         @Override
         public void pin() {
-            Integer count = pinnedCounts.get(key);
-            if (count == null) {
-                count = Integer.valueOf(0);
-                pinnedRows.put(key, this);
-            }
-            pinnedCounts.put(key, Integer.valueOf(count.intValue() + 1));
+            pinHandle(this);
         }
 
         @Override
         public void unpin() throws IllegalStateException {
-            final Integer count = pinnedCounts.get(key);
-            if (count == null) {
-                throw new IllegalStateException("Row " + row + " with key "
-                        + key + " was not pinned to begin with");
-            } else if (count.equals(Integer.valueOf(1))) {
-                pinnedRows.remove(key);
-                pinnedCounts.remove(key);
-            } else {
-                pinnedCounts.put(key, Integer.valueOf(count.intValue() - 1));
-            }
+            unpinHandle(this);
         }
 
         @Override
@@ -174,6 +160,48 @@ public abstract class AbstractRemoteDataSource<T> implements DataSource<T> {
         }
     }
 
+    /**
+     * Pins a row with given handle. This function can be overridden to do
+     * specific logic related to pinning rows.
+     * 
+     * @param handle
+     *            row handle to pin
+     */
+    protected void pinHandle(RowHandleImpl handle) {
+        Object key = handle.key;
+        Integer count = pinnedCounts.get(key);
+        if (count == null) {
+            count = Integer.valueOf(0);
+            pinnedRows.put(key, handle);
+        }
+        pinnedCounts.put(key, Integer.valueOf(count.intValue() + 1));
+    }
+
+    /**
+     * Unpins a previously pinned row with given handle. This function can be
+     * overridden to do specific logic related to unpinning rows.
+     * 
+     * @param handle
+     *            row handle to unpin
+     * 
+     * @throws IllegalStateException
+     *             if given row handle has not been pinned before
+     */
+    protected void unpinHandle(RowHandleImpl handle)
+            throws IllegalStateException {
+        Object key = handle.key;
+        final Integer count = pinnedCounts.get(key);
+        if (count == null) {
+            throw new IllegalStateException("Row " + handle.getRow()
+                    + " with key " + key + " was not pinned to begin with");
+        } else if (count.equals(Integer.valueOf(1))) {
+            pinnedRows.remove(key);
+            pinnedCounts.remove(key);
+        } else {
+            pinnedCounts.put(key, Integer.valueOf(count.intValue() - 1));
+        }
+    }
+
     @Override
     public void ensureAvailability(int firstRowIndex, int numberOfRows) {
         requestedAvailability = Range.withLength(firstRowIndex, numberOfRows);
@@ -556,35 +584,6 @@ public abstract class AbstractRemoteDataSource<T> implements DataSource<T> {
      */
     abstract public Object getRowKey(T row);
 
-    /**
-     * Marks rows as pinned when fetching new rows.
-     * <p>
-     * This collection of rows are intended to remain pinned if new rows are
-     * fetched from the data source, even if some of the pinned rows would fall
-     * off the cache and become inactive.
-     * <p>
-     * This method does nothing by itself, other than it stores the rows into a
-     * field. The implementation needs to make all the adjustments for itself.
-     * Check {@link RpcDataSourceConnector.RpcDataSource#requestRows(int, int)}
-     * for an implementation example.
-     * 
-     * @param keys
-     *            a collection of rows to keep pinned
-     * 
-     * @see #temporarilyPinnedRows
-     * @see RpcDataSourceConnector.RpcDataSource#requestRows(int, int)
-     * @deprecated You probably don't want to call this method unless you're
-     *             writing a Renderer for a selection model. Even if you are, be
-     *             very aware what this method does and how it behaves.
-     */
-    @Deprecated
-    public void transactionPin(Collection<T> rows) {
-        if (rows == null) {
-            throw new IllegalArgumentException("argument may not be null");
-        }
-        temporarilyPinnedRows = rows;
-    }
-
     protected void resetDataAndSize(int newSize) {
         dropFromCache(getCachedRange());
         cached = Range.withLength(0, 0);

client/src/com/vaadin/client/data/RpcDataSourceConnector.java

@@ -17,9 +17,6 @@
 package com.vaadin.client.data;
 
 import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.List;
 
 import com.google.gwt.json.client.JSONArray;
 import com.google.gwt.json.client.JSONObject;
@@ -50,31 +47,18 @@ public class RpcDataSourceConnector extends AbstractExtensionConnector {
 
     public class RpcDataSource extends AbstractRemoteDataSource<JSONObject> {
 
-        private Collection<JSONObject> prevRows = Collections.emptySet();
+        private DataRequestRpc rpcProxy = getRpcProxy(DataRequestRpc.class);
 
         @Override
         protected void requestRows(int firstRowIndex, int numberOfRows) {
             Range cached = getCachedRange();
 
-            Collection<JSONObject> newRows = new ArrayList<JSONObject>(
-                    temporarilyPinnedRows);
-            newRows.removeAll(prevRows);
-
-            List<String> temporarilyPinnedKeys = new ArrayList<String>(
-                    newRows.size());
-            for (JSONObject row : newRows) {
-                temporarilyPinnedKeys.add((String) getRowKey(row));
-            }
-
-            getRpcProxy(DataRequestRpc.class).requestRows(firstRowIndex,
-                    numberOfRows, cached.getStart(), cached.length(),
-                    temporarilyPinnedKeys);
-
-            prevRows = temporarilyPinnedRows;
+            rpcProxy.requestRows(firstRowIndex, numberOfRows,
+                    cached.getStart(), cached.length());
         }
 
         @Override
-        public Object getRowKey(JSONObject row) {
+        public String getRowKey(JSONObject row) {
             JSONString string = row.get(GridState.JSONKEY_ROWKEY).isString();
             if (string != null) {
                 return string.stringValue();
@@ -90,19 +74,30 @@ public class RpcDataSourceConnector extends AbstractExtensionConnector {
         }
 
         @Override
-        @SuppressWarnings("deprecation")
-        public void transactionPin(Collection<JSONObject> keys) {
-            super.transactionPin(keys);
-            if (keys.isEmpty() && !prevRows.isEmpty()) {
-                prevRows = Collections.emptySet();
-                getRpcProxy(DataRequestRpc.class)
-                        .releaseTemporarilyPinnedKeys();
+        public int size() {
+            return getState().containerSize;
+        }
+
+        @Override
+        protected void pinHandle(RowHandleImpl handle) {
+            // Server only knows if something is pinned or not. No need to pin
+            // multiple times.
+            boolean pinnedBefore = handle.isPinned();
+            super.pinHandle(handle);
+            if (!pinnedBefore) {
+                rpcProxy.setPinned(getRowKey(handle.getRow()), true);
             }
         }
 
         @Override
-        public int size() {
-            return getState().containerSize;
+        protected void unpinHandle(RowHandleImpl handle) {
+            // Row data is no longer available after it has been unpinned.
+            String key = getRowKey(handle.getRow());
+            super.unpinHandle(handle);
+            if (!handle.isPinned()) {
+                rpcProxy.setPinned(key, false);
+            }
+
         }
     }
 

client/src/com/vaadin/client/ui/grid/selection/MultiSelectionRenderer.java

@@ -16,7 +16,6 @@
 package com.vaadin.client.ui.grid.selection;
 
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashSet;
 
 import com.google.gwt.animation.client.AnimationScheduler;
@@ -34,8 +33,6 @@ import com.google.gwt.user.client.Event;
 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.data.AbstractRemoteDataSource;
-import com.vaadin.client.data.DataSource;
 import com.vaadin.client.ui.grid.Cell;
 import com.vaadin.client.ui.grid.DataAvailableEvent;
 import com.vaadin.client.ui.grid.DataAvailableHandler;
@@ -231,9 +228,6 @@ public class MultiSelectionRenderer<T> extends ComplexRenderer<Boolean> {
 
         private boolean scrollAreaShouldRebound = false;
 
-        private AbstractRemoteDataSource<T> remoteDataSource = null;
-        private Batched<T> batchedSelectionModel = null;
-
         public AutoScrollerAndSelector(final int topBound,
                 final int bottomBound, final int gradientArea,
                 final boolean selectionPaint) {
@@ -258,20 +252,11 @@ public class MultiSelectionRenderer<T> extends ComplexRenderer<Boolean> {
                 grid.setScrollTop(grid.getScrollTop() + intPixelsToScroll);
             }
 
-            @SuppressWarnings("hiding")
             int logicalRow = getLogicalRowIndex(Util.getElementFromPoint(pageX,
                     pageY));
             if (logicalRow != -1 && logicalRow != this.logicalRow) {
                 this.logicalRow = logicalRow;
                 setSelected(logicalRow, selectionPaint);
-
-                if (remoteDataSource != null && batchedSelectionModel != null) {
-                    Collection<T> pinneds = batchedSelectionModel
-                            .getSelectedRowsBatch();
-                    pinneds.addAll(batchedSelectionModel
-                            .getDeselectedRowsBatch());
-                    remoteDataSource.transactionPin(pinneds);
-                }
             }
 
             reschedule();
@@ -319,41 +304,17 @@ public class MultiSelectionRenderer<T> extends ComplexRenderer<Boolean> {
             scrollSpeed = ratio * SCROLL_TOP_SPEED_PX_SEC;
         }
 
-        @SuppressWarnings("deprecation")
         public void start(int logicalRowIndex) {
             running = true;
             setSelected(logicalRowIndex, selectionPaint);
             logicalRow = logicalRowIndex;
             reschedule();
-
-            DataSource<T> dataSource = grid.getDataSource();
-            SelectionModel<T> selectionModel = grid.getSelectionModel();
-            if (dataSource instanceof AbstractRemoteDataSource
-                    && selectionModel instanceof Batched) {
-                this.remoteDataSource = (AbstractRemoteDataSource<T>) dataSource;
-                this.batchedSelectionModel = (Batched<T>) selectionModel;
-
-                Collection<T> pinneds = batchedSelectionModel
-                        .getSelectedRowsBatch();
-                pinneds.addAll(batchedSelectionModel.getDeselectedRowsBatch());
-                remoteDataSource.transactionPin(pinneds);
-            }
         }
 
         @SuppressWarnings("deprecation")
         public void stop() {
             running = false;
 
-            if (remoteDataSource != null) {
-                // split into two lines because of Java generics not playing
-                // nice.
-                @SuppressWarnings("unchecked")
-                Collection<T> emptySet = (Collection<T>) Collections.emptySet();
-                remoteDataSource.transactionPin(emptySet);
-                remoteDataSource = null;
-                batchedSelectionModel = null;
-            }
-
             if (handle != null) {
                 handle.cancel();
                 handle = null;

client/src/com/vaadin/client/ui/grid/selection/SelectionModelMulti.java

@@ -183,6 +183,7 @@ public class SelectionModelMulti<T> extends AbstractRowHandleSelectionModel<T>
     @Override
     protected boolean deselectByHandle(RowHandle<T> handle) {
         if (selectedRows.remove(handle)) {
+
             if (!isBeingBatchSelected()) {
                 handle.unpin();
             } else {
@@ -227,6 +228,11 @@ public class SelectionModelMulti<T> extends AbstractRowHandleSelectionModel<T>
         selectionBatch.clear();
 
         final Collection<T> removed = getDeselectedRowsBatch();
+
+        // unpin deselected rows
+        for (RowHandle<T> handle : deselectionBatch) {
+            handle.unpin();
+        }
         deselectionBatch.clear();
 
         grid.fireEvent(new SelectionChangeEvent<T>(grid, added, removed,

server/src/com/vaadin/data/RpcDataProviderExtension.java

@@ -49,8 +49,6 @@ import com.vaadin.shared.ui.grid.Range;
 import com.vaadin.ui.components.grid.Grid;
 import com.vaadin.ui.components.grid.GridColumn;
 import com.vaadin.ui.components.grid.Renderer;
-import com.vaadin.ui.components.grid.selection.SelectionChangeEvent;
-import com.vaadin.ui.components.grid.selection.SelectionChangeListener;
 
 import elemental.json.Json;
 import elemental.json.JsonArray;
@@ -650,20 +648,9 @@ public class RpcDataProviderExtension extends AbstractExtension {
         rpc = getRpcProxy(DataProviderRpc.class);
 
         registerRpc(new DataRequestRpc() {
-            private Collection<String> allTemporarilyPinnedKeys = new ArrayList<String>();
-
             @Override
             public void requestRows(int firstRow, int numberOfRows,
-                    int firstCachedRowIndex, int cacheSize,
-                    List<String> temporarilyPinnedKeys) {
-
-                for (String key : temporarilyPinnedKeys) {
-                    Object itemId = keyMapper.getItemId(key);
-                    if (!keyMapper.isPinned(itemId)) {
-                        keyMapper.pin(itemId);
-                    }
-                }
-                allTemporarilyPinnedKeys.addAll(temporarilyPinnedKeys);
+                    int firstCachedRowIndex, int cacheSize) {
 
                 Range active = Range.withLength(firstRow, numberOfRows);
                 if (cacheSize != 0) {
@@ -682,52 +669,12 @@ public class RpcDataProviderExtension extends AbstractExtension {
             }
 
             @Override
-            public void releaseTemporarilyPinnedKeys() {
-                /*
-                 * This needs to be done deferredly since the selection event
-                 * comes after this RPC call.
-                 */
-
-                final SelectionChangeListener listener = new SelectionChangeListener() {
-                    @Override
-                    public void selectionChange(SelectionChangeEvent event) {
-                        for (String tempPinnedKey : allTemporarilyPinnedKeys) {
-                            /*
-                             * TODO: this could be moved into a field instead of
-                             * inline to reduce indentations.
-                             */
-
-                            /*
-                             * This works around the fact that when deselecting
-                             * and leaping through the cache, the client tries
-                             * to send a deselect event even though a row never
-                             * was selected. So, it tries to unpin something
-                             * that never was temporarily pinned.
-                             * 
-                             * If the same thing would happen while selecting
-                             * (instead of deselecting), the row would be
-                             * pinned, not because of the temporary pinning, but
-                             * because it's selected.
-                             */
-                            if (!keyMapper.isPinned(tempPinnedKey)) {
-                                continue;
-                            }
-
-                            Object itemId = keyMapper.getItemId(tempPinnedKey);
-                            Integer index = keyMapper.indexToItemId.inverse()
-                                    .get(itemId);
-                            if (!getGrid().isSelected(itemId)
-                                    && !activeRowHandler.activeRange
-                                            .contains(index.intValue())) {
-                                keyMapper.unpin(itemId);
-                            }
-                        }
-                        allTemporarilyPinnedKeys = new ArrayList<String>();
-                        getGrid().removeSelectionChangeListener(this);
-                    }
-                };
-
-                getGrid().addSelectionChangeListener(listener);
+            public void setPinned(String key, boolean isPinned) {
+                if (isPinned) {
+                    keyMapper.pin(keyMapper.getItemId(key));
+                } else {
+                    keyMapper.unpin(keyMapper.getItemId(key));
+                }
             }
         });
 

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

@@ -305,9 +305,18 @@ public class Grid extends AbstractComponent implements SelectionChangeNotifier,
         addSelectionChangeListener(new SelectionChangeListener() {
             @Override
             public void selectionChange(SelectionChangeEvent event) {
-                for (Object removedItemId : event.getRemoved()) {
-                    getKeyMapper().unpin(removedItemId);
-                }
+                /*
+                 * This listener nor anything else in the server side should
+                 * never unpin anything from KeyMapper. Pinning is mostly a
+                 * client feature and is only used when selecting something from
+                 * the server side. This is to ensure that client has the
+                 * correct key from server when the selected row is first
+                 * loaded.
+                 * 
+                 * Once client has gotten info that it is supposed to select a
+                 * row, it will pin the data from the client side as well and it
+                 * will be unpinned once it gets deselected.
+                 */
 
                 for (Object addedItemId : event.getAdded()) {
                     if (!getKeyMapper().isPinned(addedItemId)) {

shared/src/com/vaadin/shared/data/DataRequestRpc.java

@@ -16,8 +16,7 @@
 
 package com.vaadin.shared.data;
 
-import java.util.List;
-
+import com.vaadin.shared.annotations.Delayed;
 import com.vaadin.shared.communication.ServerRpc;
 
 /**
@@ -39,17 +38,20 @@ public interface DataRequestRpc extends ServerRpc {
      *            the index of the first cached row
      * @param cacheSize
      *            the number of cached rows
-     * @param temporarilyPinnedKeys
-     *            the keys that should remain pinned, even if some of these
-     *            would fall out of the cache range
      */
     public void requestRows(int firstRowIndex, int numberOfRows,
-            int firstCachedRowIndex, int cacheSize,
-            List<String> temporarilyPinnedKeys);
+            int firstCachedRowIndex, int cacheSize);
 
     /**
-     * Informs the back-end that the temporarily pinned keys in
-     * {@link #requestRows(int, int, int, int, List)} may be released.
+     * Informs the server that an item referenced with a key pinned status has
+     * changed. This is a delayed call that happens along with next rpc call to
+     * server.
+     * 
+     * @param key
+     *            key mapping to item
+     * @param isPinned
+     *            pinned status of referenced item
      */
-    public void releaseTemporarilyPinnedKeys();
+    @Delayed
+    public void setPinned(String key, boolean isPinned);
 }