diff options
8 files changed, 167 insertions, 563 deletions
diff --git a/client/src/com/vaadin/client/connectors/GridConnector.java b/client/src/com/vaadin/client/connectors/GridConnector.java index ef52a429e7..4c2e8ab4e1 100644 --- a/client/src/com/vaadin/client/connectors/GridConnector.java +++ b/client/src/com/vaadin/client/connectors/GridConnector.java @@ -19,13 +19,13 @@ package com.vaadin.client.connectors; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import java.util.logging.Logger; @@ -38,6 +38,7 @@ import com.vaadin.client.ComponentConnector; import com.vaadin.client.ConnectorHierarchyChangeEvent; import com.vaadin.client.DeferredWorker; import com.vaadin.client.MouseEventDetailsBuilder; +import com.vaadin.client.ServerConnector; import com.vaadin.client.annotations.OnStateChange; import com.vaadin.client.communication.StateChangeEvent; import com.vaadin.client.connectors.RpcDataSourceConnector.DetailsListener; @@ -79,10 +80,8 @@ import com.vaadin.client.widgets.Grid.FooterCell; import com.vaadin.client.widgets.Grid.FooterRow; import com.vaadin.client.widgets.Grid.HeaderCell; import com.vaadin.client.widgets.Grid.HeaderRow; -import com.vaadin.shared.Connector; import com.vaadin.shared.data.sort.SortDirection; import com.vaadin.shared.ui.Connect; -import com.vaadin.shared.ui.grid.DetailsConnectorChange; import com.vaadin.shared.ui.grid.EditorClientRpc; import com.vaadin.shared.ui.grid.EditorServerRpc; import com.vaadin.shared.ui.grid.GridClientRpc; @@ -420,255 +419,107 @@ public class GridConnector extends AbstractHasComponentsConnector implements } }; - private static class CustomDetailsGenerator implements DetailsGenerator { + private class CustomDetailsGenerator implements DetailsGenerator { - private final Map<Integer, ComponentConnector> indexToDetailsMap = new HashMap<Integer, ComponentConnector>(); + private final Map<String, ComponentConnector> idToDetailsMap = new HashMap<String, ComponentConnector>(); + private final Map<String, Integer> idToRowIndex = new HashMap<String, Integer>(); @Override - @SuppressWarnings("boxing") public Widget getDetails(int rowIndex) { - ComponentConnector componentConnector = indexToDetailsMap - .get(rowIndex); - if (componentConnector != null) { - return componentConnector.getWidget(); - } else { - return null; - } - } - - public void setDetailsConnectorChanges( - Set<DetailsConnectorChange> changes) { - /* - * To avoid overwriting connectors while moving them about, we'll - * take all the affected connectors, first all remove those that are - * removed or moved, then we add back those that are moved or added. - */ + JsonObject row = getWidget().getDataSource().getRow(rowIndex); - /* Remove moved/removed connectors from bookkeeping */ - for (DetailsConnectorChange change : changes) { - Integer oldIndex = change.getOldIndex(); - Connector removedConnector = indexToDetailsMap.remove(oldIndex); - - Connector connector = change.getConnector(); - assert removedConnector == null || connector == null - || removedConnector.equals(connector) : "Index " - + oldIndex + " points to " + removedConnector - + " while " + connector + " was expected"; - } - - /* Add moved/added connectors to bookkeeping */ - for (DetailsConnectorChange change : changes) { - Integer newIndex = change.getNewIndex(); - ComponentConnector connector = (ComponentConnector) change - .getConnector(); - - if (connector != null) { - assert newIndex != null : "An existing connector has a missing new index."; - - ComponentConnector prevConnector = indexToDetailsMap.put( - newIndex, connector); - - assert prevConnector == null : "Connector collision at index " - + newIndex - + " between old " - + prevConnector - + " and new " + connector; - } + if (!row.hasKey(GridState.JSONKEY_DETAILS_VISIBLE) + || row.getString(GridState.JSONKEY_DETAILS_VISIBLE) + .isEmpty()) { + return null; } - } - } - - @SuppressWarnings("boxing") - private static class DetailsConnectorFetcher implements DeferredWorker { - private static final int FETCH_TIMEOUT_MS = 5000; + String id = row.getString(GridState.JSONKEY_DETAILS_VISIBLE); + ComponentConnector componentConnector = idToDetailsMap.get(id); + idToRowIndex.put(id, rowIndex); - public interface Listener { - void fetchHasBeenScheduled(int id); - - void fetchHasReturned(int id); + return componentConnector.getWidget(); } - /** A flag making sure that we don't call scheduleFinally many times. */ - private boolean fetcherHasBeenCalled = false; - - /** A rolling counter for unique values. */ - private int detailsFetchCounter = 0; - - /** A collection that tracks the amount of requests currently underway. */ - private Set<Integer> pendingFetches = new HashSet<Integer>(5); - - private final ScheduledCommand lazyDetailsFetcher = new ScheduledCommand() { - @Override - public void execute() { - int currentFetchId = detailsFetchCounter++; - pendingFetches.add(currentFetchId); - rpc.sendDetailsComponents(currentFetchId); - fetcherHasBeenCalled = false; - - if (listener != null) { - listener.fetchHasBeenScheduled(currentFetchId); + public void updateConnectorHierarchy(List<ServerConnector> children) { + Set<String> connectorIds = new HashSet<String>(); + for (ServerConnector child : children) { + if (child instanceof ComponentConnector) { + connectorIds.add(child.getConnectorId()); + idToDetailsMap.put(child.getConnectorId(), + (ComponentConnector) child); } - - assert assertRequestDoesNotTimeout(currentFetchId); - } - }; - - private DetailsConnectorFetcher.Listener listener = null; - - private final GridServerRpc rpc; - - public DetailsConnectorFetcher(GridServerRpc rpc) { - assert rpc != null : "RPC was null"; - this.rpc = rpc; - } - - public void schedule() { - if (!fetcherHasBeenCalled) { - Scheduler.get().scheduleFinally(lazyDetailsFetcher); - fetcherHasBeenCalled = true; } - } - public void responseReceived(int fetchId) { - - if (fetchId < 0) { - /* Ignore negative fetchIds (they're pushed, not fetched) */ - return; + Set<String> removedDetails = new HashSet<String>(); + for (Entry<String, ComponentConnector> entry : idToDetailsMap + .entrySet()) { + ComponentConnector connector = entry.getValue(); + String id = connector.getConnectorId(); + if (!connectorIds.contains(id)) { + removedDetails.add(entry.getKey()); + if (idToRowIndex.containsKey(id)) { + getWidget().setDetailsVisible(idToRowIndex.get(id), + false); + } + } } - boolean success = pendingFetches.remove(fetchId); - assert success : "Received a response with an unidentified fetch id"; - - if (listener != null) { - listener.fetchHasReturned(fetchId); + for (String id : removedDetails) { + idToDetailsMap.remove(id); + idToRowIndex.remove(id); } } - - @Override - public boolean isWorkPending() { - return fetcherHasBeenCalled || !pendingFetches.isEmpty(); - } - - private boolean assertRequestDoesNotTimeout(final int fetchId) { - /* - * This method will not be compiled without asserts enabled. This - * only makes sure that any request does not time out. - * - * TODO Should this be an explicit check? Is it worth the overhead? - */ - new Timer() { - @Override - public void run() { - assert !pendingFetches.contains(fetchId) : "Fetch id " - + fetchId + " timed out."; - } - }.schedule(FETCH_TIMEOUT_MS); - return true; - } - - public void setListener(DetailsConnectorFetcher.Listener listener) { - // if more are needed, feel free to convert this into a collection. - this.listener = listener; - } } /** - * The functionality that makes sure that the scroll position is still kept - * up-to-date even if more details are being fetched lazily. + * Class for handling scrolling issues with open details. + * + * @since + * @author Vaadin Ltd */ - private class LazyDetailsScrollAdjuster implements DeferredWorker { + private class LazyDetailsScroller implements DeferredWorker { - private static final int SCROLL_TO_END_ID = -2; - private static final int NO_SCROLL_SCHEDULED = -1; - - private class ScrollStopChecker implements DeferredWorker { - private final ScheduledCommand checkCommand = new ScheduledCommand() { - @Override - public void execute() { - isScheduled = false; - if (queuedFetches.isEmpty()) { - currentRow = NO_SCROLL_SCHEDULED; - destination = null; - } - } - }; - - private boolean isScheduled = false; - - public void schedule() { - if (isScheduled) { - return; - } - Scheduler.get().scheduleDeferred(checkCommand); - isScheduled = true; - } - - @Override - public boolean isWorkPending() { - return isScheduled; - } - } - - private DetailsConnectorFetcher.Listener fetcherListener = new DetailsConnectorFetcher.Listener() { - @Override - @SuppressWarnings("boxing") - public void fetchHasBeenScheduled(int id) { - if (currentRow != NO_SCROLL_SCHEDULED) { - queuedFetches.add(id); - } - } + /* Timer value tested to work in our test cluster with slow IE8s. */ + private static final int DISABLE_LAZY_SCROLL_TIMEOUT = 1500; + /* + * Cancels details opening scroll after timeout. Avoids any unexpected + * scrolls via details opening. + */ + private Timer disableScroller = new Timer() { @Override - @SuppressWarnings("boxing") - public void fetchHasReturned(int id) { - if (currentRow == NO_SCROLL_SCHEDULED - || queuedFetches.isEmpty()) { - return; - } - - queuedFetches.remove(id); - if (currentRow == SCROLL_TO_END_ID) { - getWidget().scrollToEnd(); - } else { - getWidget().scrollToRow(currentRow, destination); - } - - /* - * Schedule a deferred call whether we should stop adjusting for - * scrolling. - * - * This is done deferredly just because we can't be absolutely - * certain whether this most recent scrolling won't cascade into - * further lazy details loading (perhaps deferredly). - */ - scrollStopChecker.schedule(); + public void run() { + targetRow = -1; } }; - private int currentRow = NO_SCROLL_SCHEDULED; - private final Set<Integer> queuedFetches = new HashSet<Integer>(); - private final ScrollStopChecker scrollStopChecker = new ScrollStopChecker(); - private ScrollDestination destination; - - public LazyDetailsScrollAdjuster() { - detailsConnectorFetcher.setListener(fetcherListener); - } + private Integer targetRow = -1; + private ScrollDestination destination = null; - public void adjustForEnd() { - currentRow = SCROLL_TO_END_ID; + public void scrollToRow(Integer row, ScrollDestination dest) { + targetRow = row; + destination = dest; + disableScroller.schedule(DISABLE_LAZY_SCROLL_TIMEOUT); } - public void adjustFor(int row, ScrollDestination destination) { - currentRow = row; - this.destination = destination; + /** + * Inform LazyDetailsScroller that a details row has opened on a row. + * + * @since + * @param rowIndex + * index of row with details now open + */ + public void detailsOpened(int rowIndex) { + if (targetRow == rowIndex) { + getWidget().scrollToRow(targetRow, destination); + disableScroller.run(); + } } @Override public boolean isWorkPending() { - return currentRow != NO_SCROLL_SCHEDULED - || !queuedFetches.isEmpty() - || scrollStopChecker.isWorkPending(); + return disableScroller.isRunning(); } } @@ -730,39 +581,46 @@ public class GridConnector extends AbstractHasComponentsConnector implements private final CustomDetailsGenerator customDetailsGenerator = new CustomDetailsGenerator(); - private final DetailsConnectorFetcher detailsConnectorFetcher = new DetailsConnectorFetcher( - getRpcProxy(GridServerRpc.class)); - private final DetailsListener detailsListener = new DetailsListener() { @Override public void reapplyDetailsVisibility(final int rowIndex, final JsonObject row) { - Scheduler.get().scheduleDeferred(new ScheduledCommand() { - @Override - public void execute() { - if (hasDetailsOpen(row)) { - getWidget().setDetailsVisible(rowIndex, true); - detailsConnectorFetcher.schedule(); - } else { + if (hasDetailsOpen(row)) { + // Command for opening details row. + ScheduledCommand openDetails = new ScheduledCommand() { + @Override + public void execute() { + // Re-apply to force redraw. getWidget().setDetailsVisible(rowIndex, false); + getWidget().setDetailsVisible(rowIndex, true); + lazyDetailsScroller.detailsOpened(rowIndex); } + }; + + if (initialChange) { + Scheduler.get().scheduleDeferred(openDetails); + } else { + Scheduler.get().scheduleFinally(openDetails); } - }); + } else { + getWidget().setDetailsVisible(rowIndex, false); + } } private boolean hasDetailsOpen(JsonObject row) { return row.hasKey(GridState.JSONKEY_DETAILS_VISIBLE) - && row.getBoolean(GridState.JSONKEY_DETAILS_VISIBLE); - } - - @Override - public void closeDetails(int rowIndex) { - getWidget().setDetailsVisible(rowIndex, false); + && row.getString(GridState.JSONKEY_DETAILS_VISIBLE) != null; } }; - private final LazyDetailsScrollAdjuster lazyDetailsScrollAdjuster = new LazyDetailsScrollAdjuster(); + private final LazyDetailsScroller lazyDetailsScroller = new LazyDetailsScroller(); + + /* + * Initially details need to behave a bit differently to allow some + * escalator magic. + */ + private boolean initialChange; @Override @SuppressWarnings("unchecked") @@ -797,11 +655,13 @@ public class GridConnector extends AbstractHasComponentsConnector implements @Override public void scrollToEnd() { - lazyDetailsScrollAdjuster.adjustForEnd(); Scheduler.get().scheduleFinally(new ScheduledCommand() { @Override public void execute() { getWidget().scrollToEnd(); + // Scrolls further if details opens. + lazyDetailsScroller.scrollToRow(dataSource.size() - 1, + ScrollDestination.END); } }); } @@ -809,11 +669,12 @@ public class GridConnector extends AbstractHasComponentsConnector implements @Override public void scrollToRow(final int row, final ScrollDestination destination) { - lazyDetailsScrollAdjuster.adjustFor(row, destination); Scheduler.get().scheduleFinally(new ScheduledCommand() { @Override public void execute() { getWidget().scrollToRow(row, destination); + // Scrolls a bit further if details opens. + lazyDetailsScroller.scrollToRow(row, destination); } }); } @@ -822,51 +683,6 @@ public class GridConnector extends AbstractHasComponentsConnector implements public void recalculateColumnWidths() { getWidget().recalculateColumnWidths(); } - - @Override - @SuppressWarnings("boxing") - public void setDetailsConnectorChanges( - Set<DetailsConnectorChange> connectorChanges, int fetchId) { - customDetailsGenerator - .setDetailsConnectorChanges(connectorChanges); - - List<DetailsConnectorChange> removedFirst = new ArrayList<DetailsConnectorChange>( - connectorChanges); - Collections.sort(removedFirst, - DetailsConnectorChange.REMOVED_FIRST_COMPARATOR); - - // refresh moved/added details rows - for (DetailsConnectorChange change : removedFirst) { - Integer oldIndex = change.getOldIndex(); - Integer newIndex = change.getNewIndex(); - - assert oldIndex == null || oldIndex >= 0 : "Got an " - + "invalid old index: " + oldIndex - + " (connector: " + change.getConnector() + ")"; - assert newIndex == null || newIndex >= 0 : "Got an " - + "invalid new index: " + newIndex - + " (connector: " + change.getConnector() + ")"; - - if (oldIndex != null) { - /* Close the old/removed index */ - getWidget().setDetailsVisible(oldIndex, false); - - if (change.isShouldStillBeVisible()) { - getWidget().setDetailsVisible(oldIndex, true); - } - } - - if (newIndex != null) { - /* - * Since the component was lazy loaded, we need to - * refresh the details by toggling it. - */ - getWidget().setDetailsVisible(newIndex, false); - getWidget().setDetailsVisible(newIndex, true); - } - } - detailsConnectorFetcher.responseReceived(fetchId); - } }); getWidget().addSelectionHandler(internalSelectionChangeHandler); @@ -919,16 +735,11 @@ public class GridConnector extends AbstractHasComponentsConnector implements } @Override - public void onUnregister() { - customDetailsGenerator.indexToDetailsMap.clear(); - - super.onUnregister(); - } - - @Override public void onStateChanged(final StateChangeEvent stateChangeEvent) { super.onStateChanged(stateChangeEvent); + initialChange = stateChangeEvent.isInitialStateChange(); + // Column updates if (stateChangeEvent.hasPropertyChanged("columns")) { @@ -1411,6 +1222,7 @@ public class GridConnector extends AbstractHasComponentsConnector implements @Override public void onConnectorHierarchyChange( ConnectorHierarchyChangeEvent connectorHierarchyChangeEvent) { + customDetailsGenerator.updateConnectorHierarchy(getChildren()); } public String getColumnId(Grid.Column<?, ?> column) { @@ -1427,8 +1239,7 @@ public class GridConnector extends AbstractHasComponentsConnector implements @Override public boolean isWorkPending() { - return detailsConnectorFetcher.isWorkPending() - || lazyDetailsScrollAdjuster.isWorkPending(); + return lazyDetailsScroller.isWorkPending(); } /** diff --git a/client/src/com/vaadin/client/connectors/RpcDataSourceConnector.java b/client/src/com/vaadin/client/connectors/RpcDataSourceConnector.java index 627ee74eca..c1b9f13ef4 100644 --- a/client/src/com/vaadin/client/connectors/RpcDataSourceConnector.java +++ b/client/src/com/vaadin/client/connectors/RpcDataSourceConnector.java @@ -64,14 +64,6 @@ public class RpcDataSourceConnector extends AbstractExtensionConnector { * @see GridState#JSONKEY_DETAILS_VISIBLE */ void reapplyDetailsVisibility(int rowIndex, JsonObject row); - - /** - * Closes details for a row. - * - * @param rowIndex - * the index of the row for which to close details - */ - void closeDetails(int rowIndex); } public class RpcDataSource extends AbstractRemoteDataSource<JsonObject> { @@ -221,11 +213,6 @@ public class RpcDataSourceConnector extends AbstractExtensionConnector { rowData.get(i)); } } - - @Override - protected void onDropFromCache(int rowIndex) { - detailsListener.closeDetails(rowIndex); - } } private final RpcDataSource dataSource = new RpcDataSource(); diff --git a/server/src/com/vaadin/data/RpcDataProviderExtension.java b/server/src/com/vaadin/data/RpcDataProviderExtension.java index b3c7972b52..6f8a7e8f7b 100644 --- a/server/src/com/vaadin/data/RpcDataProviderExtension.java +++ b/server/src/com/vaadin/data/RpcDataProviderExtension.java @@ -25,7 +25,6 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Map.Entry; import java.util.Set; import java.util.logging.Logger; @@ -49,11 +48,9 @@ import com.vaadin.server.ClientConnector; import com.vaadin.server.KeyMapper; import com.vaadin.shared.data.DataProviderRpc; import com.vaadin.shared.data.DataRequestRpc; -import com.vaadin.shared.ui.grid.DetailsConnectorChange; import com.vaadin.shared.ui.grid.GridClientRpc; import com.vaadin.shared.ui.grid.GridState; import com.vaadin.shared.ui.grid.Range; -import com.vaadin.shared.util.SharedUtil; import com.vaadin.ui.Component; import com.vaadin.ui.Grid; import com.vaadin.ui.Grid.CellReference; @@ -119,16 +116,11 @@ public class RpcDataProviderExtension extends AbstractExtension { } for (Object itemId : itemsRemoved) { - detailComponentManager.destroyDetails(itemId); itemIdToKey.remove(itemId); } for (Object itemId : itemSet) { itemIdToKey.put(itemId, getKey(itemId)); - if (visibleDetails.contains(itemId)) { - detailComponentManager.createDetails(itemId, - indexOf(itemId)); - } } } @@ -620,34 +612,11 @@ public class RpcDataProviderExtension extends AbstractExtension { private final Map<Object, Component> visibleDetailsComponents = Maps .newHashMap(); - /** A lookup map for which row contains which details component. */ - private BiMap<Integer, Component> rowIndexToDetails = HashBiMap - .create(); - - /** - * A copy of {@link #rowIndexToDetails} from its last stable state. Used - * for creating a diff against {@link #rowIndexToDetails}. - * - * @see #getAndResetConnectorChanges() - */ - private BiMap<Integer, Component> prevRowIndexToDetails = HashBiMap - .create(); - - /** - * A set keeping track on components that have been created, but not - * attached. They should be attached at some later point in time. - * <p> - * This isn't strictly requried, but it's a handy explicit log. You - * could find out the same thing by taking out all the other components - * and checking whether Grid is their parent or not. - */ - private final Set<Component> unattachedComponents = Sets.newHashSet(); - /** * Keeps tabs on all the details that did not get a component during * {@link #createDetails(Object, int)}. */ - private final Map<Object, Integer> emptyDetails = Maps.newHashMap(); + private final Set<Object> emptyDetails = Sets.newHashSet(); private Grid grid; @@ -661,19 +630,16 @@ public class RpcDataProviderExtension extends AbstractExtension { * the item id for which to create the details component. * Assumed not <code>null</code> and that a component is not * currently present for this item previously - * @param rowIndex - * the row index for {@code itemId} * @throws IllegalStateException * if the current details generator provides a component * that was manually attached, or if the same instance has * already been provided */ - public void createDetails(Object itemId, int rowIndex) - throws IllegalStateException { + public void createDetails(Object itemId) throws IllegalStateException { assert itemId != null : "itemId was null"; - Integer newRowIndex = Integer.valueOf(rowIndex); - if (visibleDetailsComponents.containsKey(itemId)) { + if (visibleDetailsComponents.containsKey(itemId) + || emptyDetails.contains(itemId)) { // Don't overwrite existing components return; } @@ -684,58 +650,26 @@ public class RpcDataProviderExtension extends AbstractExtension { DetailsGenerator detailsGenerator = grid.getDetailsGenerator(); Component details = detailsGenerator.getDetails(rowReference); if (details != null) { - String generatorName = detailsGenerator.getClass().getName(); if (details.getParent() != null) { - throw new IllegalStateException(generatorName + String name = detailsGenerator.getClass().getName(); + throw new IllegalStateException(name + " generated a details component that already " - + "was attached. (itemId: " + itemId + ", row: " - + rowIndex + ", component: " + details); - } - - if (rowIndexToDetails.containsValue(details)) { - throw new IllegalStateException(generatorName - + " provided a details component that already " - + "exists in Grid. (itemId: " + itemId + ", row: " - + rowIndex + ", component: " + details); + + "was attached. (itemId: " + itemId + + ", component: " + details + ")"); } visibleDetailsComponents.put(itemId, details); - rowIndexToDetails.put(newRowIndex, details); - unattachedComponents.add(details); - assert !emptyDetails.containsKey(itemId) : "Bookeeping thinks " + details.setParent(grid); + grid.markAsDirty(); + + assert !emptyDetails.contains(itemId) : "Bookeeping thinks " + "itemId is empty even though we just created a " + "component for it (" + itemId + ")"; } else { - assert assertItemIdHasNotMovedAndNothingIsOverwritten(itemId, - newRowIndex); - emptyDetails.put(itemId, newRowIndex); - } - - /* - * Don't attach the components here. It's done by - * GridServerRpc.sendDetailsComponents in a separate roundtrip. - */ - } - - private boolean assertItemIdHasNotMovedAndNothingIsOverwritten( - Object itemId, Integer newRowIndex) { - - Integer oldRowIndex = emptyDetails.get(itemId); - if (!SharedUtil.equals(oldRowIndex, newRowIndex)) { - - assert !emptyDetails.containsKey(itemId) : "Unexpected " - + "change of empty details row index for itemId " - + itemId + " from " + oldRowIndex + " to " - + newRowIndex; - - assert !emptyDetails.containsValue(newRowIndex) : "Bookkeeping" - + " already had another itemId for this empty index " - + "(index: " + newRowIndex + ", new itemId: " + itemId - + ")"; + emptyDetails.add(itemId); } - return true; } /** @@ -756,8 +690,6 @@ public class RpcDataProviderExtension extends AbstractExtension { return; } - rowIndexToDetails.inverse().remove(removedComponent); - removedComponent.setParent(null); grid.markAsDirty(); } @@ -773,81 +705,12 @@ public class RpcDataProviderExtension extends AbstractExtension { public Collection<Component> getComponents() { Set<Component> components = new HashSet<Component>( visibleDetailsComponents.values()); - components.removeAll(unattachedComponents); return components; } - /** - * Gets information on how the connectors have changed. - * <p> - * This method only returns the changes that have been made between two - * calls of this method. I.e. Calling this method once will reset the - * state for the next state. - * <p> - * Used internally by the Grid object. - * - * @return information on how the connectors have changed - */ - public Set<DetailsConnectorChange> getAndResetConnectorChanges() { - Set<DetailsConnectorChange> changes = new HashSet<DetailsConnectorChange>(); - - // populate diff with added/changed - for (Entry<Integer, Component> entry : rowIndexToDetails.entrySet()) { - Component component = entry.getValue(); - assert component != null : "rowIndexToDetails contains a null component"; - - Integer newIndex = entry.getKey(); - Integer oldIndex = prevRowIndexToDetails.inverse().get( - component); - - /* - * only attach components. Detaching already happened in - * destroyDetails. - */ - if (newIndex != null && oldIndex == null) { - assert unattachedComponents.contains(component) : "unattachedComponents does not contain component for index " - + newIndex + " (" + component + ")"; - component.setParent(grid); - unattachedComponents.remove(component); - } - - if (!SharedUtil.equals(oldIndex, newIndex)) { - changes.add(new DetailsConnectorChange(component, oldIndex, - newIndex, emptyDetails.containsKey(component))); - } - } - - // populate diff with removed - for (Entry<Integer, Component> entry : prevRowIndexToDetails - .entrySet()) { - Integer oldIndex = entry.getKey(); - Component component = entry.getValue(); - Integer newIndex = rowIndexToDetails.inverse().get(component); - if (newIndex == null) { - changes.add(new DetailsConnectorChange(null, oldIndex, - null, emptyDetails.containsValue(oldIndex))); - } - } - - // reset diff map - prevRowIndexToDetails = HashBiMap.create(rowIndexToDetails); - - return changes; - } - public void refresh(Object itemId) { - Component component = visibleDetailsComponents.get(itemId); - Integer rowIndex = null; - if (component != null) { - rowIndex = rowIndexToDetails.inverse().get(component); - destroyDetails(itemId); - } else { - rowIndex = emptyDetails.remove(itemId); - } - - assert rowIndex != null : "Given itemId does not map to an " - + "existing detail row (" + itemId + ")"; - createDetails(itemId, rowIndex.intValue()); + destroyDetails(itemId); + createDetails(itemId); } void setGrid(Grid grid) { @@ -927,17 +790,13 @@ public class RpcDataProviderExtension extends AbstractExtension { listener.removeListener(); } - // Wipe clean all details. - HashSet<Object> detailItemIds = new HashSet<Object>( - detailComponentManager.visibleDetailsComponents - .keySet()); - for (Object itemId : detailItemIds) { - detailComponentManager.destroyDetails(itemId); - } - listeners.clear(); activeRowHandler.activeRange = Range.withLength(0, 0); + for (Object itemId : visibleDetails) { + detailComponentManager.destroyDetails(itemId); + } + /* Mark as dirty to push changes in beforeClientResponse */ bareItemSetTriggeredSizeChange = true; markAsDirty(); @@ -1118,7 +977,14 @@ public class RpcDataProviderExtension extends AbstractExtension { rowObject.put(GridState.JSONKEY_ROWKEY, keyMapper.getKey(itemId)); if (visibleDetails.contains(itemId)) { - rowObject.put(GridState.JSONKEY_DETAILS_VISIBLE, true); + // Double check to be sure details component exists. + detailComponentManager.createDetails(itemId); + Component detailsComponent = detailComponentManager.visibleDetailsComponents + .get(itemId); + rowObject.put( + GridState.JSONKEY_DETAILS_VISIBLE, + (detailsComponent != null ? detailsComponent + .getConnectorId() : "")); } rowReference.set(itemId); @@ -1254,15 +1120,11 @@ public class RpcDataProviderExtension extends AbstractExtension { private void internalUpdateRowData(Object itemId) { int index = container.indexOfId(itemId); - if (index >= 0) { + if (activeRowHandler.activeRange.contains(index)) { JsonValue row = getRowData(getGrid().getColumns(), itemId); JsonArray rowArray = Json.createArray(); rowArray.set(0, row); rpc.setRowData(index, rowArray); - - if (isDetailsVisible(itemId)) { - detailComponentManager.createDetails(itemId, index); - } } } @@ -1399,37 +1261,21 @@ public class RpcDataProviderExtension extends AbstractExtension { * hide */ public void setDetailsVisible(Object itemId, boolean visible) { - final boolean modified; - if (visible) { - modified = visibleDetails.add(itemId); + visibleDetails.add(itemId); /* - * We don't want to create the component here, since the component - * might be out of view, and thus we don't know where the details - * should end up on the client side. This is also a great thing to - * optimize away, so that in case a lot of things would be opened at - * once, a huge chunk of data doesn't get sent over immediately. + * This might be an issue with a huge number of open rows, but as of + * now this works in most of the cases. */ - + detailComponentManager.createDetails(itemId); } else { - modified = visibleDetails.remove(itemId); + visibleDetails.remove(itemId); - /* - * Here we can try to destroy the component no matter what. The - * component has been removed and should be detached from the - * component hierarchy. The details row will be closed on the client - * side automatically. - */ detailComponentManager.destroyDetails(itemId); } - int rowIndex = indexOf(itemId); - boolean modifiedRowIsActive = activeRowHandler.activeRange - .contains(rowIndex); - if (modified && modifiedRowIsActive) { - updateRowData(itemId); - } + updateRowData(itemId); } /** @@ -1454,18 +1300,10 @@ public class RpcDataProviderExtension extends AbstractExtension { public void refreshDetails() { for (Object itemId : ImmutableSet.copyOf(visibleDetails)) { detailComponentManager.refresh(itemId); + updateRowData(itemId); } } - private int indexOf(Object itemId) { - /* - * It would be great if we could optimize this method away, since the - * normal usage of Grid doesn't need any indices to be known. It was - * already optimized away once, maybe we can do away with these as well. - */ - return container.indexOfId(itemId); - } - /** * Gets the detail component manager for this data provider * @@ -1475,13 +1313,4 @@ public class RpcDataProviderExtension extends AbstractExtension { public DetailComponentManager getDetailComponentManager() { return detailComponentManager; } - - @Override - public void detach() { - for (Object itemId : ImmutableSet.copyOf(visibleDetails)) { - detailComponentManager.destroyDetails(itemId); - } - - super.detach(); - } } diff --git a/server/src/com/vaadin/ui/Grid.java b/server/src/com/vaadin/ui/Grid.java index e9469c5bca..ea973bb3ba 100644 --- a/server/src/com/vaadin/ui/Grid.java +++ b/server/src/com/vaadin/ui/Grid.java @@ -3281,7 +3281,7 @@ public class Grid extends AbstractComponent implements SelectionNotifier, * currently extends the AbstractExtension superclass, but this fact should * be regarded as an implementation detail and subject to change in a future * major or minor Vaadin revision. - * + * * @param <T> * the type this renderer knows how to present */ @@ -3354,7 +3354,7 @@ public class Grid extends AbstractComponent implements SelectionNotifier, * is desired. For instance, a {@code Renderer<Date>} could first turn a * date value into a formatted string and return * {@code encode(dateString, String.class)}. - * + * * @param value * the value to be encoded * @param type @@ -3369,7 +3369,7 @@ public class Grid extends AbstractComponent implements SelectionNotifier, /** * An abstract base class for server-side Grid extensions. - * + * * @since 7.5 */ public static abstract class AbstractGridExtension extends @@ -3384,7 +3384,7 @@ public class Grid extends AbstractComponent implements SelectionNotifier, /** * Constructs a new Grid extension and extends given Grid. - * + * * @param grid * a grid instance */ @@ -3856,13 +3856,6 @@ public class Grid extends AbstractComponent implements SelectionNotifier, userOriginated); } } - - @Override - public void sendDetailsComponents(int fetchId) { - getRpcProxy(GridClientRpc.class).setDetailsConnectorChanges( - detailComponentManager.getAndResetConnectorChanges(), - fetchId); - } }); registerRpc(new EditorServerRpc() { @@ -6063,8 +6056,6 @@ public class Grid extends AbstractComponent implements SelectionNotifier, this.detailsGenerator = detailsGenerator; datasourceExtension.refreshDetails(); - getRpcProxy(GridClientRpc.class).setDetailsConnectorChanges( - detailComponentManager.getAndResetConnectorChanges(), -1); } /** diff --git a/shared/src/com/vaadin/shared/ui/grid/GridClientRpc.java b/shared/src/com/vaadin/shared/ui/grid/GridClientRpc.java index 3c6d993482..ac1b1d5a78 100644 --- a/shared/src/com/vaadin/shared/ui/grid/GridClientRpc.java +++ b/shared/src/com/vaadin/shared/ui/grid/GridClientRpc.java @@ -15,8 +15,6 @@ */ package com.vaadin.shared.ui.grid; -import java.util.Set; - import com.vaadin.shared.communication.ClientRpc; /** @@ -57,19 +55,4 @@ public interface GridClientRpc extends ClientRpc { * Command client Grid to recalculate column widths. */ public void recalculateColumnWidths(); - - /** - * Informs the GridConnector on how the indexing of details connectors has - * changed. - * - * @since 7.5.0 - * @param connectorChanges - * the indexing changes of details connectors - * @param fetchId - * the id of the request for fetching the changes. A negative - * number indicates a push (not requested by the client side) - */ - public void setDetailsConnectorChanges( - Set<DetailsConnectorChange> connectorChanges, int fetchId); - } diff --git a/shared/src/com/vaadin/shared/ui/grid/GridServerRpc.java b/shared/src/com/vaadin/shared/ui/grid/GridServerRpc.java index dca55c11c4..5c91f2b746 100644 --- a/shared/src/com/vaadin/shared/ui/grid/GridServerRpc.java +++ b/shared/src/com/vaadin/shared/ui/grid/GridServerRpc.java @@ -61,23 +61,6 @@ public interface GridServerRpc extends ServerRpc { List<String> oldColumnOrder); /** - * This is a trigger for Grid to send whatever has changed regarding the - * details components. - * <p> - * The components can't be sent eagerly, since they are generated as a side - * effect in - * {@link com.vaadin.data.RpcDataProviderExtension#beforeClientResponse(boolean)} - * , and that is too late to change the hierarchy. So we need this - * round-trip to work around that limitation. - * - * @since 7.5.0 - * @param fetchId - * an unique identifier for the request - * @see com.vaadin.ui.Grid#setDetailsVisible(Object, boolean) - */ - void sendDetailsComponents(int fetchId); - - /** * Informs the server that the column's visibility has been changed. * * @since 7.5.0 diff --git a/uitest/src/com/vaadin/tests/components/grid/GridDetailsDetachTest.java b/uitest/src/com/vaadin/tests/components/grid/GridDetailsDetachTest.java index fc79fd1b68..7406daeacd 100644 --- a/uitest/src/com/vaadin/tests/components/grid/GridDetailsDetachTest.java +++ b/uitest/src/com/vaadin/tests/components/grid/GridDetailsDetachTest.java @@ -70,4 +70,28 @@ public class GridDetailsDetachTest extends MultiBrowserTest { Assert.assertEquals("Spacer content not visible", "Extra data for Bean 5", spacers.get(1).getText()); } + + @Test + public void testDetachAndImmediateReattach() { + setDebug(true); + openTestURL(); + + $(GridElement.class).first().getCell(3, 0).click(); + $(GridElement.class).first().getCell(5, 0).click(); + + assertNoErrorNotifications(); + + // Detach and Re-attach Grid + $(ButtonElement.class).get(1).click(); + + assertNoErrorNotifications(); + + List<WebElement> spacers = findElements(By.className("v-grid-spacer")); + Assert.assertEquals("Not enough spacers in DOM", 2, spacers.size()); + Assert.assertEquals("Spacer content not visible", + "Extra data for Bean 3", spacers.get(0).getText()); + Assert.assertEquals("Spacer content not visible", + "Extra data for Bean 5", spacers.get(1).getText()); + } + } diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridDetailsServerTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridDetailsServerTest.java index 4ea64073f3..326dbcd55f 100644 --- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridDetailsServerTest.java +++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridDetailsServerTest.java @@ -22,7 +22,6 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.openqa.selenium.By; import org.openqa.selenium.NoSuchElementException; @@ -196,14 +195,11 @@ public class GridDetailsServerTest extends GridBasicFeaturesTest { assertEquals("Two", getGridElement().getDetails(0).getText()); } - @Ignore("This use case is not currently supported by Grid. If the detail " - + "is out of view, the component is detached from the UI and a " - + "new instance is generated when scrolled back. Support will " - + "maybe be incorporated at a later time") @Test public void hierarchyChangesWorkInDetailsWhileOutOfView() { selectMenuPath(DETAILS_GENERATOR_HIERARCHICAL); selectMenuPath(OPEN_FIRST_ITEM_DETAILS); + assertEquals("One", getGridElement().getDetails(0).getText()); scrollGridVerticallyTo(10000); selectMenuPath(CHANGE_HIERARCHY); scrollGridVerticallyTo(0); |