Apply indexToItemIndex to indices returned by findChildIndexCallback in SliverAnimatedListState
#108710
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…Callback in SliverAnimatedListState
flutter-dashboard
bot
added
the
framework
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This was referenced Jul 31, 2022
otto-dev
force-pushed
the
fix/SliverAnimatedList/findChildIndexCallback/add-indexToItemIndex
branch
from
5195766 to
de4cc53
Compare
otto-dev
force-pushed
the
fix/SliverAnimatedList/findChildIndexCallback/add-indexToItemIndex
branch
from
0e1c961 to
24fca4b
Compare
otto-dev
commented
Aug 1, 2022
Comment on lines
+417
to
+419
| expect(reorderedListEntries[0].data, equals('item 3')); | ||
| expect(reorderedListEntries[1].data, equals('item 1')); | ||
| expect(reorderedListEntries[2].data, equals('item 2')); |
Contributor
Author
There was a problem hiding this comment.
This fails on master - hence this PR.
Contributor
Author
|
CLA signed, test added, PR ready for review. |
chunhtai
reviewed
Aug 3, 2022
Contributor
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
LGTM, just some nitpicking
| _itemBuilder, | ||
| childCount: _itemsCount, | ||
| findChildIndexCallback: widget.findChildIndexCallback, | ||
| findChildIndexCallback: widget.findChildIndexCallback == null ? null : (Key key) { |
Contributor
There was a problem hiding this comment.
This should follow the other in line if else format if it is too long
findChildIndexCallback: widget.findChildIndexCallback == null
? null
: (Key key) {
....
}
| slivers: <Widget>[ | ||
| SliverAnimatedList( | ||
| key: listKey, | ||
| initialItemCount: items.length, // we will insert one item later |
| expect(tester.getTopLeft(find.text('item 0')).dy, 200); | ||
| }); | ||
|
|
||
| testWidgets('passes correctly derived index of findChildIndexCallback to inner SliverChildBuilderDelegate', (WidgetTester tester) async { |
Contributor
There was a problem hiding this comment.
Suggested change
| testWidgets('passes correctly derived index of findChildIndexCallback to inner SliverChildBuilderDelegate', (WidgetTester tester) async { | |
| testWidgets('passes correctly derived index of findChildIndexCallback to the inner SliverChildBuilderDelegate', (WidgetTester tester) async { |
Contributor
|
cc @Piinks for secondary review |
Piinks
added
f: scrolling
otto-dev
deleted the
fix/SliverAnimatedList/findChildIndexCallback/add-indexToItemIndex
branch
This was referenced Aug 6, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 6, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Aug 6, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 6, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 6, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 7, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 7, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 8, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 8, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 8, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 8, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 8, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 8, 2022
…exCallback` in `SliverAnimatedListState` (flutter/flutter#108710)
camsim99
pushed a commit
to camsim99/flutter
that referenced
this pull request
Aug 10, 2022
…ck` in `SliverAnimatedListState` (flutter#108710)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In
SliverAnimatedListState, theinsertItem()andremoveItem()index parameters are defined as if theremoveItem()operation removed the corresponding list entry immediately. The entry is only actually removed from theListViewwhen the remove animation finishes._indexToItemIndexis used to convert the user-supplied index to the internal index, which takes item removal into account.The user can also provide a
findChildIndexCallbackto theSliverAnimatedListwhich is passed on to the internalSliverChildBuilderDelegate. Sincedelegaterenders all items, including items being removed, the same_indexToItemIndexneeds to be applied to the indices returned by the user-provided function. Otherwise it is impossible for the user to implementfindChildIndexCallback, because the internal state of theSliverAnimatedListand hence the current content of thedelegateis not known to the user.findChildIndexCallbackindices originate from the same user-facing source of truth asinsertItem()andremoveItem(), and hence must likewise be converted to internal indices.The call to
_indexToItemIndexin the innerfindChildIndexCallbackwas previously omitted by oversight, making it impossible for the user to implementfindChildIndexCallbackcorrectly, resulting in unwanted rebuilds and state loss. This PR fixes this.