Grid's Details can now be Components (#16644)

Change-Id: If67dd2e86cf41c57f208a3691e2cb7a5a29c133c

client/src/com/vaadin/client/connectors/GridConnector.java

@@ -29,12 +29,14 @@ import java.util.Set;
 import java.util.logging.Logger;
 
 import com.google.gwt.core.client.Scheduler;
+import com.google.gwt.core.client.Scheduler.RepeatingCommand;
 import com.google.gwt.core.client.Scheduler.ScheduledCommand;
 import com.google.gwt.dom.client.NativeEvent;
-import com.google.gwt.user.client.ui.Label;
+import com.google.gwt.user.client.Timer;
 import com.google.gwt.user.client.ui.Widget;
 import com.vaadin.client.ComponentConnector;
 import com.vaadin.client.ConnectorHierarchyChangeEvent;
+import com.vaadin.client.DeferredWorker;
 import com.vaadin.client.MouseEventDetailsBuilder;
 import com.vaadin.client.annotations.OnStateChange;
 import com.vaadin.client.communication.StateChangeEvent;
@@ -72,8 +74,10 @@ 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.ConnectorIndexChange;
 import com.vaadin.shared.ui.grid.EditorClientRpc;
 import com.vaadin.shared.ui.grid.EditorServerRpc;
 import com.vaadin.shared.ui.grid.GridClientRpc;
@@ -103,7 +107,8 @@ import elemental.json.JsonValue;
  */
 @Connect(com.vaadin.ui.Grid.class)
 public class GridConnector extends AbstractHasComponentsConnector implements
