Clarity: [Datagrid] Update and refresh for a data grid row breaks select all toggle functionality

[gitnarendra]

Select one ... (check one with "x")

[ x ] bug
[ ] feature request
[ ] enhancement

Expected behavior

Checkbox for each row on data grid should be checked.

Actual behavior

Checkbox for each row on data grid appears unchecked.

Reproduction of behavior

Stackblitz : https://stackblitz.com/edit/clr-datagrid-server-driven-demo-n4hqd3 (updated)

1. Select an item on the data grid.
2. On item selection _“Update and Refresh”_ button will be shown on the top of grid.
3. Click the “Update and Refresh” button, here the expectation is to update a field in the row and get the updated values from back end.
4. Now try selecting all rows by clicking the select all toggle on grid header.

Environment details

• Angular version: 5.2.0 and 6.0.1

• Clarity version: 0.11.16

• OS and version: Windows Server 2012

• Browser: all

Note

If the trackBy option is not used then the grid works as expected.
Stackblitz : https://stackblitz.com/edit/clr-datagrid-server-driven-demo-geauyd

[gnomeontherun]

Did you save your stackblitz correctly? https://stackblitz.com/edit/clr-datagrid-server-driven-demo is not showing the button as you describe.

[gitnarendra]

Yes I did save the stackblitz correctly. I just clicked on the link and can see the “Update and Refresh” button upon an item selection in data grid. Did you select an item in data grid?

I have forked the stackblitz again, here is the new link https://stackblitz.com/edit/clr-datagrid-server-driven-demo-n4hqd3. Let me know if this works for you.

[gnomeontherun]

Ah it only shows with one item selected. That was my mistake.

I suspect it is an order of lifecycle events that is causing this particular issue, since if you select one, use the refresh, then paginate forward and back, it appears correctly. We have the right state, but the lifecycle is off slightly.

[gitnarendra]

I know this is not a show stopper issue, but Can we expect a fix for this issue in near future.

[mathisscott]

@gitnarendra
Sorry for the delay. What is the time pressure on this issue for your team?

[gitnarendra]

For now it's buried somewhere deep into the backlog item list as we have other priority items to catch up with. But i know it can become a pressing issue in no time so i would appreciate if we can get a fix sooner than later may be in a month time from now.

[mathisscott]

@gitnarendra
We still need to dig into why this is happening in order to determine how it can be fixed. We can't commit to it being done exactly one month from now. But we will see what we can do from a prioritization standpoint.

Thanks!

[gitnarendra]

@mathisscott
Thanks for the prompt response :-).
I have one request, Please keep me informed about the timeline to get this issue fixed, so that i can communicate the same across my team.

Thank you...!!!

[zhanivanov]

Hello everyone, I faced a similar if not the same issue with clarity's datagrid component. You can see the behavior here: https://stackblitz.com/edit/angular-qf9vvu.
1) select all
2) click refresh

After that refresh the select all functionality is broken. When I replace ngFor with clrDgItems it is working as expected but I need it to work with ngFor. If I add new entries it works correctly - it only fails if I try to assign new array with same items(with same IDs not references).

My question is - is this the same issue and if so is there any progress with it?
Thanks!

[hippee-lee]

Hi @zhangc119 - your stackblitz looks like its re-creating the same issue. I'm really sorry but due to higher priorities I had to put investigating the proper fix for this on hold for now.

[caubah]

@hippee-lee is there a known workaround for this? As Zhan pointed out polling is a central functionality that is implemented in a bunch of grids in our product.

[Jinnie]

Hi Sibin and Zhan,
A workaround would be to drop the "trackBy" clause from your *ngFor.
As you are using ngFor instead of clrDgItems, then you are probably doing a server-driven datagrid and hopefully, with server-side paging/filtering etc. So, the actual number of elements on the DOM, at the current page, is unlikely to be very large. The idea that follows is that the drop in performance from not tracking your elements by id would be negligible. Also paging/sorting/filtering operations that reload your data, are not very likely to return elements that were available before reload, such that can be reused, anyway.
Hope this helps,
Ivan

[Jinnie]

What I'm suggesting above won't work very well if you need to live-update the content of a cell only. As it will clear your selected items, for example. This might be undesirable if it happens once a second or so. But it won't leave your UI in state where the checkbox is not working at all, at least.

[caubah]

