fix: (datagrid) filtering not working well with Ivy off #5117
Conversation
gnomeontherun
left a comment
gnomeontherun
left a comment
There was a problem hiding this comment.
We've had a regression from fixes here before. I want to be 100% certain before we merge that this is correct. How have we tested this?
|
As I said, automated tests are not possible, so:
Any other testing ideas or manually performed testing by the reviewers are welcome. I won't merge this for a couple of day more to allow for more reviews. |
|
After today's tests I made a more precise selection of what should be removed - only the views marked as "destroyed". Without it - it was possible to get "pseudo-randomly" rearranged rows after update. |
|
Thanks, I wanted to be as certain as we can. It helps to put more of this detail into the issue PR description as well in the future. |
|
Thanks form me too - it was good to have one more round - we found an issue. I'm postponing the unblocking with 1 more day. |
| // Necessary with Ivy off. See https://github.com/vmware/clarity/issues/4692 | ||
| for (let i = this._displayedRows.length - 1; i >= 0; i--) { | ||
| if (this._displayedRows.get(i).destroyed) { | ||
| this._displayedRows.detach(i); |
There was a problem hiding this comment.
I think this was due to an Angular bug—something along the lines of this: angular/angular#38648.
When ViewRef destroys itself independently without going through ViewContainerRef as we do in our WrappedRow's OnDestroy cycle, the ViewContainerRef still holds the reference to the ViewRef. And that results in out-of-sync views and ViewContainerRef. So we have to have this extra removal from the ViewContainerRef itself. Though I think, instead of ViewContainerRef.detach() which detaches the DOM view of undestroyed ViewRef, ViewContainerRef.remove() might be more appropriate here as we are trying to completely remove the views that are "impartially" destroyed.
There was a problem hiding this comment.
I think I started with the ViewContainerRef.remove() at first, but it was having some side effects. Then I saw that in other part of this same TS file we're using detach() and it worked better. But I'm not sure if I was doing the .destroyed check back then, so I'll give it a try, out of curiosity at least.
Thanks for the detailed review, Shijir!
There was a problem hiding this comment.
You're right, though it probably gets reclaimed after being detached, using remove gives us finer level of control and better protection against memory leaks. Updated all 3 PRs to use remove()
issue vmware-archive#5085 Signed-off-by: Ivan Donchev <idonchev@vmware.com>
hippee-lee
dismissed stale reviews from Shijir, sis0k0, and bbogdanov
The base branch was changed.
|
@Jinnie - fyi I updated the base branch to reflect the new default for the repo. |
bbogdanov
left a comment
bbogdanov
left a comment
There was a problem hiding this comment.
What do you think about creating an issue or a discussion to revisit this code at some point in order to covert it with tests?
6 participants
issue #5085
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When Ivy is turned off, the displayedRows list accumulates old ghost items, which mess the proper child views visualization. Thus, after been filtered out, rows are not shown back when the filter is removed.
Issue Number: #5085
What is the new behavior?
Works fine both with and without Ivy
Does this PR introduce a breaking change?
Other information
Please note, that because of the complicated setup we can't make a test for the no-Ivy scenario. We can't have a test run with and without Ivy unless we run a new task with new test setup.