From d98b08ba9e5135b6d43bfb330a53042122b3283c Mon Sep 17 00:00:00 2001 From: Klaudeta <31614592+Klaudeta@users.noreply.github.com> Date: Thu, 21 Mar 2019 07:56:20 +0200 Subject: [PATCH] Make improve of caching for hierarchical data optional (#11501) Make improve of caching for hierarchical data optional Fixes #11477 --- .../client/data/AbstractRemoteDataSource.java | 87 ++++++++++---- .../connectors/RpcDataSourceConnector.java | 5 + .../GridIndexedContainerInsertSelect.java | 108 ++++++++++++++++++ .../GridIndexedContainerInsertSelectTest.java | 66 +++++++++++ 4 files changed, 242 insertions(+), 24 deletions(-) create mode 100644 uitest/src/main/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelect.java create mode 100644 uitest/src/test/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelectTest.java diff --git a/client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java b/client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java index 7a52b0ef954..2992d39d1f9 100644 --- a/client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java +++ b/client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java @@ -202,6 +202,21 @@ public void updateRow() { */ private Map invalidatedRows; + /** + * Tracking the invalidated rows inside {{@link #insertRowData(int, int)}} + * and then filling cache from those invalidated rows is a feature + * introduced to improve caching in hierarchical data in V8, but as this + * interface is also shared with V7 compatibility package, this change + * causes this issue: issue https://github.com/vaadin/framework/issues/11477 + * + * By having {#AbstractRemoteDataSource} define whether or not to track and + * then fill cache with the invalidated rows, allows different + * implementation of {#AbstractRemoteDataSource} to enable/disable this + * feature, as a consequence it is possible for V7 compatibility-package of + * this class to disabled it and fix the above issue. + */ + private boolean trackInvalidatedRows = true; + private Set dataChangeHandlers = new LinkedHashSet<>(); private CacheStrategy cacheStrategy = new CacheStrategy.DefaultCacheStrategy(); @@ -526,7 +541,10 @@ protected void setRowData(int firstRowIndex, List rowData) { if (!cached.isEmpty()) { cached = cached.combineWith(newUsefulData); // Attempt to restore invalidated items - fillCacheFromInvalidatedRows(maxCacheRange); + if (trackInvalidatedRows) { + fillCacheFromInvalidatedRows(maxCacheRange); + } + } else { cached = newUsefulData; } @@ -721,31 +739,21 @@ protected void insertRowData(int firstRowIndex, int count) { } } else if (cached.contains(firstRowIndex)) { int oldCacheEnd = cached.getEnd(); - /* - * We need to invalidate the cache from the inserted row onwards, - * since the cache wants to be a contiguous range. It doesn't - * support holes. - * - * If holes were supported, we could shift the higher part of - * "cached" and leave a hole the size of "count" in the middle. - */ - Range[] splitAt = cached.splitAt(firstRowIndex); - cached = splitAt[0]; - Range invalid = splitAt[1]; - /* - * If we already have a map in invalidatedRows, we're in a state - * where multiple row manipulations without data received have - * happened and the cache restoration is prevented completely. - */ + Range[] splitOldCache = cached.splitAt(firstRowIndex); + cached = splitOldCache[0]; + Range invalidated = splitOldCache[1]; - if (!invalid.isEmpty() && invalidatedRows == null) { - invalidatedRows = new HashMap<>(); - // Store all invalidated items to a map. Indices are updated to - // match what they should be after the insertion. - for (int i = invalid.getStart(); i < invalid.getEnd(); ++i) { - invalidatedRows.put(i + count, indexToRowMap.get(i)); - } + if (trackInvalidatedRows) { + /* + * We need to invalidate the cache from the inserted row onwards, + * since the cache wants to be a contiguous range. It doesn't + * support holes. + * + * If holes were supported, we could shift the higher part of + * "cached" and leave a hole the size of "count" in the middle. + */ + trackInvalidatedRowsFromCache(invalidated, count); } for (int i = firstRowIndex; i < oldCacheEnd; i++) { @@ -761,6 +769,24 @@ protected void insertRowData(int firstRowIndex, int count) { Profiler.leave("AbstractRemoteDataSource.insertRowData"); } + private void trackInvalidatedRowsFromCache(Range invalidated, int insertedRowCount){ + /* + * If we already have a map in invalidatedRows, we're in a state + * where multiple row manipulations without data received have + * happened and the cache restoration is prevented completely. + */ + + if (!invalidated.isEmpty() && invalidatedRows == null) { + invalidatedRows = new HashMap<>(); + // Store all invalidated items to a map. Indices are updated + // to match what they should be after the insertion. + for (int i = invalidated.getStart(); i < invalidated + .getEnd(); ++i) { + invalidatedRows.put(i + insertedRowCount, indexToRowMap.get(i)); + } + } + } + @SuppressWarnings("boxing") private void moveRowFromIndexToIndex(int oldIndex, int newIndex) { T row = indexToRowMap.remove(oldIndex); @@ -934,4 +960,17 @@ protected boolean isPinned(T row) { protected boolean canFetchData() { return true; } + + /** + * Sets whether or not to track invalidated rows inside + * {@link #insertRowData(int, int)} and use them to fill cache when + * {{@link #setRowData(int, List)}} is called. + * + * @param trackInvalidatedRows + * a boolean value specifying if to track invalidated rows or + * not, default value true + */ + public void setTrackInvalidatedRows(boolean trackInvalidatedRows) { + this.trackInvalidatedRows = trackInvalidatedRows; + } } diff --git a/compatibility-client/src/main/java/com/vaadin/v7/client/connectors/RpcDataSourceConnector.java b/compatibility-client/src/main/java/com/vaadin/v7/client/connectors/RpcDataSourceConnector.java index 0b04d204ad6..b764bfc45bf 100644 --- a/compatibility-client/src/main/java/com/vaadin/v7/client/connectors/RpcDataSourceConnector.java +++ b/compatibility-client/src/main/java/com/vaadin/v7/client/connectors/RpcDataSourceConnector.java @@ -70,6 +70,11 @@ interface DetailsListener { public class RpcDataSource extends AbstractRemoteDataSource { protected RpcDataSource() { + + /*Fixes issue https://github.com/vaadin/framework/issues/11477 + More details in AbstractRemoteDataSource.trackInvalidatedRows private field*/ + setTrackInvalidatedRows(false); + registerRpc(DataProviderRpc.class, new DataProviderRpc() { @Override public void setRowData(int firstRow, JsonArray rowArray) { diff --git a/uitest/src/main/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelect.java b/uitest/src/main/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelect.java new file mode 100644 index 00000000000..27678aedfd5 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelect.java @@ -0,0 +1,108 @@ +package com.vaadin.v7.tests.components.grid.basicfeatures.server; + +import com.vaadin.annotations.Widgetset; +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractReindeerTestUI; +import com.vaadin.ui.Button; +import com.vaadin.v7.data.Item; +import com.vaadin.v7.data.sort.Sort; +import com.vaadin.v7.data.util.AbstractInMemoryContainer; +import com.vaadin.v7.data.util.IndexedContainer; +import com.vaadin.v7.ui.CheckBox; +import com.vaadin.v7.ui.Grid; +import com.vaadin.v7.ui.HorizontalLayout; +import com.vaadin.v7.ui.VerticalLayout; + +@Widgetset("com.vaadin.v7.Vaadin7WidgetSet") +public class GridIndexedContainerInsertSelect extends AbstractReindeerTestUI { + + private static final String INFO_COLUMN_PATTERN = "InsertedIndex: %d ItemUid: %d SelectedRowIndex: %d SelectedItemUid: %s"; + private static final String COLUMN1 = "Column1"; + private static final String COLUMN2 = "Column2"; + private int currInsertionIndex = 4; + private VerticalLayout layout; + private Grid grid; + private IndexedContainer container; + + protected void setup(VaadinRequest vaadinRequest) { + layout = new VerticalLayout(); + layout.setSizeFull(); + + initGrid(); + initData(container); + selectFirstRow(); + + CheckBox checkBox = new CheckBox("Select row after insert", true); + + Button.ClickListener clickListener = e -> { + addNewItem(); + selectOnlyLastItemIfRequested(checkBox); + currInsertionIndex++; + }; + + Button button = new Button("Add row after selected row!", + clickListener); + + HorizontalLayout inputBox = new HorizontalLayout(); + + inputBox.addComponents(checkBox, button); + + layout.addComponents(grid, inputBox); + + addComponent(layout); + } + + private void initGrid() { + grid = new Grid(); + grid.setSizeFull(); + container = new IndexedContainer(); + container.addContainerProperty(COLUMN1, String.class, null); + container.addContainerProperty(COLUMN2, String.class, null); + grid.setContainerDataSource(container); + } + + private void selectFirstRow() { + grid.select("1"); + } + + private void initData(IndexedContainer container) { + createRowItem("1", "Item 1", "ItemCol2 1", container); + createRowItem("2", "Item 2", "ItemCol2 2", container); + createRowItem("3", "Item 3", "ItemCol2 3", container); + } + + private void selectOnlyLastItemIfRequested(CheckBox checkBox) { + if (checkBox.getValue()) { + grid.select(currInsertionIndex + ""); + } + } + + private void addNewItem() { + final Object selectedRow = grid.getSelectedRow(); + int currentIndex = container.indexOfId(selectedRow); + int insertIndex = currentIndex + 1; + final Item thirdItem = container.addItemAt(insertIndex, + currInsertionIndex + ""); + setRowData(thirdItem, "Item " + currInsertionIndex, + getInfoColumnContent(selectedRow, currentIndex, insertIndex)); + } + + private void setRowData(Item thirdItem, String column1Data, + String column2Data) { + thirdItem.getItemProperty(COLUMN1).setValue(column1Data); + thirdItem.getItemProperty(COLUMN2).setValue(column2Data); + } + + private String getInfoColumnContent(Object selectedRow, int currentIndex, + int insertIndex) { + return String.format(INFO_COLUMN_PATTERN, insertIndex, + currInsertionIndex, currentIndex, selectedRow); + } + + private void createRowItem(String id, String column1, String column2, + AbstractInMemoryContainer container) { + Item firstRow = container.addItem(id); + setRowData(firstRow, column1, column2); + } + +} diff --git a/uitest/src/test/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelectTest.java b/uitest/src/test/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelectTest.java new file mode 100644 index 00000000000..e09fa5f560e --- /dev/null +++ b/uitest/src/test/java/com/vaadin/v7/tests/components/grid/basicfeatures/server/GridIndexedContainerInsertSelectTest.java @@ -0,0 +1,66 @@ +package com.vaadin.v7.tests.components.grid.basicfeatures.server; + +import com.vaadin.testbench.elements.ButtonElement; +import com.vaadin.testbench.elements.GridElement; +import com.vaadin.testbench.parallel.TestCategory; +import com.vaadin.tests.tb3.SingleBrowserTest; +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; + +@TestCategory("grid") +public class GridIndexedContainerInsertSelectTest extends SingleBrowserTest { + + @Override + public void setup() throws Exception { + + super.setup(); + openTestURL(); + waitForElementPresent(By.className("v-grid")); + } + + /** + * Test asserting that issue https://github.com/vaadin/framework/issues/11477 + * is fixed. + */ + @Test + public void test_insertRowAfterSelected_newRowIsSelected() { + openTestURL(); + + // Assert that first row is already selected when ui loaded + Assert.assertTrue( + "First row should be selected to continue with the test!", + isRowSelected(0)); + + // Add new row after the selected one + $(ButtonElement.class).first().click(); + + // Assert that the new row is added correctly + Assert.assertEquals("Item 4", + $(GridElement.class).first().getRow(1).getCell(0).getText()); + + // Assert that the new added row is selected + Assert.assertTrue("Newly inserted row should be selected!", + isRowSelected(1)); + + // Select row at index 2 + $(GridElement.class).first().getRow(2).click(); + + // Add new row after the selected one + $(ButtonElement.class).first().click(); + + // Assert that the new row is added correctly + Assert.assertEquals("Item 5", + $(GridElement.class).first().getRow(3).getCell(0).getText()); + + // Assert that the new added row is selected + Assert.assertTrue("Newly inserted row should be selected!", + isRowSelected(3)); + + } + + protected boolean isRowSelected(int index) { + return $(GridElement.class).first().getRow(index).isSelected(); + } + +}