Speeds up column adding in Grid (#16474)

Grid.onStateChange is now about 40% faster when adding columns, and setting several column widths has now way less overhead. Change-Id: I7bd900324207bfb2543a1a90390665b90206aefd
vaadin · Feb 4, 2015 · 7cffb15 · 7cffb15
1 parent 103b177
commit 7cffb15
Show file tree

Hide file tree

Showing 4 changed files with 128 additions and 119 deletions.
diff --git a/client/src/com/vaadin/client/widget/escalator/ColumnConfiguration.java b/client/src/com/vaadin/client/widget/escalator/ColumnConfiguration.java
@@ -16,7 +16,7 @@
 
 package com.vaadin.client.widget.escalator;
 
-import com.vaadin.client.widgets.Escalator;
+import java.util.Map;
 
 /**
  * A representation of the columns in an instance of {@link Escalator}.
@@ -140,6 +140,23 @@ public void setColumnWidth(int index, double px)
      */
     public double getColumnWidth(int index) throws IllegalArgumentException;
 
+    /**
+     * Sets widths for a set of columns.
+     * 
+     * @param indexWidthMap
+     *            a map from column index to its respective width to be set. If
+     *            the given width for a column index is negative, the column is
+     *            resized-to-fit.
+     * @throws IllegalArgumentException
+     *             if {@code indexWidthMap} is {@code null}
+     * @throws IllegalArgumentException
+     *             if any column index in {@code indexWidthMap} is invalid
+     * @throws NullPointerException
+     *             If any value in the map is <code>null</code>
+     */
+    public void setColumnWidths(Map<Integer, Double> indexWidthMap)
+            throws IllegalArgumentException;
+
     /**
      * Returns the actual width of a column.
      * 

diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java
@@ -16,11 +16,13 @@
 package com.vaadin.client.widgets;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.ListIterator;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -1161,47 +1163,6 @@ public void scrollToRow(final int rowIndex,
         }
     }
 
-    private class ColumnAutoWidthAssignScheduler {
-        private boolean isScheduled = false;
-        private final ScheduledCommand widthCommand = new ScheduledCommand() {
-            @Override
-            public void execute() {
-                if (!isScheduled) {
-                    return;
-                }
-
-                isScheduled = false;
-
-                ColumnConfigurationImpl cc = columnConfiguration;
-                for (int col = 0; col < cc.getColumnCount(); col++) {
-                    ColumnConfigurationImpl.Column column = cc.columns.get(col);
-                    if (!column.isWidthFinalized()) {
-                        cc.setColumnWidth(col, -1);
-                        column.widthIsFinalized();
-                    }
-                }
-            }
-        };
-
-        /**
-         * Calculates the widths of all uncalculated cells once the javascript
-         * execution is done.
-         * <p>
-         * This method makes sure that any duplicate requests in the same cycle
-         * are ignored.
-         */
-        public void reschedule() {
-            if (!isScheduled) {
-                isScheduled = true;
-                Scheduler.get().scheduleFinally(widthCommand);
-            }
-        }
-
-        public void cancel() {
-            isScheduled = false;
-        }
-    }
-
     protected abstract class AbstractRowContainer implements RowContainer {
         private EscalatorUpdater updater = EscalatorUpdater.NULL;
 
@@ -1422,14 +1383,18 @@ public void insertRows(final int index, final int numberOfRows) {
                 if (rows == numberOfRows) {
                     /*
                      * We are inserting the first rows in this container. We
-                     * potentially need to autocalculate the widths for the
+                     * potentially need to set the widths for the cells for the
-                     * cells for the first time.
+                     * first time.
-                     * 
-                     * To make sure that can take the entire dataset into
-                     * account, we'll do this deferredly, so that each container
-                     * section gets populated before we start calculating.
                      */
-                    columnAutoWidthAssignScheduler.reschedule();
+                    Map<Integer, Double> colWidths = new HashMap<Integer, Double>();
+                    Double width = Double
+                            .valueOf(ColumnConfigurationImpl.Column.DEFAULT_COLUMN_WIDTH_PX);
+                    for (int i = 0; i < getColumnConfiguration()
+                            .getColumnCount(); i++) {
+                        Integer col = Integer.valueOf(i);
+                        colWidths.put(col, width);
+                    }
+                    getColumnConfiguration().setColumnWidths(colWidths);
                 }
             }
         }
