Closed rayansailani closed 1 year ago
Hi @droshev , we will not be able to validate until we get an rc version for the "Virtual scrolling" , can you please do the needful?
@I543348 It was merged a week ago. Can you validate it?
@rayansailanim, can please validate this to see if this is still a concern? We do have to dome optimisation, when dealing with large dataset such has detaching non-visible items out of changedetection and adding back when it is approaching visible area.
But there is no needs to keep expanding 2000 items, when they are not visible.
@rayansailani can you please check this and validate ?
Have evaluated the expandAll
and collapseAll
functionality. Can you please respond wrt the second query ?
"If the table data source contains expand and collapse related information that allow the table to be rendered such that some rows are expanded while others being collapsed. Regarding this - Is there a way to programmatically pass the isExpanded param to each row ?"
@fkolar
As of now, there is no way to control if a node is collapsed or expanded. I think the solution should be the same as for the #9355. Now reading again this original description @rayansailani
Speed: Does this mean this not an issue anymore? I would argue here that dealing with 2000 records in memory is not ideal but having virtual scroll in place should resolve the issue, as long as it can also remove elements which are not wihtin the rendering frame
Pragmatically pass expand/collapse state Not at the moment, but there should be general solution that should work also for the other ticket #9355.
The thing I dont understand is why would you pass already expanded nodes?
Are you trying to preserve some state for the user and trying to open up the dialog where he/she left off?
@fkolar regarding the perf queries - tested with 5000items + virtual scroll enabled, works fine @ https://stackblitz.com/edit/angular-nrnzcj
For the question Are you trying to preserve some state for the user and trying to open up the dialog where he/she left off? The usecase I was looking at was where the API response from the backend contains information regarding whether a set of parent rows should be expanded or not.
Yes this:
The usecase I was looking at was where the API response from the backend contains information regarding whether a set of parent rows should be expanded or not.
@fkolar - I'm currently having a use case where there is a tree table with infinite scrolling. I have enabled expandOnInit
as it was a requirement. When the infinite scroll page change happens, I've noticed two things:
Since I had raised a concern regarding this issue initially, would you mind taking a look at this scenario once again. I understand that this issue is now assigned to you for the programmatically expand and collapse functionality. Do I need to create another issue for tracking ? either ways, requesting you to please take a look.
Thanks.
Once the PR for this issue #9355 is merged we can also extend it for this one. With the PR we will allow app to control the check state. This can be easily applied to expand state.
But in here we need to see what is the real cause of this. We should definitely look if we can expand item more lazily just before they get to the viewport or check using debug tools what is slow here.
platformBrowserDynamic() .bootstrapModule(AppModule)
.then((module) => {
... **enableDebugTools**(appComponent);
})
Then we can run ng.profiler.timeChangeDetection();
I think if its under 10ms we should be ok, but if its over then we can check what causes this performance problems
@rayansailani , when you say 1000 rows in tree table how this number distributes across rows, nodes, leafs , children, parents , etc.
@fkolar , will test with a 1000 rows and update the metrics by running a tableRef.expandAll
instead of using expandOnInit
Thanks.
@rayansailani can u pls respond ? pls use dummy data to test this. Thanks.
Hi @fkolar, created on sample stackblitz to check for 100 rows. I found that there's lot of latency in 100 row itself. https://stackblitz.com/edit/angular-xyplrd (you can change the number of rows to be created and see)
please take a look and confirm if that's expected.
Thanks.
The main root cause are several things:
_calculateVirtualScrollRows
is the problem here, when expanding everything.
detechChanges
, freezes everything. Check if we can:
We need to test this with the enableDebugTools(componentRef);
, what is going on with the detechChanges()
. this should finish like under 5 ms
Update:
const ITEMS: ExampleItem[] = new Array(50).fill(null).map((_, mainIndex) => ({
name: 'Laptops ' + mainIndex,
description: 'pede malesuada',
children: new Array(20).fill(null).map((_, index) => ({
name: `Astro Laptop 1516 ${mainIndex} / ${index}` ,
description: 'pede malesuada',
price: {
value: 489.01,
currency: 'EUR',
},
status: 'Out of stock',
statusColor: 'negative',
date: new FdDate(2020, 2, 5),
verified: true,
children: new Array(2).fill(null).map((_, index) => ({
name: `Astro Laptop 1516 ${mainIndex} / ${index}` ,
description: 'pede malesuada',
price: {
value: 489.01,
currency: 'EUR',
},
status: 'Out of stock',
statusColor: 'negative',
date: new FdDate(2020, 2, 5),
verified: true,
}))
}))
}));
private _listenToVirtualScroll(): void {
this._subscriptions.add(
this._tableScrollDispatcher
.verticallyScrolled()
.pipe(filter(() => this.virtualScroll && !!this.bodyHeight))
.subscribe(() => {
this._calculateVirtualScrollRows();
})
);
}
Is going to be called number of time depending how many items, children you have, so it can easily get call 100ts+ of times, if you have complex tree structure.
Hi @fkolar, Thanks for the suggestions.
in the current use case of mine, I have not enabled virtual scroll and face a problem when there are a few rows in the viewport. I have made the page size to 20
. It is strange that there is an issue with expanding with such less number of rows.
I had observed a response from you on this (https://github.com/SAP/fundamental-ngx/issues/9519) PR. I will try considering something similar instead of expandOnInit
property.
Regarding the other suggestions - Apply expand only children for visible rows when expand all is called, assuming the requirements is always expand 1st level otherwise apply this to children as well and As we scroll check if expandAll was triggered and apply this to other newly visible rows. Both of them seem to be viable options. I will try investigating them. But currently there is no means to programmatically expand or collapse the rows, like how we are able to select and unselect rows in the table. This is something of a blocker for any form of investigation.
Thanks.
@rayansailani:
Update:
_calculateVirtualScrollRows
is called too often, in some cases, where you expand all, scroll in the middle - easily 2000 x times (_listenToVirtualScroll
). Update:
@mikerodonnell89 , as I was reassigned back to my original team, i will not be able to continue, i think there is no issue with expand all and I introduced PR to show how we can control expand/collapse from the APP, the rest is part of the virtual scroll , which can be called too often.
@fkolar Yes, the expand/collapse logic for individual rows is working fine (same as checked state). As for the performance issue - noticed there is no debounceTime for the scrolling subscription in _listenToVirtualScroll()
which is why the function is called so often. Adding that helps drastically cut down on the number of times _calculateVirtualScrollRows()
is called:
/** @hidden */
private _listenToVirtualScroll(): void {
this._subscriptions.add(
this._tableScrollDispatcher
.verticallyScrolled()
.pipe(filter(() => this.virtualScroll && !!this.bodyHeight),
debounceTime(50))
.subscribe(() => {
console.log('_listenToVirtualScroll');
this._calculateVirtualScrollRows();
})
);
}
But adding debounceTime causes the scroll position to lunge at the end of a scroll by the user, as shown in the video below. It does this a bit without the debounceTime but i think may be more pronounced with it. So just need to figure a way around this
https://github.com/SAP/fundamental-ngx/assets/2471874/dc24763c-b918-4590-a849-ecfc002e5c6a
This issue is obsolete as main root cause was on the application side. Once app fix is merged we will evaluate this one more time and log more concrete and targeted issue. This issue was not on the F-NGX side
Is this a bug, enhancement, or feature request?
bug
Briefly describe your proposal.
The expandAll and collapseAll method provided by the fdp-table is very slow to take effect as tested in stackblitz. It would be a huge bog down when using these methods in the application with 1000's of rows.
Query: If the table data source contains expand and collapse related information that allow the table to be rendered such that some rows are expanded while others being collapsed. Regarding this - Is there a way to programmatically pass the
isExpanded
param to each row ?Which versions of Angular and Fundamental Library for Angular are affected? (If this is a feature request, use current version.)
0.38.2
If this is a bug, please provide steps for reproducing it.
This seems more like a performance issue or concern with the expandAll and collapseAll functionality. Please refer to the code.
Please provide relevant source code if applicable.
please refer to the video for more information. (link - https://user-images.githubusercontent.com/54993789/212942376-31601e4a-eb46-49da-b58d-1c3e4ea34693.mov)
Is there anything else we should know?