Skip to content

Conversation

@xu-baolin
Copy link
Member

@xu-baolin xu-baolin commented May 6, 2022

Improve some docs for folk to troubleshoot.

Fixes #102904

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels May 6, 2022
@xu-baolin
Copy link
Member Author

// Now find the first child that ends after our end.
while (endScrollOffset < targetEndScrollOffset) {
if (!advance()) {
reachedEnd = true;
break;
}
}

RenderSliverList.performLayout() will create many children to fill up the viewport, so zero-size children may cause an infinite loop.
This is hard to catch at runtime, so only be explained through documentation.

@xu-baolin xu-baolin requested a review from Piinks May 6, 2022 10:18
@HansMuller HansMuller changed the title Improve the SliverChildBuilderDelegate docs for folk to troubleshot. Improve the SliverChildBuilderDelegate docs for folk to troubleshoot. May 6, 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.

Hey @xu-baolin! Always good to see you. :)

Comment on lines 373 to 375
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can catch this and throw an error?

Copy link
Member Author

@xu-baolin xu-baolin May 7, 2022

Choose a reason for hiding this comment

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

I've tried both solutions, but neither is perfect.

  1. Timeout check. When an exception is thrown if time out(about 3 seconds), the Flutter error thread will take about ten seconds(or more) to collect error information (because thousands of children have been created at this time), and the final error message printed to the console will overflow, and the user cannot well informed of what happened;
  2. When 100 children of zero size are created continuously, throw an exception, which is a bit hacky and maybe misjudged.
    Wondering if you have any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. you are right, this is very difficult to catch accurately.
We cannot add timeouts (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#never-check-if-a-port-is-available-before-using-it-never-add-timeouts-and-other-race-conditions), and it is possible the user could have zero sized children in the thousands preceding a child that actually has a size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can word it a bit differently, right now it sounds like a warning against this scenario. What should the user do then if they run into this?

Set a child count, return null from builder, or provide children with a size sound like options, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// and the [builder] always provide a zero-size widget, such as `Container()`
/// and the [builder] always provides a zero-size widget, such as `Container()`

Comment on lines 373 to 375
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. you are right, this is very difficult to catch accurately.
We cannot add timeouts (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#never-check-if-a-port-is-available-before-using-it-never-add-timeouts-and-other-race-conditions), and it is possible the user could have zero sized children in the thousands preceding a child that actually has a size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// [builder] always provide a zero size widget, such as `Container()`
/// [builder] always provides a zero size widget, such as `Container()`

Comment on lines 373 to 375
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can word it a bit differently, right now it sounds like a warning against this scenario. What should the user do then if they run into this?

Set a child count, return null from builder, or provide children with a size sound like options, what do you think?

@goderbauer
Copy link
Member

@xu-baolin Do you still have plans to follow up on the feedback given above?

@xu-baolin
Copy link
Member Author

I will be back soon!

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-cat
Thanks!

@Piinks Piinks added d: api docs Issues with https://api.flutter.dev/ documentation labels Jun 7, 2022
@fluttergithubbot fluttergithubbot merged commit da19a88 into flutter:master Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 8, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

d: api docs Issues with https://api.flutter.dev/ 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.

SliverChildBuilderDelegate null childCount can cause hanging hot reload, white screen

4 participants