]> source.dussan.org Git - vaadin-framework.git/commitdiff
Fix updating Grid's item set when details rows are open. (#12231)
authorAnna Koskinen <Ansku@users.noreply.github.com>
Mon, 8 Mar 2021 11:18:11 +0000 (13:18 +0200)
committerGitHub <noreply@github.com>
Mon, 8 Mar 2021 11:18:11 +0000 (13:18 +0200)
- Old details should close.
- New details should open.
- If some row has details in both old and new item set, the details row
contents should get updated.
- Updating details row contents should not break the positioning of the
rows and details below.

Fixes #12211

client/src/main/java/com/vaadin/client/connectors/grid/DetailsManagerConnector.java
client/src/main/java/com/vaadin/client/widget/escalator/RowContainer.java
client/src/main/java/com/vaadin/client/widgets/Escalator.java
client/src/main/java/com/vaadin/client/widgets/Grid.java
server/src/main/java/com/vaadin/ui/Grid.java
uitest/src/main/java/com/vaadin/tests/components/grid/GridDetailsUpdateItems.java [new file with mode: 0644]
uitest/src/main/java/com/vaadin/tests/widgetset/client/grid/EscalatorProxy.java
uitest/src/test/java/com/vaadin/tests/components/grid/GridDetailsUpdateItemsTest.java [new file with mode: 0644]

