fix reorderable_list drop animation #139362
Conversation
github-actions
bot
added
the
framework
There was a problem hiding this comment.
Since this same case is repeated below on 819 in the following else, can this instead be reworked to contain the _insertIndex! == item.index case outside of whether or not it is reversed? Might be easier to follow then
There was a problem hiding this comment.
The handling of _insertIndex! == item.index is different depending on the value of _reverse. In the case where _reverse is true, we offset the item's own extent, but in the case where _reverse is false, we don't need to do this. Therefore, it's not possible to take this case out of the _reverse condition. I believe the current implementation is the most straightforward way to handle these different scenarios.
There was a problem hiding this comment.
I still find the code very difficult to follow. The prior implementation incorporated reverse in the if clauses or when making the function calls. It is hard to tell what has actually changed here.
There was a problem hiding this comment.
Got it, I fixed some code, please continue the review
There was a problem hiding this comment.
What does extent here represent?
There was a problem hiding this comment.
The extent parameter in the _itemOffsetAt function determines which corner of the item's geometry we are interested in. If extent is true, we return the 'end' corner of the item - the bottom right corner for vertical scrolling and the top right corner for horizontal scrolling. If extent is false, we return the 'start' corner of the item - the top left corner for both vertical and horizontal scrolling. This allows us to calculate the offset of the item in the list based on its position and size.
There was a problem hiding this comment.
Thank you for explaining! Maybe we should just pass in the correct geometry rather than a bool. I think that would be easier for maintainers to follow.
|
@Piinks Hi, Are you still continuing the review? |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
This LGTM, thank you for contributing
HansMuller
force-pushed
the
fix_138994
branch
from
adb0212 to
80481af
Compare
flutter/flutter@a51e33a...2407f69 2023-12-15 [email protected] Move package:web dependency to dev dependency (flutter/flutter#139696) 2023-12-15 [email protected] Roll Flutter Engine from 9524a185b055 to 986a6fe198dc (1 revision) (flutter/flutter#140221) 2023-12-15 [email protected] Roll Packages from 1151191 to 3f2e16b (9 revisions) (flutter/flutter#140218) 2023-12-15 [email protected] Roll Flutter Engine from 7a50221733c2 to 9524a185b055 (1 revision) (flutter/flutter#140217) 2023-12-15 [email protected] Roll Flutter Engine from 767223f7a4f8 to 7a50221733c2 (1 revision) (flutter/flutter#140216) 2023-12-15 [email protected] Roll Flutter Engine from 91f65eea0c11 to 767223f7a4f8 (1 revision) (flutter/flutter#140210) 2023-12-15 [email protected] Roll Flutter Engine from a47da28c9a62 to 91f65eea0c11 (1 revision) (flutter/flutter#140207) 2023-12-15 [email protected] Roll Flutter Engine from cde1a596432d to a47da28c9a62 (1 revision) (flutter/flutter#140204) 2023-12-15 [email protected] Roll Flutter Engine from 46ff5c08a905 to cde1a596432d (1 revision) (flutter/flutter#140200) 2023-12-15 [email protected] Roll Flutter Engine from a17bb0a63b7e to 46ff5c08a905 (1 revision) (flutter/flutter#140198) 2023-12-15 [email protected] Roll Flutter Engine from 4cb3ba7a85f6 to a17bb0a63b7e (1 revision) (flutter/flutter#140196) 2023-12-15 [email protected] Roll Flutter Engine from 0e7248d43251 to 4cb3ba7a85f6 (14 revisions) (flutter/flutter#140195) 2023-12-15 [email protected] Increase versions of leak tracker libraries. (flutter/flutter#140018) 2023-12-15 [email protected] Set compile test iOS app target version to not embed Swift runtime (flutter/flutter#140188) 2023-12-15 [email protected] Cupertino text clear label (flutter/flutter#129727) 2023-12-15 [email protected] [github actions] use token from real user flutter mirror bot (flutter/flutter#140191) 2023-12-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 0e7248d43251 to 0b0fab821536 (4 revisions)" (flutter/flutter#140194) 2023-12-14 [email protected] Roll Flutter Engine from 0e7248d43251 to 0b0fab821536 (4 revisions) (flutter/flutter#140180) 2023-12-14 [email protected] feat: Add onTapAlwaysCalled in TextFormField (flutter/flutter#140089) 2023-12-14 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 3.1.3 to 4.0.0 (flutter/flutter#140177) 2023-12-14 [email protected] Roll Flutter Engine from 2140942444ea to 0e7248d43251 (2 revisions) (flutter/flutter#140176) 2023-12-14 [email protected] fix reorderable_list drop animation (flutter/flutter#139362) 2023-12-14 [email protected] Roll Flutter Engine from 997d3dfa1e74 to 2140942444ea (4 revisions) (flutter/flutter#140171) 2023-12-14 [email protected] Fix BottomNavigationBarItem label overflow (flutter/flutter#120206) 2023-12-14 [email protected] Roll Flutter Engine from a565cea256c7 to 997d3dfa1e74 (2 revisions) (flutter/flutter#140170) 2023-12-14 [email protected] Revert "Dynamic view sizing" (flutter/flutter#140165) 2023-12-14 [email protected] �: fix cupertionActionSheet design (flutter/flutter#134345) 2023-12-14 [email protected] Make improvements to existing new issue templates (flutter/flutter#140142) 2023-12-14 [email protected] Roll Flutter Engine from caf33276468b to a565cea256c7 (1 revision) (flutter/flutter#140163) 2023-12-14 [email protected] Expand and update a few release.yml categories (flutter/flutter#140120) 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],[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
3 participants
This PR fixes reorderable_list drop animation.
Fixes #138994
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.