fix: Ensure that GridListDataView#setFilter fires DataChangeEvent

[TatuLund]

Fixes #5303

[sonatype-lift]

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[sissbruecker]

I think this is a rather opinionated change, and personally I would not agree with it. The purpose of the DataChangeEvent is for notifying the component / data communicator about changes to the data, like added, removed, or updated items, which allows the component to update its presentation. Changing the filter affects the view over that data, similar to changing sort order, and as such should not trigger that event. This would also introduce new side-effects, such as closing the current editor when a filter changes.

As such, I'll close this PR and related issue. As for alternatives, for an application, which sets the filter in the first place, it should also be possible to do some other thing as part of the same step rather than adding some indirection via an event of the grid. Other than that, maybe the data view should fire an event if the viewed data changes.

[mshabarov]

Firing a DataChangeEvent for ListDataView::setFilter sounds reasonable to me.
Other data change methods like removeItem or add item(s) methods do call refreshAll explicitly.
refreshAll in Grid does Editor reset, indeed, this might not be intended by developer.
But from the other hand, it is done for other methods, why not do it for setFilter as well?
We can be safe, and only fire an event in the DataView, but this feels to me like a workaround and not consistent.
So my proposition is to remove DataCommunicator.reset from Grid (in the listener where data view fires filter or sorting changed event) and add refreshAll to the DataView in the end of setFilter.
DataView::setSortComparator also would need it.

[TatuLund]

I re-opened the PR based @mshabarov conclusion.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[mshabarov]

I propose to move refreshAll() from here to the data view.
Then the setFilter on the Flow side will be like this:

    @Override
    public AbstractListDataView<T> setFilter(SerializablePredicate<T> filter) {
        DataViewUtils.setComponentFilter(component, filter);
        fireFilteringOrSortingChangeEvent(filter,
                (SerializableComparator<T>) DataViewUtils
                        .getComponentSortComparator(component).orElse(null));
        refreshAll();
        return this;
    }

This would fix all similar issues in data view, not only the Grid.

[mshabarov]

Same issue is raised here #5465.
This confirms my theory for making the refresh all on the DataView side to fix all the components implementing DataView API.
I will make a pull request on the Flow side.

[mshabarov]

But this PR is valid. We need to get rid of extra reset call in the Grid in the end.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud