PageView scroll physics to match Android (as an opt-in) by nt4f04uNd · Pull Request #116457 · flutter/flutter

nt4f04uNd · 2022-12-03T09:38:51Z

Fixes #38357

So, this is a new reland for a #95423
Previous reland #97166

Old physics

before.mp4

ios_old.mov

New physics

after.mp4

ios_new.mov

Opt-in

This change has been proven to be breaking for the customer tests.
To opt in, users should provide pageSnappingPhysics: const PageScrollPhysics(useNewPhysics: true)

If tests are broken, this is likely because of the change in minFlingVelocity or minFlingDistance.
Previous values are respectively 50.0 and 18.0.
New values are 400.0 and 25.0.

To migrate the tests, one should change the drag velocity/distance to match the new minimum values.

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 this 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.

nt4f04uNd · 2022-12-03T09:41:05Z

Adding pageSnappingPhysics also allows for customization of the simulation (and in particular, to use the old simulation).
This might be a sufficient fix for #55103

nt4f04uNd · 2022-12-03T09:45:13Z

Is #97166 (comment) really blocking us from landing this PR? Seems pretty harmless

nt4f04uNd · 2022-12-03T09:47:34Z

It isn't a severe API break, it only breaks some tests, so should I really go through all the process outlined in the breaking change policy? https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

This seems a bit excessive, given that there's nothing to actually migrate here, after an opt-in is removed.

…articular

Piinks · 2022-12-05T23:19:52Z

after an opt-in is removed.

How will folks know to migrate without guidance? :)

There is a lot here I'd like to look at closely, I don't recall the specific failures that caused it to be reverted before. I'll be able to take a close look at this tomorrow. Nice to see you back!

Piinks

I refreshed my memory on this. At the time, the customers affected looked like simple fixes, but reading through this I am not really sure how to migrate them. Can you advise?

Piinks · 2022-12-07T23:47:28Z

+
+    // TODO(nt4f04uNd): remove this test, because it is artificial, in favor of a real one.
+    // Scroll simulations surely should be allowed to modify the speed they receive in whatever way they want.
+    // The proper fix for the original bug https://github.com/flutter/flutter/issues/24715


This issue is closed, is it not actually fixed based on this comment?

Piinks · 2022-12-07T23:49:30Z

-    // Changing velocity for time 0 may cause a sudden, unwanted damping/speedup effect
-    expect(bounce.createBallisticSimulation(position, 1000)!.dx(0), moreOrLessEquals(1000));
-    expect(clamp.createBallisticSimulation(position, 1000)!.dx(0), moreOrLessEquals(1000));
-    expect(page.createBallisticSimulation(position, 1000)!.dx(0), moreOrLessEquals(1000));


I still do not understand these changes, why are bouncing and clamping scroll physics affected by this change?
Why can we not validate the page physics here anymore? What happens in this case with this change in page physics? What will the experience be for a user if this ballistic simulation is created?

Piinks · 2022-12-07T23:50:01Z

+    // velocity depends on a ratio of velocity / pageDelta, and because they are not affected by
+    // https://github.com/flutter/flutter/issues/11599
+
+    // TODO(nt4f04uNd): remove this test, because it is artificial, in favor of a real one.


I don't follow this TODO, what is the action item or issue that needs to be revisited here?

Piinks · 2022-12-07T23:50:40Z

    this.physics,
+    this.pageSnappingPhysics,


What happens if someone provides physics?

Piinks · 2022-12-07T23:51:21Z


+  /// Controls how pages should snap to view boundaries.
+  ///
+  /// Prior being used, ombined with other scroll physics


Isn't this something the user could provide to physics?

Piinks · 2022-12-07T23:52:48Z

+  ///
+  /// Enabling this will use scroll physics which are more close to native
+  /// Android and iOS implementations.
+  final bool useNewPhysics;


How should customers migrate? Do they need to provide a custom scroll physics to every Tab or Page view? How do we migrate them off of it?

Piinks · 2022-12-07T23:53:09Z

 ///  * [ScrollPhysics], the base class which defines the API for scrolling
 ///    physics.
 ///  * [PageView.physics], which can override the physics used by a page view.
+///  * [PageScrollSimulation], which implements Android page view scroll physics, and


Flutter does not just run on Android. Does this adapt to the platform?

Piinks · 2022-12-07T23:55:38Z

+      return super.createBallisticSimulation(position, velocity);
+    }
+    final Tolerance tolerance = this.tolerance;
+    final double target = getTargetPixels(position, tolerance, velocity);


I can see this is why this method has been made public, but I am not sure what the motivation is since it is just in a test.

nt4f04uNd · 2023-02-23T17:09:32Z

Thank you for your review, this is a very valueable feedback!
I'm sorry this is taking a long time for me to respond, but I do remember about this PR and will update it as soon as I can.

Piinks · 2023-04-11T21:37:37Z

I'm sorry this is taking a long time for me to respond, but I do remember about this PR and will update it as soon as I can.

No worries at all @nt4f04uNd. I am going to close this for now so it is off our review queue. When you are ready to work on it again, please feel free to reopen it. I'll be here ready to review it. :)

Liloupar · 2024-11-25T09:47:53Z

@nt4f04uNd
Do you still have time to take another look at this PR?

PageView scroll physics to match Android

0a90870

nt4f04uNd added framework flutter/packages/flutter repository. See also f: labels. a: fidelity Matching the OEM platforms better f: scrolling Viewports, list views, slivers, etc. labels Dec 3, 2022

nt4f04uNd requested a review from Piinks December 3, 2022 09:38

flutter-dashboard Bot added the p: material_ui material_ui package in flutter/packages label Dec 3, 2022

nt4f04uNd mentioned this pull request Dec 3, 2022

Tune PageView physics for iOS fidelity #97064

Open

nt4f04uNd force-pushed the page-view-reland branch 2 times, most recently from ac85d98 to b292d0c Compare December 3, 2022 12:04

Add ability to use custom scroll physics and old scroll phyisics in p…

fdc6bc0

…articular

nt4f04uNd force-pushed the page-view-reland branch from b292d0c to fdc6bc0 Compare December 3, 2022 12:13

Piinks reviewed Dec 7, 2022

View reviewed changes

Piinks closed this Apr 11, 2023

Conversation

nt4f04uNd commented Dec 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Opt-in

Pre-launch Checklist

Uh oh!

nt4f04uNd commented Dec 3, 2022

Uh oh!

nt4f04uNd commented Dec 3, 2022

Uh oh!

nt4f04uNd commented Dec 3, 2022

Uh oh!

Piinks commented Dec 5, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2022 • 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 Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

nt4f04uNd commented Feb 23, 2023

Uh oh!

Piinks commented Apr 11, 2023

Uh oh!

Liloupar commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nt4f04uNd commented Dec 3, 2022 •

edited

Loading

Piinks Dec 7, 2022 •

edited

Loading