From 09e0e1b88e5cc8b9c28d0d3ce1d66311759b091d Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Wed, 21 Feb 2024 14:10:03 -0600 Subject: [PATCH] Deprecate redundant itemExtent in RenderSliverFixedExtentBoxAdaptor methods (#143412) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multiple methods in `RenderSliverFixedExtentBoxAdaptor` pass a `double itemExtent` for computing things like what children will be laid out, what the max scroll offset will be, and how the children will be laid out. Since `RenderSliverFixedExtentBoxAdaptor` was further subclassed to support a `itemExtentBuider` in `RenderSliverVariedExtentList`, these itemExtent parameters became useless when using that RenderObject. Reading through `RenderSliverFixedExtentBoxAdaptor.performLayout`, the remaining artifacts of passing around itemExtent make it hard to follow when it is irrelevant. `RenderSliverFixedExtentBoxAdaptor.itemExtent` is available from these methods, so it does not need to pass it. It is redundant API. Plus, if a bogus itemExtent is passed for some reason, errors will ensue and the layout will be incorrect. 💣 💥 Deprecating so we can remove these for a cleaner API. Unfortunately this is not supported by dart fix, but the fact that these methods are protected means usage outside of the framework is likely minimal. --- .../rendering/sliver_fixed_extent_list.dart | 84 +++++-- .../sliver_fixed_extent_layout_test.dart | 219 +++++++++++++++++- 2 files changed, 276 insertions(+), 27 deletions(-) diff --git a/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart b/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart index b91aad6dc73..f5d98073f58 100644 --- a/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart +++ b/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart @@ -56,14 +56,23 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda /// The layout offset for the child with the given index. /// - /// This function uses the returned value of [itemExtentBuilder] or the [itemExtent] - /// as an argument to avoid recomputing item size repeatedly during layout. + /// This function uses the returned value of [itemExtentBuilder] or the + /// [itemExtent] to avoid recomputing item size repeatedly during layout. /// /// By default, places the children in order, without gaps, starting from /// layout offset zero. + @visibleForTesting @protected - double indexToLayoutOffset(double itemExtent, int index) { + double indexToLayoutOffset( + @Deprecated( + 'The itemExtent is already available within the scope of this function. ' + 'This feature was deprecated after v3.20.0-7.0.pre.' + ) + double itemExtent, + int index, + ) { if (itemExtentBuilder == null) { + itemExtent = this.itemExtent!; return itemExtent * index; } else { double offset = 0.0; @@ -85,14 +94,23 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda /// The minimum child index that is visible at the given scroll offset. /// - /// This function uses the returned value of [itemExtentBuilder] or the [itemExtent] - /// as an argument to avoid recomputing item size repeatedly during layout. + /// This function uses the returned value of [itemExtentBuilder] or the + /// [itemExtent] to avoid recomputing item size repeatedly during layout. /// /// By default, returns a value consistent with the children being placed in /// order, without gaps, starting from layout offset zero. + @visibleForTesting @protected - int getMinChildIndexForScrollOffset(double scrollOffset, double itemExtent) { + int getMinChildIndexForScrollOffset( + double scrollOffset, + @Deprecated( + 'The itemExtent is already available within the scope of this function. ' + 'This feature was deprecated after v3.20.0-7.0.pre.' + ) + double itemExtent, + ) { if (itemExtentBuilder == null) { + itemExtent = this.itemExtent!; if (itemExtent > 0.0) { final double actual = scrollOffset / itemExtent; final int round = actual.round(); @@ -109,14 +127,23 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda /// The maximum child index that is visible at the given scroll offset. /// - /// This function uses the returned value of [itemExtentBuilder] or the [itemExtent] - /// as an argument to avoid recomputing item size repeatedly during layout. + /// This function uses the returned value of [itemExtentBuilder] or the + /// [itemExtent] to avoid recomputing item size repeatedly during layout. /// /// By default, returns a value consistent with the children being placed in /// order, without gaps, starting from layout offset zero. + @visibleForTesting @protected - int getMaxChildIndexForScrollOffset(double scrollOffset, double itemExtent) { + int getMaxChildIndexForScrollOffset( + double scrollOffset, + @Deprecated( + 'The itemExtent is already available within the scope of this function. ' + 'This feature was deprecated after v3.20.0-7.0.pre.' + ) + double itemExtent, + ) { if (itemExtentBuilder == null) { + itemExtent = this.itemExtent!; if (itemExtent > 0.0) { final double actual = scrollOffset / itemExtent - 1; final int round = actual.round(); @@ -182,9 +209,18 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda /// /// * [estimateMaxScrollOffset], which is similar but may provide inaccurate /// values. + @visibleForTesting @protected - double computeMaxScrollOffset(SliverConstraints constraints, double itemExtent) { + double computeMaxScrollOffset( + SliverConstraints constraints, + @Deprecated( + 'The itemExtent is already available within the scope of this function. ' + 'This feature was deprecated after v3.20.0-7.0.pre.' + ) + double itemExtent, + ) { if (itemExtentBuilder == null) { + itemExtent = this.itemExtent!; return childManager.childCount * itemExtent; } else { double offset = 0.0; @@ -267,7 +303,6 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda childManager.didStartLayout(); childManager.setDidUnderflow(false); - final double itemFixedExtent = itemExtent ?? 0; final double scrollOffset = constraints.scrollOffset + constraints.cacheOrigin; assert(scrollOffset >= 0.0); final double remainingExtent = constraints.remainingCacheExtent; @@ -280,10 +315,12 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda viewportMainAxisExtent: constraints.viewportMainAxisExtent, crossAxisExtent: constraints.crossAxisExtent ); + // TODO(Piinks): Clean up when deprecation expires. + const double deprecatedExtraItemExtent = -1; - final int firstIndex = getMinChildIndexForScrollOffset(scrollOffset, itemFixedExtent); + final int firstIndex = getMinChildIndexForScrollOffset(scrollOffset, deprecatedExtraItemExtent); final int? targetLastIndex = targetEndScrollOffset.isFinite ? - getMaxChildIndexForScrollOffset(targetEndScrollOffset, itemFixedExtent) : null; + getMaxChildIndexForScrollOffset(targetEndScrollOffset, deprecatedExtraItemExtent) : null; if (firstChild != null) { final int leadingGarbage = _calculateLeadingGarbage(firstIndex); @@ -294,13 +331,14 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda } if (firstChild == null) { - if (!addInitialChild(index: firstIndex, layoutOffset: indexToLayoutOffset(itemFixedExtent, firstIndex))) { + final double layoutOffset = indexToLayoutOffset(deprecatedExtraItemExtent, firstIndex); + if (!addInitialChild(index: firstIndex, layoutOffset: layoutOffset)) { // There are either no children, or we are past the end of all our children. final double max; if (firstIndex <= 0) { max = 0.0; } else { - max = computeMaxScrollOffset(constraints, itemFixedExtent); + max = computeMaxScrollOffset(constraints, deprecatedExtraItemExtent); } geometry = SliverGeometry( scrollExtent: max, @@ -319,11 +357,11 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda // Items before the previously first child are no longer present. // Reset the scroll offset to offset all items prior and up to the // missing item. Let parent re-layout everything. - geometry = SliverGeometry(scrollOffsetCorrection: indexToLayoutOffset(itemFixedExtent, index)); + geometry = SliverGeometry(scrollOffsetCorrection: indexToLayoutOffset(deprecatedExtraItemExtent, index)); return; } final SliverMultiBoxAdaptorParentData childParentData = child.parentData! as SliverMultiBoxAdaptorParentData; - childParentData.layoutOffset = indexToLayoutOffset(itemFixedExtent, index); + childParentData.layoutOffset = indexToLayoutOffset(deprecatedExtraItemExtent, index); assert(childParentData.index == index); trailingChildWithLayout ??= child; } @@ -331,7 +369,7 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda if (trailingChildWithLayout == null) { firstChild!.layout(_getChildConstraints(indexOf(firstChild!))); final SliverMultiBoxAdaptorParentData childParentData = firstChild!.parentData! as SliverMultiBoxAdaptorParentData; - childParentData.layoutOffset = indexToLayoutOffset(itemFixedExtent, firstIndex); + childParentData.layoutOffset = indexToLayoutOffset(deprecatedExtraItemExtent, firstIndex); trailingChildWithLayout = firstChild; } @@ -342,7 +380,7 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda child = insertAndLayoutChild(_getChildConstraints(index), after: trailingChildWithLayout); if (child == null) { // We have run out of children. - estimatedMaxScrollOffset = indexToLayoutOffset(itemFixedExtent, index); + estimatedMaxScrollOffset = indexToLayoutOffset(deprecatedExtraItemExtent, index); break; } } else { @@ -351,12 +389,12 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda trailingChildWithLayout = child; final SliverMultiBoxAdaptorParentData childParentData = child.parentData! as SliverMultiBoxAdaptorParentData; assert(childParentData.index == index); - childParentData.layoutOffset = indexToLayoutOffset(itemFixedExtent, childParentData.index!); + childParentData.layoutOffset = indexToLayoutOffset(deprecatedExtraItemExtent, childParentData.index!); } final int lastIndex = indexOf(lastChild!); - final double leadingScrollOffset = indexToLayoutOffset(itemFixedExtent, firstIndex); - final double trailingScrollOffset = indexToLayoutOffset(itemFixedExtent, lastIndex + 1); + final double leadingScrollOffset = indexToLayoutOffset(deprecatedExtraItemExtent, firstIndex); + final double trailingScrollOffset = indexToLayoutOffset(deprecatedExtraItemExtent, lastIndex + 1); assert(firstIndex == 0 || childScrollOffset(firstChild!)! - scrollOffset <= precisionErrorTolerance); assert(debugAssertChildListIsNonEmptyAndContiguous()); @@ -388,7 +426,7 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda final double targetEndScrollOffsetForPaint = constraints.scrollOffset + constraints.remainingPaintExtent; final int? targetLastIndexForPaint = targetEndScrollOffsetForPaint.isFinite ? - getMaxChildIndexForScrollOffset(targetEndScrollOffsetForPaint, itemFixedExtent) : null; + getMaxChildIndexForScrollOffset(targetEndScrollOffsetForPaint, deprecatedExtraItemExtent) : null; geometry = SliverGeometry( scrollExtent: estimatedMaxScrollOffset, diff --git a/packages/flutter/test/rendering/sliver_fixed_extent_layout_test.dart b/packages/flutter/test/rendering/sliver_fixed_extent_layout_test.dart index 3e7f9fa65ec..e98bd7378e3 100644 --- a/packages/flutter/test/rendering/sliver_fixed_extent_layout_test.dart +++ b/packages/flutter/test/rendering/sliver_fixed_extent_layout_test.dart @@ -148,10 +148,208 @@ void main() { expect(parent.paintsChild(children[1]), false); expect(parent.paintsChild(children[2]), true); }); + + test('RenderSliverFillViewport correctly references itemExtent, non-zero offset', () { + final List children = [ + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + ]; + final TestRenderSliverBoxChildManager childManager = TestRenderSliverBoxChildManager( + children: children, + ); + final RenderSliverFillViewport sliver = childManager.createRenderSliverFillViewport(); + final RenderViewport root = RenderViewport( + crossAxisDirection: AxisDirection.right, + offset: ViewportOffset.fixed(1200.0), + cacheExtent: 100, + children: [ sliver ], + ); + layout(root); + // These are a bogus itemExtents, and motivate the deprecation. The sliver + // knows its itemExtent, and should use its configured extent rather than + // whatever is provided through these methods. + // Also, the API is a bit redundant, so we clean! + // In this case, the true item extent is 600 to fill the viewport. + expect( + sliver.constraints.scrollOffset, + 1200.0 + ); + expect(sliver.itemExtent, 600.0); + final double layoutOffset = sliver.indexToLayoutOffset( + 150.0, // itemExtent + 10, + ); + expect(layoutOffset, 6000.0); + final int minIndex = sliver.getMinChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150.0, // itemExtent + ); + expect(minIndex, 2); + final int maxIndex = sliver.getMaxChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150, // itemExtent + ); + expect(maxIndex, 1); + final double maxScrollOffset = sliver.computeMaxScrollOffset( + sliver.constraints, + 150, // itemExtent + ); + expect(maxScrollOffset, 1800.0); + }); + + test('RenderSliverFillViewport correctly references itemExtent, zero offset', () { + final List children = [ + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + ]; + final TestRenderSliverBoxChildManager childManager = TestRenderSliverBoxChildManager( + children: children, + ); + final RenderSliverFillViewport sliver = childManager.createRenderSliverFillViewport(); + final RenderViewport root = RenderViewport( + crossAxisDirection: AxisDirection.right, + offset: ViewportOffset.zero(), + cacheExtent: 100, + children: [ sliver ], + ); + layout(root); + // These are a bogus itemExtents, and motivate the deprecation. The sliver + // knows its itemExtent, and should use its configured extent rather than + // whatever is provided through these methods. + // Also, the API is a bit redundant, so we clean! + // In this case, the true item extent is 600 to fill the viewport. + expect( + sliver.constraints.scrollOffset, + 0.0 + ); + expect(sliver.itemExtent, 600.0); + final double layoutOffset = sliver.indexToLayoutOffset( + 150.0, // itemExtent + 10, + ); + expect(layoutOffset, 6000.0); + final int minIndex = sliver.getMinChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150.0, // itemExtent + ); + expect(minIndex, 0); + final int maxIndex = sliver.getMaxChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150, // itemExtent + ); + expect(maxIndex, 0); + final double maxScrollOffset = sliver.computeMaxScrollOffset( + sliver.constraints, + 150, // itemExtent + ); + expect(maxScrollOffset, 1800.0); + }); + + test('RenderSliverFixedExtentList correctly references itemExtent, non-zero offset', () { + final List children = [ + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + ]; + final TestRenderSliverBoxChildManager childManager = TestRenderSliverBoxChildManager( + children: children, + ); + final RenderSliverFixedExtentList sliver = childManager.createRenderSliverFixedExtentList(30.0); + final RenderViewport root = RenderViewport( + crossAxisDirection: AxisDirection.right, + offset: ViewportOffset.fixed(45.0), + cacheExtent: 100, + children: [ sliver ], + ); + layout(root); + // These are a bogus itemExtents, and motivate the deprecation. The sliver + // knows its itemExtent, and should use its configured extent rather than + // whatever is provided through these methods. + // Also, the API is a bit redundant, so we clean! + // In this case, the true item extent is 30.0. + expect( + sliver.constraints.scrollOffset, + 45.0 + ); + expect(sliver.constraints.viewportMainAxisExtent, 600.0); + expect(sliver.itemExtent, 30.0); + final double layoutOffset = sliver.indexToLayoutOffset( + 150.0, // itemExtent + 10, + ); + expect(layoutOffset, 300.0); + final int minIndex = sliver.getMinChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150.0, // itemExtent + ); + expect(minIndex, 1); + final int maxIndex = sliver.getMaxChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150, // itemExtent + ); + expect(maxIndex, 1); + final double maxScrollOffset = sliver.computeMaxScrollOffset( + sliver.constraints, + 150, // itemExtent + ); + expect(maxScrollOffset, 90.0); + }); + + test('RenderSliverFixedExtentList correctly references itemExtent, zero offset', () { + final List children = [ + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + RenderSizedBox(const Size(400.0, 100.0)), + ]; + final TestRenderSliverBoxChildManager childManager = TestRenderSliverBoxChildManager( + children: children, + ); + final RenderSliverFixedExtentList sliver = childManager.createRenderSliverFixedExtentList(30.0); + final RenderViewport root = RenderViewport( + crossAxisDirection: AxisDirection.right, + offset: ViewportOffset.zero(), + cacheExtent: 100, + children: [ sliver ], + ); + layout(root); + // These are a bogus itemExtents, and motivate the deprecation. The sliver + // knows its itemExtent, and should use its configured extent rather than + // whatever is provided through these methods. + // Also, the API is a bit redundant, so we clean! + // In this case, the true item extent is 30.0. + expect( + sliver.constraints.scrollOffset, + 0.0 + ); + expect(sliver.constraints.viewportMainAxisExtent, 600.0); + expect(sliver.itemExtent, 30.0); + final double layoutOffset = sliver.indexToLayoutOffset( + 150.0, // itemExtent + 10, + ); + expect(layoutOffset, 300.0); + final int minIndex = sliver.getMinChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150.0, // itemExtent + ); + expect(minIndex, 0); + final int maxIndex = sliver.getMaxChildIndexForScrollOffset( + sliver.constraints.scrollOffset, + 150, // itemExtent + ); + expect(maxIndex, 0); + final double maxScrollOffset = sliver.computeMaxScrollOffset( + sliver.constraints, + 150, // itemExtent + ); + expect(maxScrollOffset, 90.0); + }); } int testGetMaxChildIndexForScrollOffset(double scrollOffset, double itemExtent) { - final TestRenderSliverFixedExtentBoxAdaptor renderSliver = TestRenderSliverFixedExtentBoxAdaptor(); + final TestRenderSliverFixedExtentBoxAdaptor renderSliver = TestRenderSliverFixedExtentBoxAdaptor(itemExtent: itemExtent); return renderSliver.getMaxChildIndexForScrollOffset(scrollOffset, itemExtent); } @@ -171,6 +369,15 @@ class TestRenderSliverBoxChildManager extends RenderSliverBoxChildManager { return _renderObject! as RenderSliverFillViewport; } + RenderSliverFixedExtentList createRenderSliverFixedExtentList(double itemExtent) { + assert(_renderObject == null); + _renderObject = RenderSliverFixedExtentList( + childManager: this, + itemExtent: itemExtent, + ); + return _renderObject! as RenderSliverFixedExtentList; + } + int? _currentlyUpdatingChildIndex; @override @@ -218,8 +425,12 @@ class TestRenderSliverBoxChildManager extends RenderSliverBoxChildManager { } class TestRenderSliverFixedExtentBoxAdaptor extends RenderSliverFixedExtentBoxAdaptor { - TestRenderSliverFixedExtentBoxAdaptor() - :super(childManager: TestRenderSliverBoxChildManager(children: [])); + TestRenderSliverFixedExtentBoxAdaptor({ + required double itemExtent + }) : _itemExtent = itemExtent, + super(childManager: TestRenderSliverBoxChildManager(children: [])); + + final double _itemExtent; @override int getMaxChildIndexForScrollOffset(double scrollOffset, double itemExtent) { @@ -227,5 +438,5 @@ class TestRenderSliverFixedExtentBoxAdaptor extends RenderSliverFixedExtentBoxAd } @override - double get itemExtent => throw UnimplementedError(); + double get itemExtent => _itemExtent; }