From c2b988781045d1e6723b8159ca34d6c0afcdcde8 Mon Sep 17 00:00:00 2001 From: Henrik Paul Date: Mon, 18 Nov 2013 14:50:18 +0200 Subject: [PATCH] Fixes Escalator resize issues (#12645) Change-Id: I34273d35338568833b179b61294de7462abe78f1 --- .../com/vaadin/client/ui/grid/Escalator.java | 229 +++++++++++++++++- .../src/com/vaadin/client/ui/grid/Grid.java | 13 +- .../vaadin/client/ui/grid/GridConnector.java | 14 +- .../tests/components/grid/BasicEscalator.java | 58 ++++- .../client/grid/TestGridClientRpc.java | 8 + .../client/grid/TestGridConnector.java | 20 ++ .../widgetset/client/grid/TestGridState.java | 5 +- .../widgetset/client/grid/VTestGrid.java | 18 ++ .../tests/widgetset/server/grid/TestGrid.java | 16 ++ 9 files changed, 355 insertions(+), 26 deletions(-) diff --git a/client/src/com/vaadin/client/ui/grid/Escalator.java b/client/src/com/vaadin/client/ui/grid/Escalator.java index 51b45dba59..bd9d4ba245 100644 --- a/client/src/com/vaadin/client/ui/grid/Escalator.java +++ b/client/src/com/vaadin/client/ui/grid/Escalator.java @@ -39,6 +39,7 @@ import com.google.gwt.user.client.DOM; import com.google.gwt.user.client.Element; import com.google.gwt.user.client.Window; import com.google.gwt.user.client.ui.Widget; +import com.vaadin.client.Profiler; import com.vaadin.client.Util; import com.vaadin.client.ui.grid.Escalator.JsniUtil.TouchHandlerBundle; import com.vaadin.client.ui.grid.PositionFunction.AbsolutePosition; @@ -1135,11 +1136,13 @@ public class Escalator extends Widget { } protected void recalculateSectionHeight() { + Profiler.enter("Escalator.AbstractRowContainer.recalculateSectionHeight"); final double newHeight = root.getChildCount() * ROW_HEIGHT_PX; if (newHeight != height) { height = newHeight; sectionHeightCalculated(); } + Profiler.leave("Escalator.AbstractRowContainer.recalculateSectionHeight"); } /** @@ -1153,6 +1156,8 @@ public class Escalator extends Widget { */ @Override public void refreshRows(final int index, final int numberOfRows) { + Profiler.enter("Escalator.AbstractRowContainer.refreshRows"); + assertArgumentsAreValidAndWithinRange(index, numberOfRows); if (!isAttached()) { @@ -1181,6 +1186,8 @@ public class Escalator extends Widget { refreshRow(tr, row); } } + + Profiler.leave("Escalator.AbstractRowContainer.refreshRows"); } void refreshRow(final Node tr, final int logicalRowIndex) { @@ -1344,7 +1351,7 @@ public class Escalator extends Widget { @Override protected void paintRemoveRows(final int index, final int numberOfRows) { for (int i = index; i < index + numberOfRows; i++) { - final Element tr = (Element) root.getChild(i); + final Element tr = (Element) root.getChild(index); // TODO [[widgets]] tr.removeFromParent(); } @@ -1366,6 +1373,18 @@ public class Escalator extends Widget { protected int getTopVisualRowLogicalIndex() { return 0; } + + @Override + public void insertRows(int index, int numberOfRows) { + super.insertRows(index, numberOfRows); + recalculateElementSizes(); + } + + @Override + public void removeRows(int index, int numberOfRows) { + super.removeRows(index, numberOfRows); + recalculateElementSizes(); + } } private class HeaderRowContainer extends AbstractStaticRowContainer { @@ -1422,6 +1441,10 @@ public class Escalator extends Widget { } public void updateEscalatorRowsOnScroll() { + if (visualRowOrder.isEmpty()) { + return; + } + final int topRowPos = getRowTop(visualRowOrder.getFirst()); // TODO [[mpixscroll]] final int scrollTop = tBodyScrollTop; @@ -1746,12 +1769,25 @@ public class Escalator extends Widget { return rowTopPosMap.get(tr); } + /** + * Adds new physical escalator rows to the DOM at the given index if + * there's still a need for more escalator rows. + *

+ * If Escalator already is at (or beyond) max capacity, this method does + * nothing to the DOM. + * + * @param index + * the index at which to add new escalator rows. + * Note:It is assumed that the index is both the + * visual index and the logical index. + * @param numberOfRows + * the number of rows to add at index + * @return a list of the added rows + */ private List fillAndPopulateEscalatorRowsIfNeeded( final int index, final int numberOfRows) { - final int maxEscalatorRowCapacity = (int) Math - .ceil(calculateHeight() / ROW_HEIGHT_PX) + 1; - final int escalatorRowsStillFit = maxEscalatorRowCapacity + final int escalatorRowsStillFit = getMaxEscalatorRowCapacity() - root.getChildCount(); final int escalatorRowsNeeded = Math.min(numberOfRows, escalatorRowsStillFit); @@ -1784,6 +1820,17 @@ public class Escalator extends Widget { } } + private int getMaxEscalatorRowCapacity() { + final int maxEscalatorRowCapacity = (int) Math + .ceil(calculateHeight() / ROW_HEIGHT_PX) + 1; + /* + * maxEscalatorRowCapacity can become negative if the headers and + * footers start to overlap. This is a crazy situation, but Vaadin + * blinks the components a lot, so it's feasible. + */ + return Math.max(0, maxEscalatorRowCapacity); + } + @Override protected void paintRemoveRows(final int index, final int numberOfRows) { @@ -2195,6 +2242,8 @@ public class Escalator extends Widget { @Override public void refreshRows(final int index, final int numberOfRows) { + Profiler.enter("Escalator.BodyRowContainer.refreshRows"); + final Range visualRange = convertToVisual(Range.withLength(index, numberOfRows)); @@ -2207,6 +2256,8 @@ public class Escalator extends Widget { firstLogicalRowIndex + rowNumber); } } + + Profiler.leave("Escalator.BodyRowContainer.refreshRows"); } @Override @@ -2235,6 +2286,156 @@ public class Escalator extends Widget { return 0; } } + + /** + * Make sure that there is a correct amount of escalator rows: Add more + * if needed, or remove any superfluous ones. + *

+ * This method should be called when e.g. the height of the Escalator + * changes. + */ + public void verifyEscalatorCount() { + /* + * This method indeed has a smell very similar to paintRemoveRows + * and paintInsertRows. + * + * Unfortunately, those the code can't trivially be shared, since + * there are some slight differences in the respective + * responsibilities. The "paint" methods fake the addition and + * removal of rows, and make sure to either push existing data out + * of view, or draw new data into view. Only in some special cases + * will the DOM element count change. + * + * This method, however, has the explicit responsibility to verify + * that when "something" happens, we still have the correct amount + * of escalator rows in the DOM, and if not, we make sure to modify + * that count. Only in some special cases do we need to take into + * account other things than simply modifying the DOM element count. + */ + + Profiler.enter("Escalator.BodyRowContainer.verifyEscalatorCount"); + + if (!isAttached()) { + return; + } + + final int maxEscalatorRows = getMaxEscalatorRowCapacity(); + final int neededEscalatorRows = Math.min(maxEscalatorRows, + body.getRowCount()); + final int neededEscalatorRowsDiff = neededEscalatorRows + - visualRowOrder.size(); + + if (neededEscalatorRowsDiff > 0) { + // needs more + + /* + * This is a workaround for the issue where we might be scrolled + * to the bottom, and the widget expands beyond the content + * range + */ + + final int index = visualRowOrder.size(); + final int nextLastLogicalIndex; + if (!visualRowOrder.isEmpty()) { + nextLastLogicalIndex = getLogicalRowIndex(visualRowOrder + .getLast()) + 1; + } else { + nextLastLogicalIndex = 0; + } + + final boolean contentWillFit = nextLastLogicalIndex < getRowCount() + - neededEscalatorRowsDiff; + if (contentWillFit) { + final List addedRows = fillAndPopulateEscalatorRowsIfNeeded( + index, neededEscalatorRowsDiff); + + /* + * Since fillAndPopulateEscalatorRowsIfNeeded operates on + * the assumption that index == visual index == logical + * index, we thank for the added escalator rows, but since + * they're painted in the wrong CSS position, we need to + * move them to their actual locations. + * + * Note: this is the second (see body.paintInsertRows) + * occasion where fillAndPopulateEscalatorRowsIfNeeded would + * behave "more correctly" if it only would add escalator + * rows to the DOM and appropriate bookkeping, and not + * actually populate them :/ + */ + moveAndUpdateEscalatorRows( + Range.withLength(index, addedRows.size()), index, + nextLastLogicalIndex); + } else { + /* + * TODO [[optimize]] + * + * We're scrolled so far down that all rows can't be simply + * appended at the end, since we might start displaying + * escalator rows that don't exist. To avoid the mess that + * is body.paintRemoveRows, this is a dirty hack that dumbs + * the problem down to a more basic and already-solved + * problem: + * + * 1) scroll all the way up 2) add the missing escalator + * rows 3) scroll back to the original position. + * + * Letting the browser scroll back to our original position + * will automatically solve any possible overflow problems, + * since the browser will not allow us to scroll beyond the + * actual content. + */ + + final double oldScrollTop = getScrollTop(); + setScrollTop(0); + scroller.onScroll(); + fillAndPopulateEscalatorRowsIfNeeded(index, + neededEscalatorRowsDiff); + setScrollTop(oldScrollTop); + scroller.onScroll(); + internalScrollEventCalls++; + } + } + + else if (neededEscalatorRowsDiff < 0) { + // needs less + + final ListIterator iter = visualRowOrder + .listIterator(visualRowOrder.size()); + for (int i = 0; i < -neededEscalatorRowsDiff; i++) { + // TODO [[widgets]] + final Element last = iter.previous(); + last.removeFromParent(); + iter.remove(); + } + + /* + * If we were scrolled to the bottom so that we didn't have an + * extra escalator row at the bottom, we'll probably end up with + * blank space at the bottom of the escalator, and one extra row + * above the header. + * + * Experimentation idea #1: calculate "scrollbottom" vs content + * bottom and remove one row from top, rest from bottom. This + * FAILED, since setHeight has already happened, thus we never + * will detect ourselves having been scrolled all the way to the + * bottom. + */ + + if (!visualRowOrder.isEmpty()) { + final int firstRowTop = getRowTop(visualRowOrder.getFirst()); + final double firstRowMinTop = tBodyScrollTop + - ROW_HEIGHT_PX; + if (firstRowTop < firstRowMinTop) { + final int newLogicalIndex = getLogicalRowIndex(visualRowOrder + .getLast()) + 1; + moveAndUpdateEscalatorRows(Range.withOnly(0), + visualRowOrder.size(), newLogicalIndex); + } + } + } + + Profiler.leave("Escalator.BodyRowContainer.verifyEscalatorCount"); + } } private class ColumnConfigurationImpl implements ColumnConfiguration { @@ -2416,6 +2617,9 @@ public class Escalator extends Widget { */ private static final double RATIO_OF_40_DEGREES = Math.tan(2 * Math.PI / 9); + private static final String DEFAULT_WIDTH = "400.0px"; + private static final String DEFAULT_HEIGHT = "400.0px"; + private FlyweightRow flyweightRow = new FlyweightRow(this); /** The {@code } tag. */ @@ -2538,6 +2742,10 @@ public class Escalator extends Widget { } } }); + + // init default dimensions + setHeight(null); + setWidth(null); } private void detectAndApplyPositionFunction() { @@ -2639,7 +2847,8 @@ public class Escalator extends Widget { */ @Override public void setWidth(final String width) { - super.setWidth(width); + super.setWidth(width != null && !width.isEmpty() ? width + : DEFAULT_WIDTH); recalculateElementSizes(); } @@ -2649,7 +2858,8 @@ public class Escalator extends Widget { */ @Override public void setHeight(final String height) { - super.setHeight(height); + super.setHeight(height != null && !height.isEmpty() ? height + : DEFAULT_HEIGHT); recalculateElementSizes(); } @@ -2827,6 +3037,11 @@ public class Escalator extends Widget { } private void recalculateElementSizes() { + if (!isAttached()) { + return; + } + + Profiler.enter("Escalator.recalculateElementSizes"); width = getPreciseWidth(getElement()); height = getPreciseHeight(getElement()); for (final AbstractRowContainer rowContainer : rowContainers) { @@ -2835,6 +3050,8 @@ public class Escalator extends Widget { scroller.recalculateTableWrapperSize(); scroller.recalculateScrollbarsForVirtualViewport(); + body.verifyEscalatorCount(); + Profiler.leave("Escalator.recalculateElementSizes"); } private boolean needsVerticalScrollbars() { diff --git a/client/src/com/vaadin/client/ui/grid/Grid.java b/client/src/com/vaadin/client/ui/grid/Grid.java index 8921aa3008..3c4e2d6e13 100644 --- a/client/src/com/vaadin/client/ui/grid/Grid.java +++ b/client/src/com/vaadin/client/ui/grid/Grid.java @@ -254,7 +254,8 @@ public class Grid extends Composite { }; } - // TODO Should be implemented bu the data sources + // TODO Should be implemented by the data sources + @SuppressWarnings("static-method") private EscalatorUpdater createBodyUpdater() { return new EscalatorUpdater() { @@ -453,4 +454,14 @@ public class Grid extends Composite { public boolean isFooterVisible() { return escalator.getFooter().getRowCount() > 0; } + + @Override + public void setHeight(String height) { + escalator.setHeight(height); + } + + @Override + public void setWidth(String width) { + escalator.setWidth(width); + } } diff --git a/client/src/com/vaadin/client/ui/grid/GridConnector.java b/client/src/com/vaadin/client/ui/grid/GridConnector.java index e35e3f1f78..c48c9936bc 100644 --- a/client/src/com/vaadin/client/ui/grid/GridConnector.java +++ b/client/src/com/vaadin/client/ui/grid/GridConnector.java @@ -50,17 +50,6 @@ public class GridConnector extends AbstractComponentConnector { // Maps a generated column id -> A grid column instance private Map columnIdToColumn = new HashMap(); - @Override - protected void init() { - - // FIXME Escalator bug requires to do this when running compiled. Not - // required when in devmode. Most likely Escalator.setWidth() is called - // before attach and measuring from DOM does not work then. - getWidget().setWidth(getState().width); - getWidget().setHeight(getState().height); - - } - @Override protected Grid createWidget() { // FIXME Shouldn't be needed after #12873 has been fixed. @@ -68,6 +57,7 @@ public class GridConnector extends AbstractComponentConnector { } @Override + @SuppressWarnings("unchecked") public Grid getWidget() { return (Grid) super.getWidget(); } @@ -155,7 +145,7 @@ public class GridConnector extends AbstractComponentConnector { * @param state * The state to update from */ - private void updateColumnFromState(GridColumn column, + private static void updateColumnFromState(GridColumn column, GridColumnState state) { column.setHeaderCaption(state.header); column.setFooterCaption(state.footer); diff --git a/uitest/src/com/vaadin/tests/components/grid/BasicEscalator.java b/uitest/src/com/vaadin/tests/components/grid/BasicEscalator.java index a9cd22365e..2431fc815c 100644 --- a/uitest/src/com/vaadin/tests/components/grid/BasicEscalator.java +++ b/uitest/src/com/vaadin/tests/components/grid/BasicEscalator.java @@ -16,6 +16,8 @@ package com.vaadin.tests.components.grid; +import java.util.Random; + import com.vaadin.annotations.Widgetset; import com.vaadin.server.VaadinRequest; import com.vaadin.tests.components.AbstractTestUI; @@ -39,6 +41,8 @@ public class BasicEscalator extends AbstractTestUI { public static final String INSERT_ROWS_AMOUNT = "ira"; public static final String INSERT_ROWS_BUTTON = "irb"; + private final Random random = new Random(); + @Override protected void setup(final VaadinRequest request) { final TestGrid grid = new TestGrid(); @@ -55,7 +59,6 @@ public class BasicEscalator extends AbstractTestUI { insertRowsLayout.addComponent(new Button("insert rows", new Button.ClickListener() { @Override - @SuppressWarnings("boxing") public void buttonClick(final ClickEvent event) { final int offset = Integer.parseInt(insertRowsOffset .getValue()); @@ -78,7 +81,6 @@ public class BasicEscalator extends AbstractTestUI { removeRowsLayout.addComponent(new Button("remove rows", new Button.ClickListener() { @Override - @SuppressWarnings("boxing") public void buttonClick(final ClickEvent event) { final int offset = Integer.parseInt(removeRowsOffset .getValue()); @@ -97,7 +99,6 @@ public class BasicEscalator extends AbstractTestUI { insertColumnsLayout.addComponent(new Button("insert columns", new Button.ClickListener() { @Override - @SuppressWarnings("boxing") public void buttonClick(final ClickEvent event) { final int offset = Integer.parseInt(insertColumnsOffset .getValue()); @@ -116,7 +117,6 @@ public class BasicEscalator extends AbstractTestUI { removeColumnsLayout.addComponent(new Button("remove columns", new Button.ClickListener() { @Override - @SuppressWarnings("boxing") public void buttonClick(final ClickEvent event) { final int offset = Integer.parseInt(removeColumnsOffset .getValue()); @@ -214,6 +214,56 @@ public class BasicEscalator extends AbstractTestUI { } }))); + addComponent(new Button("Resize randomly", new Button.ClickListener() { + @Override + public void buttonClick(ClickEvent event) { + int width = random.nextInt(300) + 500; + int height = random.nextInt(300) + 200; + grid.setWidth(width + "px"); + grid.setHeight(height + "px"); + } + })); + + addComponent(new Button("Random headers count", + new Button.ClickListener() { + private int headers = 1; + + @Override + public void buttonClick(ClickEvent event) { + int diff = 0; + while (diff == 0) { + final int nextHeaders = random.nextInt(4); + diff = nextHeaders - headers; + headers = nextHeaders; + } + if (diff > 0) { + grid.insertHeaders(0, diff); + } else if (diff < 0) { + grid.removeHeaders(0, -diff); + } + } + })); + + addComponent(new Button("Random footers count", + new Button.ClickListener() { + private int footers = 1; + + @Override + public void buttonClick(ClickEvent event) { + int diff = 0; + while (diff == 0) { + final int nextFooters = random.nextInt(4); + diff = nextFooters - footers; + footers = nextFooters; + } + if (diff > 0) { + grid.insertFooters(0, diff); + } else if (diff < 0) { + grid.removeFooters(0, -diff); + } + } + })); + } @Override diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridClientRpc.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridClientRpc.java index 71fcd63086..225cc34147 100644 --- a/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridClientRpc.java +++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridClientRpc.java @@ -31,4 +31,12 @@ public interface TestGridClientRpc extends ClientRpc { void scrollToColumn(int index, String destination, int padding); void setFrozenColumns(int frozenColumns); + + void insertHeaders(int index, int amount); + + void removeHeaders(int index, int amount); + + void insertFooters(int index, int amount); + + void removeFooters(int index, int amount); } diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridConnector.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridConnector.java index 7b722e03c0..b355c9e79c 100644 --- a/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridConnector.java +++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridConnector.java @@ -82,6 +82,26 @@ public class TestGridConnector extends AbstractComponentConnector { getWidget().getColumnConfiguration().setFrozenColumnCount( frozenColumns); } + + @Override + public void insertHeaders(int index, int amount) { + getWidget().getHeader().insertRows(index, amount); + } + + @Override + public void removeHeaders(int index, int amount) { + getWidget().getHeader().removeRows(index, amount); + } + + @Override + public void insertFooters(int index, int amount) { + getWidget().getFooter().insertRows(index, amount); + } + + @Override + public void removeFooters(int index, int amount) { + getWidget().getFooter().removeRows(index, amount); + } }); } diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridState.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridState.java index 8f9cd3c371..73d6ba311c 100644 --- a/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridState.java +++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/TestGridState.java @@ -22,9 +22,8 @@ import com.vaadin.shared.AbstractComponentState; * @author Vaadin Ltd */ public class TestGridState extends AbstractComponentState { - // public static final String DEFAULT_HEIGHT = "400px"; - public static final String DEFAULT_HEIGHT = "405px"; + public static final String DEFAULT_HEIGHT = "400.0px"; /* TODO: this should be "100%" before setting final. */ - public static final String DEFAULT_WIDTH = "800px"; + public static final String DEFAULT_WIDTH = "800.0px"; } diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/VTestGrid.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/VTestGrid.java index 5edc2ab0a0..e9ee461fb9 100644 --- a/uitest/src/com/vaadin/tests/widgetset/client/grid/VTestGrid.java +++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/VTestGrid.java @@ -187,4 +187,22 @@ public class VTestGrid extends Composite { data.removeColumns(offset, amount); escalator.getColumnConfiguration().removeColumns(offset, amount); } + + @Override + public void setWidth(String width) { + escalator.setWidth(width); + } + + @Override + public void setHeight(String height) { + escalator.setHeight(height); + } + + public RowContainer getHeader() { + return escalator.getHeader(); + } + + public RowContainer getFooter() { + return escalator.getFooter(); + } } diff --git a/uitest/src/com/vaadin/tests/widgetset/server/grid/TestGrid.java b/uitest/src/com/vaadin/tests/widgetset/server/grid/TestGrid.java index e37d9ccee0..2f14f21f0e 100644 --- a/uitest/src/com/vaadin/tests/widgetset/server/grid/TestGrid.java +++ b/uitest/src/com/vaadin/tests/widgetset/server/grid/TestGrid.java @@ -65,4 +65,20 @@ public class TestGrid extends AbstractComponent { public void setFrozenColumns(int frozenColumns) { rpc().setFrozenColumns(frozenColumns); } + + public void insertHeaders(int index, int amount) { + rpc().insertHeaders(index, amount); + } + + public void removeHeaders(int index, int amount) { + rpc().removeHeaders(index, amount); + } + + public void insertFooters(int index, int amount) { + rpc().insertFooters(index, amount); + } + + public void removeFooters(int index, int amount) { + rpc().removeFooters(index, amount); + } } -- 2.39.5