Improvements to detail row index handling. (#11345)

- Escalator should notify when an existing details row is moved to a new
index.
- Grid and DetailsManagerConnector should update their internal indexing
when details manager index changes in Escalator.

6 files changed, 212 insertions, 11 deletions

diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java
index d2de20f266..cddaf66719 100644
--- a/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java
@@ -17,6 +17,7 @@ package com.vaadin.client.connectors.grid;
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.TreeMap;
 
 import com.google.gwt.core.client.Scheduler;
 import com.google.gwt.core.client.Scheduler.ScheduledCommand;
@@ -31,6 +32,8 @@ import com.vaadin.client.WidgetUtil;
 import com.vaadin.client.data.DataChangeHandler;
 import com.vaadin.client.extensions.AbstractExtensionConnector;
 import com.vaadin.client.ui.layout.ElementResizeListener;
+import com.vaadin.client.widget.escalator.events.SpacerIndexChangedEvent;
+import com.vaadin.client.widget.escalator.events.SpacerIndexChangedHandler;
 import com.vaadin.client.widget.grid.HeightAwareDetailsGenerator;
 import com.vaadin.client.widgets.Grid;
 import com.vaadin.shared.Registration;
@@ -51,11 +54,13 @@ import elemental.json.JsonObject;
 public class DetailsManagerConnector extends AbstractExtensionConnector {
 
     /* Map for tracking which details are open on which row */
-    private Map<Integer, String> indexToDetailConnectorId = new HashMap<>();
+    private TreeMap<Integer, String> indexToDetailConnectorId = new TreeMap<>();
     /* Boolean flag to avoid multiple refreshes */
     private boolean refreshing;
-    /* Registration for data change handler. */
+    /* For listening data changes that originate from DataSource. */
     private Registration dataChangeRegistration;
+    /* For listening spacer index changes that originate from Escalator. */
+    private HandlerRegistration spacerIndexChangedHandlerRegistration;
 
     /**
      * Handle for the spacer visibility change handler.
@@ -187,6 +192,20 @@ public class DetailsManagerConnector extends AbstractExtensionConnector {
     @Override
     protected void extend(ServerConnector target) {
         getWidget().setDetailsGenerator(new CustomDetailsGenerator());
+        spacerIndexChangedHandlerRegistration = getWidget()
+                .addSpacerIndexChangedHandler(new SpacerIndexChangedHandler() {
+                    @Override
+                    public void onSpacerIndexChanged(
+                            SpacerIndexChangedEvent event) {
+                        // Move spacer from old index to new index. Escalator is
+                        // responsible for making sure the new index doesn't
+                        // already contain a spacer.
+                        String connectorId = indexToDetailConnectorId
+                                .remove(event.getOldIndex());
+                        indexToDetailConnectorId.put(event.getNewIndex(),
+                                connectorId);
+                    }
+                });
         dataChangeRegistration = getWidget().getDataSource()
                 .addDataChangeHandler(new DetailsChangeHandler());
 
@@ -238,6 +257,7 @@ public class DetailsManagerConnector extends AbstractExtensionConnector {
         dataChangeRegistration = null;
 
         spacerVisibilityChangeRegistration.removeHandler();
+        spacerIndexChangedHandlerRegistration.removeHandler();
 
         indexToDetailConnectorId.clear();
     }
diff --git a/client/src/main/java/com/vaadin/client/widget/escalator/RowContainer.java b/client/src/main/java/com/vaadin/client/widget/escalator/RowContainer.java
index 577b2c0877..3d60d21d2f 100644
--- a/client/src/main/java/com/vaadin/client/widget/escalator/RowContainer.java
+++ b/client/src/main/java/com/vaadin/client/widget/escalator/RowContainer.java
@@ -75,6 +75,17 @@ public interface RowContainer {
                 throws IllegalArgumentException;
 
         /**
+         * Checks whether the given rowIndex contains a spacer.
+         *
+         * @param rowIndex
+         *            the row index for the queried spacer.
+         * @return {@code true} if spacer for given row index exists,
+         *         {@code false} otherwise
+         * @since
+         */
+        boolean spacerExists(int rowIndex);
+
+        /**
          * Sets a new spacer updater.
          * <p>
          * Spacers that are currently visible will be updated, i.e.
diff --git a/client/src/main/java/com/vaadin/client/widget/escalator/events/SpacerIndexChangedEvent.java b/client/src/main/java/com/vaadin/client/widget/escalator/events/SpacerIndexChangedEvent.java
new file mode 100644
index 0000000000..f7a8df507c
--- /dev/null
+++ b/client/src/main/java/com/vaadin/client/widget/escalator/events/SpacerIndexChangedEvent.java
@@ -0,0 +1,82 @@
+/*
+ * Copyright 2000-2018 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.client.widget.escalator.events;
+
+import com.google.gwt.event.shared.GwtEvent;
+
+/**
+ * Event fired when a spacer element is moved to a new index in Escalator.
+ *
+ * @author Vaadin Ltd
+ * @since
+ */
+public class SpacerIndexChangedEvent
+        extends GwtEvent<SpacerIndexChangedHandler> {
+
+    /**
+     * Handler type.
+     */
+    public static final Type<SpacerIndexChangedHandler> TYPE = new Type<>();
+
+    public static final Type<SpacerIndexChangedHandler> getType() {
+        return TYPE;
+    }
+
+    private final int oldIndex;
+    private final int newIndex;
+
+    /**
+     * Creates a spacer index changed event.
+     *
+     * @param oldIndex
+     *            old index of row to which the spacer belongs
+     * @param newIndex
+     *            new index of row to which the spacer belongs
+     */
+    public SpacerIndexChangedEvent(int oldIndex, int newIndex) {
+        this.oldIndex = oldIndex;
+        this.newIndex = newIndex;
+    }
+
+    /**
+     * Gets the old row index to which the spacer element belongs.
+     *
+     * @return the old row index to which the spacer element belongs
+     */
+    public int getOldIndex() {
+        return oldIndex;
+    }
+
+    /**
+     * Gets the new row index to which the spacer element belongs.
+     *
+     * @return the new row index to which the spacer element belongs
+     */
+    public int getNewIndex() {
+        return newIndex;
+    }
+
+    @Override
+    public Type<SpacerIndexChangedHandler> getAssociatedType() {
+        return TYPE;
+    }
+
+    @Override
+    protected void dispatch(SpacerIndexChangedHandler handler) {
+        handler.onSpacerIndexChanged(this);
+    }
+
+}
diff --git a/client/src/main/java/com/vaadin/client/widget/escalator/events/SpacerIndexChangedHandler.java b/client/src/main/java/com/vaadin/client/widget/escalator/events/SpacerIndexChangedHandler.java
new file mode 100644
index 0000000000..fd5bc5e167
--- /dev/null
+++ b/client/src/main/java/com/vaadin/client/widget/escalator/events/SpacerIndexChangedHandler.java
@@ -0,0 +1,36 @@
+/*
+ * Copyright 2000-2018 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.client.widget.escalator.events;
+
+import com.google.gwt.event.shared.EventHandler;
+
+/**
+ * Event handler for a spacer index changed event.
+ *
+ * @author Vaadin Ltd
+ * @since
+ */
+public interface SpacerIndexChangedHandler extends EventHandler {
+
+    /**
+     * Called when a spacer index changed event is fired, when a spacer's index
+     * changes.
+     *
+     * @param event
+     *            the spacer index changed event
+     */
+    public void onSpacerIndexChanged(SpacerIndexChangedEvent event);
+}
diff --git a/client/src/main/java/com/vaadin/client/widgets/Escalator.java b/client/src/main/java/com/vaadin/client/widgets/Escalator.java
index 8df0ace3d8..d926e0e2be 100644
--- a/client/src/main/java/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/main/java/com/vaadin/client/widgets/Escalator.java
@@ -90,6 +90,7 @@ import com.vaadin.client.widget.escalator.ScrollbarBundle.VerticalScrollbarBundl
 import com.vaadin.client.widget.escalator.Spacer;
 import com.vaadin.client.widget.escalator.SpacerUpdater;
 import com.vaadin.client.widget.escalator.events.RowHeightChangedEvent;
+import com.vaadin.client.widget.escalator.events.SpacerIndexChangedEvent;
 import com.vaadin.client.widget.escalator.events.SpacerVisibilityChangedEvent;
 import com.vaadin.client.widget.grid.events.ScrollEvent;
 import com.vaadin.client.widget.grid.events.ScrollHandler;
@@ -4125,6 +4126,11 @@ public class Escalator extends Widget
         }
 
         @Override
+        public boolean spacerExists(int rowIndex) {
+            return spacerContainer.spacerExists(rowIndex);
+        }
+
+        @Override
         public void setSpacerUpdater(SpacerUpdater spacerUpdater)
                 throws IllegalArgumentException {
             spacerContainer.setSpacerUpdater(spacerUpdater);
@@ -4972,16 +4978,19 @@ public class Escalator extends Widget
             }
 
             /**
-             * Sets a new row index for this spacer. Also updates the bookeeping
-             * at {@link SpacerContainer#rowIndexToSpacer}.
+             * Sets a new row index for this spacer. Also updates the
+             * bookkeeping at {@link SpacerContainer#rowIndexToSpacer}.
              */
             @SuppressWarnings("boxing")
             public void setRowIndex(int rowIndex) {
                 SpacerImpl spacer = rowIndexToSpacer.remove(this.rowIndex);
                 assert this == spacer : "trying to move an unexpected spacer.";
+                int oldIndex = this.rowIndex;
                 this.rowIndex = rowIndex;
                 root.setPropertyInt(SPACER_LOGICAL_ROW_PROPERTY, rowIndex);
                 rowIndexToSpacer.put(this.rowIndex, this);
+
+                fireEvent(new SpacerIndexChangedEvent(oldIndex, this.rowIndex));
             }
 
             /**
diff --git a/client/src/main/java/com/vaadin/client/widgets/Grid.java b/client/src/main/java/com/vaadin/client/widgets/Grid.java
index 6537b8642c..9f82d3339f 100755
--- a/client/src/main/java/com/vaadin/client/widgets/Grid.java
+++ b/client/src/main/java/com/vaadin/client/widgets/Grid.java
@@ -76,7 +76,11 @@ import com.google.gwt.user.client.ui.MenuItem;
 import com.google.gwt.user.client.ui.PopupPanel;
 import com.google.gwt.user.client.ui.ResizeComposite;
 import com.google.gwt.user.client.ui.Widget;
-import com.vaadin.client.*;
+import com.vaadin.client.BrowserInfo;
+import com.vaadin.client.ComputedStyle;
+import com.vaadin.client.DeferredWorker;
+import com.vaadin.client.Focusable;
+import com.vaadin.client.WidgetUtil;
 import com.vaadin.client.WidgetUtil.Reference;
 import com.vaadin.client.data.DataChangeHandler;
 import com.vaadin.client.data.DataSource;
@@ -102,6 +106,8 @@ import com.vaadin.client.widget.escalator.Spacer;
 import com.vaadin.client.widget.escalator.SpacerUpdater;
 import com.vaadin.client.widget.escalator.events.RowHeightChangedEvent;
 import com.vaadin.client.widget.escalator.events.RowHeightChangedHandler;
+import com.vaadin.client.widget.escalator.events.SpacerIndexChangedEvent;
+import com.vaadin.client.widget.escalator.events.SpacerIndexChangedHandler;
 import com.vaadin.client.widget.escalator.events.SpacerVisibilityChangedEvent;
 import com.vaadin.client.widget.escalator.events.SpacerVisibilityChangedHandler;
 import com.vaadin.client.widget.grid.AutoScroller;
@@ -6365,6 +6371,15 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
             }
         });
 
+        addSpacerIndexChangedHandler(new SpacerIndexChangedHandler() {
+            @Override
+            public void onSpacerIndexChanged(SpacerIndexChangedEvent event) {
+                // remove old index and add new index
+                visibleDetails.remove(event.getOldIndex());
+                visibleDetails.add(event.getNewIndex());
+            }
+        });
+
         // Sink header events and key events
         sinkEvents(getHeader().getConsumedEvents());
         sinkEvents(Arrays.asList(BrowserEvents.KEYDOWN, BrowserEvents.KEYUP,
@@ -8758,6 +8773,19 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
     }
 
     /**
+     * Adds a spacer index changed handler to the underlying escalator.
+     *
+     * @param handler
+     *            the handler to be called when a spacer's index changes
+     * @return the registration object with which the handler can be removed
+     * @since
+     */
+    public HandlerRegistration addSpacerIndexChangedHandler(
+            SpacerIndexChangedHandler handler) {
+        return escalator.addHandler(handler, SpacerIndexChangedEvent.TYPE);
+    }
+
+    /**
      * Adds a low-level DOM event handler to this Grid. The handler is inserted
      * into the given position in the list of handlers. The handlers are invoked
      * in order. If the
@@ -9419,15 +9447,30 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
          * wrong.
          *
          * see GridSpacerUpdater.init for implementation details.
+         *
+         * The order of operations isn't entirely stable. Sometimes Escalator
+         * knows about the spacer visibility updates first and doesn't need
+         * updating again but Grid's visibleDetails set still does.
          */
 
         boolean isVisible = isDetailsVisible(rowIndex);
-        if (visible && !isVisible) {
-            escalator.getBody().setSpacer(rowIndex, DETAILS_ROW_INITIAL_HEIGHT);
-            visibleDetails.add(rowIndexInteger);
-        } else if (!visible && isVisible) {
-            escalator.getBody().setSpacer(rowIndex, -1);
-            visibleDetails.remove(rowIndexInteger);
+        boolean isVisibleInEscalator = escalator.getBody()
+                .spacerExists(rowIndex);
+        if (visible) {
+            if (!isVisibleInEscalator) {
+                escalator.getBody().setSpacer(rowIndex,
+                        DETAILS_ROW_INITIAL_HEIGHT);
+            }
+            if (!isVisible) {
+                visibleDetails.add(rowIndexInteger);
+            }
+        } else {
+            if (isVisibleInEscalator) {
+                escalator.getBody().setSpacer(rowIndex, -1);
+            }
+            if (isVisible) {
+                visibleDetails.remove(rowIndexInteger);
+            }
         }
     }