Skip to content

Fix precision error in NestedScrollView #87801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fluttergithubbot merged 5 commits into from
Aug 25, 2021
Merged

Fix precision error in NestedScrollView #87801

fluttergithubbot merged 5 commits into from
Aug 25, 2021

Conversation

@nt4f04uNd
Copy link
Member

@nt4f04uNd nt4f04uNd commented Aug 6, 2021 •
edited
Loading

Fixes #63825

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.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 6, 2021
@google-cla google-cla bot added the cla: yes label Aug 6, 2021
nt4f04uNd
nt4f04uNd commented Aug 6, 2021
View reviewed changes
@goderbauer goderbauer requested a review from Piinks August 11, 2021 21:45
@Piinks Piinks added the f: scrolling Viewports, list views, slivers, etc. label Aug 17, 2021
@Piinks
Copy link
Contributor

Piinks commented Aug 17, 2021

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

nt4f04uNd
nt4f04uNd commented Aug 17, 2021
View reviewed changes
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;
Copy link
Member Author

@nt4f04uNd nt4f04uNd Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@Piinks Piinks Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

nt4f04uNd commented Aug 17, 2021

Done

Piinks
Piinks reviewed Aug 20, 2021
View reviewed changes
@nt4f04uNd
Copy link
Member Author

nt4f04uNd commented Aug 20, 2021

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
Copy link
Contributor

Piinks commented Aug 20, 2021

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
Copy link
Member Author

nt4f04uNd commented Aug 20, 2021 •
edited
Loading

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

Piinks
Piinks approved these changes Aug 25, 2021
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter_LGTM
Thank you for the contribution!

@fluttergithubbot fluttergithubbot merged commit 59f3f23 into flutter:master Aug 25, 2021
@nt4f04uNd
Copy link
Member Author

nt4f04uNd commented Aug 25, 2021

Thank you for the review !

@nt4f04uNd 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
@zmtzawqlp
## 5.0.0
753c5a1 
* Migrate to 2.5
* Merge code from flutter/flutter#87801
* Fix exception that findRenderObject throw exception even
@bleroux bleroux mentioned this pull request May 22, 2023
Merged
auto-submit bot pushed a commit that referenced this pull request May 24, 2023
@bleroux
Fix ScrollPosition overscroll precision error (#127321)
2b105ac 
## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Another exception was thrown: 'package:flutter/src/widgets/nested_scroll_view.dart': Failed assertion: line 1608 pos 12: 'result': is not true.

3 participants

@nt4f04uNd @Piinks @fluttergithubbot