diff options
3 files changed, 246 insertions, 85 deletions
diff --git a/server/src/com/vaadin/ui/Table.java b/server/src/com/vaadin/ui/Table.java index 3a1ab82be5..34d48a9b18 100644 --- a/server/src/com/vaadin/ui/Table.java +++ b/server/src/com/vaadin/ui/Table.java @@ -422,61 +422,11 @@ public class Table extends AbstractSelect implements Action.Container, private Object currentPageFirstItemId = null; /* - * This class stores the hashcode and scroll position of the previous - * container so that the scroll position can be restored if the same - * container is removed and then re-added. This resolves #14581. + * If all rows get removed then scroll position of the previous container + * can be restored after re-adding/replacing rows via addAll(). This + * resolves #14581. */ - protected static class ScrollPositionRepairOnReAddAllRowsData implements - Serializable { - - private static final long serialVersionUID = 1L; - // current page first item index (to repair scroll position) - private int itemIndex; - /* - * hashCode() of container before it was cleared via - * container.removeAllItems(); - */ - private int containerHashCode; - - public ScrollPositionRepairOnReAddAllRowsData() { - itemIndex = -1; - containerHashCode = -1; - } - - public int getItemId() { - return itemIndex; - } - - public void setItemId(int itemId) { - itemIndex = itemId; - } - - public void resetItemId() { - itemIndex = -1; - } - - public void setContainerData(Container container) { - if (container != null) { - containerHashCode = container.hashCode(); - } else { - containerHashCode = -1; - } - } - - public boolean needRepairScrollPosition(Container newContainer) { - return (itemIndex != -1) && isTheSameContainer(newContainer); - } - - private boolean isTheSameContainer(Container newContainer) { - boolean theSame = false; - if (newContainer != null) { - theSame = (newContainer.hashCode() == containerHashCode); - } - return theSame; - } - } - - private final ScrollPositionRepairOnReAddAllRowsData scrollRepairOnReAdding = new ScrollPositionRepairOnReAddAllRowsData(); + private int repairOnReAddAllRowsDataScrollPositionItemIndex = -1; /** * Index of the first item on the current page. @@ -2734,10 +2684,6 @@ public class Table extends AbstractSelect implements Action.Container, newDataSource = new IndexedContainer(); } - if (scrollRepairOnReAdding != null) { - // this is important if container is set for table after filling - scrollRepairOnReAdding.setContainerData(newDataSource); - } Collection<Object> generated; if (columnGenerators != null) { generated = columnGenerators.keySet(); @@ -4571,14 +4517,22 @@ public class Table extends AbstractSelect implements Action.Container, int currentFirstItemIndex = getCurrentPageFirstItemIndex(); if (event.getContainer().size() == 0) { - scrollRepairOnReAdding.setItemId(getCurrentPageFirstItemIndex()); + repairOnReAddAllRowsDataScrollPositionItemIndex = getCurrentPageFirstItemIndex(); } else { - if (scrollRepairOnReAdding.needRepairScrollPosition(event - .getContainer())) { - currentFirstItemIndex = scrollRepairOnReAdding.getItemId(); + if (repairOnReAddAllRowsDataScrollPositionItemIndex != -1) { + currentFirstItemIndex = repairOnReAddAllRowsDataScrollPositionItemIndex; + /* + * Reset repairOnReAddAllRowsDataScrollPositionItemIndex. + * + * Next string should be commented (removed) if we want to have + * possibility to restore scroll position during adding items to + * container one by one via add() but not only addAll(). The + * problem in this case: we cannot track what happened between + * add() and add()... So it is ambiguous where to stop restore + * scroll position. + */ + repairOnReAddAllRowsDataScrollPositionItemIndex = -1; } - scrollRepairOnReAdding.resetItemId(); - scrollRepairOnReAdding.setContainerData(event.getContainer()); } // ensure that page still has first item in page, ignore buffer refresh diff --git a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java index d9cbf007df..df06580dae 100644 --- a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java +++ b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRows.java @@ -11,6 +11,12 @@ import com.vaadin.ui.Button; import com.vaadin.ui.Button.ClickListener; import com.vaadin.ui.Table; +/** + * Scroll position should be restored when removing and re-adding all rows in + * Table. + * + * @author Vaadin Ltd + */ public class TableRepairsScrollPositionOnReAddingAllRows extends AbstractTestUI { private static final long serialVersionUID = 1L; @@ -19,32 +25,130 @@ public class TableRepairsScrollPositionOnReAddingAllRows extends AbstractTestUI protected void setup(VaadinRequest request) { final BeanItemContainer<TableItem> cont = new BeanItemContainer<TableItem>( TableItem.class); - final List<TableItem> itemList = new ArrayList<TableItem>(); + final List<TableItem> restoringItemList = new ArrayList<TableItem>(); - Button button1 = new Button("ReAdd rows"); - button1.setId("button1"); - button1.addClickListener(new ClickListener() { + final Table table = new Table(); + table.setWidth("400px"); + table.setPageLength(-1); + table.setContainerDataSource(cont); + table.setSelectable(true); + + Button buttonRestore = new Button("Restore table rows"); + buttonRestore.setId("buttonRestore"); + buttonRestore.addClickListener(new ClickListener() { @Override public void buttonClick(com.vaadin.ui.Button.ClickEvent event) { cont.removeAllItems(); - cont.addAll(itemList); + cont.addAll(restoringItemList); } }); + Button buttonReAddAllViaAddAll = new Button("Re-add rows all at once"); + buttonReAddAllViaAddAll.setId("buttonReAddAllViaAddAll"); + buttonReAddAllViaAddAll.addClickListener(new ClickListener() { + + @Override + public void buttonClick(com.vaadin.ui.Button.ClickEvent event) { + List<TableItem> originalItemIds = new ArrayList<TableItem>(cont + .getItemIds()); + cont.removeAllItems(); + cont.addAll(originalItemIds); + } + }); + + Button buttonReplaceByAnotherCollectionViaAddAll = new Button( + "Replace by another items (via addAll())"); + buttonReplaceByAnotherCollectionViaAddAll + .setId("buttonReplaceByAnotherCollectionViaAddAll"); + buttonReplaceByAnotherCollectionViaAddAll + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + // create new collection (of different items) with other + // size + List<TableItem> itemList = new ArrayList<TableItem>(); + for (int i = 0; i < 79; i++) { + TableItem ti = new TableItem(); + ti.setName("AnotherItem1_" + i); + itemList.add(ti); + } + cont.addAll(itemList); + } + }); + + Button buttonReplaceByAnotherCollectionViaAdd = new Button( + "Replace by another items (via add(), add()..)"); + buttonReplaceByAnotherCollectionViaAdd + .setId("buttonReplaceByAnotherCollectionViaAdd"); + buttonReplaceByAnotherCollectionViaAdd + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + for (int i = 0; i < 81; i++) { + TableItem ti = new TableItem(); + ti.setName("AnotherItem2_" + i); + // add one by one in container + cont.addBean(ti); + } + } + }); + + Button buttonReplaceBySubsetOfSmallerSize = new Button( + "Replace rows by sub-set of smaller size (size not enought for restoring scroll position)"); + buttonReplaceBySubsetOfSmallerSize + .setId("buttonReplaceBySubsetOfSmallerSize"); + buttonReplaceBySubsetOfSmallerSize + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + cont.addAll(restoringItemList.subList(0, 20)); + } + }); + + Button buttonReplaceByWholeSubsetPlusOnNew = new Button( + "Replace rows by whole subset plus one new item"); + buttonReplaceByWholeSubsetPlusOnNew + .setId("buttonReplaceByWholeSubsetPlusOnNew"); + buttonReplaceByWholeSubsetPlusOnNew + .addClickListener(new ClickListener() { + + @Override + public void buttonClick( + com.vaadin.ui.Button.ClickEvent event) { + cont.removeAllItems(); + + List<TableItem> list = new ArrayList<TableItem>( + restoringItemList); + TableItem ti = new TableItem(); + ti.setName("AnotherItem3_" + 80); + list.add(ti); + cont.addAll(list); + } + }); + for (int i = 0; i < 80; i++) { TableItem ti = new TableItem(); - ti.setName("Name_" + i); - itemList.add(ti); + ti.setName("Item_" + i); + restoringItemList.add(ti); cont.addBean(ti); } - final Table table = new Table(); - table.setPageLength(-1); - table.setContainerDataSource(cont); - table.setSelectable(true); - - getLayout().addComponent(button1); + getLayout().addComponent(buttonReAddAllViaAddAll); + getLayout().addComponent(buttonReplaceByAnotherCollectionViaAddAll); + getLayout().addComponent(buttonReplaceByAnotherCollectionViaAdd); + getLayout().addComponent(buttonReplaceBySubsetOfSmallerSize); + getLayout().addComponent(buttonReplaceByWholeSubsetPlusOnNew); + getLayout().addComponent(buttonRestore); getLayout().addComponent(table); } diff --git a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java index 2fbdd8b581..a3e7f29dfe 100644 --- a/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java +++ b/uitest/src/com/vaadin/tests/components/table/TableRepairsScrollPositionOnReAddingAllRowsTest.java @@ -31,6 +31,12 @@ import com.vaadin.testbench.screenshot.ImageComparison; import com.vaadin.testbench.screenshot.ReferenceNameGenerator; import com.vaadin.tests.tb3.MultiBrowserTest; +/** + * Scroll position should be restored when removing and re-adding all rows in + * Table. + * + * @author Vaadin Ltd + */ public class TableRepairsScrollPositionOnReAddingAllRowsTest extends MultiBrowserTest { @@ -39,29 +45,126 @@ public class TableRepairsScrollPositionOnReAddingAllRowsTest extends throws InterruptedException { openTestURL(); - WebElement buttonReAddRows = findElement(By.id("button1")); + WebElement row0 = $(TableElement.class).first().getCell(0, 0); + int rowLocation0 = row0.getLocation().getY(); + // case 1 scrollUp(); waitUntilNot(new ExpectedCondition<Boolean>() { @Override public Boolean apply(WebDriver input) { - return $(TableElement.class).first().getCell(49, 0) == null; + return $(TableElement.class).first().getCell(48, 0) == null; } }, 10); - WebElement row = $(TableElement.class).first().getCell(49, 0); + WebElement row = $(TableElement.class).first().getCell(48, 0); int rowLocation = row.getLocation().getY(); - buttonReAddRows.click(); + // This button is for re-adding all rows (original itemIds) at once + // (removeAll() + addAll()) + hitButton("buttonReAddAllViaAddAll"); - row = $(TableElement.class).first().getCell(49, 0); + row = $(TableElement.class).first().getCell(48, 0); int newRowLocation = row.getLocation().getY(); // ranged check because IE9 consistently misses the mark by 1 pixel assertThat( - "Scroll position should be the same as before Re-Adding all rows", - (double) newRowLocation, closeTo(rowLocation, 1)); + "Scroll position should be the same as before Re-Adding rows via addAll()", + (double) newRowLocation, + closeTo(rowLocation, row.getSize().height + 1)); + + // case 2 + scrollUp(); + + waitUntilNot(new ExpectedCondition<Boolean>() { + @Override + public Boolean apply(WebDriver input) { + return $(TableElement.class).first().getCell(48, 0) == null; + } + }, 10); + + row = $(TableElement.class).first().getCell(48, 0); + rowLocation = row.getLocation().getY(); + + // This button is for replacing all rows at once (removeAll() + + // addAll()) + hitButton("buttonReplaceByAnotherCollectionViaAddAll"); + + row = $(TableElement.class).first().getCell(48, 0); + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat( + "Scroll position should be the same as before Replacing rows via addAll()", + (double) newRowLocation, + closeTo(rowLocation, row.getSize().height + 1)); + + // case 3 + // This button is for replacing all rows one by one (removeAll() + add() + // + add()..) + hitButton("buttonReplaceByAnotherCollectionViaAdd"); + + row = $(TableElement.class).first().getCell(0, 0); + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat("Scroll position should be 0", (double) newRowLocation, + closeTo(rowLocation0, 1)); + + // case 4 + // This button is for restoring initial list and for scrolling to 0 + // position + hitButton("buttonRestore"); + scrollUp(); + + waitUntilNot(new ExpectedCondition<Boolean>() { + @Override + public Boolean apply(WebDriver input) { + return $(TableElement.class).first().getCell(48, 0) == null; + } + }, 10); + + // This button is for replacing all rows at once but the count of rows + // is less then first index to scroll + hitButton("buttonReplaceBySubsetOfSmallerSize"); + + row = $(TableElement.class).first().getCell(5, 0); + + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat("Scroll position should be 0", (double) newRowLocation, + closeTo(rowLocation0, 1)); + + // case 5 + // This button is for restoring initial list and for scrolling to 0 + // position + hitButton("buttonRestore"); + scrollUp(); + + waitUntilNot(new ExpectedCondition<Boolean>() { + @Override + public Boolean apply(WebDriver input) { + return $(TableElement.class).first().getCell(48, 0) == null; + } + }, 10); + + row = $(TableElement.class).first().getCell(48, 0); + rowLocation = row.getLocation().getY(); + + // This button is for replacing by whole original sub-set of items plus + // one new + hitButton("buttonReplaceByWholeSubsetPlusOnNew"); + + row = $(TableElement.class).first().getCell(48, 0); + newRowLocation = row.getLocation().getY(); + + // ranged check because IE9 consistently misses the mark by 1 pixel + assertThat("Scroll position should be the same as before Replacing", + (double) newRowLocation, + closeTo(rowLocation, row.getSize().height + 1)); + } private void scrollUp() { @@ -69,6 +172,6 @@ public class TableRepairsScrollPositionOnReAddingAllRowsTest extends By.className("v-table-body-wrapper")); JavascriptExecutor js = new TestBenchCommandExecutor(getDriver(), new ImageComparison(), new ReferenceNameGenerator()); - js.executeScript("arguments[0].scrollTop = " + 1200, actualElement); + js.executeScript("arguments[0].scrollTop = " + 1205, actualElement); } } |