Column remove/insertion doesn't refresh the entire row (#13334)

Change-Id: Id224b7225e570b3fb593de1b27a026ae161bf526

5 files changed, 134 insertions, 35 deletions

diff --git a/client/src/com/vaadin/client/ui/grid/ColumnConfiguration.java b/client/src/com/vaadin/client/ui/grid/ColumnConfiguration.java
index f523fdbbd4..d1baf5ba99 100644
--- a/client/src/com/vaadin/client/ui/grid/ColumnConfiguration.java
+++ b/client/src/com/vaadin/client/ui/grid/ColumnConfiguration.java
@@ -30,6 +30,11 @@ public interface ColumnConfiguration {
      * <p>
      * If any of the removed columns were frozen, the number of frozen columns
      * will be reduced by the number of the removed columns that were frozen.
+     * <p>
+     * <em>Note:</em> This method simply removes the given columns, and does not
+     * do much of anything else. Especially if you have column spans, you
+     * probably need to run {@link #refreshColumns(int, int)} or
+     * {@link RowContainer#refreshRows(int, int)}
      * 
      * @param index
      *            the index of the first column to be removed
@@ -61,8 +66,9 @@ public interface ColumnConfiguration {
      * <p>
      * <em>Note:</em> Only the contents of the inserted columns will be
      * rendered. If inserting new columns affects the contents of existing
-     * columns, {@link RowContainer#refreshRows(int, int)} needs to be called as
-     * appropriate.
+     * columns (e.g. you have column spans),
+     * {@link RowContainer#refreshRows(int, int)} or
+     * {@link #refreshColumns(int, int)} needs to be called as appropriate.
      * 
      * @param index
      *            the index of the column before which new columns are inserted,
@@ -142,4 +148,29 @@ public interface ColumnConfiguration {
      *             if <code>index</code> is not a valid column index
      */
     public int getColumnWidthActual(int index) throws IllegalArgumentException;
+
+    /**
+     * Refreshes a range of rows in the current row containers in each Escalator
+     * section.
+     * <p>
+     * The data for the refreshed columns is queried from the current cell
+     * renderer.
+     * 
+     * @param index
+     *            the index of the first row that will be updated
+     * @param numberOfRows
+     *            the number of rows to update, starting from the index
+     * @throws IndexOutOfBoundsException
+     *             if any integer number in the range
+     *             <code>[index..(index+numberOfColumns)]</code> is not an
+     *             existing column index.
+     * @throws IllegalArgumentException
+     *             if {@code numberOfColumns} is less than 1.
+     * @see RowContainer#setEscalatorUpdater(EscalatorUpdater)
+     * @see Escalator#getHeader()
+     * @see Escalator#getBody()
+     * @see Escalator#getFooter()
+     */
+    public void refreshColumns(int index, int numberOfColumns)
+            throws IndexOutOfBoundsException, IllegalArgumentException;
 }
\ No newline at end of file
diff --git a/client/src/com/vaadin/client/ui/grid/Escalator.java b/client/src/com/vaadin/client/ui/grid/Escalator.java
index 2ca90fe802..3aaa1f6c06 100644
--- a/client/src/com/vaadin/client/ui/grid/Escalator.java
+++ b/client/src/com/vaadin/client/ui/grid/Escalator.java
@@ -1415,12 +1415,28 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
          */
         @Override
         // overridden because of JavaDoc
-        public abstract void refreshRows(final int index, final int numberOfRows);
+        public void refreshRows(final int index, final int numberOfRows) {
+            Range rowRange = Range.withLength(index, numberOfRows);
+            Range colRange = Range.withLength(0, getColumnConfiguration()
+                    .getColumnCount());
+            refreshCells(rowRange, colRange);
+        }
+
+        protected abstract void refreshCells(Range logicalRowRange,
+                Range colRange);
 
-        void refreshRow(final TableRowElement tr, final int logicalRowIndex) {
+        void refreshRow(TableRowElement tr, int logicalRowIndex) {
+            refreshRow(tr, logicalRowIndex, Range.withLength(0,
+                    getColumnConfiguration().getColumnCount()));
+        }
+
+        void refreshRow(final TableRowElement tr, final int logicalRowIndex,
+                Range colRange) {
             flyweightRow.setup(tr, logicalRowIndex,
                     columnConfiguration.getCalculatedColumnWidths());
-            updater.update(flyweightRow, flyweightRow.getCells());
+            Iterable<FlyweightCell> cellsToUpdate = flyweightRow.getCells(
+                    colRange.getStart(), colRange.length());
+            updater.update(flyweightRow, cellsToUpdate);
 
             /*
              * the "assert" guarantees that this code is run only during
@@ -1503,16 +1519,6 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
                     setColumnFrozen(col, true);
                 }
             }
-
-            /*
-             * Because we might insert columns where affected by colspans, it's
-             * easiest to simply redraw everything when columns are modified.
-             * 
-             * Yes, this is a TODO [[optimize]].
-             */
-            if (getRowCount() > 0) {
-                refreshRows(0, getRowCount());
-            }
         }
 
         /**
@@ -1569,6 +1575,7 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
             }
 
             getEscalatorUpdater().postAttach(flyweightRow, cells);
+            getEscalatorUpdater().update(flyweightRow, cells);
 
             assert flyweightRow.teardown();
         }
@@ -1861,6 +1868,14 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
 
             return new Cell(domRowIndex, domColumnIndex, cellElement);
         }
+
+        void refreshColumns(int index, int numberOfColumns) {
+            if (getRowCount() > 0) {
+                Range rowRange = Range.withLength(0, getRowCount());
+                Range colRange = Range.withLength(index, numberOfColumns);
+                refreshCells(rowRange, colRange);
+            }
+        }
     }
 
     private abstract class AbstractStaticRowContainer extends
@@ -1954,10 +1969,11 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
         protected abstract void sectionHeightCalculated();
 
         @Override
-        public void refreshRows(final int index, final int numberOfRows) {
+        protected void refreshCells(Range logicalRowRange, Range colRange) {
             Profiler.enter("Escalator.AbstractStaticRowContainer.refreshRows");
 
-            assertArgumentsAreValidAndWithinRange(index, numberOfRows);
+            assertArgumentsAreValidAndWithinRange(logicalRowRange.getStart(),
+                    logicalRowRange.length());
 
             if (!isAttached()) {
                 return;
@@ -1975,9 +1991,10 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
                  * TODO [[rowheight]]: nudge rows down with
                  * refreshRowPositions() as needed
                  */
-                for (int row = index; row < index + numberOfRows; row++) {
+                for (int row = logicalRowRange.getStart(); row < logicalRowRange
+                        .getEnd(); row++) {
                     final TableRowElement tr = getTrByVisualIndex(row);
-                    refreshRow(tr, row);
+                    refreshRow(tr, row, colRange);
                 }
             }
 
@@ -3112,11 +3129,10 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
         }
 
         @Override
-        public void refreshRows(final int index, final int numberOfRows) {
+        protected void refreshCells(Range logicalRowRange, Range colRange) {
             Profiler.enter("Escalator.BodyRowContainer.refreshRows");
 
-            final Range visualRange = convertToVisual(Range.withLength(index,
-                    numberOfRows));
+            final Range visualRange = convertToVisual(logicalRowRange);
 
             if (!visualRange.isEmpty()) {
                 final int firstLogicalRowIndex = getLogicalRowIndex(visualRowOrder
@@ -3124,7 +3140,7 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
                 for (int rowNumber = visualRange.getStart(); rowNumber < visualRange
                         .getEnd(); rowNumber++) {
                     refreshRow(visualRowOrder.get(rowNumber),
-                            firstLogicalRowIndex + rowNumber);
+                            firstLogicalRowIndex + rowNumber, colRange);
                 }
             }
 
@@ -3576,22 +3592,27 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
             body.verifyEscalatorCount();
 
             if (getColumnConfiguration().getColumnCount() > 0) {
-                readjustRows(header);
-                readjustRows(body);
-                readjustRows(footer);
+                reapplyRowWidths(header);
+                reapplyRowWidths(body);
+                reapplyRowWidths(footer);
             }
+
+            /*
+             * Colspans make any kind of automatic clever content re-rendering
+             * impossible: As soon as anything has colspans, removing one might
+             * reveal further colspans, modifying the DOM structure once again,
+             * ending in a cascade of updates. Because we don't know how the
+             * data is updated.
+             * 
+             * So, instead, we don't do anything. The client code is responsible
+             * for re-rendering the content (if so desired). Everything Just
+             * Works (TM) if colspans aren't used.
+             */
         }
 
-        private void readjustRows(AbstractRowContainer container) {
+        private void reapplyRowWidths(AbstractRowContainer container) {
             if (container.getRowCount() > 0) {
                 container.reapplyRowWidths();
-
-                /*
-                 * FIXME: Because we might remove columns where affected by
-                 * colspans, it's easiest to simply redraw everything when
-                 * columns are modified.
-                 */
-                container.refreshRows(0, container.getRowCount());
             }
         }
 
@@ -3710,6 +3731,18 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
                 horizontalScrollbar.setScrollPos(scroller.lastScrollLeft
                         + insertedColumnsWidth);
             }
+
+            /*
+             * Colspans make any kind of automatic clever content re-rendering
+             * impossible: As soon as anything has colspans, adding one might
+             * affect surrounding colspans, modifying the DOM structure once
+             * again, ending in a cascade of updates. Because we don't know how
+             * the data is updated.
+             * 
+             * So, instead, we don't do anything. The client code is responsible
+             * for re-rendering the content (if so desired). Everything Just
+             * Works (TM) if colspans aren't used.
+             */
         }
 
         @Override
@@ -3840,6 +3873,28 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
             }
             return widthsArray;
         }
+
+        @Override
+        public void refreshColumns(int index, int numberOfColumns)
+                throws IndexOutOfBoundsException, IllegalArgumentException {
+            if (numberOfColumns < 1) {
+                throw new IllegalArgumentException(
+                        "Number of columns must be 1 or greater (was "
+                                + numberOfColumns + ")");
+            }
+
+            if (index < 0 || index + numberOfColumns > getColumnCount()) {
+                throw new IndexOutOfBoundsException("The given "
+                        + "column range (" + index + ".."
+                        + (index + numberOfColumns)
+                        + ") was outside of the current number of columns ("
+                        + getColumnCount() + ")");
+            }
+
+            header.refreshColumns(index, numberOfColumns);
+            body.refreshColumns(index, numberOfColumns);
+            footer.refreshColumns(index, numberOfColumns);
+        }
     }
 
     // abs(atan(y/x))*(180/PI) = n deg, x = 1, solve y
diff --git a/client/src/com/vaadin/client/ui/grid/RowContainer.java b/client/src/com/vaadin/client/ui/grid/RowContainer.java
index 8b03ef3c36..f969f90fc9 100644
--- a/client/src/com/vaadin/client/ui/grid/RowContainer.java
+++ b/client/src/com/vaadin/client/ui/grid/RowContainer.java
@@ -109,7 +109,7 @@ public interface RowContainer {
     /**
      * Refreshes a range of rows in the current row container.
      * <p>
-     * The data for the refreshed rows are queried from the current cell
+     * The data for the refreshed rows is queried from the current cell
      * renderer.
      * 
      * @param index
diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
index 60b9d5cc7d..08dc1d2eae 100644
--- a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
+++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
@@ -456,6 +456,13 @@ public class EscalatorBasicClientFeaturesWidget extends
                         .getColumnCount() - 1, 1);
             }
         }, menupath);
+
+        addMenuCommand("Refresh first column", new ScheduledCommand() {
+            @Override
+            public void execute() {
+                escalator.getColumnConfiguration().refreshColumns(0, 1);
+            }
+        }, menupath);
     }
 
     private void createHeaderRowsMenu() {
diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorProxy.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorProxy.java
index 866fdbd5c2..f1d64a50e7 100644
--- a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorProxy.java
+++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorProxy.java
@@ -80,6 +80,12 @@ public class EscalatorProxy extends Escalator {
                 throws IllegalArgumentException {
             return columnConfiguration.getColumnWidthActual(index);
         }
+
+        @Override
+        public void refreshColumns(int index, int numberOfColumns)
+                throws IndexOutOfBoundsException, IllegalArgumentException {
+            columnConfiguration.refreshColumns(index, numberOfColumns);
+        }
     }
 
     private class RowContainerProxy implements RowContainer {