[New feature] Allowing the ListView slivers to have different extents while still having scrolling performance

[xu-baolin]

Fixes #113431

Currently we only support specifying all slivers to have the same extent.
This patch introduces an itemExtentBuilder property for ListView, allowing the slivers to have different extents while still having scrolling performance, especially when the scroll position changes drastically(such as scrolling by the scrollbar or controller.jumpTo()).

@Piinks Hi, Any thoughts about this? :)

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[Piinks]

This looks very exciting! Thank you for putting this together!!
I am have been meaning to get to something similar, just have not had the opportunity yet. I thought this type of approach would resolve #52207

[Piinks]

What do you think of passing an index plus a little bit more information about the viewport?

My original thought for this was to see what folks thought of the TableSpanExtent API in the TableView (flutter/packages#4536) and maybe move that into the framework.
This section of the design document talks about it (links direct to that section): https://docs.google.com/document/d/15ecTZE1g3WeswLGFWrnEgMP6SyL6jDRdxOgPsczOcV0/edit?resourcekey=0-yNd_qFhiPjz6z2TgezWc0A#heading=h.a8uex5ucyiso

It defines multiple ways for developers to set an extent, based on the viewport, or the preceding scroll extent, or as the result of an operation, etc.

I figured if it worked well for folks (we did already test it in a usability study), we could use it here as well, making it a super class like ChildScrollExtent or some such - names are hard. 😆

[xu-baolin]

What do you think of passing an index plus a little bit more information about the viewport?

What do you think of exposing the SliverConstraints directly or just some of the key properity of viewport?

[Piinks]

Hmmm, that's a good idea! It won't communicate the information about how much the SliverList has laid out already though, since the SliverConstraints relate to the whole sliver and not its contents. What parts of SliverConstraints would be relevant to the user when building an item of the list? Definitely the viewportExtent I think. Maybe we can wrap those parts up in an object with additional info like how much extent has been laid out.

[xu-baolin]

I have wraped some layout relation properity into SliverLayoutDimensions and exposed it to the callback.

Will finish the test cases ASAP.

[Piinks]

So excited about this. 😁 Thanks for the update!

[Piinks]

Suggested change

	/// Holds the relation dimensions of the [RenderSliver] during layout.
	/// Relates the dimensions of the [RenderSliver] during layout.
	///
	/// Used by...

Can add a reference to the item extent callback from sliver list / list view?

[Piinks]

Suggested change

	ItemExtentGetter? get itemExtentCallback => null;
	ItemExtentGetter? get itemExtentBuilder => null;

WDYT of this name?

[Piinks]

I think this is nearly there. Thank you very much for putting this together, and for your patience. I found the issue requesting this and added it to the description.

[Piinks]

Could this be nullable? Or maybe we can add an assertion as the body of this getter that says this should not be used?

[xu-baolin]

Yes, I have change it to nullable and add a assertion at the beginning of RenderSliverFixedExtentBoxAdaptor.performLayout()

    assert((itemExtent != null && itemExtentBuilder == null) ||
        (itemExtent == null && itemExtentBuilder != null));
    assert(itemExtentBuilder != null || (itemExtent!.isFinite && itemExtent! >= 0));

[Piinks]

In general, does this need to be it's own class? Would it be possible to use SliverFixedExtentList for both cases? I am curious for your thinking first, no need to change the code right now. As a developers, I wonder, what is the difference between Fixed and Explicit?

[xu-baolin]

I have considered extending SliverFixedExtentList to support variable extent.
But I'm worried that this will break the developer's understanding of SliverFixedExtentList.
Maybe we need a better name to tell them the difference between the two.
It's really hard to name it, any suggestions? 😄

[Piinks]

But I'm worried that this will break the developer's understanding of SliverFixedExtentList.

I agree with you on this. Sorry if my question led to some code changes we should revert now. Let's keep SliverFixedExtent as it is, and integrate itemExtentBuilder into a new sliver like you proposed. :)

It's really hard to name it, any suggestions?

Hmmmm I agree names are hard.
What if...

• SliverFixedExtentList, uses itemExtent, one fixed extent for all children
• SliverVariedExtentList, uses itemExtentBuilder, children have extents provided by the builder

Then we can have them refer to one another for developers to choose what they would prefer to use.

As I was thinking about it this weekend, I think having separate render objects is a good idea. Keeps the code clearer, easier to follow, and maintain

[xu-baolin]

That's a great name : )

[Piinks]

What if when null, this builder just returns itemExtent for every index by default? Maybe it could simplify the code below with if (itemExtentBuilder == null)?

[xu-baolin]

I have added some docs for this.

  /// The main-axis extent builder of each item.
  ///
  /// If this is non-null, the [itemExtent] must be null.
  /// If this is null, the [itemExtent] must be non-null.
  ItemExtentBuilder? get itemExtentBuilder => null;

[GeylanKalafMohe]

Hello please approve

[Piinks]

LGTM! I am really excited for folks to have this, thank you so much for your work on this! 🎉

[oddko]

Thank you so much for your great work !

[xu-baolin]

@Piinks Hi, Thank you very much for reviewing. Hope you guys enjoyed your trip to Shanghai last week : ) Oh, by the way, I have recently been trying to introduce a caching mechanism to ListView to reduce unnecessary layout operations to further optimize its scrolling performance. I hope to find a better solution.

[Piinks]

Hope you guys enjoyed your trip to Shanghai last week : )

Absolutely! Meeting each other was a highlight of the trip! 🎉

Oh, by the way, I have recently been trying to introduce a caching mechanism to ListView to reduce unnecessary layout operations to further optimize its scrolling performance. I hope to find a better solution.

This is really interesting, I would be happy to hear more if you are interested in pursuing it. I think in some cases caching could work, we could see what it looks like to give developers the option. Not sure. :)