-        SimpleManagedLayout, RpcDataSourceConnector.DetailsListener {
+        SimpleManagedLayout, RpcDataSourceConnector.DetailsListener,
+        DeferredWorker {
 
     private static final class CustomCellStyleGenerator implements
             CellStyleGenerator<JsonObject> {
@@ -362,11 +367,119 @@ public class GridConnector extends AbstractHasComponentsConnector implements
         }
     }
 
-    private class CustomDetailsGenerator implements DetailsGenerator {
+    private static class CustomDetailsGenerator implements DetailsGenerator {
+
+        private final Map<Integer, ComponentConnector> indexToDetailsMap = new HashMap<Integer, ComponentConnector>();
+
         @Override
+        @SuppressWarnings("boxing")
         public Widget getDetails(int rowIndex) {
-            // TODO
-            return new Label("[todo]");
+            ComponentConnector componentConnector = indexToDetailsMap
+                    .get(rowIndex);
+            if (componentConnector != null) {
+                return componentConnector.getWidget();
+            } else {
+                return null;
+            }
+        }
+
+        public void setDetailsConnectorChanges(Set<ConnectorIndexChange> 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.
+             */
+
+            /* Remove moved/removed connectors from bookkeeping */
+            for (ConnectorIndexChange 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 (ConnectorIndexChange 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;
+                }
+            }
+        }
+    }
+
+    @SuppressWarnings("boxing")
+    private class DetailsConnectorFetcher implements DeferredWorker {
+
+        /** 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);
+                getRpcProxy(GridServerRpc.class).sendDetailsComponents(
+                        currentFetchId);
+                fetcherHasBeenCalled = false;
+
+                assert assertRequestDoesNotTimeout(currentFetchId);
+            }
+        };
+
+        public void schedule() {
+            if (!fetcherHasBeenCalled) {
+                Scheduler.get().scheduleFinally(lazyDetailsFetcher);
+                fetcherHasBeenCalled = true;
+            }
+        }
+
+        public void responseReceived(int fetchId) {
+            boolean success = pendingFetches.remove(fetchId);
+            assert success : "Received a response with an unidentified fetch 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);
+                }
+            }.schedule(1000);
+            return true;
         }
     }
 
@@ -417,6 +530,10 @@ public class GridConnector extends AbstractHasComponentsConnector implements
 
     private String lastKnownTheme = null;
 
+    private final CustomDetailsGenerator customDetailsGenerator = new CustomDetailsGenerator();
+
+    private final DetailsConnectorFetcher detailsConnectorFetcher = new DetailsConnectorFetcher();
+
     @Override
     @SuppressWarnings("unchecked")
     public Grid<JsonObject> getWidget() {
@@ -469,6 +586,24 @@ public class GridConnector extends AbstractHasComponentsConnector implements
             public void recalculateColumnWidths() {
                 getWidget().recalculateColumnWidths();
             }
+
+            @Override
+            public void setDetailsConnectorChanges(
+                    Set<ConnectorIndexChange> connectorChanges, int fetchId) {
+                customDetailsGenerator
+                        .setDetailsConnectorChanges(connectorChanges);
+
+                // refresh moved/added details rows
+                for (ConnectorIndexChange change : connectorChanges) {
+                    Integer newIndex = change.getNewIndex();
+                    if (newIndex != null) {
+                        int index = newIndex.intValue();
+                        getWidget().setDetailsVisible(index, false);
+                        getWidget().setDetailsVisible(index, true);
+                    }
+                }
+                detailsConnectorFetcher.responseReceived(fetchId);
+            }
         });
 
         getWidget().addSelectionHandler(internalSelectionChangeHandler);
@@ -512,7 +647,7 @@ public class GridConnector extends AbstractHasComponentsConnector implements
 
         getWidget().setEditorHandler(new CustomEditorHandler());
 
-        getWidget().setDetailsGenerator(new CustomDetailsGenerator());
+        getWidget().setDetailsGenerator(customDetailsGenerator);
 
         getLayoutManager().registerDependency(this, getWidget().getElement());
 
@@ -1017,5 +1152,12 @@ public class GridConnector extends AbstractHasComponentsConnector implements
         } else {
             getWidget().setDetailsVisible(rowIndex, false);
         }
+
+        detailsConnectorFetcher.schedule();
+    }
+
+    @Override
+    public boolean isWorkPending() {
+        return detailsConnectorFetcher.isWorkPending();
     }
 }

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

@@ -570,6 +570,7 @@ public abstract class AbstractRemoteDataSource<T> implements DataSource<T> {
         Profiler.leave("AbstractRemoteDataSource.insertRowData");
     }
 
+    @SuppressWarnings("boxing")
     private void moveRowFromIndexToIndex(int oldIndex, int newIndex) {
         T row = indexToRowMap.remove(oldIndex);
         if (indexToRowMap.containsKey(newIndex)) {

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

@@ -51,6 +51,7 @@ import com.vaadin.ui.Grid;
 import com.vaadin.ui.Grid.CellReference;
 import com.vaadin.ui.Grid.CellStyleGenerator;
 import com.vaadin.ui.Grid.Column;
+import com.vaadin.ui.Grid.DetailComponentManager;
 import com.vaadin.ui.Grid.RowReference;
 import com.vaadin.ui.Grid.RowStyleGenerator;
 import com.vaadin.ui.renderers.Renderer;
@@ -113,6 +114,7 @@ public class RpcDataProviderExtension extends AbstractExtension {
                 final Object itemId = indexToItemId.get(ii);
 
                 if (!isPinned(itemId)) {
+                    detailComponentManager.destroyDetails(itemId);
                     itemIdToKey.remove(itemId);
                     indexToItemId.remove(ii);
                 }
@@ -154,6 +156,7 @@ public class RpcDataProviderExtension extends AbstractExtension {
                     }
 
                     indexToItemId.forcePut(index, itemId);
+                    detailComponentManager.createDetails(itemId, index);
                 }
                 index++;
             }
@@ -747,14 +750,18 @@ public class RpcDataProviderExtension extends AbstractExtension {
      */
     private Set<Object> visibleDetails = new HashSet<Object>();
 
+    private DetailComponentManager detailComponentManager;
+
     /**
      * Creates a new data provider using the given container.
      * 
      * @param container
      *            the container to make available
      */
-    public RpcDataProviderExtension(Indexed container) {
+    public RpcDataProviderExtension(Indexed container,
+            DetailComponentManager detailComponentManager) {
         this.container = container;
+        this.detailComponentManager = detailComponentManager;
         rpc = getRpcProxy(DataProviderRpc.class);
 
         registerRpc(new DataRequestRpc() {
@@ -1018,6 +1025,10 @@ public class RpcDataProviderExtension extends AbstractExtension {
             JsonArray rowArray = Json.createArray();
             rowArray.set(0, row);
             rpc.setRowData(index, rowArray);
+
+            if (isDetailsVisible(itemId)) {
+                detailComponentManager.createDetails(itemId, index);
+            }
         }
     }
 
@@ -1155,10 +1166,28 @@ public class RpcDataProviderExtension extends AbstractExtension {
      */
     public void setDetailsVisible(Object itemId, boolean visible) {
         final boolean modified;
+
         if (visible) {
             modified = 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.
+             */
+
         } else {
             modified = 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 = keyMapper.getIndex(itemId);

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

@@ -36,6 +36,8 @@ import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+import com.google.gwt.thirdparty.guava.common.collect.BiMap;
+import com.google.gwt.thirdparty.guava.common.collect.HashBiMap;
 import com.google.gwt.thirdparty.guava.common.collect.Sets;
 import com.google.gwt.thirdparty.guava.common.collect.Sets.SetView;
 import com.vaadin.data.Container;
@@ -75,6 +77,7 @@ import com.vaadin.server.KeyMapper;
 import com.vaadin.server.VaadinSession;
 import com.vaadin.shared.MouseEventDetails;
 import com.vaadin.shared.data.sort.SortDirection;
+import com.vaadin.shared.ui.grid.ConnectorIndexChange;
 import com.vaadin.shared.ui.grid.EditorClientRpc;
 import com.vaadin.shared.ui.grid.EditorServerRpc;
 import com.vaadin.shared.ui.grid.GridClientRpc;
@@ -183,6 +186,11 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
         /**
          * This method is called for whenever a new details row needs to be
          * generated.
+         * <p>
+         * <em>Note:</em> If a component gets generated, it may not be manually
+         * attached anywhere, nor may it be a reused instance &ndash; each
+         * invocation of this method should produce a unique and isolated
+         * component instance.
          * 
          * @param rowReference
          *            the reference for the row for which to generate details
@@ -2809,6 +2817,208 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
         }
     }
 
+    /**
+     * A class that makes detail component related internal communication
+     * possible between {@link RpcDataProviderExtension} and grid.
+     * 
+     * @since
+     * @author Vaadin Ltd
+     */
+    public final class DetailComponentManager implements Serializable {
+        /**
+         * This map represents all the components that have been requested for
+         * each item id.
+         * <p>
+         * Normally this map is consistent with what is displayed in the
+         * component hierarchy (and thus the DOM). The only time this map is out
+         * of sync with the DOM is between the any calls to
+         * {@link #createDetails(Object, int)} or
+         * {@link #destroyDetails(Object)}, and
+         * {@link GridClientRpc#setDetailsConnectorChanges(Set)}.
+         * <p>
+         * This is easily checked: if {@link #unattachedComponents} is
+         * {@link Collection#isEmpty() empty}, then this field is consistent
+         * with the connector hierarchy.
+         */
+        private final Map<Object, Component> visibleDetailsComponents = new HashMap<Object, Component>();
+
+        /** 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 = new HashSet<Component>();
+
+        /**
+         * Creates a details component by the request of the client side, with
+         * the help of the user-defined {@link DetailsGenerator}.
+         * <p>
+         * Also keeps internal bookkeeping up to date.
+         * 
+         * @param itemId
+         *            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 {
+            assert itemId != null : "itemId was null";
+            Integer newRowIndex = Integer.valueOf(rowIndex);
+
+            assert !visibleDetailsComponents.containsKey(itemId) : "itemId already has a component. Should be destroyed first.";
+
+            RowReference rowReference = new RowReference(Grid.this);
+            rowReference.set(itemId);
+
+            Component details = getDetailsGenerator().getDetails(rowReference);
+            if (details != null) {
+
+                if (details.getParent() != null) {
+                    String generatorName = getDetailsGenerator().getClass()
+                            .getName();
+                    throw new IllegalStateException(generatorName
+                            + " generated a details component that already "
+                            + "was attached. (itemId: " + itemId + ", row: "
+                            + rowIndex + ", component: " + details);
+                }
+
+                if (rowIndexToDetails.containsValue(details)) {
+                    String generatorName = getDetailsGenerator().getClass()
+                            .getName();
+                    throw new IllegalStateException(generatorName
+                            + " provided a details component that already "
+                            + "exists in Grid. (itemId: " + itemId + ", row: "
+                            + rowIndex + ", component: " + details);
+                }
+
+                visibleDetailsComponents.put(itemId, details);
+                rowIndexToDetails.put(newRowIndex, details);
+                unattachedComponents.add(details);
+            }
+
+            /*
+             * Don't attach the components here. It's done by
+             * GridServerRpc.sendDetailsComponents in a separate roundtrip.
+             */
+        }
+
+        /**
+         * Destroys correctly a details component, by the request of the client
+         * side.
+         * <p>
+         * Also keeps internal bookkeeping up to date.
+         * 
+         * @param itemId
+         *            the item id for which to destroy the details component
+         */
+        public void destroyDetails(Object itemId) {
+            Component removedComponent = visibleDetailsComponents
+                    .remove(itemId);
+            if (removedComponent == null) {
+                return;
+            }
+
+            rowIndexToDetails.inverse().remove(removedComponent);
+
+            removedComponent.setParent(null);
+            markAsDirty();
+        }
+
+        /**
+         * Gets all details components that are currently attached to the grid.
+         * <p>
+         * Used internally by the Grid object.
+         * 
+         * @return all details components that are currently attached to the
+         *         grid
+         */
+        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
+         */
+        Set<ConnectorIndexChange> getAndResetConnectorChanges() {
+            Set<ConnectorIndexChange> changes = new HashSet<ConnectorIndexChange>();
+
+            // 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.this);
+                    unattachedComponents.remove(component);
+                }
+
+                if (!SharedUtil.equals(oldIndex, newIndex)) {
+                    changes.add(new ConnectorIndexChange(component, oldIndex,
+                            newIndex));
+                }
+            }
+
+            // 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 ConnectorIndexChange(null, oldIndex, null));
+                }
+            }
+
+            // reset diff map
+            prevRowIndexToDetails = HashBiMap.create(rowIndexToDetails);
+
+            return changes;
+        }
+    }
+
     /**
      * The data source attached to the grid
      */
@@ -2916,8 +3126,15 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
 
     private EditorErrorHandler editorErrorHandler = new DefaultEditorErrorHandler();
 
+    /**
+     * The user-defined details generator.
+     * 
+     * @see #setDetailsGenerator(DetailsGenerator)
+     */
     private DetailsGenerator detailsGenerator = DetailsGenerator.NULL;
 
+    private final DetailComponentManager detailComponentManager = new DetailComponentManager();
+
     private static final Method SELECTION_CHANGE_METHOD = ReflectTools
             .findMethod(SelectionListener.class, "select", SelectionEvent.class);
 
@@ -3118,6 +3335,13 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
                 fireEvent(new ItemClickEvent(Grid.this, item, itemId,
                         propertyId, details));
             }
+
+            @Override
+            public void sendDetailsComponents(int fetchId) {
+                getRpcProxy(GridClientRpc.class).setDetailsConnectorChanges(
+                        detailComponentManager.getAndResetConnectorChanges(),
+                        fetchId);
+            }
         });
 
         registerRpc(new EditorServerRpc() {
@@ -3278,7 +3502,8 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
             sortOrder.clear();
         }
 
-        datasourceExtension = new RpcDataProviderExtension(container);
+        datasourceExtension = new RpcDataProviderExtension(container,
+                detailComponentManager);
         datasourceExtension.extend(this, columnKeys);
 
         /*
@@ -4607,6 +4832,9 @@ public class Grid extends AbstractComponent implements SelectionNotifier,
         }
 
         componentList.addAll(getEditorFields());
+
+        componentList.addAll(detailComponentManager.getComponents());
+
         return componentList.iterator();
     }
 

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

@@ -47,7 +47,7 @@ public class DataProviderExtension {
         container = new IndexedContainer();
         populate(container);
 
-        dataProvider = new RpcDataProviderExtension(container);
+        dataProvider = new RpcDataProviderExtension(container, null);
         keyMapper = dataProvider.getKeyMapper();
     }
 

shared/src/com/vaadin/shared/ui/grid/ConnectorIndexChange.java

@@ -0,0 +1,143 @@
+/*
+ * 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.shared.ui.grid;
+
+import java.io.Serializable;
+
+import com.vaadin.shared.Connector;
+
+/**
+ * A description of an indexing modification for a connector. This is used by
+ * Grid by internal bookkeeping updates.
+ * 
+ * @since
+ * @author Vaadin Ltd
+ */
+public class ConnectorIndexChange implements Serializable {
+
+    private Connector connector;
+    private Integer oldIndex;
+    private Integer newIndex;
+
+    /** Create a new connector index change */
+    public ConnectorIndexChange() {
+    }
+
+    /**
+     * Convenience constructor for setting all the fields in one line.
+     * <p>
+     * Calling this constructor will also assert that the state of the pojo is
+     * consistent by internal assumptions.
+     * 
+     * @param connector
+     *            the changed connector
+     * @param oldIndex
+     *            the old index
+     * @param newIndex
+     *            the new index
+     */
+    public ConnectorIndexChange(Connector connector, Integer oldIndex,
+            Integer newIndex) {
+        this.connector = connector;
+        this.oldIndex = oldIndex;
+        this.newIndex = newIndex;
+
+        assert assertStateIsOk();
+    }
+
+    private boolean assertStateIsOk() {
+        assert (connector != null && newIndex != null)
+                || (connector == null && oldIndex != null && newIndex == null) : "connector: "
+                + nullityString(connector)
+                + ", oldIndex: "
+                + nullityString(oldIndex)
+                + ", newIndex: "
+                + nullityString(newIndex);
+        return true;
+    }
+
+    private static String nullityString(Object object) {
+        return object == null ? "null" : "non-null";
+    }
+
+    /**
+     * Gets the old index for the connector.
+     * <p>
+     * If <code>null</code>, the connector is recently added. This means that
+     * {@link #getConnector()} is expected not to return <code>null</code>.
+     * 
+     * @return the old index for the connector
+     */
+    public Integer getOldIndex() {
+        assert assertStateIsOk();
+        return oldIndex;
+    }
+
+    /**
+     * Gets the new index for the connector.
+     * <p>
+     * If <code>null</code>, the connector should be removed. This means that
+     * {@link #getConnector()} is expected to return <code>null</code> as well.
+     * 
+     * @return the new index for the connector
+     */
+    public Integer getNewIndex() {
+        assert assertStateIsOk();
+        return newIndex;
+    }
+
+    /**
+     * Gets the changed connector.
+     * 
+     * @return the changed connector. Might be <code>null</code>
+     */
+    public Connector getConnector() {
+        assert assertStateIsOk();
+        return connector;
+    }
+
+    /**
+     * Sets the changed connector.
+     * 
+     * @param connector
+     *            the changed connector. May be <code>null</code>
+     */
+    public void setConnector(Connector connector) {
+        this.connector = connector;
+    }
+
+    /**
+     * Sets the old index
+     * 
+     * @param oldIndex
+     *            the old index. May be <code>null</code> if a new connector is
+     *            being inserted
+     */
+    public void setOldIndex(Integer oldIndex) {
+        this.oldIndex = oldIndex;
+    }
+
+    /**
+     * Sets the new index
+     * 
+     * @param newIndex
+     *            the new index. May be <code>null</code> if a connector is
+     *            being removed
+     */
+    public void setNewIndex(Integer newIndex) {
+        this.newIndex = newIndex;
+    }
+}

shared/src/com/vaadin/shared/ui/grid/GridClientRpc.java

@@ -15,6 +15,8 @@
  */
 package com.vaadin.shared.ui.grid;
 
+import java.util.Set;
+
 import com.vaadin.shared.communication.ClientRpc;
 
 /**
@@ -55,4 +57,17 @@ public interface GridClientRpc extends ClientRpc {
      */
     public void recalculateColumnWidths();
 
+    /**
+     * Informs the GridConnector on how the indexing of details connectors has
+     * changed.
+     * 
+     * @since
+     * @param connectorChanges
+     *            the indexing changes of details connectors
+     * @param fetchId
+     *            the id of the request for fetching the changes
+     */
+    public void setDetailsConnectorChanges(
+            Set<ConnectorIndexChange> connectorChanges, int fetchId);
+
 }

shared/src/com/vaadin/shared/ui/grid/GridServerRpc.java

@@ -47,4 +47,21 @@ public interface GridServerRpc extends ServerRpc {
      *            mouse event details
      */
     void itemClick(String rowKey, String columnId, MouseEventDetails details);
+
+    /**
+     * 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
+     * @param fetchId
+     *            an unique identifier for the request
+     * @see com.vaadin.ui.Grid#setDetailsVisible(Object, boolean)
+     */
+    void sendDetailsComponents(int fetchId);
 }

uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicFeatures.java

@@ -46,6 +46,7 @@ import com.vaadin.tests.components.AbstractComponentTest;
 import com.vaadin.ui.Button;
 import com.vaadin.ui.Button.ClickEvent;
 import com.vaadin.ui.Button.ClickListener;
+import com.vaadin.ui.Component;
 import com.vaadin.ui.Grid;
 import com.vaadin.ui.Grid.CellReference;
 import com.vaadin.ui.Grid.CellStyleGenerator;
@@ -58,6 +59,8 @@ import com.vaadin.ui.Grid.RowReference;
 import com.vaadin.ui.Grid.RowStyleGenerator;
 import com.vaadin.ui.Grid.SelectionMode;
 import com.vaadin.ui.Grid.SelectionModel;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.Panel;
 import com.vaadin.ui.renderers.DateRenderer;
 import com.vaadin.ui.renderers.HtmlRenderer;
 import com.vaadin.ui.renderers.NumberRenderer;
@@ -109,6 +112,8 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
         }
     };
 
