Skip to content
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.

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

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

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

Conversation

ugur-vaadin
Copy link
Contributor

@ugur-vaadin ugur-vaadin commented Nov 7, 2024

Description

Currently, the value provider in any type of created grid column runs regardless of its visibility. This can cause unnecessary load if the tasks are heavy.

This PR adds the following API to Column in order to provide the option to not run the data generators for hidden columns:

  • boolean isGenerateDataWhenHidden()
  • Column<T> setGenerateDataWhenHidden(boolean generateDataWhenHidden)

The default value for generateDataWhenHidden is true, therefore this keeps the current behaviour unless it is explicitly set false using the API.

Fixes #6737

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

@ugur-vaadin ugur-vaadin changed the title refactor: schedule renderers to only run for visible columns refactor!: schedule renderers to only run for visible columns Nov 8, 2024
@ugur-vaadin ugur-vaadin marked this pull request as ready for review November 8, 2024 07:59
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

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

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

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.

.map(dataGenerator -> grid
.addDataGenerator((DataGenerator) dataGenerator))
.orElse(null);
grid.getDataProvider().refreshAll();
Copy link
Contributor

Choose a reason for hiding this comment

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

If the app calls refreshAll before making a column visible, that results in two calls to the data provider now.

In general I'm uncertain about calling refreshAll in beforeClientResponse, which is the same phase that data communicator uses to prepare data to send to the client. Is there a reason that this has to be called in beforeClientResponse, or could this already be called in setVisible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it would be better to do it that way. However, directly calling in setVisible does not work and I'm not sure what would be a good way to refactor the refresh.

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.

@tomivirkki
Copy link
Member

Would making the data generator run conditionally based on the column's visibility fix the original issue?
Proto: 1a5cb52

@ugur-vaadin ugur-vaadin force-pushed the refactor-schedule-renderers-to-run-for-visible-cols branch from 9c98369 to 982bb82 Compare November 12, 2024 09:02
@ugur-vaadin
Copy link
Contributor Author

Would making the data generator run conditionally based on the column's visibility fix the original issue? Proto: 1a5cb52

Adapted the suggestion. It works nicely and is cleaner.

@ugur-vaadin ugur-vaadin changed the title refactor!: schedule renderers to only run for visible columns refactor!: run data generators only for visible columns Nov 12, 2024
@ugur-vaadin ugur-vaadin changed the title refactor!: run data generators only for visible columns feat: add generate data when hidden api for column Nov 18, 2024
Copy link

sonarcloud bot commented Nov 19, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hidden Grid Column should not run code
4 participants