index f605329fd913d1db88be325df4e65e79d98d0b17..7e7389e65d0a96bb89a25bcd414d652e4869307e 100644 (file)
@@ -702,6 +702,7 @@ public class DetailsManagerConnector extends AbstractExtensionConnector {
                 // updated, replace reference
                 indexToDetailConnectorId.put(rowIndex, id);
                 newOrUpdatedDetails = true;
+                getWidget().resetVisibleDetails(rowIndex);
             }
         } else {
             // new Details content, listeners will get attached to the connector
index 1b05eeb488b20619ce57e212b2e90a8c27c33a28..fc82bd87d7654e459704676c0a1b51ebf509076a 100644 (file)
@@ -85,6 +85,16 @@ public interface RowContainer {
          */
         boolean spacerExists(int rowIndex);
 
+        /**
+         * Updates the spacer corresponding with the given rowIndex to currently
+         * provided contents.
+         *
+         * @since
+         * @param rowIndex
+         *            the row index for the spacer in need of updating
+         */
+        void resetSpacer(int rowIndex);
+
         /**
          * Sets a new spacer updater.
          * <p>
index 31b439a6d8b54b5a52d6a32aed5a0f0a1d4cd170..f48177fa84ad3bb9c31ef007ea34e6f702f3cc39 100644 (file)
@@ -4964,6 +4964,11 @@ public class Escalator extends Widget
             return spacerContainer.spacerExists(rowIndex);
         }
 
+        @Override
+        public void resetSpacer(int rowIndex) {
+            spacerContainer.resetSpacer(rowIndex);
+        }
+
         @Override
         public void setSpacerUpdater(SpacerUpdater spacerUpdater)
                 throws IllegalArgumentException {
@@ -6111,8 +6116,10 @@ public class Escalator extends Widget
                 root.getStyle().setHeight(height + defaultCellBorderBottomSize,
                         Unit.PX);
 
-                // move the visible spacers getRow row onwards.
-                shiftSpacerPositionsAfterRow(getRow(), heightDiff);
+                if (!delayRepositioning) {
+                    // move the visible spacers getRow row onwards.
+                    shiftSpacerPositionsAfterRow(getRow(), heightDiff);
+                }
 
                 /*
                  * If we're growing, we'll adjust the scroll size first, then
@@ -6178,7 +6185,7 @@ public class Escalator extends Widget
                             tBodyScrollTop + moveDiff);
                     verticalScrollbar.setScrollPosByDelta(moveDiff);
 
-                } else {
+                } else if (!delayRepositioning) {
                     body.shiftRowPositions(getRow(), heightDiff);
                 }
 
@@ -6336,6 +6343,8 @@ public class Escalator extends Widget
         /** Width of the spacers' decos. Calculated once then cached. */
         private double spacerDecoWidth = 0.0D;
 
+        private boolean delayRepositioning = false;
+
         public void setSpacer(int rowIndex, double height)
                 throws IllegalArgumentException {
 
@@ -6376,6 +6385,23 @@ public class Escalator extends Widget
             return false;
         }
 
+        void resetSpacer(int rowIndex) {
+            if (spacerExists(rowIndex)) {
+                delayRepositioning = true;
+                double oldHeight = getSpacer(rowIndex).getHeight();
+                removeSpacer(rowIndex);
+                // real height will be determined later
+                insertNewSpacer(rowIndex, 0);
+                // reposition content below this point to match lack of height,
+                // otherwise later repositioning will fail
+                if (oldHeight > 0) {
+                    shiftSpacerPositionsAfterRow(rowIndex, -oldHeight);
+                    body.shiftRowPositions(rowIndex, -oldHeight);
+                }
+                delayRepositioning = false;
+            }
+        }
+
         @SuppressWarnings("boxing")
         void scrollToSpacer(int spacerIndex, ScrollDestination destination,
                 int padding) {
index 792fcc98b0b89e8b00ae8751e769e92206cf8fea..d2b3860ef663b4021c5b7b5b079ebd83b0e993c8 100755 (executable)
@@ -9661,6 +9661,17 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
         return visibleDetails.contains(Integer.valueOf(rowIndex));
     }
 
+    /**
+     * Reset the details row with current contents.
+     *
+     * @since
+     * @param rowIndex
+     *            the index of the row for which details should be reset
+     */
+    public void resetVisibleDetails(int rowIndex) {
+        escalator.getBody().resetSpacer(rowIndex);
+    }
+
     /**
      * Update details row height.
      *
index ec891396e9f832e65c37fb94ee93e3b37349c04a..f84dbd40cbdae55d4530abab911c5a9722121cc1 100644 (file)
@@ -4997,7 +4997,17 @@ public class Grid<T> extends AbstractListing<T> implements HasComponents,
 
     @Override
     protected void internalSetDataProvider(DataProvider<T, ?> dataProvider) {
+        boolean newProvider = getDataProvider() != dataProvider;
         super.internalSetDataProvider(dataProvider);
+        if (newProvider) {
+            Set<T> oldVisibleDetails = new HashSet<>(
+                    detailsManager.visibleDetails);
+            oldVisibleDetails.forEach(item -> {
+                // close all old details even if the same item exists in the new
+                // provider
+                detailsManager.setDetailsVisible(item, false);
+            });
+        }
         for (Column<T, ?> column : getColumns()) {
             column.updateSortable();
         }
diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridDetailsUpdateItems.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridDetailsUpdateItems.java
new file mode 100644 (file)
index 0000000..d84e046
--- /dev/null
@@ -0,0 +1,74 @@
+package com.vaadin.tests.components.grid;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+
+import com.vaadin.data.ValueProvider;
+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.Label;
+import com.vaadin.ui.VerticalLayout;
+
+public class GridDetailsUpdateItems extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        addComponent(createExamplleLayout());
+    }
+
+    private VerticalLayout createExamplleLayout() {
+        Collection<String> firstCollection = Arrays.asList("Hello", ",",
+                "world!");
+        Collection<String> secondCollection = Arrays.asList("My", "name", "is",
+                "Sarah");
+        Collection<String> thirdCollection = Arrays.asList("red", "blue");
+        Collection<String> fourthCollection = Arrays.asList("spring", "summer",
+                "autumn", "winter");
+
+        VerticalLayout mainLayout = new VerticalLayout();
+        Grid<Collection<String>> grid = new Grid<>();
+        grid.setDetailsGenerator(collection -> {
+            VerticalLayout detailLayout = new VerticalLayout();
+            collection.forEach(
+                    item -> detailLayout.addComponent(new Label(item)));
+            return detailLayout;
+        });
+        ValueProvider<Collection<String>, String> valueProvider = collection -> String
+                .join(" ", collection);
+        grid.addColumn(valueProvider).setCaption("Header");
+
+        List<Collection<String>> itemsInitial = Arrays.asList(firstCollection,
+                secondCollection, thirdCollection, fourthCollection);
+        grid.setItems(itemsInitial);
+        for (Collection<String> tmp : itemsInitial) {
+            grid.setDetailsVisible(tmp, true);
+        }
+        mainLayout.addComponent(grid);
+
+        Button clickButton = new Button("Change items", event -> {
+            List<Collection<String>> itemsOverwrite = Arrays
+                    .asList(secondCollection, fourthCollection);
+            grid.setItems(itemsOverwrite);
+            for (Collection<String> tmp : itemsOverwrite) {
+                grid.setDetailsVisible(tmp, true);
+            }
+        });
+        mainLayout.addComponent(clickButton);
+
+        return mainLayout;
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 12211;
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Details should update and not break the positioning "
+                + "when the item set is changed.";
+    }
+}
index 65fe75e9fa857b01133fe07ebf80921a0d8ab60a..404556f2d6caa0dcf9ffdd21573852d8e6ed14a5 100644 (file)
@@ -116,6 +116,11 @@ public class EscalatorProxy extends Escalator {
             return rowContainer.spacerExists(rowIndex);
         }
 
+        @Override
+        public void resetSpacer(int rowIndex) {
+            rowContainer.resetSpacer(rowIndex);
+        }
+
         @Override
         public void setSpacerUpdater(SpacerUpdater spacerUpdater)
                 throws IllegalArgumentException {
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridDetailsUpdateItemsTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridDetailsUpdateItemsTest.java
new file mode 100644 (file)
index 0000000..3f690ca
--- /dev/null
@@ -0,0 +1,53 @@
+package com.vaadin.tests.components.grid;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.number.IsCloseTo.closeTo;
+import static org.junit.Assert.assertNotEquals;
+
+import org.junit.Test;
+
+import com.vaadin.testbench.TestBenchElement;
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.testbench.elements.GridElement.GridCellElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class GridDetailsUpdateItemsTest extends MultiBrowserTest {
+
+    @Test
+    public void testDetailsUpdateWithItems() {
+        openTestURL();
+        GridElement grid = $(GridElement.class).first();
+        ButtonElement button = $(ButtonElement.class).first();
+
+        String details0 = grid.getDetails(0).getText();
+        System.out.println("details: " + details0);
+
+        // change the contents
+        button.click();
+        sleep(200);
+
+        TestBenchElement detailCell0 = grid.getDetails(0);
+        // ensure contents have updated
+        String updatedDetails0 = detailCell0.getText();
+        assertNotEquals("Details should not be empty", "", updatedDetails0);
+        assertNotEquals("Unexpected detail contents for row 0", details0,
+                updatedDetails0);
+
+        GridCellElement cell1_0 = grid.getCell(1, 0);
+        TestBenchElement detailCell1 = grid.getDetails(1);
+
+        // ensure positioning is correct
+        assertDirectlyAbove(detailCell0, cell1_0);
+        assertDirectlyAbove(cell1_0, detailCell1);
+    }
+
+    private void assertDirectlyAbove(TestBenchElement above,
+            TestBenchElement below) {
+        int aboveBottom = above.getLocation().getY()
+                + above.getSize().getHeight();
+        int belowTop = below.getLocation().getY();
+        assertThat("Unexpected positions.", (double) aboveBottom,
+                closeTo(belowTop, 1d));
+    }
+}