fix: findChildIndexCallback to take seperators into account for seperated named constructor in ListView and SliverList #174491
Conversation
github-actions
bot
added
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where findChildIndexCallback did not account for separators in ListView.separated and SliverList.separated. The change correctly wraps the callback to multiply the returned index by two. The accompanying tests verify that state is preserved correctly with this change. My review focuses on improving maintainability by reducing code duplication in both the implementation and the new tests. I've suggested extracting the duplicated logic into helper functions.
| findChildIndexCallback: findChildIndexCallback == null | ||
| ? null | ||
| : (Key key) { | ||
| final int? itemIndex = findChildIndexCallback(key); | ||
| return itemIndex == null ? null : itemIndex * 2; | ||
| }, |
There was a problem hiding this comment.
This logic for wrapping findChildIndexCallback is also present in SliverList.separated. To improve maintainability and avoid code duplication, consider extracting this logic into a shared helper function that can be used in both places.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
rkishan516
force-pushed
the
seperated-builder-index-callback
branch
from
e4d3aa8 to
e3b0250
Compare
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
While our tests pass, this is probably going to break anyone currently using ListView.separated.findChildIndexCallback because they are probably multiplying that result by 2 already to workaround this.
I think we should deprecate this property but only on the separated constructor. And replace with a ListView.separated.findItemIndexCallback. Is this possible? What do you think @rkishan516?
Yes, it is possible. I was just worried about |
rkishan516
force-pushed
the
seperated-builder-index-callback
branch
from
e3b0250 to
55dd1f6
Compare
github-actions
bot
added
the
c: tech-debt
rkishan516
force-pushed
the
seperated-builder-index-callback
branch
2 times, most recently
from
8014b57 to
6904036
Compare
rkishan516
force-pushed
the
seperated-builder-index-callback
branch
from
2851b13 to
7cce0fc
Compare
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Is it possible we might want to keep the original callback since it is used the keep track of children when they are re-ordered? There is an open pending PR that add a separated constructor to the reorderable list classes.
|
cc @chunhtai who might know if we need to preserve the original callback. |
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
change lgtm, but we need a migration guide and follows https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
|
I see no good reason to preserve the original callback. since we don't provide a way to build different separator based on index, and you can't swap a separator's location with other item. |
rkishan516
force-pushed
the
seperated-builder-index-callback
branch
from
7cce0fc to
8ef53d1
Compare
github-actions
bot
removed
the
c: tech-debt
I have added migration guide at flutter/website#12636 |
|
Can you add the migration guide to the PR description so it is easy to find? |
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
… for seperated named constructor in ListView and SliverList (flutter/flutter#174491)
…10408) Manual roll requested by [email protected] flutter/flutter@31a8481...ee23168 2025-11-07 [email protected] Roll Packages from f13bad3 to 3caa48b (5 revisions) (flutter/flutter#178164) 2025-11-07 [email protected] Fix text input actions in DropdownMenu. (flutter/flutter#177313) 2025-11-07 [email protected] Roll Skia from f838c4b31edb to 581d1ecd5029 (1 revision) (flutter/flutter#178157) 2025-11-07 [email protected] Roll Skia from eb3c5b280ae6 to f838c4b31edb (3 revisions) (flutter/flutter#178149) 2025-11-07 [email protected] Roll Skia from 360fe72b5bf4 to eb3c5b280ae6 (1 revision) (flutter/flutter#178147) 2025-11-07 [email protected] Roll Skia from 116f237bb39d to 360fe72b5bf4 (4 revisions) (flutter/flutter#178146) 2025-11-07 [email protected] Roll Fuchsia Linux SDK from cm88aTLui5yorSGYQ... to qDVe2mrpSgQdxra7p... (flutter/flutter#178144) 2025-11-07 [email protected] fix: findChildIndexCallback to take seperators into account for seperated named constructor in ListView and SliverList (flutter/flutter#174491) 2025-11-06 [email protected] [web] Remove unnecessary android_sdk dep (flutter/flutter#178078) 2025-11-06 [email protected] Add haptic notifications support. (flutter/flutter#177721) 2025-11-06 [email protected] Allow label to be used to compute InputDecorator Intrinsic width (flutter/flutter#178101) 2025-11-06 [email protected] Respect product flavor abiFilters by adding a `disable-abi-filtering` Android project flag. (flutter/flutter#177753) 2025-11-06 [email protected] Use aria-hidden attribute for platform view accessibility on web (flutter/flutter#177969) 2025-11-06 [email protected] [tool] Fix IP parsing by using Uri constructor (flutter/flutter#178083) 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
…ated named constructor in ListView and SliverList (flutter#174491) FindChildIndexCallback to take seperators into account for seperated named constructor in ListView and SliverList fixes: flutter#174261 ## Migration guide flutter/website#12636 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…ated named constructor in ListView and SliverList (flutter#174491) FindChildIndexCallback to take seperators into account for seperated named constructor in ListView and SliverList fixes: flutter#174261 ## Migration guide flutter/website#12636 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
4 participants
FindChildIndexCallback to take seperators into account for seperated named constructor in ListView and SliverList
fixes: #174261
Migration guide
flutter/website#12636
Pre-launch Checklist
///).