+    private Panel detailsPanel;
+
     @Override
     @SuppressWarnings("unchecked")
     protected Grid constructComponent() {
@@ -1054,6 +1059,64 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
     }
 
     private void createDetailsActions() {
+        createClickAction("custom details generator", "Details",
+                new Command<Grid, Void>() {
+                    @Override
+                    public void execute(Grid c, Void value, Object data) {
+                        grid.setDetailsGenerator(new Grid.DetailsGenerator() {
+                            private int seq = 0;
+
+                            @Override
+                            public Component getDetails(
+                                    RowReference rowReference) {
+                                return new Label("You are watching item id "
+                                        + rowReference.getItemId() + " ("
+                                        + (seq++) + ")");
+                            }
+                        });
+                    }
+                }, null);
+        createClickAction("hierarchy details generator", "Details",
+                new Command<Grid, Void>() {
+                    @Override
+                    public void execute(Grid c, Void value, Object data) {
+                        grid.setDetailsGenerator(new Grid.DetailsGenerator() {
+                            @Override
+                            public Component getDetails(
+                                    RowReference rowReference) {
+                                detailsPanel = new Panel();
+                                detailsPanel.setContent(new Label("One"));
+                                return detailsPanel;
+                            }
+                        });
+                    }
+                }, null);
+
+        createClickAction("change hierarchy in generator", "Details",
+                new Command<Grid, Void>() {
+                    @Override
+                    public void execute(Grid c, Void value, Object data) {
+                        Label label = (Label) detailsPanel.getContent();
+                        if (label.getValue().equals("One")) {
+                            detailsPanel.setContent(new Label("Two"));
+                        } else {
+                            detailsPanel.setContent(new Label("One"));
+                        }
+                    }
+                }, null);
+
+        createClickAction("toggle firstItemId", "Details",
+                new Command<Grid, Void>() {
+                    @Override
+                    public void execute(Grid g, Void value, Object data) {
+                        Object firstItemId = g.getContainerDataSource()
+                                .firstItemId();
+                        boolean toggle = g.isDetailsVisible(firstItemId);
+                        g.setDetailsVisible(firstItemId, !toggle);
+                        g.setDetailsVisible(firstItemId, toggle);
+                    }
+                }, null);
+
         createBooleanAction("firstItemId", "Details", false,
                 new Command<Grid, Boolean>() {
                     @Override
@@ -1063,6 +1126,17 @@ public class GridBasicFeatures extends AbstractComponentTest<Grid> {
                                 .firstItemId(), visible);
                     }
                 });
