Skip to content

Conversation

@xu-baolin
Copy link
Member

@xu-baolin xu-baolin commented Jan 4, 2022

Fixes #90953

Update content dimensions first when layout to prevent scroll offset changes.

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jan 4, 2022
@xu-baolin xu-baolin requested a review from Hixie January 4, 2022 08:18
@Hixie
Copy link
Contributor

Hixie commented Jan 5, 2022

Calling performLayout from within performLayout is really dubious... are we sure about this?

@Hixie
Copy link
Contributor

Hixie commented Jan 5, 2022

(cc @Piinks who may have opinions on this code)

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 @xu-baolin, happy new year! 🎉

I wonder how we handle this in other viewports, isn't there a correction we apply if !offset.applyContentDimensions?

Copy link
Member Author

@xu-baolin xu-baolin Jan 6, 2022

Choose a reason for hiding this comment

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

Adjust the content dimensions for there is a special test case which allows the delegate to provide a null widget child.

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic here is difficult to understand, so I linked this test case in the comments.

@xu-baolin
Copy link
Member Author

Calling performLayout from within performLayout is really dubious... are we sure about this?

Yes, it seemed weird to do so, I changed a solution.

@xu-baolin xu-baolin requested a review from Piinks January 6, 2022 09:10
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 looks a lot better, thanks for updating!

Comment on lines 770 to 771
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this comment in the test itself? If a future change breaks the test, it will be helpful to have that context there as well.

Comment on lines -666 to -671
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move this? It does not seem to change later in the course of laying out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just move to where first use it.

@Piinks Piinks changed the title [RenderListWheelViewport]Re-layout if the content dimension changes and causes the scroll offset change. [RenderListWheelViewport] Update content dimension to prevent scroll offset changes Jan 7, 2022
@Piinks Piinks changed the title [RenderListWheelViewport] Update content dimension to prevent scroll offset changes [RenderListWheelViewport] Update content dimensions to prevent scroll offset changes Jan 7, 2022
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.

LGTM, nice!

@fluttergithubbot fluttergithubbot merged commit ff60778 into flutter:master Jan 12, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ListWheelScrollView doesn't scroll to last item when items are removed

4 participants