Skip to content

feat: add generate data when hidden api for column #6798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

feat: add generate data when hidden api for column #6798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2fb7739
refactor: schedule renderers to only run for visible columns
ugur-vaadin Nov 7, 2024
361ccde
fix: remove unnecessary line
ugur-vaadin Nov 7, 2024
4128359
fix: send items only once to the client
ugur-vaadin Nov 8, 2024
9959a86
refactor: patch data generator instead of scheduling renderer
ugur-vaadin Nov 11, 2024
c359c25
chore: remove extra empty lines
ugur-vaadin Nov 11, 2024
982bb82
fix: override all methods when patching data generator
ugur-vaadin Nov 12, 2024
337eab2
test: revert unit test asserions
ugur-vaadin Nov 12, 2024
fb5372d
refactor: schedule renderers to only run for visible columns
ugur-vaadin Nov 7, 2024
b53b814
fix: remove unnecessary line
ugur-vaadin Nov 7, 2024
d1457c6
fix: send items only once to the client
ugur-vaadin Nov 8, 2024
1ec6437
refactor: patch data generator instead of scheduling renderer
ugur-vaadin Nov 11, 2024
332c296
chore: remove extra empty lines
ugur-vaadin Nov 11, 2024
70bf3c0
fix: override all methods when patching data generator
ugur-vaadin Nov 12, 2024
17f7d54
test: revert unit test asserions
ugur-vaadin Nov 12, 2024
0e9df50
Merge branch 'refactor-schedule-renderers-to-run-for-visible-cols' of…
ugur-vaadin Nov 18, 2024
2a4b11f
Merge branch 'main' into refactor-schedule-renderers-to-run-for-visib…
vursen Nov 18, 2024
dd3d186
feat: introduce generate data when hidden api
ugur-vaadin Nov 18, 2024
76b5180
Merge branch 'main' into refactor-schedule-renderers-to-run-for-visib…
ugur-vaadin Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions ...-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/AbstractColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,14 @@ public Grid<?> getGrid() {
/**
* {@inheritDoc}
* <p>
* Note that column related data is sent to the client side even if the
* column is invisible. Use {@link Grid#removeColumn(Column)} to remove
* column (or don't add the column all) and avoid sending extra data.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link Grid#removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @see Grid#removeColumn(Column)
* @see Column#setGenerateDataWhenHidden(boolean)
*/
@Override
public void setVisible(boolean visible) {
Expand Down
161 changes: 128 additions & 33 deletions ...-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the utility of this change: This sort of moves the performance issue to a different use-case, which is showing and hiding columns. In that case it now needs to reload all data whenever a single column is made visible. Previously it would load everything up front, but then doesn't have to reload everything if you show a single column.

What is everyone's thought on this?

Copy link
Member

@tomivirkki tomivirkki Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, hiding and unhiding columns shouldn't need to result in a data provider call, but only the data generators should need to be rerun for the already loaded items.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that whoever created this issue wouldn't expect additional data provider calls as part of this change.

Since there's a trade-off, maybe it could be turned into an option. Though it's a bit challenging to communicate what this even does.

Copy link
Member

@tomivirkki tomivirkki Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This nasty prototype method in Grid does the job without additional data provider calls, but it requires reflection. I hope there's a better way (maybe storing the value in Grid.setRequestedRange):

private void regenerateData() {
        try {
        var communicator = getDataCommunicator();
        var requestedRangeField = communicator.getClass()
                .getDeclaredField("requestedRange");
        requestedRangeField.setAccessible(true);
        var requestedRange = (Range) requestedRangeField
                .get(communicator);

        for (int i = requestedRange.getStart(); i < requestedRange.getEnd(); i++) {
            var item = communicator.getItem(i);
            if (item != null) {
                getDataCommunicator().refresh(item);
            }
        }
    } catch (Exception e) {
        // ignore
    }
}

Copy link
Contributor

@vursen vursen Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be related: #6713

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added API to make this optional, and kept the default behaviour the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding a column, it sends two full updates to the client. One based on the old column config, and then another one with the new config. Also calls the data provider twice.

Bildschirmfoto 2024-11-08 um 10 34 10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a fix and the test for it to handle this case.

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -431,9 +432,10 @@ public static void setDefaultMultiSortPriority(MultiSortPriority priority) {
* Server-side component for the {@code <vaadin-grid-column>} element.
*
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link Grid#removeColumn(Column)} or use
* {@link #setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @param <T>
Expand All @@ -449,6 +451,7 @@ public static class Column<T> extends AbstractColumn<Column<T>> {
private String columnKey; // defined and used by the user

private boolean sortingEnabled;
private boolean generateDataWhenHidden = true;

private Component editorComponent;
private EditorRenderer<T> editorRenderer;
Expand Down Expand Up @@ -500,8 +503,48 @@ public Column(Grid<T> grid, String columnId, Renderer<T> renderer) {
.getDataGenerator();

if (dataGenerator.isPresent()) {
var generator = dataGenerator.get();

// 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<T>() {
@Override
public void generateData(T item, JsonObject jsonObject) {
if (isGenerateDataWhenHidden()
|| 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
.addDataGenerator(dataGenerator.get());
.addDataGenerator(conditionalDataGenerator);
}
}

@Override
public void setVisible(boolean visible) {
// Data has to be generated for previously hidden columns
boolean resetDataCommunicator = !isGenerateDataWhenHidden()
&& visible && !isVisible();
super.setVisible(visible);
if (resetDataCommunicator) {
getGrid().getDataCommunicator().reset();
}
}

Expand Down Expand Up @@ -770,7 +813,7 @@ public Column<T> setComparator(Comparator<T> 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 <V extends Comparable<? super V>> Column<T> setComparator(
ValueProvider<T, V> keyExtractor) {
Expand Down Expand Up @@ -895,6 +938,49 @@ public boolean isSortable() {
return sortingEnabled;
}

/**
* Sets whether the data for this column should be generated and sent to
* the client even when the column is hidden. By default, data for
* hidden columns is generated and sent to the client.
* <p>
* Setting this property to {@code false} will prevent the data for this
* column from being generated and sent to the client when the column is
* hidden. Alternatively, you can remove the column using
* {@link Grid#removeColumn(Column)} or avoid adding the column
* altogether.
* </p>
*
* @param generateDataWhenHidden
* {@code true} to generate data even when the column is
* hidden, {@code false} otherwise
* @return this column
*/
public Column<T> setGenerateDataWhenHidden(
boolean generateDataWhenHidden) {
if (this.generateDataWhenHidden == generateDataWhenHidden) {
return this;
}
// Data has to be generated for hidden columns.
if (!isVisible() && generateDataWhenHidden
&& !isGenerateDataWhenHidden()) {
getGrid().getDataCommunicator().reset();
}
this.generateDataWhenHidden = generateDataWhenHidden;
return this;
}

/**
* Returns whether the data for this column is generated and sent to the
* client when the column is hidden. The default is {@code true}.
*
* @return {@code true} if data is generated even when the column is
* hidden, {@code false} otherwise
* @see #setGenerateDataWhenHidden(boolean)
*/
public boolean isGenerateDataWhenHidden() {
return generateDataWhenHidden;
}

/**
* Sets a header text to the column.
* <p>
Expand Down Expand Up @@ -1851,9 +1937,10 @@ protected GridArrayUpdater createDefaultArrayUpdater(
* see {@link #addColumn(Renderer)}.
* </p>
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
* <p>
* <em>NOTE:</em> This method is a shorthand for
Expand Down Expand Up @@ -1884,9 +1971,10 @@ public Column<T> addColumn(ValueProvider<T, ?> valueProvider) {
* see {@link #addColumn(Renderer)}.
* </p>
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @param valueProvider
Expand Down Expand Up @@ -1946,10 +2034,12 @@ private String formatValueToSendToTheClient(Object value) {
* <em>NOTE:</em> Using {@link ComponentRenderer} is not as efficient as the
* built in renderers or using {@link LitRenderer}.
* </p>
*
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @param componentProvider
Expand All @@ -1975,9 +2065,10 @@ public <V extends Component> Column<T> addComponentColumn(
* {@link ValueProvider}.
*
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @see Column#setComparator(ValueProvider)
Expand Down Expand Up @@ -2014,9 +2105,10 @@ public <V extends Comparable<? super V>> Column<T> addColumn(
* or using {@link LitRenderer}.
* </p>
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
* <p>
* <em>NOTE:</em> This method is a shorthand for
Expand Down Expand Up @@ -2052,9 +2144,10 @@ public Column<T> addColumn(Renderer<T> renderer) {
* or using {@link LitRenderer}.
* </p>
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @param renderer
Expand Down Expand Up @@ -2159,9 +2252,10 @@ protected BiFunction<Renderer<T>, String, Column<T>> getDefaultColumnFactory() {
* from a bean type with {@link #Grid(Class)}.
*
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* <p>
Expand Down Expand Up @@ -2200,9 +2294,10 @@ public Column<T> addColumn(String propertyName) {
* from a bean type with {@link #Grid(Class)}.
*
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @see #addColumn(String)
Expand Down Expand Up @@ -2275,11 +2370,11 @@ private Object runPropertyValueGetter(PropertyDefinition<T, ?> property,
* <p>
* <strong>Note:</strong> This method can only be used for a Grid created
* from a bean type with {@link #Grid(Class)}.
*
* <p>
* 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.
* By default, every added column sends data to the client side regardless
* of its visibility state. To avoid sending extra data, either remove the
* column using {@link #removeColumn(Column)} or use
* {@link Column#setGenerateDataWhenHidden(boolean)}.
* </p>
*
* @param propertyNames
Expand Down
Loading