Fix precision error in NestedScrollView by nt4f04uNd · Pull Request #87801 · flutter/flutter

nt4f04uNd · 2021-08-06T12:07:20Z

flutter/packages/flutter/lib/src/widgets/nested_scroll_view.dart

Lines 1719 to 1720 in 81fac68

    
           final bool result = super.applyMoveTo(value + metrics.correctionOffset); 
        
           assert(result); // since we tried to pass an in-range value, it shouldn't ever overflow

The root of the error was here, because metrics.correctionOffset sometimes came a little bit bigger than it needed to be, because of the precision error caused by calculations here

flutter/packages/flutter/lib/src/widgets/nested_scroll_view.dart

Lines 979 to 984 in 81fac68

    
           extra = _outerPosition!.maxScrollExtent - _outerPosition!.pixels; 
        
           assert(extra >= 0.0); 
        
           minRange = pixels; 
        
           maxRange = pixels + extra; 
        
           assert(minRange <= maxRange); 
        
           correctionOffset = _outerPosition!.pixels - pixels;

So because of that the applyMoveTo returned false, as it was overflowing by a small number

Also a small refactorings in lsq_solver.dart and sliver_fixed_extent_list.dart:

changed <= to < in few places where precisionErrorTolerance was used, because that's the way it is in the rest of the codebase
removed the function used only in one place

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

packages/flutter/lib/src/widgets/nested_scroll_view.dart

Piinks · 2021-08-17T19:51:39Z

Can you add more info to your PR description that explains why it fixes the linked bug?

nt4f04uNd · 2021-08-17T19:59:17Z

packages/flutter/lib/src/widgets/scroll_activity.dart

  @protected
  bool applyMoveTo(double value) {
-    return delegate.setPixels(value) == 0.0;
+    return delegate.setPixels(value).abs() < precisionErrorTolerance;


Can you add more info to your PR description that explains why it fixes the linked bug?

Without this change the test I added fails with the error from the linked issue

So I've checked out this change and I see this, but now I wonder, why? And What are all of the other changes for in the other files? Those do not appear to be covered by the test here.

It would be really helpful as a code reviewer if the PR description explained what the changes are for and why. A comment inline here will be [hard to find/disappear when resolved] and unhelpful for people who may come across this change later.

nt4f04uNd · 2021-08-17T21:33:44Z

Done

packages/flutter/lib/src/gestures/lsq_solver.dart

packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart

nt4f04uNd · 2021-08-20T18:23:46Z

Those are refactorings without any semantic change, I believe they don't need tests. Do I need to submit them as a different PR and get a test-exemption from Hixie?

Piinks · 2021-08-20T18:26:12Z

Those are refactorings without any semantic change, I believe they don't need tests. Do I need to submit them as a different PR and get a test-exemption from Hixie?

Changing <= to < is semantic? 🙃

nt4f04uNd · 2021-08-20T18:30:43Z

In case with such a small number I believe it's not. I'm removing these changes from the current PR

Piinks

Thank you for the contribution!

nt4f04uNd · 2021-08-25T22:57:10Z

Thank you for the review !

* Migrate to 2.5 * Merge code from flutter/flutter#87801 * Fix exception that findRenderObject throw exception even

## Description This PR fixes a precision error in ~~`ClampingScrollPhysics`~~ `ScrollPosition` that leads to `StretchingOverscrollIndicator` stretching its content unexpectedly in some devices (see #126561 where this is visible in `TabBarView` and the test added in this PR where it reproduces with a `PageView`). ~~This PR also contains a change to `nested_scroll_view.dart` because the first change (the one in `ClampingScrollPhysics`) breaks the precision error test added by #87801 ## Related Issue Fixes #126561 ## Tests Adds 1 test.

nt4f04uNd added 2 commits August 6, 2021 15:04

init

61f14e1

changed test name

2229ff2

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 6, 2021

google-cla bot added the cla: yes label Aug 6, 2021

nt4f04uNd commented Aug 6, 2021

View reviewed changes

packages/flutter/lib/src/widgets/nested_scroll_view.dart Outdated Show resolved Hide resolved

goderbauer requested a review from Piinks August 11, 2021 21:45

Piinks added the f: scrolling Viewports, list views, slivers, etc. label Aug 17, 2021

nt4f04uNd commented Aug 17, 2021

View reviewed changes

nt4f04uNd added 2 commits August 17, 2021 23:04

review feedback

81fac68

changed test name

4c754f3

Piinks reviewed Aug 20, 2021

View reviewed changes

packages/flutter/lib/src/gestures/lsq_solver.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart Outdated Show resolved Hide resolved

+

def569a

Piinks approved these changes Aug 25, 2021

View reviewed changes

Piinks added the waiting for tree to go green label Aug 25, 2021

fluttergithubbot merged commit 59f3f23 into flutter:master Aug 25, 2021

nt4f04uNd deleted the ieee2 branch August 25, 2021 22:57

zmtzawqlp added a commit to fluttercandies/extended_nested_scroll_view that referenced this pull request Sep 9, 2021

## 5.0.0

753c5a1

* Migrate to 2.5 * Merge code from flutter/flutter#87801 * Fix exception that findRenderObject throw exception even

bleroux mentioned this pull request May 22, 2023

Fix ScrollPosition overscroll precision error #127321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix precision error in NestedScrollView#87801

Fix precision error in NestedScrollView#87801
fluttergithubbot merged 5 commits intoflutter:masterfrom
nt4f04uNd:ieee2

nt4f04uNd commented Aug 6, 2021 •

edited

Loading

Uh oh!

Uh oh!

Piinks commented Aug 17, 2021

Uh oh!

nt4f04uNd Aug 17, 2021 •

edited

Loading

Uh oh!

Piinks Aug 17, 2021

Uh oh!

nt4f04uNd commented Aug 17, 2021

Uh oh!

Uh oh!

Uh oh!

nt4f04uNd commented Aug 20, 2021

Uh oh!

Piinks commented Aug 20, 2021

Uh oh!

nt4f04uNd commented Aug 20, 2021 •

edited

Loading

Uh oh!

Piinks left a comment

Uh oh!

nt4f04uNd commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	final bool result = super.applyMoveTo(value + metrics.correctionOffset);
	assert(result); // since we tried to pass an in-range value, it shouldn't ever overflow

	extra = _outerPosition!.maxScrollExtent - _outerPosition!.pixels;
	assert(extra >= 0.0);
	minRange = pixels;
	maxRange = pixels + extra;
	assert(minRange <= maxRange);
	correctionOffset = _outerPosition!.pixels - pixels;

Conversation

nt4f04uNd commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Piinks commented Aug 17, 2021

Uh oh!

nt4f04uNd Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

nt4f04uNd commented Aug 17, 2021

Uh oh!

Uh oh!

Uh oh!

nt4f04uNd commented Aug 20, 2021

Uh oh!

Piinks commented Aug 20, 2021

Uh oh!

nt4f04uNd commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

nt4f04uNd commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nt4f04uNd commented Aug 6, 2021 •

edited

Loading

nt4f04uNd Aug 17, 2021 •

edited

Loading

nt4f04uNd commented Aug 20, 2021 •

edited

Loading