recommendDeferredLoading by dnfield · Pull Request #49319 · flutter/flutter

dnfield · 2020-01-22T22:06:01Z

Description

Split from #48536

This implements a more generic method of deferring work based on scrolling. This will be patched back into #48536, but is split out to make review easier.

Related Issues

#48536
#48305

Tests

I added the following tests:

Tests that check we can lazily build either cheap or expensive widgets based on scroll velocity as driven by ScrollController.animateTo or a ballistic simulation (e.g. from a fling).

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

Piinks

LG(reat!)TM, thank you!

Piinks · 2020-01-24T00:25:25Z

+  /// heuristic depending on whether the transform was increasing or decreasing
+  /// the overall scale between the global screen and local scrollable
+  /// coordinate systems.
+  bool recommendDeferredLoading(double velocity) {


Should we add some documentation regarding @goderbauer's comment on the original PR to clarify expectations around quick, back and forth scrolling?

Added another paragraph.

To be clear - the pathological case would require that the scrollable be animating so fast that it not be usable, or that it be both animating that fast and have some kind of transform applied to it such that it appeared to not be animating that fast.

Hixie · 2020-01-24T00:57:38Z

+  /// to avoid doing work that will not have a chance to appear before a new
+  /// frame is rendered.
+  ///
+  /// For example, a list of images would use this logic to delay decoding


would -> could

Hixie · 2020-01-24T00:58:20Z

+  /// heuristic depending on whether the transform was increasing or decreasing
+  /// the overall scale between the global screen and local scrollable
+  /// coordinate systems.
+  bool recommendDeferredLoading(double velocity) {


seems like at a minimum we should pass in a Context here

or if that's not possible, the ScrollMetrics, or some such

looks like you could pass both in

You mean a BuildContext?

Yes, I could pass both in. I can see how ScrollMetrics could be helpful, I'm less clear on how a BuildContext would be.

Went with the build context of the original calling element - per chat discussion could be useful for getting MediaQuery or like

gaaclarke · 2020-01-24T00:54:49Z

+  /// heuristic depending on whether the transform was increasing or decreasing
+  /// the overall scale between the global screen and local scrollable
+  /// coordinate systems.
+  bool recommendDeferredLoading(double velocity) {


Please state what returning 'true' means.

gaaclarke · 2020-01-24T00:55:24Z

+  /// The actual work of this is delegated to the [physics] via
+  /// [ScrollPhysics.recommendDeferredScrolling] called with the current
+  /// [activity]'s [ScrollActivity.velocity].
+  bool recommendDeferredLoading() {


Please state what returning true means.

Hixie · 2020-01-24T01:00:03Z

+      return velocity.abs() > maxPhysicalPixels;
+    }
+    return parent.recommendDeferredLoading(velocity);
+  }


this function should be lower in the class for consistency with the current ordering

probably before or after shouldAcceptUserOffset

Hixie · 2020-01-24T01:00:57Z

+  /// The actual work of this is delegated to the [physics] via
+  /// [ScrollPhysics.recommendDeferredScrolling] called with the current
+  /// [activity]'s [ScrollActivity.velocity].
+  bool recommendDeferredLoading() {


could be a getter? not sure how i feel about it

The other simiilarly named ones are methods - just seems consistent this way to me.

goderbauer

LGTM

goderbauer · 2020-01-24T17:38:10Z

+  /// Provides a heuristic to determine if expensive frame-bound tasks should be
+  /// deferred.
+  ///
+  /// The velocity parameter must not be null, but may be positive, negative, or


update this to also mention the other parameters?

goderbauer · 2020-01-24T17:38:57Z

+  /// The default implementation is stateless, and simply provides a point-in-
+  /// time decision about how fast the scrollable is scrolling. It would always
+  /// return true for a scrollable that is animating back and forth at high
+  ///  velocity in a loop. It is assumed that callers will handle such


nit: remove extra space?

goderbauer · 2020-01-24T17:41:53Z

+  ///
+  /// Returning true from this method indicates that the current scroll velocity
+  /// is great enough that expensive operations impacting the UI should be
+  /// deferred.


+1 for this extensive doc comment!

goderbauer · 2020-01-24T17:44:03Z

+
+class SuperPessimisticScrollPhysics extends ScrollPhysics {
+  const SuperPessimisticScrollPhysics({ScrollPhysics parent}) : super(parent: parent);
+


nit: remove one blank line

goderbauer · 2020-01-24T17:44:14Z

+      textDirection: TextDirection.ltr,
+      child: ListView.builder(
+        controller: controller,
+        physics: const NeverScrollableScrollPhysics(),


Why this? Shouldn't this also work if the user is allowed to scroll? (same for the next test)

This doesn't do what I thought it did, removed.

goderbauer · 2020-01-24T17:46:00Z

+      duration: const Duration(seconds: 2),
+      curve: Curves.fastLinearToSlowEaseIn,
+    );
+


Maybe also jump to a point half-way through the animation and assert that at that point only cheap widgets are being shown on screen?

goderbauer · 2020-01-24T17:48:07Z

+    expect(cheapWidgets, 20);
+  });
+
+  testWidgets('Can recommendDeferredLoadingForContext - override heuristic', (WidgetTester tester) async {


how does this test show that the overridden method was actually used?

Maybe (also) have a test with physics that always defer no mater what and check that it only builds cheap boxes?

I'll add state to the physics to assert that it's called.

I think with that the other test isn't strictly necessary, right?

I guess... although it could still be a wired combination of some kind of default defer logic and this :P

Ahh fair. Ok.

goderbauer · 2020-01-24T17:49:48Z

+  /// Returning true from this method indicates that the current scroll velocity
+  /// is great enough that expensive operations impacting the UI should be
+  /// deferred.
+  bool recommendDeferredLoading(double velocity, ScrollMetrics metrics, BuildContext context) {


Wondering: Should we make this take an object to make it easier to add more information? Or are we pretty certain that this is all the information this will ever need?

With the context you can find anything public the scrollable has. The only way we'd need more is if we want to add access to more private/protected properties of scrolling related classes.

But if we make it an object I guess we can more easily change this without breaking in the future. Any suggestions for names?

Talked offline - it's probably not worth comlecting this right now.

recommendDeferredLoading

2b21d0a

dnfield requested review from Hixie and goderbauer January 22, 2020 22:06

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 22, 2020

googlebot added the cla: yes label Jan 22, 2020

newline

e830b2c

dnfield requested a review from gaaclarke January 22, 2020 22:07

This comment has been minimized.

Sign in to view

dnfield added 2 commits January 22, 2020 14:27

another test

c36c86f

typo

b2f2017

This was referenced Jan 23, 2020

Defer image decoding when scrolling fast. #48536

Closed

Defer image decoding when scrolling fast #49389

Merged

Piinks added the f: scrolling Viewports, list views, slivers, etc. label Jan 24, 2020

Piinks approved these changes Jan 24, 2020

View reviewed changes

docs

100ed72

Hixie reviewed Jan 24, 2020

View reviewed changes

gaaclarke approved these changes Jan 24, 2020

View reviewed changes

Hixie reviewed Jan 24, 2020

View reviewed changes

dnfield added 4 commits January 23, 2020 19:15

review

e6a1d2e

Make metrics and context available

4f506d2

update test for new parameters

522a935

Merge remote-tracking branch 'upstream/master' into physics_fast

cc49989

goderbauer approved these changes Jan 24, 2020

View reviewed changes

Add more tests, tweak docs

1695087

dnfield added the waiting for tree to go green label Jan 24, 2020

fluttergithubbot merged commit bcfc293 into flutter:master Jan 25, 2020

Piinks mentioned this pull request Feb 16, 2021

Scrolling over a large delta in a very short amount of time in ListView.builder is significantly slower #75399

Open

github-actions Bot locked as resolved and limited conversation to collaborators Aug 2, 2021


		class SuperPessimisticScrollPhysics extends ScrollPhysics {
		const SuperPessimisticScrollPhysics({ScrollPhysics parent}) : super(parent: parent);

Conversation

dnfield commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

This comment has been minimized.

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jan 22, 2020 •

edited

Loading