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

Possible performance issue in 3.0 #38

Closed
kinex opened this issue May 7, 2020 · 22 comments · Fixed by #52
Closed

Possible performance issue in 3.0 #38

kinex opened this issue May 7, 2020 · 22 comments · Fixed by #52

Comments

@kinex
Copy link

kinex commented May 7, 2020

I tried upgrading form 2.31 to 3.0. After the upgrade the parent widget where I have GroupedListView gets rebuilt every time I scroll the list view. In other words the widget including GroupedListView is rebuilt repeatedly unnecessarily. This makes the list very slow. Issue gets fixed if I revert to 2.3.1. Any ideas what could be wrong?

Simplified build method where the problem occurs:

  @override
  Widget build(BuildContext context) {
     return ChangeNotifierProvider<ItemsViewModel>.value(
        value: _items,
        child: Consumer<ItemsViewModel>(
            builder: (context, viewModel, _) {
            return GroupedListView(elements: viewModels.items,...);
          }
...
@Dimibe
Copy link
Owner

Dimibe commented May 7, 2020

@kinex Do you have set the option useStickyGroupSeparators to true in the version 2.3.1?
This option is by default active since version 3.0.0 and I think thats where the performance issues comes from. So workaround might be to set this option explicitly to false in verison 3.0.0.
I'am working on a fix and let you know when I have any updates on this.

Thanks for your help.

@kinex
Copy link
Author

kinex commented May 7, 2020

Thanks for your quick response. I am not setting it to true in 2.31. I will try your suggested workaround soon.

@icnahom
Copy link

icnahom commented May 21, 2020

This option is by default active since version 3.0.0 and I think thats where the performance issues comes from. So workaround might be to set this option explicitly to false in verison 3.0.0.

I think it's better to have it as false by default, Dimibe. Now people will be forced to add one line for the feature they don't want.

@joshuaboltzmc
Copy link

I noticed a similar issue in my app that was using Grouped List 2.3.0, and setting the useStickyGroupSeparators value to false fixed my performance issues. But, without this setting, how can i get sticky headers?

I tried updating to 3.1.0 and am getting basically the same results where if i have it set to true, it causes performance issues when scrolling.

@joshuaboltzmc
Copy link

Upon further looking, it looks like what's happening is every time the sticky header changes (as i scroll down, if currently on "A", when the "A" tries to change to "B", which assuming is changing state) makes some child widgets in my grouped list widget rebuild. Those child widgets use provider to manage and track their state.

@zanesc
Copy link

zanesc commented Jun 2, 2020

I am not sure if this is related but I am using 3.1.0 with useStickyGroupSeparators set to true and two items in my list of about 50 items are swapping positions. I can see the text constantly swap between the two while I scroll. The performance is a lot better if I disable useStickyGroupSeparators but I really want it. Thanks for all of your work so far.

With useStickyGroupSeparators set to false, the same two elements that were swapping positions are permanently swapped into the incorrect position. They are the 4th (index 3) and 9th (index 8) items in the 11th group (index 10). The live swapping was happening on both iOS and Android with useStickyGroupSeparators set to true, but with it set to false the permanent swap only happens on Android. Tested on 2 iOS and 2 Android devices.

Edit: scratch that... it's happening on both iOS and Android. Some really odd behaviour... on each device the items are in the correct order in one orientation but if I switch the orientation of the device the items swap.

@ermand-hoxha
Copy link

I have the same problem, i'm using version 3.1.0. Is there a solution for it?

@danilonogueirateixeira
Copy link

I have the same problem, when useStickyGroupSeparators set to true two elements that were swapping positions.

@Dimibe
Copy link
Owner

Dimibe commented Jun 23, 2020

Version 3.2.0 has just been released.

Performance issues:
There should be an increase in performance, since the widget is now only rebuild when necessary and not all the time.
Please let me know if there are still performance issues. I'm working on further performance improvements but this fix maybe should fix some of the issues.

Items swapping:
Problems with items swapping positions may occure when items are equals in aspects of comparison but display different values. The widgets sorts the items internally, so different items in terms of their builded widget should also clearly distinguish when sorted/compared.

@ermand-hoxha
Copy link

I still have that issue, also with the new version 3.2.0.

@Dimibe
Copy link
Owner

Dimibe commented Jun 23, 2020

@ermand-hoxha Performance Issues or the issue with the items being swapped?

@ermand-hoxha
Copy link

Performance issue

@allanwolski
Copy link

I see a performance improvement, but I have a problem when the header items swapped.

How to fix this?

@Dimibe
Copy link
Owner

Dimibe commented Jun 24, 2020

@allanwolski Like I mentioned this problem most likely comes from your elements not being sorted correctly. Please make sure your items are being sorted according their display value or sort them yourself and disable automatic sorting with the option sort: false in the list.

@Dimibe
Copy link
Owner

Dimibe commented Jun 24, 2020

I have created another package which depends on the scrollable_positioned_list because this was a highly requested feature.
Due to the underlying structure I think the package has better performance as well. If some of you want to try it out, here is the link: https://github.com/Dimibe/sticky_grouped_list.

@joshuaboltzmc
Copy link

I've tried that Sticky Grouped List, which is very identical in setup to Grouped List, and I am still seeing performance issues on a Flutter web project. Mobile is fine, but may have the same issues, although less noticeable because it's a small render.

The issue is that i have a grouped list view, and within each row (aka client), i access some state from Provider about the number of projects that client has. I notice that as i scroll down the page, and it comes to a new sticky header, it rebuilds the state or something which makes the page scroll lag because it's rebuilding the state for that count number for each row under that sticky header it just entered.

Perhaps this can be fixed in this package, or perhaps I have an improvement to make in how I am handling the state, or perhaps i can tell the sticky grouped list widget to not re-build that state on that child component on each row.

@zanesc
Copy link

zanesc commented Jul 17, 2020

Just tested 3.2.3 and useStickyGroupSeparators is still unusable. Performance is a littler better but there is still noticeable lag when the sticky group separate is changing.

The issue with items swapping still persists. The items are order correctly. New in 3.2.3, the items now swap live while scrolling when the sticky group separator is changing. This only happens with useStickyGroupSeparators set to true. It works perfectly otherwise.

The sticky_grouped_list package has the exact same issues.

@Dimibe
Copy link
Owner

Dimibe commented Jul 18, 2020

@zanesc
When set useStickyGroupSeparators to true the list package the items internally. This is done by comparing the groupBy value of the elements and the elements itself. If the items are swapping that is because either the sorting of the groupBy values or the elements is not consistent. If thats the case each time the widget rebuilds the order may change.
To fix this you need to fix your code so that the sorting is clear. Here you can find the sorting code of the widget to check on yourself.

Currently I see no possibility to change the prevent the widget from rebuilding when the sticky header changes.
Thats also the reason why the swapping of your items only occur with the sticky headers, because the rebuild is only needed to change them.

@zanesc
Copy link

zanesc commented Jul 18, 2020

@Dimibe The elements are ordered correctly. The categories are ordered by a categorySortOrder int and there are no duplicates, and the items are ordered by a sortOrder int with no duplicates within their respective categories.

This isn't an issue for me as I would not be using the stickyGroupSeperators because of the noticeable lag when the sticky headers change. I haven't taken a look at your code but are you sure there is no way to separate the sticky header widget from the list widget such that only the sticky header widget rebuilds when necessary and not the list widget?

Maybe @danilonogueirateixeira can shed some light on their case to confirm that their elements are ordered correctly as well. If you'd like me to I can create a new issue for this to track any other users who may be having the same issue.

If I get the time I will create a project to reproduce the issue to share with you, but I can't at the moment.

@Dimibe
Copy link
Owner

Dimibe commented Jul 24, 2020

Performance issues fixed in version 3.3.0

@zanesc Thank you for your detailed explanation. I released a new version 3.3.0 where the list does not rebuild when the sticky group header changes. With that I expect the performance issues to disappear. Therefore I have closed this issue.

For the issue with the changing order please create a new issue if still present. It would appreciate a small example with shows the issue because I was not able to reproduce it.

@joshuaboltzmc
Copy link

Thanks so much for fixing this! The performance is much better using 3.3.0!

@zanesc
Copy link

zanesc commented Aug 6, 2020

Thanks @Dimibe v3.3.0 is much better!

I fixed the ordering issue by setting sort to false on GroupedListView.

The only other issue I see is that the header on the list is still visible below the sticky group header which creates an overlap but I guess this was the sacrifice that you had to make in order to not rebuild the list when the header changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants