From e93fb3a0e1dea3e9165dbf6463b5658ec9c89e00 Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Tue, 27 Jan 2015 14:44:24 +0200 Subject: [PATCH] Defer RPC calls in RpcDataProvider to avoid cache issues (#16505) This patch defers cache refresh and row adding/removing. These calls are omitted completely when making initial response to the client or updating the size with bare ItemSetChangeEvents. Change-Id: I6b350680b3e2381caf6a66c9a4ad283207d024dc --- .../vaadin/data/RpcDataProviderExtension.java | 112 +++++++++++++----- .../tests/components/grid/GridInTabSheet.java | 30 ++++- .../components/grid/GridInTabSheetTest.java | 41 +++++++ 3 files changed, 150 insertions(+), 33 deletions(-) diff --git a/server/src/com/vaadin/data/RpcDataProviderExtension.java b/server/src/com/vaadin/data/RpcDataProviderExtension.java index 5da95c3b5c..d4e40ee915 100644 --- a/server/src/com/vaadin/data/RpcDataProviderExtension.java +++ b/server/src/com/vaadin/data/RpcDataProviderExtension.java @@ -509,14 +509,9 @@ public class RpcDataProviderExtension extends AbstractExtension { * the number of rows removed at firstIndex */ public void removeRows(int firstIndex, int count) { - int lastRemoved = firstIndex + count; - if (lastRemoved < activeRange.getStart()) { - /* firstIndex < lastIndex < start */ - activeRange = activeRange.offsetBy(-count); - } else if (firstIndex < activeRange.getEnd()) { - final Range deprecated = Range.between( - Math.max(activeRange.getStart(), firstIndex), - Math.min(activeRange.getEnd(), lastRemoved + 1)); + Range removed = Range.withLength(firstIndex, count); + if (removed.intersects(activeRange)) { + final Range deprecated = removed.restrictTo(activeRange); for (int i = deprecated.getStart(); i < deprecated.getEnd(); ++i) { Object itemId = keyMapper.itemIdAtIndex(i); // Item doesn't exist anymore. @@ -526,7 +521,11 @@ public class RpcDataProviderExtension extends AbstractExtension { activeRange = Range.withLength(activeRange.getStart(), activeRange.length() - deprecated.length()); } else { - /* end <= firstIndex, no need to do anything */ + if (removed.getEnd() < activeRange.getStart()) { + /* firstIndex < lastIndex < start */ + activeRange = activeRange.offsetBy(-count); + } + /* else: end <= firstIndex, no need to do anything */ } } } @@ -670,11 +669,15 @@ public class RpcDataProviderExtension extends AbstractExtension { for (GridValueChangeListener listener : listeners.values()) { listener.removeListener(); } + listeners.clear(); activeRowHandler.activeRange = Range.withLength(0, 0); keyMapper.setActiveRange(Range.withLength(0, 0)); keyMapper.indexToItemId.clear(); - rpc.resetDataAndSize(event.getContainer().size()); + + /* Mark as dirty to push changes in beforeClientResponse */ + bareItemSetTriggeredSizeChange = true; + markAsDirty(); } } }; @@ -683,14 +686,24 @@ public class RpcDataProviderExtension extends AbstractExtension { private KeyMapper columnKeys; - /* Has client been initialized */ - private boolean clientInitialized = false; + /** RpcDataProvider should send the current cache again. */ + private boolean refreshCache = false; private RowReference rowReference; private CellReference cellReference; + /** Set of updated item ids */ private Set updatedItemIds = new HashSet(); + /** + * Queued RPC calls for adding and removing rows. Queue will be handled in + * {@link beforeClientResponse} + */ + private List rowChanges = new ArrayList(); + + /** Size possibly changed with a bare ItemSetChangeEvent */ + private boolean bareItemSetTriggeredSizeChange = false; + /** * Creates a new data provider using the given container. * @@ -732,11 +745,15 @@ public class RpcDataProviderExtension extends AbstractExtension { } + /** + * {@inheritDoc} + *

+ * RpcDataProviderExtension makes all actual RPC calls from this function + * based on changes in the container. + */ @Override public void beforeClientResponse(boolean initial) { - if (initial) { - clientInitialized = true; - + if (initial || bareItemSetTriggeredSizeChange) { /* * Push initial set of rows, assuming Grid will initially be * rendered scrolled to the top and with a decent amount of rows @@ -749,12 +766,30 @@ public class RpcDataProviderExtension extends AbstractExtension { int numberOfRows = Math.min(40, size); pushRowData(0, numberOfRows, 0, 0); + } else { + // Only do row changes if not initial response. + for (Runnable r : rowChanges) { + r.run(); + } + + // Send current rows again if needed. + if (refreshCache) { + int firstRow = activeRowHandler.activeRange.getStart(); + int numberOfRows = activeRowHandler.activeRange.length(); + + pushRowData(firstRow, numberOfRows, firstRow, numberOfRows); + } } for (Object itemId : updatedItemIds) { internalUpdateRowData(itemId); } + + // Clear all changes. + rowChanges.clear(); + refreshCache = false; updatedItemIds.clear(); + bareItemSetTriggeredSizeChange = false; super.beforeClientResponse(initial); } @@ -865,30 +900,51 @@ public class RpcDataProviderExtension extends AbstractExtension { * @param count * the number of rows inserted at index */ - private void insertRowData(int index, int count) { - if (clientInitialized) { - rpc.insertRowData(index, count); + private void insertRowData(final int index, final int count) { + if (rowChanges.isEmpty()) { + markAsDirty(); } + /* + * Since all changes should be processed in a consistent order, we don't + * send the RPC call immediately. beforeClientResponse will decide + * whether to send these or not. Valid situation to not send these is + * initial response or bare ItemSetChange event. + */ + rowChanges.add(new Runnable() { + @Override + public void run() { + rpc.insertRowData(index, count); + } + }); + activeRowHandler.insertRows(index, count); } /** * Informs the client side that rows have been removed from the data source. * - * @param firstIndex + * @param index * the index of the first row removed * @param count * the number of rows removed * @param firstItemId * the item id of the first removed item */ - private void removeRowData(int firstIndex, int count) { - if (clientInitialized) { - rpc.removeRowData(firstIndex, count); + private void removeRowData(final int index, final int count) { + if (rowChanges.isEmpty()) { + markAsDirty(); } - activeRowHandler.removeRows(firstIndex, count); + /* See comment in insertRowData */ + rowChanges.add(new Runnable() { + @Override + public void run() { + rpc.removeRowData(index, count); + } + }); + + activeRowHandler.removeRows(index, count); } /** @@ -922,14 +978,10 @@ public class RpcDataProviderExtension extends AbstractExtension { * Pushes a new version of all the rows in the active cache range. */ public void refreshCache() { - if (!clientInitialized) { - return; + if (!refreshCache) { + refreshCache = true; + markAsDirty(); } - - int firstRow = activeRowHandler.activeRange.getStart(); - int numberOfRows = activeRowHandler.activeRange.length(); - - pushRowData(firstRow, numberOfRows, firstRow, numberOfRows); } @Override diff --git a/uitest/src/com/vaadin/tests/components/grid/GridInTabSheet.java b/uitest/src/com/vaadin/tests/components/grid/GridInTabSheet.java index 6c7f254a0d..cddbd390f2 100644 --- a/uitest/src/com/vaadin/tests/components/grid/GridInTabSheet.java +++ b/uitest/src/com/vaadin/tests/components/grid/GridInTabSheet.java @@ -20,6 +20,8 @@ import com.vaadin.tests.components.AbstractTestUI; import com.vaadin.ui.Button; import com.vaadin.ui.Button.ClickEvent; import com.vaadin.ui.Grid; +import com.vaadin.ui.Grid.CellReference; +import com.vaadin.ui.Grid.CellStyleGenerator; import com.vaadin.ui.Grid.SelectionMode; import com.vaadin.ui.Label; import com.vaadin.ui.TabSheet; @@ -36,8 +38,8 @@ public class GridInTabSheet extends AbstractTestUI { grid.addRow(i); } - sheet.addTab(grid); - sheet.addTab(new Label("Hidden")); + sheet.addTab(grid, "Grid"); + sheet.addTab(new Label("Hidden"), "Label"); addComponent(sheet); addComponent(new Button("Add row to Grid", new Button.ClickListener() { @@ -64,6 +66,28 @@ public class GridInTabSheet extends AbstractTestUI { } } })); - } + addComponent(new Button("Add CellStyleGenerator", + new Button.ClickListener() { + @Override + public void buttonClick(ClickEvent event) { + grid.setCellStyleGenerator(new CellStyleGenerator() { + @Override + public String getStyle(CellReference cellReference) { + int rowIndex = ((Integer) cellReference + .getItemId()).intValue(); + Object propertyId = cellReference + .getPropertyId(); + if (rowIndex % 4 == 1) { + return null; + } else if (rowIndex % 4 == 3 + && "Column 1".equals(propertyId)) { + return null; + } + return propertyId.toString().replace(' ', '_'); + } + }); + } + })); + } } diff --git a/uitest/src/com/vaadin/tests/components/grid/GridInTabSheetTest.java b/uitest/src/com/vaadin/tests/components/grid/GridInTabSheetTest.java index cd165e4678..168496e9df 100644 --- a/uitest/src/com/vaadin/tests/components/grid/GridInTabSheetTest.java +++ b/uitest/src/com/vaadin/tests/components/grid/GridInTabSheetTest.java @@ -23,6 +23,7 @@ import org.junit.Test; import com.vaadin.testbench.elements.ButtonElement; import com.vaadin.testbench.elements.GridElement; import com.vaadin.testbench.elements.NotificationElement; +import com.vaadin.testbench.elements.TabSheetElement; import com.vaadin.tests.annotations.TestCategory; import com.vaadin.tests.tb3.MultiBrowserTest; @@ -43,10 +44,45 @@ public class GridInTabSheetTest extends MultiBrowserTest { assertEquals("" + (100 + i), getGridElement().getCell(i, 1) .getText()); } + + assertNoNotification(); + } + + private void assertNoNotification() { assertFalse("There was an unexpected error notification", isElementPresent(NotificationElement.class)); } + @Test + public void testAddManyRowsWhenGridIsHidden() { + setDebug(true); + openTestURL(); + + TabSheetElement tabsheet = $(TabSheetElement.class).first(); + tabsheet.openTab("Label"); + for (int i = 0; i < 50; ++i) { + addGridRow(); + } + + tabsheet.openTab("Grid"); + + assertNoNotification(); + } + + @Test + public void testAddCellStyleGeneratorWhenGridIsHidden() { + setDebug(true); + openTestURL(); + + TabSheetElement tabsheet = $(TabSheetElement.class).first(); + tabsheet.openTab("Label"); + addCellStyleGenerator(); + + tabsheet.openTab("Grid"); + + assertNoNotification(); + } + private void removeGridRow() { $(ButtonElement.class).caption("Remove row from Grid").first().click(); } @@ -55,6 +91,11 @@ public class GridInTabSheetTest extends MultiBrowserTest { $(ButtonElement.class).caption("Add row to Grid").first().click(); } + private void addCellStyleGenerator() { + $(ButtonElement.class).caption("Add CellStyleGenerator").first() + .click(); + } + private GridElement getGridElement() { return $(GridElement.class).first(); } -- 2.39.5