Merge branch 'master-18493' into grid-unbuffered-editor

Conflicts:
	client/src/com/vaadin/client/connectors/GridConnector.java
	server/src/com/vaadin/data/RpcDataProviderExtension.java
	server/src/com/vaadin/ui/Grid.java

Change-Id: Ie8931fbae322c93aeb409e8a6d393623ba9d9dc6

1 files changed, 43 insertions, 219 deletions

diff --git a/server/src/com/vaadin/data/RpcDataProviderExtension.java b/server/src/com/vaadin/data/RpcDataProviderExtension.java
index 55947c98d1..d81d024d72 100644
--- a/server/src/com/vaadin/data/RpcDataProviderExtension.java
+++ b/server/src/com/vaadin/data/RpcDataProviderExtension.java
@@ -24,7 +24,6 @@ import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
 
 import com.google.gwt.thirdparty.guava.common.collect.BiMap;
@@ -45,11 +44,9 @@ import com.vaadin.server.AbstractExtension;
 import com.vaadin.server.ClientConnector;
 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.Column;
@@ -111,15 +108,13 @@ 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 (detailComponentManager.visibleDetails.contains(itemId)) {
-                    detailComponentManager.createDetails(itemId,
-                            indexOf(itemId));
+                if (visibleDetails.contains(itemId)) {
+                    detailComponentManager.createDetails(itemId);
                 }
             }
         }
@@ -603,7 +598,8 @@ public class RpcDataProviderExtension extends AbstractExtension {
      * @since 7.5.0
      * @author Vaadin Ltd
      */
-    public static final class DetailComponentManager implements DataGenerator {
+    // TODO this should probably be a static nested class
+    public final class DetailComponentManager implements DataGenerator {
         /**
          * This map represents all the components that have been requested for
          * each item id.
@@ -611,9 +607,8 @@ public class RpcDataProviderExtension extends AbstractExtension {
          * 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)}.
+         * {@link #createDetails(Object)} 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
@@ -622,40 +617,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();
-
-        /**
-         * This map represents all the details that are user-defined as visible.
-         * This does not reflect the status in the DOM.
+         * {@link #createDetails(Object)}.
          */
-        private Set<Object> visibleDetails = new HashSet<Object>();
+        private final Set<Object> emptyDetails = Sets.newHashSet();
 
         private Grid grid;
 
@@ -669,19 +635,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;
             }
@@ -692,58 +655,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;
         }
 
         /**
@@ -764,8 +695,6 @@ public class RpcDataProviderExtension extends AbstractExtension {
                 return;
             }
 
-            rowIndexToDetails.inverse().remove(removedComponent);
-
             removedComponent.setParent(null);
             grid.markAsDirty();
         }
@@ -781,81 +710,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) {
@@ -947,17 +807,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();
@@ -982,6 +838,13 @@ public class RpcDataProviderExtension extends AbstractExtension {
     /** Size possibly changed with a bare ItemSetChangeEvent */
     private boolean bareItemSetTriggeredSizeChange = false;
 
+    /**
+     * This map represents all the details that are user-defined as visible.
+     * This does not reflect the status in the DOM.
+     */
+    // TODO this should probably be inside DetailComponentManager
+    private Set<Object> visibleDetails = new HashSet<Object>();
+
     private final DetailComponentManager detailComponentManager = new DetailComponentManager();
 
     private Set<DataGenerator> dataGenerators = new LinkedHashSet<DataGenerator>();
@@ -1233,15 +1096,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);
-            }
         }
     }
 
@@ -1318,37 +1177,21 @@ public class RpcDataProviderExtension extends AbstractExtension {
      *            hide
      */
     public void setDetailsVisible(Object itemId, boolean visible) {
-        final boolean modified;
-
         if (visible) {
-            modified = detailComponentManager.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 = detailComponentManager.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);
     }
 
     /**
@@ -1362,7 +1205,7 @@ public class RpcDataProviderExtension extends AbstractExtension {
      *         visible in the DOM
      */
     public boolean isDetailsVisible(Object itemId) {
-        return detailComponentManager.visibleDetails.contains(itemId);
+        return visibleDetails.contains(itemId);
     }
 
     /**
@@ -1371,21 +1214,12 @@ public class RpcDataProviderExtension extends AbstractExtension {
      * @since 7.5.0
      */
     public void refreshDetails() {
-        for (Object itemId : ImmutableSet
-                .copyOf(detailComponentManager.visibleDetails)) {
+        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
      * 
@@ -1395,14 +1229,4 @@ public class RpcDataProviderExtension extends AbstractExtension {
     public DetailComponentManager getDetailComponentManager() {
         return detailComponentManager;
     }
-
-    @Override
-    public void detach() {
-        for (Object itemId : ImmutableSet
-                .copyOf(detailComponentManager.visibleDetails)) {
-            detailComponentManager.destroyDetails(itemId);
-        }
-
-        super.detach();
-    }
 }