From 7d2d091ecbb0b995dc46074725eabb456609cdf3 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Wed, 11 May 2016 22:47:07 +0300 Subject: [PATCH] Eliminate rounding errors for GridLayout expand ratios (#19797) Change-Id: Idf05dde5d6526fafee618fd3e2eb1afa63fab7bc --- .../com/vaadin/client/ui/VGridLayout.java | 32 +++++------ .../ui/gridlayout/GridLayoutConnector.java | 4 +- .../main/java/com/vaadin/ui/GridLayout.java | 46 +++++---------- .../shared/ui/gridlayout/GridLayoutState.java | 2 + .../GridLayoutExpandWithManyRows.java | 57 +++++++++++++++++++ .../GridLayoutExpandWithManyRowsTest.java | 40 +++++++++++++ 6 files changed, 129 insertions(+), 52 deletions(-) create mode 100644 uitest/src/main/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRows.java create mode 100644 uitest/src/test/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRowsTest.java diff --git a/client/src/main/java/com/vaadin/client/ui/VGridLayout.java b/client/src/main/java/com/vaadin/client/ui/VGridLayout.java index 42bcb5060a..9e13bb7fb2 100644 --- a/client/src/main/java/com/vaadin/client/ui/VGridLayout.java +++ b/client/src/main/java/com/vaadin/client/ui/VGridLayout.java @@ -61,10 +61,10 @@ public class VGridLayout extends ComplexPanel { public int[] rowHeights; /** For internal use only. May be removed or replaced in the future. */ - public int[] colExpandRatioArray; + public float[] colExpandRatioArray; /** For internal use only. May be removed or replaced in the future. */ - public int[] rowExpandRatioArray; + public float[] rowExpandRatioArray; int[] minColumnWidths; @@ -142,7 +142,7 @@ public class VGridLayout extends ComplexPanel { void expandRows() { if (!isUndefinedHeight()) { int usedSpace = calcRowUsedSpace(); - int[] actualExpandRatio = calcRowExpandRatio(); + float[] actualExpandRatio = calcRowExpandRatio(); // Round down to avoid problems with fractions (100.1px available -> // can use 100, not 101) int availableSpace = (int) LayoutManager.get(client) @@ -150,13 +150,12 @@ public class VGridLayout extends ComplexPanel { int excessSpace = availableSpace - usedSpace; int distributed = 0; if (excessSpace > 0) { - int expandRatioSum = 0; + float expandRatioSum = 0; for (int i = 0; i < rowHeights.length; i++) { expandRatioSum += actualExpandRatio[i]; } for (int i = 0; i < rowHeights.length; i++) { - int ew = excessSpace * actualExpandRatio[i] - / expandRatioSum; + int ew = (int) (excessSpace * actualExpandRatio[i] / expandRatioSum); rowHeights[i] = minRowHeights[i] + ew; distributed += ew; } @@ -171,8 +170,8 @@ public class VGridLayout extends ComplexPanel { } } - private int[] calcRowExpandRatio() { - int[] actualExpandRatio = new int[minRowHeights.length]; + private float[] calcRowExpandRatio() { + float[] actualExpandRatio = new float[minRowHeights.length]; for (int i = 0; i < minRowHeights.length; i++) { if (hiddenEmptyRow(i)) { actualExpandRatio[i] = 0; @@ -224,7 +223,7 @@ public class VGridLayout extends ComplexPanel { void expandColumns() { if (!isUndefinedWidth()) { int usedSpace = calcColumnUsedSpace(); - int[] actualExpandRatio = calcColumnExpandRatio(); + float[] actualExpandRatio = calcColumnExpandRatio(); // Round down to avoid problems with fractions (100.1px available -> // can use 100, not 101) int availableSpace = (int) LayoutManager.get(client) @@ -232,13 +231,12 @@ public class VGridLayout extends ComplexPanel { int excessSpace = availableSpace - usedSpace; int distributed = 0; if (excessSpace > 0) { - int expandRatioSum = 0; + float expandRatioSum = 0; for (int i = 0; i < columnWidths.length; i++) { expandRatioSum += actualExpandRatio[i]; } for (int i = 0; i < columnWidths.length; i++) { - int ew = excessSpace * actualExpandRatio[i] - / expandRatioSum; + int ew = (int) (excessSpace * actualExpandRatio[i] / expandRatioSum); columnWidths[i] = minColumnWidths[i] + ew; distributed += ew; } @@ -256,8 +254,8 @@ public class VGridLayout extends ComplexPanel { /** * Calculates column expand ratio. */ - private int[] calcColumnExpandRatio() { - int[] actualExpandRatio = new int[minColumnWidths.length]; + private float[] calcColumnExpandRatio() { + float[] actualExpandRatio = new float[minColumnWidths.length]; for (int i = 0; i < minColumnWidths.length; i++) { if (!hiddenEmptyColumn(i)) { actualExpandRatio[i] = colExpandRatioArray[i]; @@ -537,7 +535,7 @@ public class VGridLayout extends ComplexPanel { private static void distributeSpanSize(int[] dimensions, int spanStartIndex, int spanSize, int spacingSize, int size, - int[] expansionRatios) { + float[] expansionRatios) { int allocated = dimensions[spanStartIndex]; for (int i = 1; i < spanSize; i++) { allocated += spacingSize + dimensions[spanStartIndex + i]; @@ -563,8 +561,8 @@ public class VGridLayout extends ComplexPanel { // expansion ratios expansion = neededExtraSpace / spanSize; } else { - expansion = neededExtraSpace * expansionRatios[itemIndex] - / totalExpansion; + expansion = (int) (neededExtraSpace + * expansionRatios[itemIndex] / totalExpansion); } dimensions[itemIndex] += expansion; allocatedExtraSpace += expansion; diff --git a/client/src/main/java/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java b/client/src/main/java/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java index 4d1ce692ad..b450092d54 100644 --- a/client/src/main/java/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java +++ b/client/src/main/java/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java @@ -117,8 +117,8 @@ public class GridLayoutConnector extends AbstractComponentContainerConnector cell.updateCell(childComponentData); } - layout.colExpandRatioArray = uidl.getIntArrayAttribute("colExpand"); - layout.rowExpandRatioArray = uidl.getIntArrayAttribute("rowExpand"); + layout.colExpandRatioArray = getState().colExpand; + layout.rowExpandRatioArray = getState().rowExpand; layout.updateMarginStyleNames(new MarginInfo(getState().marginsBitmask)); layout.updateSpacingStyleName(getState().spacing); diff --git a/server/src/main/java/com/vaadin/ui/GridLayout.java b/server/src/main/java/com/vaadin/ui/GridLayout.java index 3b9272b034..214d0c567f 100644 --- a/server/src/main/java/com/vaadin/ui/GridLayout.java +++ b/server/src/main/java/com/vaadin/ui/GridLayout.java @@ -461,58 +461,38 @@ public class GridLayout extends AbstractLayout implements */ @Override public void paintContent(PaintTarget target) throws PaintException { - final Integer[] columnExpandRatioArray = new Integer[getColumns()]; - final Integer[] rowExpandRatioArray = new Integer[getRows()]; + // TODO Remove once LegacyComponent is no longer implemented + } - int realColExpandRatioSum = 0; + public void beforeClientResponse(boolean initial) { + super.beforeClientResponse(initial); + + getState().colExpand = new float[getColumns()]; float colSum = getExpandRatioSum(columnExpandRatio); if (colSum == 0) { - // no columns has been expanded, all cols have same expand - // rate - float equalSize = 1 / (float) getColumns(); - int myRatio = Math.round(equalSize * 1000); + // no cols have been expanded for (int i = 0; i < getColumns(); i++) { - columnExpandRatioArray[i] = myRatio; + getState().colExpand[i] = 1f; } - realColExpandRatioSum = myRatio * getColumns(); } else { for (int i = 0; i < getColumns(); i++) { - int myRatio = Math - .round((getColumnExpandRatio(i) / colSum) * 1000); - columnExpandRatioArray[i] = myRatio; - realColExpandRatioSum += myRatio; + getState().colExpand[i] = getColumnExpandRatio(i); } } - int realRowExpandRatioSum = 0; + getState().rowExpand = new float[getRows()]; float rowSum = getExpandRatioSum(rowExpandRatio); if (rowSum == 0) { // no rows have been expanded - float equalSize = 1 / (float) getRows(); - int myRatio = Math.round(equalSize * 1000); for (int i = 0; i < getRows(); i++) { - rowExpandRatioArray[i] = myRatio; + getState().rowExpand[i] = 1f; } - realRowExpandRatioSum = myRatio * getRows(); } else { - for (int cury = 0; cury < getRows(); cury++) { - int myRatio = Math - .round((getRowExpandRatio(cury) / rowSum) * 1000); - rowExpandRatioArray[cury] = myRatio; - realRowExpandRatioSum += myRatio; + for (int i = 0; i < getRows(); i++) { + getState().rowExpand[i] = getRowExpandRatio(i); } } - // correct possible rounding error - if (rowExpandRatioArray.length > 0) { - rowExpandRatioArray[0] -= realRowExpandRatioSum - 1000; - } - if (columnExpandRatioArray.length > 0) { - columnExpandRatioArray[0] -= realColExpandRatioSum - 1000; - } - target.addAttribute("colExpand", columnExpandRatioArray); - target.addAttribute("rowExpand", rowExpandRatioArray); - } private float getExpandRatioSum(Map ratioMap) { diff --git a/shared/src/main/java/com/vaadin/shared/ui/gridlayout/GridLayoutState.java b/shared/src/main/java/com/vaadin/shared/ui/gridlayout/GridLayoutState.java index b84c2673f4..7d637aa33f 100644 --- a/shared/src/main/java/com/vaadin/shared/ui/gridlayout/GridLayoutState.java +++ b/shared/src/main/java/com/vaadin/shared/ui/gridlayout/GridLayoutState.java @@ -40,6 +40,8 @@ public class GridLayoutState extends AbstractLayoutState { public Set explicitColRatios = new HashSet(); public Map childData = new HashMap(); public boolean hideEmptyRowsAndColumns = false; + public float[] rowExpand; + public float[] colExpand; public static class ChildComponentData implements Serializable { public int column1; diff --git a/uitest/src/main/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRows.java b/uitest/src/main/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRows.java new file mode 100644 index 0000000000..a901093e31 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRows.java @@ -0,0 +1,57 @@ +package com.vaadin.tests.components.gridlayout; + +import com.vaadin.annotations.Theme; +import com.vaadin.server.VaadinRequest; +import com.vaadin.ui.Component; +import com.vaadin.ui.GridLayout; +import com.vaadin.ui.Label; +import com.vaadin.ui.Panel; +import com.vaadin.ui.UI; + +@Theme("tests-valo") +public class GridLayoutExpandWithManyRows extends UI { + + static final int POPULATED_ROWS = 20; + static int ROW_COUNT = 58; + + public static class ColoredLabel extends Label { + private static int colorNumber = 0; + + public ColoredLabel() { + super(); + addStyleName("color-label"); + addStyleName("color-" + (colorNumber++) % 10); + } + } + + @Override + protected void init(VaadinRequest request) { + for (int i = 0; i < 10; i++) { + getPage().getStyles().add(".color-" + i + " {" // + + "background-color: hsl(" + (i * 90) + ", 60%, 70%);" // + + "}"); + } + + GridLayout gridLayout = new GridLayout(6, ROW_COUNT); + for (int i = 0; i < ROW_COUNT; i++) { + gridLayout.setRowExpandRatio(i, 1); + } + gridLayout.setSizeFull(); + for (int i = 0; i < POPULATED_ROWS; i++) { + int upperLeftRow = i * 2; + int upperLeftCol = 0; + int lowerRightCol = 5; + int lowerRightRow = upperLeftRow + 1; + ColoredLabel coloredLabel = new ColoredLabel(); + coloredLabel.setSizeFull(); + gridLayout.addComponent(coloredLabel, upperLeftCol, upperLeftRow, + lowerRightCol, lowerRightRow); + } + + gridLayout.setHeight("500%"); + Component root = new Panel(gridLayout); + root.setSizeFull(); + setContent(root); + + } +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRowsTest.java b/uitest/src/test/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRowsTest.java new file mode 100644 index 0000000000..693fdc73db --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/gridlayout/GridLayoutExpandWithManyRowsTest.java @@ -0,0 +1,40 @@ +package com.vaadin.tests.components.gridlayout; + +import java.util.List; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; + +import com.vaadin.testbench.elements.GridLayoutElement; +import com.vaadin.tests.tb3.SingleBrowserTest; + +public class GridLayoutExpandWithManyRowsTest extends SingleBrowserTest { + + @Test + public void equalRowHeights() { + openTestURL(); + GridLayoutElement gridlayout = $(GridLayoutElement.class).first(); + + // Rows are expanded using integer pixels and leftover pixels are added + // to the first N rows. + // The tests uses rowspan=2 so one row in the DOM should be max 2 pixels + // lower than the first row + List slots = gridlayout.findElements(By + .className("v-gridlayout-slot")); + Assert.assertEquals(GridLayoutExpandWithManyRows.POPULATED_ROWS, + slots.size()); + + int firstRowHeight = slots.get(0).getSize().height; + int lastRowHeight = firstRowHeight; + for (int i = 1; i < GridLayoutExpandWithManyRows.POPULATED_ROWS; i++) { + int rowHeight = slots.get(i).getSize().height; + Assert.assertTrue(rowHeight <= firstRowHeight); + Assert.assertTrue(rowHeight >= firstRowHeight - 2); + Assert.assertTrue(rowHeight <= lastRowHeight); + + lastRowHeight = rowHeight; + } + } +} -- 2.39.5