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

[core] Apply all filters on a row before passing to the next one #2870

Merged
merged 23 commits into from
Oct 26, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Oct 14, 2021

Part of #2725
Requires #2673

The new behavior is very similar to what we have with the sorting.

  • Only one forceUpdate per applyFilters even when multiple filters are applied
  • Do not compute useless filter values if linkOperator = "and" and a previous filter failed
  • Do not compute useless filter values if linkOperator = "or" and a previous filter passed

If we have 2 rows (row1 and row2) and 2 filters (filterA and filterB)
Before this PR, the execution order is

  • Apply filterA on row1
  • Apply filterA on row2
  • Apply filterB on row1
  • Apply filterB on row2

After this PR, the execution order is

  • Apply filterA on row1
  • Apply filterB on row1
  • Apply filterA on row2
  • Apply filterB on row2

Executing all filters on a row in a single pass will help me apply constraints on the tree data (for instance do not filter the child if the parents is not passing the filters).

Do you see any interest to let users apply a single filter like they currently can ? It is not resetting the other filters so I don't really see use-cases where the behavior is not broken.
And it creates a visibleRows that is not in sync with the filterModel, which we should avoid.


Breaking changes

// Rename + remove from public API
-apiRef.current.applyFilters
+apiRef.current.unsafe_applyFilters

// Remove
-apiRef.current.applyFilter

// Rename
-apiRef.current.applyFilterLinkOperator
+apiRef.current.setFilterLinkOperator

// Rename
-apiRef.current.upsertFilter
+apiRef.current.upsertFilterItem

// Rename
-apiRef.current.deleteFilter
+apiRef.current.deleteFilterItem

For applyFilters, you should never have to call it directly since all the other method that update the rows or the filterModel are calling it directly or not.

@flaviendelangle flaviendelangle self-assigned this Oct 14, 2021
@flaviendelangle flaviendelangle added component: data grid This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes labels Oct 14, 2021
@flaviendelangle flaviendelangle marked this pull request as draft October 14, 2021 14:50
@flaviendelangle flaviendelangle changed the title [core] Rework filter application to prepare for tree data [WIP] [core] Apply all filters on a row before passing to the next one Oct 15, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 18, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 18, 2021
@flaviendelangle flaviendelangle marked this pull request as ready for review October 19, 2021 11:33
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Niice.

@mui mui deleted a comment from github-actions bot Oct 21, 2021
@DanailH
Copy link
Member

DanailH commented Oct 25, 2021

Looks great!

Do you see any interest to let users apply a single filter like they currently can ? It is not resetting the other filters so I don't really see use-cases where the behavior is not broken.

They can still apply all filters right, it's just a matter of using the other method and passing in an array with a single item in it. That way a single filter will be applied.
Maybe if this is not used internally we can remove it - another option would be to prefix it with UNSTABLE_ and keep it for a while?

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Oct 25, 2021

They can still apply all filters right, it's just a matter of using the other method and passing in an array with a single item in it. That way a single filter will be applied.

No, you don't pass the filter items to applyFilters, it just takes the items from the state.
And for me it is the wanted behavior. The state.filter.filterModel is the source of truth of what the filters are, and applyFilters is just apply this filterModel to generate the new visibleRows.

And if a user want to edit the filterModel, he has setFilterModel or the wrappers applyFilterLinkOperator (should be renamed setFilterLinkOperator I think), upsertFilter and deleteFilter.

All of them calls applyFilters after updating the filterModel. So I'm not even sure exposijng applyFilters makes sense but I may be missing a use case. For me we should have 4 methods exposed:

  • setFilterModel : the user passes the whole new filterModel
  • setFilterLinkOperator : the user passes the filter link operator, does not modify the items (wrapper of setFilterModel)
  • upsertFilterItem : the user passes one item that is added / edited, does not modify the other items and the link operator (wrapper of setFilterModel)
  • deleteFilterItem : the user passes one item that is removed, does not modify the other items and the link operator (wrapper of setFilterModel)

(+ the filter panel visibility methods which are not directly related)

I am not even sure setFilterLinkOperator is worth it, it's a one liner wrapper.

Maybe if this is not used internally we can remove it - another option would be to prefix it with UNSTABLE_ and keep it for a while?

I does not exist anymore since we don't apply filter by filter but row by row.
The behavior was broken before but I don't think I can even keep this broken behavior.


Side note : I will do a follow up PR one day so that deleteFilter, upsertFilter and applyFilterLinkOperator uses setFilterModel instead of direct setGridState.
I think we should try to reduce the amount of distinct setGridState in those feature hooks, it is easier to track when there is no more than 2-3 functions that edit the state.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 26, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 26, 2021
@flaviendelangle flaviendelangle merged commit 6af3730 into mui:next Oct 26, 2021
@flaviendelangle flaviendelangle deleted the filter-rework branch October 26, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants