Skip to content
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.

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

Add macOS-specific scroll physics #108298

Merged
merged 5 commits into from Sep 19, 2022
Merged

Conversation

moffatman
Copy link
Contributor

image

Dotted line = BouncingScrollPhysics
Solid line = new BouncingDesktopScrollPhysics
X-marks = Recorded data

Fixes #107752

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.

@flutter-dashboard flutter-dashboard bot added f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jul 25, 2022
@goderbauer goderbauer requested a review from Piinks July 26, 2022 22:13
@hellohuanlin
Copy link
Contributor

im having trouble understanding this graph. is y-axis the initial distance or initial speed?

@Piinks Piinks added platform-mac Building on or for macOS specifically a: desktop Running on desktop a: fidelity Matching the OEM platforms better labels Aug 1, 2022
@moffatman
Copy link
Contributor Author

im having trouble understanding this graph. is y-axis the initial distance or initial speed?

The y-axis is velocity

@Piinks
Copy link
Contributor

Piinks commented Aug 9, 2022

Thanks so much for this @moffatman! And apologies for the delay, I should be finished reviewing today

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.

You want to update the getScrollPhysics of CupertinoScrollBehavior as well. 👍

///
/// To obtain a velocity, call [getVelocity] or [getVelocityEstimate]. The
/// estimated velocity is typically used as the initial flinging velocity of a
/// `Scrollable`, when its drag gesture ends.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the docs for the iOS version, there is a reference to https://developer.apple.com/documentation/uikit/uiscrollviewdelegate/1619385-scrollviewwillenddragging, is that also relevant here? Or is there another, more relevant reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a direct analogue, in AppKit the scroll momentum state is seen based on the incoming events' [NSEvent momentumPhase]. The part which converts the velocity to the momentum is not exposed at all.

packages/flutter/lib/src/physics/friction_simulation.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/scroll_physics.dart Outdated Show resolved Hide resolved
packages/flutter/test/cupertino/picker_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/refresh_indicator_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/nested_scroll_view_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/page_view_test.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/scroll_physics.dart Outdated Show resolved Hide resolved
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 is looking really good. Apologies for the delay while I was gone.

packages/flutter/lib/src/gestures/velocity_tracker.dart Outdated Show resolved Hide resolved
Comment on lines 20 to 24
double g = guess;
for (int i = 0; i < iterations; i++) {
g = g - (f(g) - target) / df(g);
}
return g;
Copy link
Contributor

Choose a reason for hiding this comment

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

The style guide discourages abbreviations https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-abbreviations

Can you expand this shorthand so it is easier to understand?

packages/flutter/lib/src/widgets/scroll_configuration.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/scroll_physics.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/widgets/scroll_physics.dart Outdated Show resolved Hide resolved
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 is really great, thank you for your incredible attention to detail. :)
LGTM!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 19, 2022
@auto-submit auto-submit bot merged commit 70b19ff into flutter:master Sep 19, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 20, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 20, 2022
@appinteractive
Copy link

which flutter version will this land in?

@moffatman
Copy link
Contributor Author

@appinteractive Has been in beta channel for a while, should be in the next stable version 3.7. No idea when that releases though

@hellohuanlin
Copy link
Contributor

IIRC next release will be around next Jan @appinteractive

@appinteractive
Copy link

appinteractive commented Dec 22, 2022

Thanks for the response, tried it in 3.7 pre but I still have scroll that has way to little friction on macOS, do I need to specify anything for it to work? Using MaterialApp as a base because Cupertino is barely usable with plugins. (Missing Material)

@moffatman
Copy link
Contributor Author

@appinteractive Can you share some code which reproduces the issue? If you are setting custom scroll physics on any widget, the correct physics for macOS should be BouncingScrollPhysics(decelerationRate: ScrollDecelerationRate.fast). But if you never set physics on any widget, it should be getting the correct physics by default.

@GroovinChip
Copy link
Contributor

GroovinChip commented Jan 27, 2023

@Piinks After the release of Flutter 3.7 with this included, I'm seeing The Scrollbar's ScrollController has no ScrollPosition attached. errors for apps using macos_ui. I assume I need to remove or alter some of the custom scrolling code in the library, but my efforts so far haven't yielded any results. I read the desktop scrolling migration guide and I am only more confused. Should I remove all usages of our custom MacosScrollbar, as well as our custom MacosScrollBehavior?

EDIT: It's also worth mentioning that the errors I'm getting do not trace which scrollable the issue is arising from :/

@Piinks
Copy link
Contributor

Piinks commented Jan 27, 2023

Hi @GroovinChip, it sounds like there may be a regression. Can you please file an issue with the last flutter version you were using and a minimal reproduction?
We won’t be able to diagnose a potential regression in an old PR.

@GroovinChip
Copy link
Contributor

@Piinks Sure thing! Didn't realize it could be a regression :)

@GroovinChip
Copy link
Contributor

Hi @GroovinChip, it sounds like there may be a regression. Can you please file an issue with the last flutter version you were using and a minimal reproduction? We won’t be able to diagnose a potential regression in an old PR.

Done - #119323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop a: fidelity Matching the OEM platforms better autosubmit Merge PR when tree becomes green via auto submit App f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-mac Building on or for macOS specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrolling with trackpad continues momentum animation for too long
5 participants