Skip to content

Conversation

@chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Nov 22, 2023

The code doesn't consider indices of children may not be continuous after rebuild.

fixes #138749

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Nov 22, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy over the code from RenderSliverFixedExtentBoxAdaptor

Copy link
Member

Choose a reason for hiding this comment

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

Could the logic live in a common place (e.g. the RenderSliverMultiBoxAdaptor base class) and be shared between the two instead of duplicating it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to expose these method because they are implementation detail. since this are only two instance, I prefer to keep it as is. We can revisit this if there are more use case for these logic

@goderbauer
Copy link
Member

Would you mind reverting some of the dartfmt changes that snuck into this PR?

@chunhtai
Copy link
Contributor Author

Would you mind reverting some of the dartfmt changes that snuck into this PR?

done.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Could the logic live in a common place (e.g. the RenderSliverMultiBoxAdaptor base class) and be shared between the two instead of duplicating it?

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 22, 2023

auto label is removed for flutter/flutter/138915, due to - The status or check suite Mac_build_test flutter_gallery__transition_perf_e2e_ios has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Mac framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac build_tests_2_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux build_tests_2_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux build_tests_1_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows customer_testing has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac_arm64 flutter_packaging_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux flutter_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit auto-submit bot merged commit d92d2c1 into flutter:master Nov 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 24, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 24, 2023
flutter/flutter@106667e...cf05f3c

2023-11-24 [email protected] Roll Flutter Engine from 7e640faa6adf to b1e0bf62fa35 (1 revision) (flutter/flutter#138979)
2023-11-24 [email protected] Roll Flutter Engine from d67ac2c561ca to 7e640faa6adf (2 revisions) (flutter/flutter#138969)
2023-11-24 [email protected] Roll Flutter Engine from 152790b5811b to d67ac2c561ca (1 revision) (flutter/flutter#138965)
2023-11-23 [email protected] Roll Flutter Engine from 4a344e050401 to 152790b5811b (1 revision) (flutter/flutter#138959)
2023-11-23 [email protected] Roll Flutter Engine from 6e71c3642abf to 4a344e050401 (1 revision) (flutter/flutter#138958)
2023-11-23 [email protected] Roll Flutter Engine from bec0dacaae41 to 6e71c3642abf (2 revisions) (flutter/flutter#138956)
2023-11-23 [email protected] [flutter_tools] Fix bad state future already completed in flutter logs (flutter/flutter#138517)
2023-11-23 [email protected] Fix SliverGrid garbage collection issue (flutter/flutter#138915)
2023-11-23 [email protected] Roll Flutter Engine from f331e0afaf2e to bec0dacaae41 (6 revisions) (flutter/flutter#138954)
2023-11-23 [email protected] Roll Packages from 2102327 to 6c5ad9e (1 revision) (flutter/flutter#138950)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
The code doesn't consider indices of children may not be continuous after rebuild.

fixes flutter#138749
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App 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.

Crash when using SliverGrid, findChildIndexCallback, a sparsely populated grid and updating the children

2 participants