From 8b1b06f571d0abde4e11043823712ee5e4927fe5 Mon Sep 17 00:00:00 2001 From: Zhe Sun <31067185+ZheSun88@users.noreply.github.com> Date: Fri, 2 Jul 2021 19:15:56 +0300 Subject: [PATCH] Ensure removing a row does not cause exceptions in detail row handling. (#12330) (#12331) Fixes: #12328 Authored-by: Anna Koskinen --- .../grid/DetailsManagerConnector.java | 23 +++++++ .../grid/GridRemoveItemAllDetailsOpen.java | 63 +++++++++++++++++++ .../GridRemoveItemAllDetailsOpenTest.java | 62 ++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpen.java create mode 100644 uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpenTest.java diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java index 5290effdfa..b419aef724 100644 --- a/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java +++ b/client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java @@ -17,6 +17,7 @@ package com.vaadin.client.connectors.grid; import java.util.HashMap; import java.util.Map; +import java.util.Map.Entry; import java.util.TreeMap; import com.google.gwt.core.client.Scheduler; @@ -680,6 +681,18 @@ public class DetailsManagerConnector extends AbstractExtensionConnector { getWidget().setDetailsVisible(rowIndex, false); } + private void detachDetailsIfFound(String connectorId) { + if (indexToDetailConnectorId.containsValue(connectorId)) { + for (Entry entry : indexToDetailConnectorId + .entrySet()) { + if (connectorId.equals(entry.getValue())) { + detachDetails(entry.getKey()); + return; + } + } + } + } + private boolean refreshDetails(int rowIndex) { String id = getDetailsComponentConnectorId(rowIndex); String oldId = indexToDetailConnectorId.get(rowIndex); @@ -706,6 +719,11 @@ public class DetailsManagerConnector extends AbstractExtensionConnector { indexToDetailConnectorId.remove(rowIndex); } else { // updated, replace reference + + // ensure that the detail contents aren't still attached to some + // other row that hasn't been refreshed yet + detachDetailsIfFound(id); + indexToDetailConnectorId.put(rowIndex, id); newOrUpdatedDetails = true; getWidget().resetVisibleDetails(rowIndex); @@ -714,6 +732,11 @@ public class DetailsManagerConnector extends AbstractExtensionConnector { // new Details content, listeners will get attached to the connector // when Escalator requests for the Details through // CustomDetailsGenerator#getDetails(int) + + // ensure that the detail contents aren't still attached to some + // other row that hasn't been refreshed yet + detachDetailsIfFound(id); + indexToDetailConnectorId.put(rowIndex, id); newOrUpdatedDetails = true; getWidget().setDetailsVisible(rowIndex, true); diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpen.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpen.java new file mode 100644 index 0000000000..55a604971d --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpen.java @@ -0,0 +1,63 @@ +package com.vaadin.tests.components.grid; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import com.vaadin.data.provider.ListDataProvider; +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.ui.Button; +import com.vaadin.ui.Grid; +import com.vaadin.ui.HorizontalLayout; +import com.vaadin.ui.Label; + +public class GridRemoveItemAllDetailsOpen extends AbstractTestUI { + + @Override + protected void setup(VaadinRequest request) { + List data = new ArrayList<>( + Arrays.asList("row1", "row2", "row3", "row4")); + Grid grid = createGrid(); + + ListDataProvider dataProvider = new ListDataProvider<>(data); + grid.setDataProvider(dataProvider); + data.forEach(item -> grid.setDetailsVisible(item, true)); + + Button removeBtn = new Button("Remove selected item"); + removeBtn.addClickListener(event -> { + data.remove(grid.getSelectedItems().iterator().next()); + dataProvider.refreshAll(); + grid.deselectAll(); + }); + addComponent(removeBtn); + addComponent(grid); + } + + private Grid createGrid() { + Grid grid = new Grid<>(); + grid.setHeight("400px"); + grid.addColumn(item -> item).setCaption("column").setId("column"); + grid.setDetailsGenerator(item -> { + Button closeBtn = new Button("Close"); + closeBtn.addClickListener( + clickEvent -> grid.setDetailsVisible(item, false)); + return new HorizontalLayout(new Label("Item details: " + item), + closeBtn); + }); + grid.addItemClickListener( + itemClick -> grid.setDetailsVisible(itemClick.getItem(), true)); + return grid; + } + + @Override + protected Integer getTicketNumber() { + return 12328; + } + + @Override + protected String getTestDescription() { + return "Removing selected item (first or second)" + + "should not cause a client side exception."; + } +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpenTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpenTest.java new file mode 100644 index 0000000000..25db6e44a8 --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridRemoveItemAllDetailsOpenTest.java @@ -0,0 +1,62 @@ +package com.vaadin.tests.components.grid; + +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +import com.vaadin.testbench.elements.ButtonElement; +import com.vaadin.testbench.elements.GridElement; +import com.vaadin.tests.tb3.MultiBrowserTest; + +public class GridRemoveItemAllDetailsOpenTest extends MultiBrowserTest { + + @Test + public void removeFirstItem() { + openTestURL(); + ButtonElement removeButton = $(ButtonElement.class).first(); + GridElement grid = $(GridElement.class).first(); + + String detailsText = grid.getDetails(2).getText(); + String expected = "Item details: row3"; + assertContains(expected, detailsText); + + grid.getCell(0, 0).click(); + removeButton.click(); + + waitUntilLoadingIndicatorNotVisible(); + + detailsText = grid.getDetails(1).getText(); + assertContains(expected, detailsText); + detailsText = grid.getDetails(2).getText(); + expected = "Item details: row4"; + assertContains(expected, detailsText); + } + + @Test + public void removeSecondItem() { + openTestURL(); + ButtonElement removeButton = $(ButtonElement.class).first(); + GridElement grid = $(GridElement.class).first(); + + String detailsText = grid.getDetails(2).getText(); + String expected = "Item details: row3"; + assertContains(expected, detailsText); + + grid.getCell(1, 0).click(); + removeButton.click(); + + waitUntilLoadingIndicatorNotVisible(); + + detailsText = grid.getDetails(1).getText(); + assertContains(expected, detailsText); + detailsText = grid.getDetails(2).getText(); + expected = "Item details: row4"; + assertContains(expected, detailsText); + } + + private void assertContains(String expected, String detailsText) { + assertTrue("Unexpected details contents: " + detailsText + + " (expected: " + expected + ")", + detailsText.contains(expected)); + } +} -- 2.39.5