+
+        createBooleanAction("lastItemId-5", "Details", false,
+                new Command<Grid, Boolean>() {
+                    @Override
+                    @SuppressWarnings("boxing")
+                    public void execute(Grid g, Boolean visible, Object data) {
+                        Object fifthLastItemId = g.getContainerDataSource()
+                                .getItemIds(ROWS - 6, 1).get(0);
+                        g.setDetailsVisible(fifthLastItemId, visible);
+                    }
+                });
     }
 
     @Override

uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridDetailsServerTest.java

@@ -15,21 +15,43 @@
  */
 package com.vaadin.tests.components.grid.basicfeatures.server;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+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;
 
-import com.vaadin.testbench.annotations.RunLocally;
-import com.vaadin.testbench.parallel.Browser;
+import com.vaadin.testbench.TestBenchElement;
+import com.vaadin.tests.components.grid.basicfeatures.GridBasicFeatures;
 import com.vaadin.tests.components.grid.basicfeatures.GridBasicFeaturesTest;
 
-@RunLocally(Browser.PHANTOMJS)
 public class GridDetailsServerTest extends GridBasicFeaturesTest {
+    /**
+     * The reason to why last item details wasn't selected is that since it will
+     * exist only after the viewport has been scrolled into view, we wouldn't be
+     * able to scroll that particular details row into view, making tests
+     * awkward with two scroll commands back to back.
+     */
+    private static final int ALMOST_LAST_ITEM_INDEX = GridBasicFeatures.ROWS - 5;
+    private static final String[] ALMOST_LAST_ITEM_DETAILS = new String[] {
+            "Component", "Details", "lastItemId-5" };
+
     private static final String[] FIRST_ITEM_DETAILS = new String[] {
             "Component", "Details", "firstItemId" };
+    private static final String[] TOGGLE_FIRST_ITEM_DETAILS = new String[] {
+            "Component", "Details", "toggle firstItemId" };
+    private static final String[] CUSTOM_DETAILS_GENERATOR = new String[] {
+            "Component", "Details", "custom details generator" };
+    private static final String[] HIERARCHY_DETAILS_GENERATOR = new String[] {
+            "Component", "Details", "hierarchy details generator" };
+    private static final String[] CHANGE_HIERARCHY = new String[] {
+            "Component", "Details", "change hierarchy in generator" };
 
     @Before
     public void setUp() {
@@ -53,7 +75,9 @@ public class GridDetailsServerTest extends GridBasicFeaturesTest {
     public void closeVisibleDetails() {
         selectMenuPath(FIRST_ITEM_DETAILS);
         selectMenuPath(FIRST_ITEM_DETAILS);
-        getGridElement().getDetails(0);
+
+        // this will throw before assertNull
+        assertNull(getGridElement().getDetails(0));
     }
 
     @Test
@@ -73,4 +97,72 @@ public class GridDetailsServerTest extends GridBasicFeaturesTest {
         getGridElement().scroll(0);
         getGridElement().getDetails(0);
     }
+
+    @Test
+    public void componentIsVisibleClientSide() {
+        selectMenuPath(CUSTOM_DETAILS_GENERATOR);
+        selectMenuPath(FIRST_ITEM_DETAILS);
+
+        TestBenchElement details = getGridElement().getDetails(0);
+        assertNotNull("No widget detected inside details",
+                details.findElement(By.className("v-widget")));
+    }
+
+    @Test
+    public void togglingAVisibleDetailsRowWithSeparateRoundtrips() {
+        selectMenuPath(CUSTOM_DETAILS_GENERATOR);
+        selectMenuPath(FIRST_ITEM_DETAILS); // open
+        selectMenuPath(FIRST_ITEM_DETAILS); // close
+        selectMenuPath(FIRST_ITEM_DETAILS); // open
+
+        TestBenchElement details = getGridElement().getDetails(0);
+        assertNotNull("No widget detected inside details",
+                details.findElement(By.className("v-widget")));
+    }
+
+    @Test
+    public void togglingAVisibleDetailsRowWithOneRoundtrip() {
+        selectMenuPath(CUSTOM_DETAILS_GENERATOR);
+        selectMenuPath(FIRST_ITEM_DETAILS); // open
+
+        assertTrue("Unexpected generator content",
+                getGridElement().getDetails(0).getText().endsWith("(0)"));
+        selectMenuPath(TOGGLE_FIRST_ITEM_DETAILS);
+        assertTrue("New component was not displayed in the client",
+                getGridElement().getDetails(0).getText().endsWith("(1)"));
+    }
+
+    @Test
+    @Ignore("This will be patched with https://dev.vaadin.com/review/#/c/7917/")
+    public void almosLastItemIdIsRendered() {
+        selectMenuPath(CUSTOM_DETAILS_GENERATOR);
+        selectMenuPath(ALMOST_LAST_ITEM_DETAILS);
+        scrollGridVerticallyTo(100000);
+
+        TestBenchElement details = getGridElement().getDetails(
+                ALMOST_LAST_ITEM_INDEX);
+        assertNotNull(details);
+        assertTrue("Unexpected details content",
+                details.getText().endsWith(ALMOST_LAST_ITEM_INDEX + " (0)"));
+    }
+
+    @Test
+    public void hierarchyChangesWorkInDetails() {
+        selectMenuPath(HIERARCHY_DETAILS_GENERATOR);
+        selectMenuPath(FIRST_ITEM_DETAILS);
+        assertEquals("One", getGridElement().getDetails(0).getText());
+        selectMenuPath(CHANGE_HIERARCHY);
+        assertEquals("Two", getGridElement().getDetails(0).getText());
+    }
+
+    @Test
+    @Ignore("This will be patched with https://dev.vaadin.com/review/#/c/7917/")
+    public void hierarchyChangesWorkInDetailsWhileOutOfView() {
+        selectMenuPath(HIERARCHY_DETAILS_GENERATOR);
+        selectMenuPath(FIRST_ITEM_DETAILS);
+        scrollGridVerticallyTo(10000);
+        selectMenuPath(CHANGE_HIERARCHY);
+        scrollGridVerticallyTo(0);
+        assertEquals("Two", getGridElement().getDetails(0).getText());
+    }
 }