@@ -3808,19 +3773,12 @@ public Cell getCell(Element element) {
 
     private class ColumnConfigurationImpl implements ColumnConfiguration {
         public class Column {
-            private static final int DEFAULT_COLUMN_WIDTH_PX = 100;
+            public static final double DEFAULT_COLUMN_WIDTH_PX = 100;
 
             private double definedWidth = -1;
             private double calculatedWidth = DEFAULT_COLUMN_WIDTH_PX;
             private boolean measuringRequested = false;
 
-            /**
-             * If a column has been created (either via insertRow or
-             * insertColumn), it will be given an arbitrary width, and only then
-             * a width will be defined.
-             */
-            private boolean widthHasBeenFinalized = false;
-
             public void setWidth(double px) {
                 definedWidth = px;
 
@@ -3884,15 +3842,6 @@ public boolean measureAndSetWidthIfNeeded() {
             private void calculateWidth() {
                 calculatedWidth = getMaxCellWidth(columns.indexOf(this));
             }
-
-            public void widthIsFinalized() {
-                columnAutoWidthAssignScheduler.cancel();
-                widthHasBeenFinalized = true;
-            }
-
-            public boolean isWidthFinalized() {
-                return widthHasBeenFinalized;
-            }
         }
 
         private final List<Column> columns = new ArrayList<Column>();
@@ -4082,13 +4031,17 @@ public void insertColumns(final int index, final int numberOfColumns) {
             body.paintInsertColumns(index, numberOfColumns, frozen);
             footer.paintInsertColumns(index, numberOfColumns, frozen);
 
-            // fix autowidth
+            // fix initial width
             if (header.getRowCount() > 0 || body.getRowCount() > 0
                     || footer.getRowCount() > 0) {
-                for (int col = index; col < index + numberOfColumns; col++) {
+
-                    getColumnConfiguration().setColumnWidth(col, -1);
+                Map<Integer, Double> colWidths = new HashMap<Integer, Double>();
-                    columnConfiguration.columns.get(col).widthIsFinalized();
+                Double width = Double.valueOf(Column.DEFAULT_COLUMN_WIDTH_PX);
+                for (int i = index; i < index + numberOfColumns; i++) {
+                    Integer col = Integer.valueOf(i);
+                    colWidths.put(col, width);
                 }
+                getColumnConfiguration().setColumnWidths(colWidths);
             }
 
             // Adjust scrollbar
@@ -4170,16 +4123,30 @@ public int getFrozenColumnCount() {
         @Override
         public void setColumnWidth(int index, double px)
                 throws IllegalArgumentException {
-            checkValidColumnIndex(index);
+            setColumnWidths(Collections.singletonMap(Integer.valueOf(index),
+                    Double.valueOf(px)));
+        }
 
-            columns.get(index).setWidth(px);
+        @Override
-            columns.get(index).widthIsFinalized();
+        public void setColumnWidths(Map<Integer, Double> indexWidthMap)
-            widthsArray = null;
+                throws IllegalArgumentException {
 
-            /*
+            if (indexWidthMap == null) {
-             * TODO [[optimize]]: only modify the elements that are actually
+                throw new IllegalArgumentException("indexWidthMap was null");
-             * modified.
+            }
-             */
+
+            if (indexWidthMap.isEmpty()) {
+                return;
+            }
+
+            for (Entry<Integer, Double> entry : indexWidthMap.entrySet()) {
+                int index = entry.getKey().intValue();
+                double width = entry.getValue().doubleValue();
+                checkValidColumnIndex(index);
+                columns.get(index).setWidth(width);
+            }
+
+            widthsArray = null;
             header.reapplyColumnWidths();
             body.reapplyColumnWidths();
             footer.reapplyColumnWidths();
@@ -4373,8 +4340,6 @@ public void execute() {
         }
     };
 
-    private final ColumnAutoWidthAssignScheduler columnAutoWidthAssignScheduler = new ColumnAutoWidthAssignScheduler();
-
     /**
      * Creates a new Escalator widget instance.
      */
@@ -5144,9 +5109,7 @@ public HandlerRegistration addScrollHandler(ScrollHandler handler) {
 
     @Override
     public boolean isWorkPending() {
-        return body.domSorter.waiting
+        return body.domSorter.waiting || verticalScrollbar.isWorkPending()
-                || columnAutoWidthAssignScheduler.isScheduled
-                || verticalScrollbar.isWorkPending()
                 || horizontalScrollbar.isWorkPending();
     }
 

diff --git a/client/src/com/vaadin/client/widgets/Grid.java b/client/src/com/vaadin/client/widgets/Grid.java
@@ -22,6 +22,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -2217,21 +2218,67 @@ private void calculate() {
             rescheduleCount = 0;
 
             assert !dataIsBeingFetched : "Trying to calculate column widths even though data is still being fetched.";
-            /*
+
-             * At this point we assume that no data is being fetched anymore.
+            if (columnsAreGuaranteedToBeWiderThanGrid()) {
-             * Everything's rendered in the DOM. Now we just make sure
+                applyColumnWidths();
-             * everything fits as it should.
+            } else {
-             */
+                applyColumnWidthsWithExpansion();
+            }
+        }
+
+        private boolean columnsAreGuaranteedToBeWiderThanGrid() {
+            double freeSpace = escalator.getInnerWidth();
+            for (Column<?, ?> column : getColumns()) {
+                if (column.getWidth() >= 0) {
+                    freeSpace -= column.getWidth();
+                } else if (column.getMinimumWidth() >= 0) {
+                    freeSpace -= column.getMinimumWidth();
+                }
+            }
+            return freeSpace < 0;
+        }
+
+        @SuppressWarnings("boxing")
+        private void applyColumnWidths() {
+
+            /* Step 1: Apply all column widths as they are. */
+
+            Map<Integer, Double> selfWidths = new LinkedHashMap<Integer, Double>();
+            List<Column<?, T>> columns = getColumns();
+            for (int index = 0; index < columns.size(); index++) {
+                selfWidths.put(index, columns.get(index).getWidth());
+            }
+            Grid.this.escalator.getColumnConfiguration().setColumnWidths(
+                    selfWidths);
 
             /*
-             * Quick optimization: if the sum of fixed widths and minimum widths
+             * Step 2: Make sure that each column ends up obeying their min/max
-             * is greater than the grid can display, we already know that things
+             * width constraints if defined as autowidth. If constraints are
-             * will be squeezed and no expansion will happen.
+             * violated, fix it.
              */
-            if (gridWasTooNarrowAndEverythingWasFixedAlready()) {
+
-                return;
+            Map<Integer, Double> constrainedWidths = new LinkedHashMap<Integer, Double>();
+            for (int index = 0; index < columns.size(); index++) {
+                Column<?, T> column = columns.get(index);
+
+                boolean hasAutoWidth = column.getWidth() < 0;
+                if (!hasAutoWidth) {
+                    continue;
+                }
+
+                // TODO: bug: these don't honor the CSS max/min. :(
+                double actualWidth = column.getWidthActual();
+                if (actualWidth < getMinWidth(column)) {
+                    constrainedWidths.put(index, column.getMinimumWidth());
+                } else if (actualWidth > getMaxWidth(column)) {
+                    constrainedWidths.put(index, column.getMaximumWidth());
+                }
             }
+            Grid.this.escalator.getColumnConfiguration().setColumnWidths(
+                    constrainedWidths);
+        }
 
+        private void applyColumnWidthsWithExpansion() {
             boolean someColumnExpands = false;
             int totalRatios = 0;
             double reservedPixels = 0;
@@ -2416,32 +2463,6 @@ private void calculate() {
             } while (minWidthsCausedReflows);
         }
 
-        private boolean gridWasTooNarrowAndEverythingWasFixedAlready() {
-            double freeSpace = escalator.getInnerWidth();
-            for (Column<?, ?> column : getColumns()) {
-                if (column.getWidth() >= 0) {
-                    freeSpace -= column.getWidth();
-                } else if (column.getMinimumWidth() >= 0) {
-                    freeSpace -= column.getMinimumWidth();
-                }
-            }
-
-            if (freeSpace < 0) {
-                for (Column<?, ?> column : getColumns()) {
-                    column.doSetWidth(column.getWidth());
-
-                    boolean wasFixedWidth = column.getWidth() <= 0;
-                    boolean newWidthIsSmallerThanMinWidth = column
-                            .getWidthActual() < getMinWidth(column);
-                    if (wasFixedWidth && newWidthIsSmallerThanMinWidth) {
-                        column.doSetWidth(column.getMinimumWidth());
-                    }
-                }
-            }
-
-            return freeSpace < 0;
-        }
-
         private int getExpandRatio(Column<?, ?> column,
                 boolean someColumnExpands) {
             int expandRatio = column.getExpandRatio();

diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorProxy.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorProxy.java
@@ -15,6 +15,8 @@
  */
 package com.vaadin.tests.widgetset.client.grid;
 
+import java.util.Map;
+
 import com.google.gwt.dom.client.Element;
 import com.google.gwt.dom.client.TableRowElement;
 import com.google.gwt.dom.client.TableSectionElement;
@@ -87,6 +89,12 @@ public void refreshColumns(int index, int numberOfColumns)
                 throws IndexOutOfBoundsException, IllegalArgumentException {
             columnConfiguration.refreshColumns(index, numberOfColumns);
         }
+
+        @Override
+        public void setColumnWidths(Map<Integer, Double> indexWidthMap)
+                throws IllegalArgumentException {
+            columnConfiguration.setColumnWidths(indexWidthMap);
+        }
     }
 
     private class RowContainerProxy implements RowContainer {