From 03a3457f10a18f8a024b8adc2bbd0deac0fe2443 Mon Sep 17 00:00:00 2001 From: David Garcia Date: Mon, 2 Nov 2020 22:23:04 +0100 Subject: [PATCH] fix(itemExtent): Fix rounded issue using precisionErrorTolerance (#68199) --- .../rendering/sliver_fixed_extent_list.dart | 14 +++- .../sliver_fixed_extent_layout_test.dart | 82 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) 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 2073226100..9a75f5a3fc 100644 --- a/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart +++ b/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart @@ -85,7 +85,15 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda /// order, without gaps, starting from layout offset zero. @protected int getMaxChildIndexForScrollOffset(double scrollOffset, double itemExtent) { - return itemExtent > 0.0 ? math.max(0, (scrollOffset / itemExtent).ceil() - 1) : 0; + if (itemExtent > 0.0) { + final double actual = scrollOffset / itemExtent - 1; + final int round = actual.round(); + if (_isWithinPrecisionErrorTolerance(actual, round)) { + return math.max(0, round); + } + return math.max(0, actual.ceil()); + } + return 0; } /// Called to estimate the total scrollable extents of this object. @@ -350,3 +358,7 @@ class RenderSliverFixedExtentList extends RenderSliverFixedExtentBoxAdaptor { markNeedsLayout(); } } + +bool _isWithinPrecisionErrorTolerance(double actual, int round) { + return (actual - round).abs() < precisionErrorTolerance; +} 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 8c4fd336dd..67a4c8ea61 100644 --- a/packages/flutter/test/rendering/sliver_fixed_extent_layout_test.dart +++ b/packages/flutter/test/rendering/sliver_fixed_extent_layout_test.dart @@ -41,6 +41,74 @@ void main() { expect(children[1].attached, false); expect(children[2].attached, true); }); + + group('getMaxChildIndexForScrollOffset', () { + // Regression test for https://github.com/flutter/flutter/issues/68182 + + const double genericItemExtent = 600.0; + const double extraValueToNotHaveRoundingIssues = 0.0000001; // 6 zeros + const double extraValueToHaveRoundingIssues = 0.00000001; // 7 zeros + + test('should be 0 when item extent is 0', () { + const double offsetValueWhichDoesntCare = 1234; + final int actual = testGetMaxChildIndexForScrollOffset(offsetValueWhichDoesntCare, 0); + expect(actual, 0); + }); + + test('should be 0 when offset is 0', () { + final int actual = testGetMaxChildIndexForScrollOffset(0, genericItemExtent); + expect(actual, 0); + }); + + test('should be 0 when offset is equal to item extent', () { + final int actual = testGetMaxChildIndexForScrollOffset(genericItemExtent, genericItemExtent); + expect(actual, 0); + }); + + test('should be 1 when offset is greater than item extent', () { + final int actual = testGetMaxChildIndexForScrollOffset( + genericItemExtent + 1, genericItemExtent); + expect(actual, 1); + }); + + test('should be 1 when offset is slightly greater than item extent', () { + final int actual = testGetMaxChildIndexForScrollOffset( + genericItemExtent + extraValueToNotHaveRoundingIssues, genericItemExtent); + expect(actual, 1); + }); + + test('should be 4 when offset is four times and a half greater than item extent', () { + final int actual = testGetMaxChildIndexForScrollOffset( + genericItemExtent * 4.5, genericItemExtent); + expect(actual, 4); + }); + + test('should be 5 when offset is 6 times greater than item extent', () { + const double anotherGenericItemExtent = 414.0; + final int actual = testGetMaxChildIndexForScrollOffset( + anotherGenericItemExtent * 6, anotherGenericItemExtent); + expect(actual, 5); + }); + + test('should be 5 when offset is 6 times greater than a specific item extent where the division will return more than 13 zero decimals', () { + const double itemExtentSpecificForAProblematicSreenSize = 411.42857142857144; + final int actual = testGetMaxChildIndexForScrollOffset( + itemExtentSpecificForAProblematicSreenSize * 6 + extraValueToHaveRoundingIssues, + itemExtentSpecificForAProblematicSreenSize); + expect(actual, 5); + }); + + test('should be 0 when offset is 0.00000001 times greater than item extent where the divison will return more than 13 zero decimals', () { + final int actual = testGetMaxChildIndexForScrollOffset( + genericItemExtent + extraValueToHaveRoundingIssues, genericItemExtent); + expect(actual, 0); + }); + }); +} + +int testGetMaxChildIndexForScrollOffset(double scrollOffset, double itemExtent) { + final TestRenderSliverFixedExtentBoxAdaptor renderSliver = TestRenderSliverFixedExtentBoxAdaptor(); + return renderSliver.getMaxChildIndexForScrollOffset(scrollOffset, itemExtent); } class TestRenderSliverBoxChildManager extends RenderSliverBoxChildManager { @@ -103,3 +171,17 @@ class TestRenderSliverBoxChildManager extends RenderSliverBoxChildManager { @override void setDidUnderflow(bool value) { } } + +class TestRenderSliverFixedExtentBoxAdaptor extends RenderSliverFixedExtentBoxAdaptor { + TestRenderSliverFixedExtentBoxAdaptor() + :super(childManager: TestRenderSliverBoxChildManager(children: [])); + + @override + // ignore: unnecessary_overrides + int getMaxChildIndexForScrollOffset(double scrollOffset, double itemExtent) { + return super.getMaxChildIndexForScrollOffset(scrollOffset, itemExtent); + } + + @override + double get itemExtent => throw UnimplementedError(); +}