Thanks Ivan,
Yes and no - trackBy is actually intentionally dropped in those pages due to performance reasons - we are using infinite scrolling in all of our pages and this very cases in order to load certain chunks/pages of data we need to fetch all of the previous pages. And again the page in context loads a list of requests which considering the SaaS context of our product might turn to have thousands of records.

[Jinnie]

Yes, with this scenario you will have a large number of DOM elements. You will need the trackby.

[Jinnie]

I managed to take a look at the root cause for this, though I don't have a fix at the present moment.
I'll describe my findings for whoever will be working on this.

So, when you change data source -> the clr-datagrid-rows get updated.
The datagrid component has subscription to rows.changes -> this subscription sets items.all on the Item service with new values
This change emits an allChanges event <- In the meantime, the selection service listens for this event and when it comes, it updates the items it holds with the new ones. It actually matches old items and new items using the trackby function.

The catch is that, when we have trackby in the ngFor - the actual rows from step 1 are not really changed. The rows.changes is not triggered and the whole chain of actions described above does not happen.
As a result - the _current_ collection in the selection service retains both the old objects and new objects. The actual row items are linked to the new ones, while the selection service works with the old ones.

If you are adding, or removing objects, then it should work. But it will fail when you are updating existing objects, as in Zhan's demo.

[Jinnie]

If you update the refresh() method on the stackblitz like this:

refresh() {
this.serverTrackByIdUsers = [
{ id: 1, name: 'test 1' },
{ id: 2, name: updated test 2 },
{ id: 3, name: 'test 3' },
];
}

you will see that the 1st refresh works okay (because it introduces the new item3), while the next updates fail.

[kathygit]

OMG, t hat is exactly what I noticed. I wonder if we can trigger the rows.changes manually ?
Does it have a performance impact same as removing trackby ?

[kathygit]

I remove trackBy, the selection is still wrong.

[clrDgItem]="task" [(clrDgSelected)]="task.selected">

I also tried without async, it still fails
[clrDgItem]="task" [(clrDgSelected)]="task.selected">

At this point, this work around doesn't work for me.
Almost all our views have server driven paging, and half of them have multi-select.
Please let me know when this issue will be fixed.

[Jinnie]

Yes, @kathygit, trackby only "fixes" visually the select-all functionality, but the main problem remains underneath, as we still have the "orphaned" selected items in the list, which have the same data inside, but are different objects.
Depending on how your data gets updated, you may try to clear the selection when new data arrives. This will work if the user makes manual updates and losing selection is only annoying, but does not really ruin what the user is trying to do, but won't be a good idea if you are polling for updates and changing some rows once per second or so.

[kathygit]

Thanks lot for the help ! @Jinnie
This workaround works for me:
1) remove trackby
2) add subscription to query to clear out the selection

[Jinnie]

Well, if it works when we add or remove data, but does not work when we only alter data, then we may sacrifice one item to trigger an update.
Here is a stackblitz with the workaround idea:
https://stackblitz.com/edit/issue-2265-workaround
It is only a POC, you will have to modify it to suit your needs - this demo accepts that you are tracking data by item.id. Also, you have to make sure it will not crash for empty data, etc.
Also, as it removes an item, this will cause a short "flickering" effect as the datagrid gets shorter for a moment. This will be unpleasant with polling. You may try to add a placeholder item instead, or only modify the id, or set css height the same, to keep the length of the grid the same. Sorry, I don't have time to polish it too much at the moment.

edit: adding this to the remove method seems to do the work good enough: this.serverTrackByIdUsers.push({});

[Jinnie]

One last update.
The correct and easy way to force refresh is
...
@ViewChild(ClrDatagrid) grid: ClrDatagrid;
...
this.grid.rows.notifyOnChanges();

Example updated:
https://stackblitz.com/edit/issue-2265-workaround

[zhanivanov]

I tried the workaround and I can confirm that it is working as expected. Thanks!

[tarangkhandelwal]

Hi,
We are looking to have this fixed. The workaround suggested works but not sure if the performance hit would be big for us.

We have a pattern of inline editing inside the grid.

Is this planned for v3 ?

Thanks!

[gnomeontherun]

@Jinnie can you take a look at this one as well?

[zoom2ashish]

@Jinnie I had the fix with "this.grid.rows.notifyOnChanges();" as you had suggested and it was working fine till Clarity v2. I recently upgraded to Clarity v3 + Angular 10, and I am not seeing "Object Unsubscribed" error when the grid is loaded.

If I remove this particular line, then error is not happening and grid is loading fine.