From: Teemu Suo-Anttila Date: Wed, 8 Oct 2014 14:30:13 +0000 (+0300) Subject: Fix RpcDataSource to use RPC for row pins/unpins (#13334) X-Git-Tag: 7.4.0.beta1~9^2~132 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=7622128012cd60bb82612a18b7c6582ac0e842ae;p=vaadin-framework.git 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 --- diff --git a/client/src/com/vaadin/client/data/AbstractRemoteDataSource.java b/client/src/com/vaadin/client/data/AbstractRemoteDataSource.java index 45e1533662..5052b9019b 100644 --- a/client/src/com/vaadin/client/data/AbstractRemoteDataSource.java +++ b/client/src/com/vaadin/client/data/AbstractRemoteDataSource.java @@ -76,32 +76,18 @@ public abstract class AbstractRemoteDataSource implements DataSource { } } - 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 implements DataSource { } } + /** + * 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 implements DataSource { */ abstract public Object getRowKey(T row); - /** - * Marks rows as pinned when fetching new rows. - *

- * 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. - *

- * 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 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); diff --git a/client/src/com/vaadin/client/data/RpcDataSourceConnector.java b/client/src/com/vaadin/client/data/RpcDataSourceConnector.java index deb3b5ed7c..cc1e294fb8 100644 --- a/client/src/com/vaadin/client/data/RpcDataSourceConnector.java +++ b/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 { - private Collection prevRows = Collections.emptySet(); + private DataRequestRpc rpcProxy = getRpcProxy(DataRequestRpc.class); @Override protected void requestRows(int firstRowIndex, int numberOfRows) { Range cached = getCachedRange(); - Collection newRows = new ArrayList( - temporarilyPinnedRows); - newRows.removeAll(prevRows); - - List temporarilyPinnedKeys = new ArrayList( - 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 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); + } + } } 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 5228a466a2..b059beca54 100644 --- a/client/src/com/vaadin/client/ui/grid/selection/MultiSelectionRenderer.java +++ b/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 extends ComplexRenderer { private boolean scrollAreaShouldRebound = false; - private AbstractRemoteDataSource remoteDataSource = null; - private Batched batchedSelectionModel = null; - public AutoScrollerAndSelector(final int topBound, final int bottomBound, final int gradientArea, final boolean selectionPaint) { @@ -258,20 +252,11 @@ public class MultiSelectionRenderer extends ComplexRenderer { 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 pinneds = batchedSelectionModel - .getSelectedRowsBatch(); - pinneds.addAll(batchedSelectionModel - .getDeselectedRowsBatch()); - remoteDataSource.transactionPin(pinneds); - } } reschedule(); @@ -319,41 +304,17 @@ public class MultiSelectionRenderer extends ComplexRenderer { scrollSpeed = ratio * SCROLL_TOP_SPEED_PX_SEC; } - @SuppressWarnings("deprecation") public void start(int logicalRowIndex) { running = true; setSelected(logicalRowIndex, selectionPaint); logicalRow = logicalRowIndex; reschedule(); - - DataSource dataSource = grid.getDataSource(); - SelectionModel selectionModel = grid.getSelectionModel(); - if (dataSource instanceof AbstractRemoteDataSource - && selectionModel instanceof Batched) { - this.remoteDataSource = (AbstractRemoteDataSource) dataSource; - this.batchedSelectionModel = (Batched) selectionModel; - - Collection 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 emptySet = (Collection) Collections.emptySet(); - remoteDataSource.transactionPin(emptySet); - remoteDataSource = null; - batchedSelectionModel = null; - } - if (handle != null) { handle.cancel(); handle = null; 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 b6ecc945e2..33b5354570 100644 --- a/client/src/com/vaadin/client/ui/grid/selection/SelectionModelMulti.java +++ b/client/src/com/vaadin/client/ui/grid/selection/SelectionModelMulti.java @@ -183,6 +183,7 @@ public class SelectionModelMulti extends AbstractRowHandleSelectionModel @Override protected boolean deselectByHandle(RowHandle handle) { if (selectedRows.remove(handle)) { + if (!isBeingBatchSelected()) { handle.unpin(); } else { @@ -227,6 +228,11 @@ public class SelectionModelMulti extends AbstractRowHandleSelectionModel selectionBatch.clear(); final Collection removed = getDeselectedRowsBatch(); + + // unpin deselected rows + for (RowHandle handle : deselectionBatch) { + handle.unpin(); + } deselectionBatch.clear(); grid.fireEvent(new SelectionChangeEvent(grid, added, removed, diff --git a/server/src/com/vaadin/data/RpcDataProviderExtension.java b/server/src/com/vaadin/data/RpcDataProviderExtension.java index 99ff57089c..68f40f6cf3 100644 --- a/server/src/com/vaadin/data/RpcDataProviderExtension.java +++ b/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 allTemporarilyPinnedKeys = new ArrayList(); - @Override public void requestRows(int firstRow, int numberOfRows, - int firstCachedRowIndex, int cacheSize, - List 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(); - 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)); + } } }); diff --git a/server/src/com/vaadin/ui/components/grid/Grid.java b/server/src/com/vaadin/ui/components/grid/Grid.java index fe03d589c0..8203f9c344 100644 --- a/server/src/com/vaadin/ui/components/grid/Grid.java +++ b/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)) { diff --git a/shared/src/com/vaadin/shared/data/DataRequestRpc.java b/shared/src/com/vaadin/shared/data/DataRequestRpc.java index 000d196be9..637a353447 100644 --- a/shared/src/com/vaadin/shared/data/DataRequestRpc.java +++ b/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 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); }