Fix Grid initial render performance (#10579)

Fixes #10232

5 files changed, 150 insertions, 79 deletions

diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
index 5d66489ed1..d238b27b00 100644
--- a/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/ColumnConnector.java
@@ -207,7 +207,7 @@ public class ColumnConnector extends AbstractExtensionConnector {
             // If the grid itself was unregistered there is no point in spending
             // time to remove columns (and have problems with frozen columns)
             // before throwing everything away
-            parent.removeColumn(column);
+            parent.removeColumnMapping(column);
             parent = null;
         }
         column = null;
diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java
index ff83763d7b..fca1feab25 100644
--- a/client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java
@@ -40,6 +40,7 @@ import com.vaadin.client.MouseEventDetailsBuilder;
 import com.vaadin.client.TooltipInfo;
 import com.vaadin.client.WidgetUtil;
 import com.vaadin.client.annotations.OnStateChange;
+import com.vaadin.client.communication.StateChangeEvent;
 import com.vaadin.client.connectors.AbstractListingConnector;
 import com.vaadin.client.connectors.grid.ColumnConnector.CustomColumn;
 import com.vaadin.client.data.DataSource;
@@ -57,6 +58,7 @@ import com.vaadin.client.widgets.Grid;
 import com.vaadin.client.widgets.Grid.Column;
 import com.vaadin.client.widgets.Grid.FooterRow;
 import com.vaadin.client.widgets.Grid.HeaderRow;
+import com.vaadin.client.widgets.Grid.SelectionColumn;
 import com.vaadin.shared.MouseEventDetails;
 import com.vaadin.shared.data.sort.SortDirection;
 import com.vaadin.shared.ui.Connect;
@@ -231,14 +233,12 @@ public class GridConnector extends AbstractListingConnector
 
             @Override
             public void scrollToStart() {
-                Scheduler.get()
-                        .scheduleFinally(() -> grid.scrollToStart());
+                Scheduler.get().scheduleFinally(() -> grid.scrollToStart());
             }
 
             @Override
             public void scrollToEnd() {
-                Scheduler.get()
-                        .scheduleFinally(() -> grid.scrollToEnd());
+                Scheduler.get().scheduleFinally(() -> grid.scrollToEnd());
                 addDetailsRefreshCallback(() -> {
                     if (rowHasDetails(grid.getDataSource().size() - 1)) {
                         grid.scrollToEnd();
@@ -256,7 +256,8 @@ public class GridConnector extends AbstractListingConnector
         grid.setRowStyleGenerator(rowRef -> {
             JsonObject json = rowRef.getRow();
             return json.hasKey(GridState.JSONKEY_ROWSTYLE)
-                    ? json.getString(GridState.JSONKEY_ROWSTYLE) : null;
+                    ? json.getString(GridState.JSONKEY_ROWSTYLE)
+                    : null;
         });
         grid.setCellStyleGenerator(cellRef -> {
             JsonObject row = cellRef.getRow();
@@ -312,13 +313,27 @@ public class GridConnector extends AbstractListingConnector
         layout();
     }
 
-    @SuppressWarnings("unchecked")
-    @OnStateChange("columnOrder")
+    @Override
+    public void onStateChanged(StateChangeEvent stateChangeEvent) {
+        super.onStateChanged(stateChangeEvent);
+
+        if (!getState().columnOrder.containsAll(idToColumn.keySet())) {
+            updateColumns();
+        } else if (stateChangeEvent.hasPropertyChanged("columnOrder")) {
+            updateColumnOrder();
+        }
+
+        if (stateChangeEvent.hasPropertyChanged("header")) {
+            updateHeader();
+        }
+        if (stateChangeEvent.hasPropertyChanged("footer")) {
+            updateFooter();
+        }
+    }
+
     void updateColumnOrder() {
-        Scheduler.get()
-                .scheduleFinally(() -> getWidget().setColumnOrder(
-                        getState().columnOrder.stream().map(this::getColumn)
-                                .toArray(size -> new Column[size])));
+        getWidget().setColumnOrder(getState().columnOrder.stream()
+                .map(this::getColumn).toArray(size -> new CustomColumn[size]));
     }
 
     @OnStateChange("columnResizeMode")
@@ -329,7 +344,6 @@ public class GridConnector extends AbstractListingConnector
     /**
      * Updates the grid header section on state change.
      */
-    @OnStateChange("header")
     void updateHeader() {
         final Grid<JsonObject> grid = getWidget();
         final SectionState state = getState().header;
@@ -449,7 +463,6 @@ public class GridConnector extends AbstractListingConnector
     /**
      * Updates the grid footer section on state change.
      */
-    @OnStateChange("footer")
     void updateFooter() {
         final Grid<JsonObject> grid = getWidget();
         final SectionState state = getState().footer;
@@ -500,22 +513,68 @@ public class GridConnector extends AbstractListingConnector
     public void addColumn(CustomColumn column, String id) {
         assert !columnToIdMap.containsKey(column) && !columnToIdMap
                 .containsValue(id) : "Column with given id already exists.";
-        getWidget().addColumn(column);
         columnToIdMap.put(column, id);
         idToColumn.put(id, column);
+
+        if (idToColumn.keySet().containsAll(getState().columnOrder)) {
+            // All columns are available.
+            updateColumns();
+        }
     }
 
     /**
-     * Removes a column from Grid widget. This method also removes communication
-     * id mapping for the column.
+     * Updates the widgets columns to match the map in this connector.
+     */
+    protected void updateColumns() {
+        List<Column<?, JsonObject>> currentColumns = getWidget().getColumns();
+
+        List<CustomColumn> columnOrder = getState().columnOrder.stream()
+                .map(this::getColumn).collect(Collectors.toList());
+
+        if (isColumnOrderCorrect(currentColumns, columnOrder)) {
+            // All up to date
+            return;
+        }
+
+        Grid<JsonObject> grid = getWidget();
+
+        // Remove old column
+        currentColumns.stream()
+                .filter(col -> !(columnOrder.contains(col)
+                        || col instanceof SelectionColumn))
+                .forEach(grid::removeColumn);
+
+        // Add new columns
+        grid.addColumns(columnOrder.stream()
+                .filter(col -> !currentColumns.contains(col))
+                .toArray(CustomColumn[]::new));
+
+        // Make sure order is correct.
+        grid.setColumnOrder(
+                columnOrder.toArray(new CustomColumn[columnOrder.size()]));
+    }
+
+    private boolean isColumnOrderCorrect(List<Column<?, JsonObject>> current,
+            List<CustomColumn> order) {
+        List<Column<?, JsonObject>> columnsToCompare = current;
+        if (current.size() > 0 && current.get(0) instanceof SelectionColumn) {
+            // Remove selection column.
+            columnsToCompare = current.subList(1, current.size());
+        }
+        return columnsToCompare.equals(order);
+    }
+
+    /**
+     * Removes the given column from mappings in this Connector.
      *
      * @param column
-     *            column to remove
+     *            column to remove from the mapping
      */
-    public void removeColumn(CustomColumn column) {
+    public void removeColumnMapping(CustomColumn column) {
         assert columnToIdMap
                 .containsKey(column) : "Given Column does not exist.";
-        getWidget().removeColumn(column);
+
+        // Remove mapping. Columns are removed from Grid when state changes.
         String id = columnToIdMap.remove(column);
         idToColumn.remove(id);
     }
@@ -593,7 +652,6 @@ public class GridConnector extends AbstractListingConnector
     @Override
     public void setChildComponents(List<ComponentConnector> children) {
         childComponents = children;
-
     }
 
     @Override
@@ -634,7 +692,8 @@ public class GridConnector extends AbstractListingConnector
                     if (cellDescriptions != null
                             && cellDescriptions.hasKey(id)) {
                         return new TooltipInfo(cellDescriptions.getString(id),
-                                ((CustomColumn) column).getTooltipContentMode());
+                                ((CustomColumn) column)
+                                        .getTooltipContentMode());
                     } else if (row.hasKey(GridState.JSONKEY_ROWDESCRIPTION)) {
                         return new TooltipInfo(
                                 row.getString(GridState.JSONKEY_ROWDESCRIPTION),
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 4c28ffe244..a790ccb314 100755
--- a/client/src/main/java/com/vaadin/client/widgets/Grid.java
+++ b/client/src/main/java/com/vaadin/client/widgets/Grid.java
@@ -26,6 +26,7 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -2858,7 +2859,7 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
         public void rowsAddedToBody(Range added) {
             boolean bodyHasFocus = containerWithFocus == escalator.getBody();
             boolean insertionIsAboveFocusedCell = added
-                    .getStart() <= rowWithFocus;
+                    .getStart() < rowWithFocus;
             if (bodyHasFocus && insertionIsAboveFocusedCell) {
                 rowWithFocus += added.length();
                 rowWithFocus = Math.min(rowWithFocus,
@@ -5494,7 +5495,8 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
          * @see Grid#isEditorActive()
          */
         public Column<C, T> setEditable(boolean editable) {
-            if (editable != this.editable && grid.isEditorActive()) {
+            if (editable != this.editable && grid != null
+                    && grid.isEditorActive()) {
                 throw new IllegalStateException(
                         "Cannot change column editable status while the editor is active");
             }
@@ -6524,10 +6526,12 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
      *            the columns to add
      */
     public void addColumns(Column<?, T>... columns) {
-        int count = getColumnCount();
-        for (Column<?, T> column : columns) {
-            addColumn(column, count++);
+        if (columns.length == 0) {
+            // Nothing to add.
+            return;
         }
+        addColumnsSkipSelectionColumnCheck(Arrays.asList(columns),
+                getVisibleColumns().size());
     }
 
     /**
@@ -6564,45 +6568,43 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
                     + "before the selection column");
         }
 
-        addColumnSkipSelectionColumnCheck(column, index);
+        addColumnsSkipSelectionColumnCheck(Collections.singleton(column),
+                index);
         return column;
     }
 
-    private void addColumnSkipSelectionColumnCheck(Column<?, T> column,
-            int index) {
-        // Register column with grid
-        columns.add(index, column);
+    private void addColumnsSkipSelectionColumnCheck(
+            Collection<Column<?, T>> columnsToAdd, int startIndex) {
+        AtomicInteger index = new AtomicInteger(startIndex);
+        columnsToAdd.forEach(col -> {
+            // Register column with grid
+            columns.add(index.getAndIncrement(), col);
 
-        header.addColumn(column);
-        footer.addColumn(column);
+            header.addColumn(col);
+            footer.addColumn(col);
 
-        // Register this grid instance with the column
-        ((Column<?, T>) column).setGrid(this);
+            // Register this grid instance with the column
+            col.setGrid(this);
+        });
 
-        // Grid knows about hidden columns, Escalator only knows about what is
-        // visible so column indexes do not match
-        if (!column.isHidden()) {
-            int escalatorIndex = index;
-            for (int existingColumn = 0; existingColumn < index; existingColumn++) {
-                if (getColumn(existingColumn).isHidden()) {
-                    escalatorIndex--;
-                }
-            }
-            escalator.getColumnConfiguration().insertColumns(escalatorIndex, 1);
-        }
+        escalator.getColumnConfiguration().insertColumns(startIndex,
+                (int) columnsToAdd.stream().filter(col -> !col.isHidden())
+                        .count());
 
-        // Reapply column width
-        column.reapplyWidth();
+        columnsToAdd.forEach(col -> {
+            // Reapply column width
+            col.reapplyWidth();
 
-        // Sink all renderer events
-        Set<String> events = new HashSet<>();
-        events.addAll(getConsumedEventsForRenderer(column.getRenderer()));
+            // Sink all renderer events
+            Set<String> events = new HashSet<>();
+            events.addAll(getConsumedEventsForRenderer(col.getRenderer()));
 
-        if (column.isHidable()) {
-            columnHider.updateColumnHidable(column);
-        }
+            if (col.isHidable()) {
+                columnHider.updateColumnHidable(col);
+            }
 
-        sinkEvents(events);
+            sinkEvents(events);
+        });
     }
 
     private void sinkEvents(Collection<String> events) {
@@ -6659,8 +6661,6 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
                     .removeColumns(visibleColumnIndex, 1);
         }
 
-        updateFrozenColumns();
-
         header.removeColumn(column);
         footer.removeColumn(column);
 
@@ -6672,6 +6672,8 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
         if (column.isHidable()) {
             columnHider.removeColumnHidingToggle(column);
         }
+
+        updateFrozenColumns();
     }
 
     /**
@@ -7167,11 +7169,6 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
         assert escalator.getBody().getRowCount() == 0;
 
         int size = dataSource.size();
-        if (size == -1 && isAttached()) {
-            // Exact size is not yet known, start with some reasonable guess
-            // just to get an initial backend request going
-            size = getEscalator().getMaxVisibleRowCount();
-        }
         if (size > 0) {
             escalator.getBody().insertRows(0, size);
         }
@@ -7223,7 +7220,7 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
         // for the escalator the hidden columns are not in the frozen column
         // count, but for grid they are. thus need to convert the index
         for (int i = 0; i < frozenColumnCount; i++) {
-            if (getColumn(i).isHidden()) {
+            if (i >= getColumnCount() || getColumn(i).isHidden()) {
                 numberOfColumns--;
             }
         }
@@ -8021,7 +8018,8 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
             cellFocusHandler.offsetRangeBy(1);
             selectionColumn = new SelectionColumn(selectColumnRenderer);
 
-            addColumnSkipSelectionColumnCheck(selectionColumn, 0);
+            addColumnsSkipSelectionColumnCheck(
+                    Collections.singleton(selectionColumn), 0);
 
             selectionColumn.initDone();
         } else {
@@ -8663,12 +8661,6 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
 
     private void setColumnOrder(boolean isUserOriginated,
             Column<?, T>... orderedColumns) {
-        List<Column<?, T>> oldOrder = new ArrayList<>(columns);
-        ColumnConfiguration conf = getEscalator().getColumnConfiguration();
-
-        // Trigger ComplexRenderer.destroy for old content
-        conf.removeColumns(0, conf.getColumnCount());
-
         List<Column<?, T>> newOrder = new ArrayList<>();
         if (selectionColumn != null) {
             newOrder.add(selectionColumn);
@@ -8686,15 +8678,28 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
         }
 
         if (columns.size() != newOrder.size()) {
-            columns.removeAll(newOrder);
-            newOrder.addAll(columns);
+            columns.stream().filter(col -> !newOrder.contains(col))
+                    .forEach(newOrder::add);
         }
-        columns = newOrder;
 
-        List<Column<?, T>> visibleColumns = getVisibleColumns();
+        if (columns.equals(newOrder)) {
+            // No changes in order.
+            return;
+        }
+
+        // Prepare event for firing
+        ColumnReorderEvent<T> event = new ColumnReorderEvent<>(columns,
+                newOrder, isUserOriginated);
+
+        // Trigger ComplexRenderer.destroy for old content
+        ColumnConfiguration conf = getEscalator().getColumnConfiguration();
+        conf.removeColumns(0, conf.getColumnCount());
+
+        // Update the order
+        columns = newOrder;
 
         // Do ComplexRenderer.init and render new content
-        conf.insertColumns(0, visibleColumns.size());
+        conf.insertColumns(0, getVisibleColumns().size());
 
         // Number of frozen columns should be kept same #16901
         updateFrozenColumns();
@@ -8714,8 +8719,7 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
 
         columnHider.updateTogglesOrder();
 
-        fireEvent(
-                new ColumnReorderEvent<>(oldOrder, newOrder, isUserOriginated));
+        fireEvent(event);
     }
 
     /**
diff --git a/server/src/main/java/com/vaadin/data/ValueContext.java b/server/src/main/java/com/vaadin/data/ValueContext.java
index cbdd92dc56..266c506032 100644
--- a/server/src/main/java/com/vaadin/data/ValueContext.java
+++ b/server/src/main/java/com/vaadin/data/ValueContext.java
@@ -156,7 +156,6 @@ public class ValueContext implements Serializable {
      * @return the optional of {@code HasValue}
      * @since 8.1
      */
-    @SuppressWarnings("unused")
     public Optional<HasValue<?>> getHasValue() {
         return Optional.ofNullable(hasValue);
     }
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetachTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetachTest.java
index 930b1c7036..15527633e6 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetachTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveColumnAndDetachTest.java
@@ -72,4 +72,13 @@ public class GridRemoveColumnAndDetachTest extends SingleBrowserTest {
         $(ButtonElement.class).id("remove3").click();
         assertVisibleFrozenColumns(2);
     }
+
+    @Test
+    public void allColumnsFrozenRemoveLast() {
+        openTestURL("debug");
+        $(ButtonElement.class).id("remove3").click();
+        $(ButtonElement.class).id("remove2").click();
+        assertVisibleFrozenColumns(1);
+        assertNoErrorNotifications();
+    }
 }