From 97d80510edf06d954c7589864e707d89d60b3e97 Mon Sep 17 00:00:00 2001 From: Anthony Guerreiro Date: Wed, 9 Jul 2014 16:48:28 +0300 Subject: [PATCH] Fix table.setCurrentPageFirstItemIndex(n) to load only needed rows (#14135) Change-Id: I0186ce32f915b39a012bb653e501b0cad72a9f32 --- .../com/vaadin/client/ui/VScrollTable.java | 37 +++++- .../SetPageFirstItemLoadsNeededRowsOnly.java | 109 ++++++++++++++++++ ...tPageFirstItemLoadsNeededRowsOnlyTest.java | 58 ++++++++++ 3 files changed, 202 insertions(+), 2 deletions(-) create mode 100644 uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnly.java create mode 100644 uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnlyTest.java diff --git a/client/src/com/vaadin/client/ui/VScrollTable.java b/client/src/com/vaadin/client/ui/VScrollTable.java index 6a2a753d0e..6a1e8664ed 100644 --- a/client/src/com/vaadin/client/ui/VScrollTable.java +++ b/client/src/com/vaadin/client/ui/VScrollTable.java @@ -196,6 +196,8 @@ public class VScrollTable extends FlowPanel implements HasWidgets, /** For internal use only. May be removed or replaced in the future. */ public boolean immediate; + private boolean updatedReqRows = true; + private boolean nullSelectionAllowed = true; private SelectMode selectMode = SelectMode.NONE; @@ -2393,10 +2395,32 @@ public class VScrollTable extends FlowPanel implements HasWidgets, // if client connection is busy, don't bother loading it more VConsole.log("Postponed rowfetch"); schedule(250); + } else if (!updatedReqRows && allRenderedRowsAreNew()) { + + /* + * If all rows are new, there might have been a server-side call + * to Table.setCurrentPageFirstItemIndex(int) In this case, + * scrolling event takes way too late, and all the rows from + * previous viewport to this one were requested. + * + * This should prevent requesting unneeded rows by updating + * reqFirstRow and reqRows before needing them. See (#14135) + */ + + setReqFirstRow((firstRowInViewPort - (int) (pageLength * cache_rate))); + int last = firstRowInViewPort + (int) (cache_rate * pageLength) + + pageLength - 1; + if (last >= totalRows) { + last = totalRows - 1; + } + setReqRows(last - getReqFirstRow() + 1); + updatedReqRows = true; + schedule(250); } else { int firstRendered = scrollBody.getFirstRendered(); int lastRendered = scrollBody.getLastRendered(); + if (lastRendered > totalRows) { lastRendered = totalRows - 1; } @@ -6267,6 +6291,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets, public Widget getWidgetForPaintable() { return this; } + } protected class VScrollTableGeneratedRow extends VScrollTableRow { @@ -6998,8 +7023,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets, return; } - if (firstRowInViewPort - pageLength * cache_rate > lastRendered - || firstRowInViewPort + pageLength + pageLength * cache_rate < firstRendered) { + if (allRenderedRowsAreNew()) { // need a totally new set of rows rowRequestHandler .setReqFirstRow((firstRowInViewPort - (int) (pageLength * cache_rate))); @@ -7010,6 +7034,7 @@ public class VScrollTable extends FlowPanel implements HasWidgets, } rowRequestHandler.setReqRows(last - rowRequestHandler.getReqFirstRow() + 1); + updatedReqRows = false; rowRequestHandler.deferRowFetch(); return; } @@ -7032,6 +7057,14 @@ public class VScrollTable extends FlowPanel implements HasWidgets, } } + private boolean allRenderedRowsAreNew() { + int firstRowInViewPort = calcFirstRowInViewPort(); + int firstRendered = scrollBody.getFirstRendered(); + int lastRendered = scrollBody.getLastRendered(); + return (firstRowInViewPort - pageLength * cache_rate > lastRendered || firstRowInViewPort + + pageLength + pageLength * cache_rate < firstRendered); + } + protected int calcFirstRowInViewPort() { return (int) Math.ceil(scrollTop / scrollBody.getRowHeight()); } diff --git a/uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnly.java b/uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnly.java new file mode 100644 index 0000000000..e643ef5fa0 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnly.java @@ -0,0 +1,109 @@ +/* + * Copyright 2000-2014 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.tests.components.table; + +import java.io.Serializable; +import java.util.List; + +import com.vaadin.data.util.BeanContainer; +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.ui.Label; +import com.vaadin.ui.Table; +import com.vaadin.ui.VerticalLayout; + +/** + * + * @author Vaadin Ltd + */ + +@SuppressWarnings("serial") +public class SetPageFirstItemLoadsNeededRowsOnly extends AbstractTestUI { + + /* + * (non-Javadoc) + * + * @see com.vaadin.tests.components.AbstractTestUI#setup(com.vaadin.server. + * VaadinRequest) + */ + @Override + protected void setup(VaadinRequest request) { + final VerticalLayout layout = new VerticalLayout(); + addComponent(layout); + + final Label label = new Label(""); + addComponent(label); + + BeanContainer beans = new BeanContainer( + Bean.class) { + @Override + public List getItemIds(int startIndex, int numberOfIds) { + label.setValue("rows requested: " + numberOfIds); + return super.getItemIds(startIndex, numberOfIds); + } + }; + + beans.setBeanIdProperty("i"); + for (int i = 0; i < 2000; i++) { + beans.addBean(new Bean(i)); + } + + final Table table = new Table("Beans", beans); + table.setVisibleColumns(new Object[] { "i" }); + layout.addComponent(table); + + table.setCurrentPageFirstItemIndex(table.size() - 1); + } + + /* + * (non-Javadoc) + * + * @see com.vaadin.tests.components.AbstractTestUI#getTestDescription() + */ + @Override + protected String getTestDescription() { + return "Only cached rows and rows in viewport should be rendered after " + + "calling table.setCurrentPageFirstItemIndex(n) - as opposed to all rows " + + "between the previous position and new position"; + } + + /* + * (non-Javadoc) + * + * @see com.vaadin.tests.components.AbstractTestUI#getTicketNumber() + */ + @Override + protected Integer getTicketNumber() { + return 14135; + } + + public class Bean implements Serializable { + + private Integer i; + + public Bean(Integer i) { + this.i = i; + } + + public Integer getI() { + return i; + } + + public void setI(Integer i) { + this.i = i; + } + } +} diff --git a/uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnlyTest.java b/uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnlyTest.java new file mode 100644 index 0000000000..ebf53e9a6d --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/table/SetPageFirstItemLoadsNeededRowsOnlyTest.java @@ -0,0 +1,58 @@ +/* + * Copyright 2000-2014 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.tests.components.table; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +import com.vaadin.testbench.elements.LabelElement; +import com.vaadin.tests.tb3.MultiBrowserTest; + +/** + * + * @author Vaadin Ltd + */ +public class SetPageFirstItemLoadsNeededRowsOnlyTest extends MultiBrowserTest { + + /* + * expectedRowsRequested is related to VScrollTable's cache_rate and + * pageLength. See for instance VScrollTable.ensureCacheFilled(). + * + * This also takes into account if the visible rows are at the very start or + * end of the table, if the user scrolled or the + * Table.setCurrentPageFirstItemIndex(int) method was used. + * + * This value should not change if cache_rate and pageLength are not changed + * as well, and if this test remains constant: the table is scrolled to the + * very end (done in the actual UI: SetPageFirstItemLoadsNeededRowsOnly). + */ + private int expectedRowsRequested = 45; + + @Test + public void verifyLoadedRows() throws InterruptedException { + + openTestURL(); + + // wait for events to be processed in UI after loading page + sleep(2000); + + String labelValue = $(LabelElement.class).get(1).getText(); + String expectedLabelValue = "rows requested: " + expectedRowsRequested; + String errorMessage = "Too many rows were requested"; + assertEquals(errorMessage, expectedLabelValue, labelValue); + } +} -- 2.39.5