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

Add a lazy SliverTree #114299

Open
naamapps opened this issue Oct 29, 2022 · 6 comments · May be fixed by #147171
Open

Add a lazy SliverTree #114299

naamapps opened this issue Oct 29, 2022 · 6 comments · May be fixed by #147171
Assignees
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter design doc Tracks a design discussion document f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. P3 Issues that are less important to the Flutter project 📜 Design doc announcement was posted to Discord. Remove to have the issue reannounced. team-framework Owned by Framework team triaged-framework Triaged by Framework team waiting for PR to land (fixed) A fix is in flight

Comments

@naamapps
Copy link

naamapps commented Oct 29, 2022

📜 Design Document


Related:

Use case

Currently when trying to implement tree UIs (for example: comments section with nested comments - see Instagram's comments for reference) there is no way to do it but to flatten the list and use one SliverList to show the data in the UI.

Proposal

Add a way to add a SliverList inside of a SliverList.
This will allow the CustomListView to scroll with all the data while also perform nicely with all the Slivers benefits.

In my use case, this is critical in SliverAnimatedList.

If you have any other solutions I will be happy to discuss.

Solutions I tried

  1. Using a ListView.Builder inside the SliverList builder function with shrinkWrap: true. - Poor Performance
  2. As pointed above - Normalizing the nested list with the list.expand function and using this list as the data source. - Poor Developer Experience - because everytime new data arrives we need to manage the flattened list which is not ideal.

Code example

CustomScrollView(
    slivers: [
      SliverList(
        delegate: SliverChildBuilderDelegate(
          (context, index) {
            final data = widget.comments[index];
            return SliverList(
              delegate: SliverChildBuilderDelegate(
                (context, index) {
                  if (index == 0) {
                    return CommentWidget(data);
                  } else {
                    return CommentWidget(
                        data.comments[index - 1],
                        isNested: true);
                  }
                },
                childCount: data.comments.length + 1,
              ),
            );
          },
          childCount: widget.comments.length,
        ),
      ),
    ]
)
@jonahwilliams
Copy link
Member

Not quite SliverList, but this would be called something like SliverGroup. I don't believe we've writen it yet. FYI @Piinks if we have a bug for this already

@naamapps
Copy link
Author

Thanks for the quick response @jonahwilliams,
Just to clarify, we need a widget that gets a builder function in order to get the most performence out of this feature.
Adding a widget that gets a children array is not quite helpful here since we can have plenty of nested items.

@danagbemava-nc danagbemava-nc added in triage Presently being triaged by the triage team c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. c: proposal A detailed proposal for a change to Flutter and removed in triage Presently being triaged by the triage team labels Oct 31, 2022
@Piinks
Copy link
Contributor

Piinks commented Oct 31, 2022

I think there are a lot of options here, and lot of work currently in progress that will help this scenario.

Adding a widget that gets a children array is not quite helpful here since we can have plenty of nested items.

@jonahwilliams is right, a SliverGroup would be good here, if the contents of the SliverGroup are lazy loading, then the entire group will be lazy loading. We're planning on implementing this in the near future, it is being tracked in #33137 cc @thkim1011

However, it sounds like having a Tree builder widget would be more appropriate for this use case - and I am actually working on one right now. 😄 I am adding this issue into the 2D scrolling project for tracking so that I remember to come back and report on the design as it is coming along. There are other related issues there that you may be interested in where folks have shared their own solutions, packages and such while we get closer to finishing the tree widget.

SliverList supports box children, and while I don't know what it would look like to support boxes and/or sliver children, it may unnecessarily complicate the API, especially if we have new features on the way that will better support this use case.

@Piinks Piinks changed the title Nested SliverList in CustomScrollView for tree UIs Lazy building scrolling tree Oct 31, 2022
@naamapps
Copy link
Author

Thanks @Piinks for the detailed information.

I think that SliverList supporting both box and slivers as children actually simplifies the API, don't you think?
It is more friendly for new flutter devs and also more intuitive in my opinion..

I prefer building my own solutions than using a widget with limited customizations.
I have set in my project an infra for scrolling (with plenty of customization - animations, states and more) and it would really complicate the code to add a new widget..

That is why I suggested to nest a SliverList inside a SliverList, just because it is logical and also easy to use and understand as a developer.

Nevertheless, I look forward to see your solutions!
Hope to hear from you soon,
Thanks

@goderbauer goderbauer added the P3 Issues that are less important to the Flutter project label Nov 1, 2022
@flutter-triage-bot flutter-triage-bot bot added team-framework Owned by Framework team triaged-framework Triaged by Framework team labels Jul 8, 2023
@zhujiageng
Copy link

zhujiageng commented Oct 9, 2023

any update on this? it can help solve many performance issues.

@Piinks Piinks changed the title Lazy building scrolling tree Add a SliverTree Nov 3, 2023
@Piinks Piinks changed the title Add a SliverTree Add a lazy SliverTree Nov 3, 2023
@Piinks Piinks self-assigned this Dec 12, 2023
auto-submit bot pushed a commit that referenced this issue Apr 12, 2024
This cleans up a few sliver classes, like moving RenderSliverVariedExtentList to the rendering layer (it was in the widgets layer), and moving SliverVariedExtentList to live with its sibling subclasses, SliverFixedExtentList, SliverList, and so on.
I moved these while working on SliverTree, so figure I should break out into a separate change.

SliverTree and SliverCarousel (both inbound in separate changes) will also be subclasses of RenderSliverFixedExtentBoxAdaptor, organizing them together felt easier to work with.

Related to #114299 and #125980
@flutter-triage-bot flutter-triage-bot bot added the Bot is counting down the days until it unassigns the issue label Apr 16, 2024
@flutter-triage-bot
Copy link

This issue is assigned to @Piinks but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

@Piinks Piinks linked a pull request Apr 22, 2024 that will close this issue
9 tasks
@Piinks Piinks added waiting for PR to land (fixed) A fix is in flight and removed Bot is counting down the days until it unassigns the issue labels Apr 22, 2024
gilnobrega pushed a commit to gilnobrega/flutter that referenced this issue Apr 22, 2024
This cleans up a few sliver classes, like moving RenderSliverVariedExtentList to the rendering layer (it was in the widgets layer), and moving SliverVariedExtentList to live with its sibling subclasses, SliverFixedExtentList, SliverList, and so on.
I moved these while working on SliverTree, so figure I should break out into a separate change.

SliverTree and SliverCarousel (both inbound in separate changes) will also be subclasses of RenderSliverFixedExtentBoxAdaptor, organizing them together felt easier to work with.

Related to flutter#114299 and flutter#125980
@Piinks Piinks added the design doc Tracks a design discussion document label Apr 24, 2024
@flutter-triage-bot flutter-triage-bot bot added the 📜 Design doc announcement was posted to Discord. Remove to have the issue reannounced. label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter design doc Tracks a design discussion document f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. P3 Issues that are less important to the Flutter project 📜 Design doc announcement was posted to Discord. Remove to have the issue reannounced. team-framework Owned by Framework team triaged-framework Triaged by Framework team waiting for PR to land (fixed) A fix is in flight
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants