Skip to content

F
Front-End
Unverified Commit 09e0e1b8 authored by Kate Lovett's avatar Kate Lovett Committed by GitHub
Browse files
Options

Deprecate redundant itemExtent in RenderSliverFixedExtentBoxAdaptor methods (#143412) 

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.
parent b09a015f
Hide whitespace changes
Inline Side-by-side
Showing with 276 additions and 27 deletions
packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart
View file @ 09e0e1b8
......@@ -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,
......
packages/flutter/test/rendering/sliver_fixed_extent_layout_test.dart
View file @ 09e0e1b8
......@@ -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<RenderBox> children = <RenderBox>[
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: <RenderSliver>[ 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<RenderBox> children = <RenderBox>[
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: <RenderSliver>[ 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<RenderBox> children = <RenderBox>[
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: <RenderSliver>[ 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<RenderBox> children = <RenderBox>[
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: <RenderSliver>[ 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: <RenderBox>[]));
TestRenderSliverFixedExtentBoxAdaptor({
required double itemExtent
}) : _itemExtent = itemExtent,
super(childManager: TestRenderSliverBoxChildManager(children: <RenderBox>[]));
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;
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment

HIAST - GitLab