Skip to content

PageView scroll physics to match Android #95423

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 6 commits into from
Jan 22, 2022
Merged

PageView scroll physics to match Android #95423

fluttergithubbot merged 6 commits into from
Jan 22, 2022

Conversation

@nt4f04uNd
Copy link
Member

@nt4f04uNd nt4f04uNd commented Dec 16, 2021 •
edited
Loading

Fixes #38357

Compared with tabs_overlay app

The main problem with the current PageScrollPhysics is that they are ScrollSpringSimulation based, while Android implementation defines the exact duration the page view must move. This makes that both small and large fling simulations settle at the end, occupying unnecessary duration, during which user can't really interact with UI.

This PR copies the Android logic and makes it default.

Further discussion is in the issue #38357

Video before PR
before.mp4
Video after PR
after.mp4

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.

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Dec 16, 2021
@nt4f04uNd nt4f04uNd mentioned this pull request Dec 16, 2021
Open
@nt4f04uNd nt4f04uNd requested a review from Piinks December 16, 2021 20:25
@nt4f04uNd nt4f04uNd added the a: fidelity Matching the OEM platforms better label Dec 16, 2021
@nt4f04uNd nt4f04uNd marked this pull request as draft December 17, 2021 03:31
@nt4f04uNd nt4f04uNd marked this pull request as ready for review December 17, 2021 04:24
Hixie
Hixie reviewed Dec 22, 2021
View reviewed changes
packages/flutter/lib/src/widgets/page_view.dart Outdated
Copy link
Contributor

@Hixie Hixie Dec 22, 2021

Choose a reason for hiding this comment

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

(see discussion in the issue)

Piinks
Piinks reviewed Jan 7, 2022
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.

Hey @nt4f04uNd, happy new year! 🎉

You work on these various overlay apps are really great, thank you! I appreciate your care and attention to fidelity.

I see there are still some outstanding questions for @Hixie in the issue. Another thought though, if this is the default on Android, what about other platforms? Should this add a platform check and maintain the original behavior for platforms that are not Android?

@nt4f04uNd
Copy link
Member Author

nt4f04uNd commented Jan 7, 2022

Happy new year too!

Should this add a platform check and maintain the original behavior for platforms that are not Android?

I think not, because PageView is only a thing in Android world (if I'm not mistaken), and new behavior is better for usability, because it makes the animation more responsive to user's gestures.

@Hixie
Copy link
Contributor

Hixie commented Jan 8, 2022

PageView surely applies on any number of platforms. It's just a ListView that snaps by item.

@nt4f04uNd
Copy link
Member Author

nt4f04uNd commented Jan 13, 2022

PageView surely applies on any number of platforms. It's just a ListView that snaps by item.

Oh ok.

So I spent some time and built an iOS implementation for the tabs_overlay. I unfortunately don't have a real iOS device on my hands at the moment, but as far as I can tell from running it in debug mode - the old implementation is not accurate with native iOS either. It seems that their implementation is similar to Android's - the faster you drag, there's noticably less duration, and no settling at the end.

It seems that it would be fine to not add the platform check at the moment, and file a new issue for tuning it for iOS.

Video (with the spring implementation in Flutter)
video.mp4
Video (with the new implementation in Flutter)
video2.mp4

@nt4f04uNd nt4f04uNd requested a review from Hixie January 16, 2022 01:15
@Piinks
Copy link
Contributor

Piinks commented Jan 21, 2022

self-ping to put this at the top of my inbox for review tomorrow :)

Piinks
Piinks reviewed Jan 21, 2022
View reviewed changes
packages/flutter/test/widgets/page_view_test.dart Outdated
Copy link
Contributor

@Piinks Piinks Jan 21, 2022

Choose a reason for hiding this comment

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

Docs in tests! I love it!

Piinks
Piinks approved these changes Jan 21, 2022
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.

This LGTM, thank you for writing the overlay app for iOS too. It will definitely come in handy! Have you filed an issue for following up on that? You certainly do not need to fix it, but it would be good to have a tracking issue for it.

@nt4f04uNd nt4f04uNd mentioned this pull request Jan 22, 2022
Open
@nt4f04uNd
Copy link
Member Author

nt4f04uNd commented Jan 22, 2022

Filed #97064 for iOS

@fluttergithubbot fluttergithubbot merged commit daef082 into flutter:master Jan 22, 2022
@nt4f04uNd nt4f04uNd deleted the page-view branch January 23, 2022 16:50
Piinks added a commit that referenced this pull request Jan 24, 2022
@Piinks
Revert "PageView scroll physics to match Android (#95423)"
74fc61a 
This reverts commit daef082.
@Piinks Piinks mentioned this pull request Jan 24, 2022
Merged
@Piinks
Copy link
Contributor

Piinks commented Jan 24, 2022

Hey @nt4f04uNd I am going to revert this, there were some internal customers broken by this change. Can you re-open this as a new PR and we can discuss re-landing it there? Thanks!

@nt4f04uNd nt4f04uNd restored the page-view branch January 24, 2022 21:05
@nt4f04uNd nt4f04uNd mentioned this pull request Jan 24, 2022
Closed
8 tasks
Piinks added a commit that referenced this pull request Jan 24, 2022
@Piinks
Revert "PageView scroll physics to match Android (#95423)" (#97150)
6b6cea6 
This reverts commit daef082.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 4, 2022
@engine-flutter-autoroll
daef082 PageView scroll physics to match Android (flutter/flutter#95423)
6f5eef7
@Piinks Piinks mentioned this pull request Feb 15, 2022
Closed
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
@nt4f04uNd @clocksmith
PageView scroll physics to match Android (flutter#95423)
5dcd6a9
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
@Piinks @clocksmith
Revert "PageView scroll physics to match Android (flutter#95423)" (fl…
6e27b2d 
…utter#97150)

This reverts commit daef082.
@Piinks Piinks mentioned this pull request Jul 15, 2022
Merged
8 tasks
@nt4f04uNd nt4f04uNd mentioned this pull request Dec 3, 2022
Closed
8 tasks
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

@Hixie Hixie Awaiting requested review from Hixie

Assignees

No one assigned

Labels

a: fidelity Matching the OEM platforms better 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.

TabView swipe animates too slow compared to Android

4 participants

@nt4f04uNd @Hixie @Piinks @fluttergithubbot