feat: add generate data when hidden api for column#6798
feat: add generate data when hidden api for column#6798ugur-vaadin wants to merge 14 commits intomainfrom
Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
}There was a problem hiding this comment.
Added API to make this optional, and kept the default behaviour the same.
| .map(dataGenerator -> grid | ||
| .addDataGenerator((DataGenerator) dataGenerator)) | ||
| .orElse(null); | ||
| grid.getDataProvider().refreshAll(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added a fix and the test for it to handle this case.
|
Would making the data generator run conditionally based on the column's visibility fix the original issue? |
9c98369 to
982bb82
Compare
Adapted the suggestion. It works nicely and is cleaner. |
|
76b5180 to
71be4a3
Compare
|




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
Columnin 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
generateDataWhenHiddenistrue, therefore this keeps the current behaviour unless it is explicitly setfalseusing the API.Fixes #6737
Type of change
Checklist