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
recommendDeferredLoading #49319
recommendDeferredLoading #49319
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG(reat!)TM, thank you!
/// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some documentation regarding @goderbauer's comment on the original PR to clarify expectations around quick, back and forth scrolling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would -> could
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like at a minimum we should pass in a Context here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or if that's not possible, the ScrollMetrics, or some such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you could pass both in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with the build context of the original calling element - per chat discussion could be useful for getting MediaQuery or like
/// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please state what returning 'true' means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/// The actual work of this is delegated to the [physics] via | ||
/// [ScrollPhysics.recommendDeferredScrolling] called with the current | ||
/// [activity]'s [ScrollActivity.velocity]. | ||
bool recommendDeferredLoading() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please state what returning true means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return velocity.abs() > maxPhysicalPixels; | ||
} | ||
return parent.recommendDeferredLoading(velocity); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function should be lower in the class for consistency with the current ordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably before or after shouldAcceptUserOffset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/// The actual work of this is delegated to the [physics] via | ||
/// [ScrollPhysics.recommendDeferredScrolling] called with the current | ||
/// [activity]'s [ScrollActivity.velocity]. | ||
bool recommendDeferredLoading() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be a getter? not sure how i feel about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other simiilarly named ones are methods - just seems consistent this way to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this to also mention the other parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove extra space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/// | ||
/// Returning true from this method indicates that the current scroll velocity | ||
/// is great enough that expensive operations impacting the UI should be | ||
/// deferred. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this extensive doc comment!
|
||
class SuperPessimisticScrollPhysics extends ScrollPhysics { | ||
const SuperPessimisticScrollPhysics({ScrollPhysics parent}) : super(parent: parent); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove one blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
textDirection: TextDirection.ltr, | ||
child: ListView.builder( | ||
controller: controller, | ||
physics: const NeverScrollableScrollPhysics(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this? Shouldn't this also work if the user is allowed to scroll? (same for the next test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do what I thought it did, removed.
duration: const Duration(seconds: 2), | ||
curve: Curves.fastLinearToSlowEaseIn, | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
expect(cheapWidgets, 20); | ||
}); | ||
|
||
testWidgets('Can recommendDeferredLoadingForContext - override heuristic', (WidgetTester tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess... although it could still be a wired combination of some kind of default defer logic and this :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh fair. Ok.
/// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked offline - it's probably not worth comlecting this right now.
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.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.