From 2fb773938add8eb025cbccb6ce463cfe6f7d7fb8 Mon Sep 17 00:00:00 2001 From: ugur-vaadin Date: Thu, 7 Nov 2024 21:35:38 +0200 Subject: [PATCH 1/7] refactor: schedule renderers to only run for visible columns --- .../com/vaadin/flow/component/grid/Grid.java | 165 ++++----- .../grid/GridHiddenColumnRenderingTest.java | 329 ++++++++++++++++++ .../flow/component/gridpro/GridPro.java | 5 - 3 files changed, 394 insertions(+), 105 deletions(-) create mode 100644 vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index 67defdcdc0..85047b9625 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -31,6 +31,7 @@ import java.util.Set; import java.util.function.BiFunction; import java.util.function.BinaryOperator; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -430,12 +431,6 @@ public static void setDefaultMultiSortPriority(MultiSortPriority priority) { /** * Server-side component for the {@code } element. * - *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

- * * @param * type of the underlying grid this column is compatible with */ @@ -449,6 +444,7 @@ public static class Column extends AbstractColumn> { private String columnKey; // defined and used by the user private boolean sortingEnabled; + private boolean rendererSetupScheduled; private Component editorComponent; private EditorRenderer editorRenderer; @@ -489,20 +485,8 @@ public Column(Grid grid, String columnId, Renderer renderer) { super(grid); Objects.requireNonNull(renderer); this.columnInternalId = columnId; - this.renderer = renderer; - comparator = (a, b) -> 0; - - rendering = renderer.render(getElement(), (KeyMapper) getGrid() - .getDataCommunicator().getKeyMapper()); - - Optional> dataGenerator = rendering - .getDataGenerator(); - - if (dataGenerator.isPresent()) { - columnDataGeneratorRegistration = grid - .addDataGenerator(dataGenerator.get()); - } + setRenderer(renderer); } protected void destroyDataGenerators() { @@ -546,32 +530,11 @@ public Renderer getRenderer() { public Column setRenderer(Renderer renderer) { this.renderer = Objects.requireNonNull(renderer, "Renderer must not be null."); - - destroyDataGenerators(); - if (rendering != null) { - rendering.getRegistration().remove(); - } - - rendering = renderer.render(getElement(), (KeyMapper) getGrid() - .getDataCommunicator().getKeyMapper()); - - columnDataGeneratorRegistration = rendering.getDataGenerator() - .map(dataGenerator -> grid - .addDataGenerator((DataGenerator) dataGenerator)) - .orElse(null); - - // The editor renderer is a wrapper around the regular renderer, so - // we need to apply it again afterwards - if (editorRenderer != null) { - Rendering editorRendering = editorRenderer - .render(getElement(), null); - editorDataGeneratorRegistration = editorRendering - .getDataGenerator() - .map(dataGenerator -> grid.addDataGenerator( - (DataGenerator) dataGenerator)) - .orElse(null); - } - + clearRendering(); + rendererSetupScheduled = true; + getElement().getNode() + .runWhenAttached(ui -> scheduleRendererSetup()); + addAttachListener(e -> scheduleRendererSetup()); getGrid().getDataCommunicator().reset(); return this; } @@ -770,7 +733,7 @@ public Column setComparator(Comparator comparator) { * the value provider used to extract the {@link Comparable} * sort key * @return this column - * @see Comparator#comparing(java.util.function.Function) + * @see Comparator#comparing(Function) */ public > Column setComparator( ValueProvider keyExtractor) { @@ -1172,6 +1135,18 @@ public Column setRowHeader(boolean rowHeader) { return this; } + @Override + public void setVisible(boolean visible) { + boolean isInitiallyVisible = isVisible(); + super.setVisible(visible); + if (isInitiallyVisible && !visible) { + clearRendering(); + } + if (!isInitiallyVisible && visible) { + scheduleRendererSetup(); + } + } + @Override protected Column getBottomLevelColumn() { return this; @@ -1181,15 +1156,54 @@ protected Column getBottomLevelColumn() { private void setupColumnEditor() { editorRenderer = new EditorRenderer<>((Editor) grid.getEditor(), columnInternalId); + setupEditorRenderer(); + } + + private void setupRenderer() { + if (renderer == null) { + return; + } + rendering = renderer.render(getElement(), (KeyMapper) getGrid() + .getDataCommunicator().getKeyMapper()); + columnDataGeneratorRegistration = rendering.getDataGenerator() + .map(dataGenerator -> grid + .addDataGenerator((DataGenerator) dataGenerator)) + .orElse(null); + grid.getDataProvider().refreshAll(); + } + private void setupEditorRenderer() { + if (editorRenderer == null) { + return; + } Rendering editorRendering = editorRenderer.render(getElement(), null); + editorDataGeneratorRegistration = editorRendering.getDataGenerator() + .map(dataGenerator -> grid + .addDataGenerator((DataGenerator) dataGenerator)) + .orElse(null); + } - Optional> dataGenerator = editorRendering - .getDataGenerator(); - if (dataGenerator.isPresent()) { - editorDataGeneratorRegistration = grid - .addDataGenerator((DataGenerator) dataGenerator.get()); + private void scheduleRendererSetup() { + if (rendererSetupScheduled) { + return; + } + rendererSetupScheduled = true; + getUI().ifPresent(ui -> ui.beforeClientResponse(this, ctx -> { + if (rendererSetupScheduled && isVisible()) { + setupRenderer(); + // The editor renderer is a wrapper around the regular + // renderer, so we need to apply it again afterward. + setupEditorRenderer(); + } + rendererSetupScheduled = false; + })); + } + + private void clearRendering() { + destroyDataGenerators(); + if (rendering != null) { + rendering.getRegistration().remove(); } } } @@ -1851,11 +1865,6 @@ protected GridArrayUpdater createDefaultArrayUpdater( * see {@link #addColumn(Renderer)}. *

*

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

- *

* NOTE: This method is a shorthand for * {@link #addColumn(ValueProvider, BiFunction)} *

@@ -1883,11 +1892,6 @@ public Column addColumn(ValueProvider valueProvider) { * {@link #addComponentColumn(ValueProvider)}. For using build-in renderers, * see {@link #addColumn(Renderer)}. *

- *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

* * @param valueProvider * the value provider @@ -1946,11 +1950,6 @@ private String formatValueToSendToTheClient(Object value) { * NOTE: Using {@link ComponentRenderer} is not as efficient as the * built in renderers or using {@link LitRenderer}. *

- *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

* * @param componentProvider * a value provider that will return a component for the given @@ -1974,12 +1973,6 @@ public Column addComponentColumn( * automatically configured using the return type of the given * {@link ValueProvider}. * - *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

- * * @see Column#setComparator(ValueProvider) * @see Column#setSortProperty(String...) * @see #removeColumn(Column) @@ -2014,11 +2007,6 @@ public > Column addColumn( * or using {@link LitRenderer}. *

*

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

- *

* NOTE: This method is a shorthand for * {@link #addColumn(Renderer, BiFunction)} *

@@ -2051,11 +2039,6 @@ public Column addColumn(Renderer renderer) { * {@link ComponentRenderer} is not as efficient as the built in renderers * or using {@link LitRenderer}. *

- *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

* * @param renderer * the renderer used to create the grid cell structure @@ -2159,12 +2142,6 @@ protected BiFunction, String, Column> getDefaultColumnFactory() { * from a bean type with {@link #Grid(Class)}. * *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

- * - *

* Note: This method is a shorthand for * {@link #addColumn(String, BiFunction)} *

@@ -2199,12 +2176,6 @@ public Column addColumn(String propertyName) { * Note: This method can only be used for a Grid created * from a bean type with {@link #Grid(Class)}. * - *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

- * * @see #addColumn(String) * @see #removeColumn(Column) * @@ -2276,12 +2247,6 @@ private Object runPropertyValueGetter(PropertyDefinition property, * Note: This method can only be used for a Grid created * from a bean type with {@link #Grid(Class)}. * - *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link Grid#removeColumn(Column)} to avoid sending extra data. - *

- * * @param propertyNames * the property names of the new columns, not null * @see #addColumn(String) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java new file mode 100644 index 0000000000..bd617dcc19 --- /dev/null +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java @@ -0,0 +1,329 @@ +/* + * Copyright 2000-2024 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.flow.component.grid; + +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.IntStream; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import com.vaadin.flow.component.Component; +import com.vaadin.flow.component.UI; +import com.vaadin.flow.component.html.NativeButton; +import com.vaadin.flow.data.renderer.LitRenderer; +import com.vaadin.flow.data.renderer.Renderer; +import com.vaadin.flow.function.ValueProvider; +import com.vaadin.flow.server.VaadinSession; + +public class GridHiddenColumnRenderingTest { + + private static final int ITEM_COUNT = 10; + + private UI ui; + + private Grid grid; + + private AtomicInteger callCount; + + @Before + public void setup() { + ui = new UI(); + UI.setCurrent(ui); + VaadinSession session = Mockito.mock(VaadinSession.class); + Mockito.when(session.hasLock()).thenReturn(true); + ui.getInternals().setSession(session); + grid = new Grid<>(); + grid.setItems(getItems()); + ui.add(grid); + fakeClientCommunication(); + callCount = new AtomicInteger(0); + } + + @After + public void tearDown() { + UI.setCurrent(null); + } + + @Test + public void columnWithValueProvider_rendererCalledOncePerItem() { + addColumnWithValueProvider(); + initiallyVisibleColumn_assertRendererCalledOncePerItem(); + } + + @Test + public void initiallyHiddenColumnWithValueProvider_rendererNotCalled() { + Grid.Column column = addColumnWithValueProvider(); + initiallyHiddenColumn_assertRendererNotCalled(column); + } + + @Test + public void columnWithValueProvider_setHidden_rendererNotCalled() { + Grid.Column column = addColumnWithValueProvider(); + initiallyVisibleColumn_setHidden_assertRendererNotCalled(column); + } + + @Test + public void initiallyHiddenColumnWithValueProvider_setVisible_rendererCalledOncePerItem() { + Grid.Column column = addColumnWithValueProvider(); + initiallyHiddenColumn_setVisible_assertRendererCalledOncePerItem( + column); + } + + @Test + public void columnWithValueProvider_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { + Grid.Column column = addColumnWithValueProvider(); + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + column); + } + + @Test + public void initiallyHiddenColumnWithValueProvider_toggleHiddenTwiceInRoundTrip_rendererNotCalled() { + Grid.Column column = addColumnWithValueProvider(); + initiallyHiddenColumn_toggleHiddenTwiceInRoundTrip_assertRendererNotCalled( + column); + } + + @Test + public void componentColumn_rendererCalledOncePerItem() { + addComponentColumn(); + initiallyVisibleColumn_assertRendererCalledOncePerItem(); + } + + @Test + public void initiallyHiddenComponentColumn_rendererNotCalled() { + Grid.Column column = addComponentColumn(); + initiallyHiddenColumn_assertRendererNotCalled(column); + } + + @Test + public void componentColumn_setHidden_rendererNotCalled() { + Grid.Column column = addComponentColumn(); + initiallyVisibleColumn_setHidden_assertRendererNotCalled(column); + } + + @Test + public void initiallyHiddenComponentColumn_setVisible_rendererCalledOncePerItem() { + Grid.Column column = addComponentColumn(); + initiallyHiddenColumn_setVisible_assertRendererCalledOncePerItem( + column); + } + + @Test + public void componentColumn_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { + Grid.Column column = addComponentColumn(); + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + column); + } + + @Test + public void initiallyHiddenComponentColumn_toggleHiddenTwiceInRoundTrip_rendererNotCalled() { + Grid.Column column = addComponentColumn(); + initiallyHiddenColumn_toggleHiddenTwiceInRoundTrip_assertRendererNotCalled( + column); + } + + @Test + public void columnWithCustomRenderer_rendererCalledOncePerItem() { + addColumnWithCustomRenderer(); + initiallyVisibleColumn_assertRendererCalledOncePerItem(); + } + + @Test + public void initiallyHiddenColumnWithCustomRenderer_rendererNotCalled() { + Grid.Column column = addColumnWithCustomRenderer(); + initiallyHiddenColumn_assertRendererNotCalled(column); + } + + @Test + public void columnWithCustomRenderer_setHidden_rendererNotCalled() { + Grid.Column column = addColumnWithCustomRenderer(); + initiallyVisibleColumn_setHidden_assertRendererNotCalled(column); + } + + @Test + public void initiallyHiddenColumnWithCustomRenderer_setVisible_rendererCalledOncePerItem() { + Grid.Column column = addColumnWithCustomRenderer(); + initiallyHiddenColumn_setVisible_assertRendererCalledOncePerItem( + column); + } + + @Test + public void columnWithCustomRenderer_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { + Grid.Column column = addColumnWithCustomRenderer(); + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + column); + } + + @Test + public void initiallyHiddenColumnWithCustomRenderer_toggleHiddenTwiceInRoundTrip_rendererNotCalled() { + Grid.Column column = addColumnWithCustomRenderer(); + initiallyHiddenColumn_toggleHiddenTwiceInRoundTrip_assertRendererNotCalled( + column); + } + + @Test + public void columnWithValueProvider_detachAndReattachGrid_rendererCalledOncePerItem() { + addColumnWithValueProvider(); + initiallyVisibleColumn_detachAndReattachGrid_assertRendererCalledOncePerItem(); + } + + @Test + public void initiallyHiddenColumnWithValueProvider_detachAndReattachGrid_rendererNotCalled() { + Grid.Column column = addColumnWithValueProvider(); + initiallyHiddenColumn_detachAndReattachGrid_assertRendererNotCalled( + column); + } + + @Test + public void componentColumn_detachAndReattachGrid_rendererCalledOncePerItem() { + addComponentColumn(); + initiallyVisibleColumn_detachAndReattachGrid_assertRendererCalledOncePerItem(); + } + + @Test + public void initiallyHiddenComponentColumn_detachAndReattachGrid_rendererNotCalled() { + Grid.Column column = addComponentColumn(); + initiallyHiddenColumn_detachAndReattachGrid_assertRendererNotCalled( + column); + } + + @Test + public void columnWithCustomRenderer_detachAndReattachGrid_rendererCalledOncePerItem() { + addColumnWithCustomRenderer(); + initiallyVisibleColumn_detachAndReattachGrid_assertRendererCalledOncePerItem(); + } + + @Test + public void initiallyHiddenColumnWithCustomRenderer_detachAndReattachGrid_rendererNotCalled() { + Grid.Column column = addColumnWithCustomRenderer(); + initiallyHiddenColumn_detachAndReattachGrid_assertRendererNotCalled( + column); + } + + private Grid.Column addColumnWithValueProvider() { + return grid.addColumn(getValueProvider()); + } + + private Grid.Column addComponentColumn() { + return grid.addComponentColumn(this::getComponent); + } + + private Grid.Column addColumnWithCustomRenderer() { + return grid.addColumn(getCustomRenderer()); + } + + private void initiallyHiddenColumn_detachAndReattachGrid_assertRendererNotCalled( + Grid.Column column) { + column.setVisible(false); + ui.remove(grid); + fakeClientCommunication(); + ui.add(grid); + fakeClientCommunication(); + Assert.assertEquals(0, callCount.get()); + } + + private void initiallyVisibleColumn_detachAndReattachGrid_assertRendererCalledOncePerItem() { + ui.remove(grid); + fakeClientCommunication(); + callCount.set(0); + ui.add(grid); + fakeClientCommunication(); + Assert.assertEquals(ITEM_COUNT, callCount.get()); + } + + private void initiallyHiddenColumn_assertRendererNotCalled( + Grid.Column column) { + column.setVisible(false); + fakeClientCommunication(); + Assert.assertEquals(0, callCount.get()); + } + + private void initiallyVisibleColumn_assertRendererCalledOncePerItem() { + fakeClientCommunication(); + Assert.assertEquals(ITEM_COUNT, callCount.get()); + } + + private void initiallyVisibleColumn_setHidden_assertRendererNotCalled( + Grid.Column column) { + fakeClientCommunication(); + callCount.set(0); + column.setVisible(false); + fakeClientCommunication(); + Assert.assertEquals(0, callCount.get()); + } + + private void initiallyHiddenColumn_setVisible_assertRendererCalledOncePerItem( + Grid.Column column) { + column.setVisible(false); + fakeClientCommunication(); + column.setVisible(true); + fakeClientCommunication(); + Assert.assertEquals(ITEM_COUNT, callCount.get()); + } + + private void initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + Grid.Column column) { + fakeClientCommunication(); + column.setVisible(false); + column.setVisible(true); + callCount.set(0); + fakeClientCommunication(); + Assert.assertEquals(ITEM_COUNT, callCount.get()); + } + + private void initiallyHiddenColumn_toggleHiddenTwiceInRoundTrip_assertRendererNotCalled( + Grid.Column column) { + column.setVisible(false); + fakeClientCommunication(); + column.setVisible(true); + column.setVisible(false); + fakeClientCommunication(); + Assert.assertEquals(0, callCount.get()); + } + + private ValueProvider getValueProvider() { + return s -> { + callCount.incrementAndGet(); + return s; + }; + } + + private Component getComponent(String string) { + callCount.incrementAndGet(); + return new NativeButton(string); + } + + private Renderer getCustomRenderer() { + return LitRenderer. of("
${item.displayName}
") + .withProperty("displayName", getValueProvider()); + } + + private List getItems() { + return IntStream.range(0, ITEM_COUNT).mapToObj(i -> "Item " + i) + .toList(); + } + + private void fakeClientCommunication() { + ui.getInternals().getStateTree().runExecutionsBeforeClientResponse(); + ui.getInternals().getStateTree().collectChanges(ignore -> { + }); + } +} diff --git a/vaadin-grid-pro-flow-parent/vaadin-grid-pro-flow/src/main/java/com/vaadin/flow/component/gridpro/GridPro.java b/vaadin-grid-pro-flow-parent/vaadin-grid-pro-flow/src/main/java/com/vaadin/flow/component/gridpro/GridPro.java index 24305fae19..132dab27ef 100644 --- a/vaadin-grid-pro-flow-parent/vaadin-grid-pro-flow/src/main/java/com/vaadin/flow/component/gridpro/GridPro.java +++ b/vaadin-grid-pro-flow-parent/vaadin-grid-pro-flow/src/main/java/com/vaadin/flow/component/gridpro/GridPro.java @@ -181,11 +181,6 @@ private Object getItemId(E item) { /** * Server-side component for the {@code } element. * - *

- * Every added column sends data to the client side regardless of its - * visibility state. Don't add a new column at all or use - * {@link GridPro#removeColumn(Column)} to avoid sending extra data. - * * @param * type of the underlying grid this column is compatible with */ From 361ccded19a8d83fa7c68bc97997d1101bdce245 Mon Sep 17 00:00:00 2001 From: ugur-vaadin Date: Thu, 7 Nov 2024 23:28:08 +0200 Subject: [PATCH 2/7] fix: remove unnecessary line --- .../src/main/java/com/vaadin/flow/component/grid/Grid.java | 1 - 1 file changed, 1 deletion(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index 85047b9625..40dde15a76 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -531,7 +531,6 @@ public Column setRenderer(Renderer renderer) { this.renderer = Objects.requireNonNull(renderer, "Renderer must not be null."); clearRendering(); - rendererSetupScheduled = true; getElement().getNode() .runWhenAttached(ui -> scheduleRendererSetup()); addAttachListener(e -> scheduleRendererSetup()); From 4128359be28bbc55c4052f15feca29e2686a9c7f Mon Sep 17 00:00:00 2001 From: ugur-vaadin Date: Fri, 8 Nov 2024 15:26:50 +0200 Subject: [PATCH 3/7] fix: send items only once to the client --- .../com/vaadin/flow/component/grid/Grid.java | 1 - .../grid/GridHiddenColumnRenderingTest.java | 37 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index 40dde15a76..a030159d68 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -534,7 +534,6 @@ public Column setRenderer(Renderer renderer) { getElement().getNode() .runWhenAttached(ui -> scheduleRendererSetup()); addAttachListener(e -> scheduleRendererSetup()); - getGrid().getDataCommunicator().reset(); return this; } diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java index bd617dcc19..c13b7e865a 100644 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java @@ -28,6 +28,7 @@ import com.vaadin.flow.component.Component; import com.vaadin.flow.component.UI; import com.vaadin.flow.component.html.NativeButton; +import com.vaadin.flow.data.provider.DataProvider; import com.vaadin.flow.data.renderer.LitRenderer; import com.vaadin.flow.data.renderer.Renderer; import com.vaadin.flow.function.ValueProvider; @@ -218,6 +219,42 @@ public void initiallyHiddenColumnWithCustomRenderer_detachAndReattachGrid_render column); } + @Test + public void columnWithCustomRenderer_setAnotherRenderer_onlyNewRendererCalled() { + Grid.Column column = addColumnWithCustomRenderer(); + fakeClientCommunication(); + callCount.set(0); + AtomicInteger newRendererCallCount = new AtomicInteger(0); + Renderer newRenderer = LitRenderer + . of("${item.displayName}") + .withProperty("displayName", s -> { + newRendererCallCount.incrementAndGet(); + return s; + }); + column.setRenderer(newRenderer); + fakeClientCommunication(); + Assert.assertEquals(0, callCount.get()); + Assert.assertEquals(ITEM_COUNT, newRendererCallCount.get()); + } + + @Test + public void addColumn_itemsSentOnlyOnce() { + List items = getItems(); + AtomicInteger fetchCount = new AtomicInteger(0); + DataProvider dataProvider = DataProvider + .fromCallbacks(query -> { + fetchCount.incrementAndGet(); + return items.stream().skip(query.getOffset()) + .limit(query.getLimit()); + }, query -> items.size()); + grid.setDataProvider(dataProvider); + fakeClientCommunication(); + fetchCount.set(0); + addColumnWithValueProvider(); + fakeClientCommunication(); + Assert.assertEquals(1, fetchCount.get()); + } + private Grid.Column addColumnWithValueProvider() { return grid.addColumn(getValueProvider()); } From 9959a86b677a329954cd0fb963eedabde3d22c2b Mon Sep 17 00:00:00 2001 From: ugur-vaadin Date: Mon, 11 Nov 2024 23:02:48 +0200 Subject: [PATCH 4/7] refactor: patch data generator instead of scheduling renderer --- .../com/vaadin/flow/component/grid/Grid.java | 125 +++++++++--------- .../grid/GridHiddenColumnRenderingTest.java | 16 +-- 2 files changed, 71 insertions(+), 70 deletions(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index a030159d68..75fa35c2ba 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -444,7 +444,6 @@ public static class Column extends AbstractColumn> { private String columnKey; // defined and used by the user private boolean sortingEnabled; - private boolean rendererSetupScheduled; private Component editorComponent; private EditorRenderer editorRenderer; @@ -485,8 +484,38 @@ public Column(Grid grid, String columnId, Renderer renderer) { super(grid); Objects.requireNonNull(renderer); this.columnInternalId = columnId; + this.renderer = renderer; + comparator = (a, b) -> 0; - setRenderer(renderer); + + rendering = renderer.render(getElement(), (KeyMapper) getGrid() + .getDataCommunicator().getKeyMapper()); + + Optional> dataGenerator = rendering + .getDataGenerator(); + + if (dataGenerator.isPresent()) { + var generator = dataGenerator.get(); + DataGenerator conditionalDataGenerator = (item, + jsonObject) -> { + if (Column.this.isVisible()) { + generator.generateData(item, jsonObject); + } + }; + + columnDataGeneratorRegistration = grid + .addDataGenerator(conditionalDataGenerator); + } + } + + @Override + public void setVisible(boolean visible) { + boolean resetDataCommunicator = visible && !isVisible(); + super.setVisible(visible); + if (resetDataCommunicator) { + getGrid().getDataCommunicator().reset(); + + } } protected void destroyDataGenerators() { @@ -530,10 +559,33 @@ public Renderer getRenderer() { public Column setRenderer(Renderer renderer) { this.renderer = Objects.requireNonNull(renderer, "Renderer must not be null."); - clearRendering(); - getElement().getNode() - .runWhenAttached(ui -> scheduleRendererSetup()); - addAttachListener(e -> scheduleRendererSetup()); + + destroyDataGenerators(); + if (rendering != null) { + rendering.getRegistration().remove(); + } + + rendering = renderer.render(getElement(), (KeyMapper) getGrid() + .getDataCommunicator().getKeyMapper()); + + columnDataGeneratorRegistration = rendering.getDataGenerator() + .map(dataGenerator -> grid + .addDataGenerator((DataGenerator) dataGenerator)) + .orElse(null); + + // The editor renderer is a wrapper around the regular renderer, so + // we need to apply it again afterwards + if (editorRenderer != null) { + Rendering editorRendering = editorRenderer + .render(getElement(), null); + editorDataGeneratorRegistration = editorRendering + .getDataGenerator() + .map(dataGenerator -> grid.addDataGenerator( + (DataGenerator) dataGenerator)) + .orElse(null); + } + + getGrid().getDataCommunicator().reset(); return this; } @@ -1133,18 +1185,6 @@ public Column setRowHeader(boolean rowHeader) { return this; } - @Override - public void setVisible(boolean visible) { - boolean isInitiallyVisible = isVisible(); - super.setVisible(visible); - if (isInitiallyVisible && !visible) { - clearRendering(); - } - if (!isInitiallyVisible && visible) { - scheduleRendererSetup(); - } - } - @Override protected Column getBottomLevelColumn() { return this; @@ -1154,54 +1194,15 @@ protected Column getBottomLevelColumn() { private void setupColumnEditor() { editorRenderer = new EditorRenderer<>((Editor) grid.getEditor(), columnInternalId); - setupEditorRenderer(); - } - private void setupRenderer() { - if (renderer == null) { - return; - } - rendering = renderer.render(getElement(), (KeyMapper) getGrid() - .getDataCommunicator().getKeyMapper()); - columnDataGeneratorRegistration = rendering.getDataGenerator() - .map(dataGenerator -> grid - .addDataGenerator((DataGenerator) dataGenerator)) - .orElse(null); - grid.getDataProvider().refreshAll(); - } - - private void setupEditorRenderer() { - if (editorRenderer == null) { - return; - } Rendering editorRendering = editorRenderer.render(getElement(), null); - editorDataGeneratorRegistration = editorRendering.getDataGenerator() - .map(dataGenerator -> grid - .addDataGenerator((DataGenerator) dataGenerator)) - .orElse(null); - } - private void scheduleRendererSetup() { - if (rendererSetupScheduled) { - return; - } - rendererSetupScheduled = true; - getUI().ifPresent(ui -> ui.beforeClientResponse(this, ctx -> { - if (rendererSetupScheduled && isVisible()) { - setupRenderer(); - // The editor renderer is a wrapper around the regular - // renderer, so we need to apply it again afterward. - setupEditorRenderer(); - } - rendererSetupScheduled = false; - })); - } - - private void clearRendering() { - destroyDataGenerators(); - if (rendering != null) { - rendering.getRegistration().remove(); + Optional> dataGenerator = editorRendering + .getDataGenerator(); + if (dataGenerator.isPresent()) { + editorDataGeneratorRegistration = grid + .addDataGenerator((DataGenerator) dataGenerator.get()); } } } diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java index c13b7e865a..144eeaf160 100644 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java @@ -89,9 +89,9 @@ public void initiallyHiddenColumnWithValueProvider_setVisible_rendererCalledOnce } @Test - public void columnWithValueProvider_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { + public void columnWithValueProvider_toggleHiddenTwiceInRoundTrip_rendererCalledAtMostOncePerItem() { Grid.Column column = addColumnWithValueProvider(); - initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( column); } @@ -128,9 +128,9 @@ public void initiallyHiddenComponentColumn_setVisible_rendererCalledOncePerItem( } @Test - public void componentColumn_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { + public void componentColumn_toggleHiddenTwiceInRoundTrip_rendererCalledAtMostOncePerItem() { Grid.Column column = addComponentColumn(); - initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( column); } @@ -167,9 +167,9 @@ public void initiallyHiddenColumnWithCustomRenderer_setVisible_rendererCalledOnc } @Test - public void columnWithCustomRenderer_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { + public void columnWithCustomRenderer_toggleHiddenTwiceInRoundTrip_rendererCalledAtMostOncePerItem() { Grid.Column column = addColumnWithCustomRenderer(); - initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( column); } @@ -316,14 +316,14 @@ private void initiallyHiddenColumn_setVisible_assertRendererCalledOncePerItem( Assert.assertEquals(ITEM_COUNT, callCount.get()); } - private void initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( + private void initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( Grid.Column column) { fakeClientCommunication(); column.setVisible(false); column.setVisible(true); callCount.set(0); fakeClientCommunication(); - Assert.assertEquals(ITEM_COUNT, callCount.get()); + Assert.assertTrue(callCount.get() <= ITEM_COUNT); } private void initiallyHiddenColumn_toggleHiddenTwiceInRoundTrip_assertRendererNotCalled( From c359c25ef9f1bc232854a1d16537f774d1203afb Mon Sep 17 00:00:00 2001 From: ugur-vaadin Date: Mon, 11 Nov 2024 23:04:36 +0200 Subject: [PATCH 5/7] chore: remove extra empty lines --- .../src/main/java/com/vaadin/flow/component/grid/Grid.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index 75fa35c2ba..ea28967d38 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -502,7 +502,6 @@ public Column(Grid grid, String columnId, Renderer renderer) { generator.generateData(item, jsonObject); } }; - columnDataGeneratorRegistration = grid .addDataGenerator(conditionalDataGenerator); } @@ -514,7 +513,6 @@ public void setVisible(boolean visible) { super.setVisible(visible); if (resetDataCommunicator) { getGrid().getDataCommunicator().reset(); - } } From 982bb82065db7cc433083c629d62fa4fa139fb41 Mon Sep 17 00:00:00 2001 From: ugur-vaadin Date: Tue, 12 Nov 2024 10:29:40 +0200 Subject: [PATCH 6/7] fix: override all methods when patching data generator --- .../com/vaadin/flow/component/grid/Grid.java | 29 ++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index ea28967d38..546688c2d4 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -496,10 +496,31 @@ public Column(Grid grid, String columnId, Renderer renderer) { if (dataGenerator.isPresent()) { var generator = dataGenerator.get(); - DataGenerator conditionalDataGenerator = (item, - jsonObject) -> { - if (Column.this.isVisible()) { - generator.generateData(item, jsonObject); + + // Use an anonymous class instead of Lambda to prevent potential + // deserialization issues when used with Grid + // see https://github.com/vaadin/flow-components/issues/6256 + var conditionalDataGenerator = new DataGenerator() { + @Override + public void generateData(T item, JsonObject jsonObject) { + if (Column.this.isVisible()) { + generator.generateData(item, jsonObject); + } + } + + @Override + public void destroyData(T item) { + generator.destroyData(item); + } + + @Override + public void destroyAllData() { + generator.destroyAllData(); + } + + @Override + public void refreshData(T item) { + generator.refreshData(item); } }; columnDataGeneratorRegistration = grid From 337eab26fd29f714113ad23f04b1beeccaabce37 Mon Sep 17 00:00:00 2001 From: ugur-vaadin Date: Tue, 12 Nov 2024 11:06:34 +0200 Subject: [PATCH 7/7] test: revert unit test asserions --- .../grid/GridHiddenColumnRenderingTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java index 144eeaf160..c13b7e865a 100644 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridHiddenColumnRenderingTest.java @@ -89,9 +89,9 @@ public void initiallyHiddenColumnWithValueProvider_setVisible_rendererCalledOnce } @Test - public void columnWithValueProvider_toggleHiddenTwiceInRoundTrip_rendererCalledAtMostOncePerItem() { + public void columnWithValueProvider_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { Grid.Column column = addColumnWithValueProvider(); - initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( column); } @@ -128,9 +128,9 @@ public void initiallyHiddenComponentColumn_setVisible_rendererCalledOncePerItem( } @Test - public void componentColumn_toggleHiddenTwiceInRoundTrip_rendererCalledAtMostOncePerItem() { + public void componentColumn_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { Grid.Column column = addComponentColumn(); - initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( column); } @@ -167,9 +167,9 @@ public void initiallyHiddenColumnWithCustomRenderer_setVisible_rendererCalledOnc } @Test - public void columnWithCustomRenderer_toggleHiddenTwiceInRoundTrip_rendererCalledAtMostOncePerItem() { + public void columnWithCustomRenderer_toggleHiddenTwiceInRoundTrip_rendererCalledOncePerItem() { Grid.Column column = addColumnWithCustomRenderer(); - initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( + initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( column); } @@ -316,14 +316,14 @@ private void initiallyHiddenColumn_setVisible_assertRendererCalledOncePerItem( Assert.assertEquals(ITEM_COUNT, callCount.get()); } - private void initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledAtMostOncePerItem( + private void initiallyVisibleColumn_toggleHiddenTwiceInRoundTrip_assertRendererCalledOncePerItem( Grid.Column column) { fakeClientCommunication(); column.setVisible(false); column.setVisible(true); callCount.set(0); fakeClientCommunication(); - Assert.assertTrue(callCount.get() <= ITEM_COUNT); + Assert.assertEquals(ITEM_COUNT, callCount.get()); } private void initiallyHiddenColumn_toggleHiddenTwiceInRoundTrip_assertRendererNotCalled(