[DataGrid] Create a new lookup with all the filtered rows, collapsed or not

[flaviendelangle]

Part of #3610

I will modify the default behavior in #3610 or in a standalone PR if #3610 is merged before so that the CSV export has the collapsed rows by default.

---

In term of naming, we are stuck before v6 with our current "visible" prefix which is not great.
So right now we have:

• visible => all the rows given to the pagination process, they pass the filtering process AND they are not collapsed.
• filtered => all the rows that pass the filtering process, whether they are collapsed or not.

We should find a better name than visible in v6 and keep visible for the use case where we are talking about the rows currently displayed on the screen.

---

Closed in favor of #3736 (CircleCI has a weird bug)

[mui-bot]

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	140.5	316	219.8	222.62	71.262
Sort 100k rows	ms	289.4	606.6	467.7	466.84	106.477
Select 100k rows	ms	131.4	266.5	208.7	196.32	46.045
Deselect 100k rows	ms	95.5	300.6	156	155.44	75.303

Generated by 🚫 dangerJS against 6177b40

[cherniavskii]

So rows that are collapsed will be included in visibleRowsLookup, but with value set to false?

[flaviendelangle]

The behavior of visibleRowsLookup remains the same
The collapsed rows are included with value set to false yes

[alexfauquette]

When reading it alone, it feels like "Why are you saving two times the same thing?"
A comment such as "visibleRowsLookup will be modified by the treeData and rowGrouping hook" could help to keep in mind which part of the code will reuse the difference between them.

By the way, don't you need to use visibleRowsLookup: {...filteredRowsLookup} to avoid side effects on filteredRowsLookup when modifying visibleRowsLookup?

[flaviendelangle]

This lookup can be huge so I wanted to avoid doubling the memory allocated.
Mutating those lookup should cause tons of other issues since they are used in lots of places.

But the trade-off is debatable. Maybe the RAM is not an issue even for large list of rows.

[flaviendelangle]

I'll add a comment to explain why we return the same thing twice 👍

[alexfauquette]

If you do not double the memory consumption, how do you do to double the number of information per row?

The only solution to avoid useless consumption of memory I see is to do the copy in tree data function, such that if tree data and grouping is not used, the objects are the same (which is not a problem)

[flaviendelangle]

For the flat tree, I dont do visibleRowsLookup: {...filteredRowsLookup}, that way I don't increase the memory used.
For non-flat tree, I do double the memory used, but as you say, the two lookups being different it's logical.

The line we are commenting is only used for the flat tree.

[cherniavskii]

visible => all the rows given to the pagination process, they pass the filtering process AND they are not collapsed.
filtered => all the rows that pass the filtering process, whether they are collapsed or not.

So visible is a subset of filtered excluding collapsed rows, right?
In that case we may consider renaming visible to expanded in v6:

-gridVisibleSortedRowEntriesSelector
+gridExpandedSortedRowEntriesSelector

[alexfauquette]

I have added a point in #3287 for the v6 naming modification

[alexfauquette]

When reading it alone, it feels like "Why are you saving two times the same thing?"
A comment such as "visibleRowsLookup will be modified by the treeData and rowGrouping hook" could help to keep in mind which part of the code will reuse the difference between them.

By the way, don't you need to use visibleRowsLookup: {...filteredRowsLookup} to avoid side effects on filteredRowsLookup when modifying visibleRowsLookup?

[flaviendelangle]

@cherniavskii yes I am talking about it in the description of the PR. 👍

[cherniavskii]

@cherniavskii yes I am talking about it in the description of the PR. 👍

Right, I've read it and that's my proposal for renaming :)
Also, I just wanted to make sure I understand it correctly, hence the question

[flaviendelangle]

OK :)

For me we have two possibilities in v6:

1. The new gridFilteredSortedRowEntriesSelector is used on the demo in place of gridVisibleSortedRowEntriesSelector. In which case we can reserve gridVisibleSortedRowEntriesSelector for tree-related scenarios and hence rename it gridExpandedSortedRowEntriesSelector

2. gridVisibleSortedRowEntriesSelector remains the one we use most of the time because we prefer our code to be "tree-ready". In which case, renaming it with a name like gridExpandedSortedRowEntriesSelector can bring confusion for users not using the tree-related features

The 1. is nice if it's viable because the naming feels right.

[cherniavskii]

OK :)

For me we have two possibilities in v6:

1. The new gridFilteredSortedRowEntriesSelector is used on the demo in place of gridVisibleSortedRowEntriesSelector. In which case we can reserve gridVisibleSortedRowEntriesSelector for tree-related scenarios and hence rename it gridExpandedSortedRowEntriesSelector
2. gridVisibleSortedRowEntriesSelector remains the one we use most of the time because we prefer our code to be "tree-ready". In which case, renaming it with a name like gridExpandedSortedRowEntriesSelector can bring confusion for users not using the tree-related features

The 1. is nice if it's viable because the naming feels right.

Right, nr. 2 really might be confusing

[flaviendelangle]

I don't understand why the build fails like that...