Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MergeableMaterial: Fix adding a slice and separating it #128804

Merged
merged 2 commits into from Jul 5, 2023

Conversation

Snonky
Copy link
Contributor

@Snonky Snonky commented Jun 13, 2023

Fixes #128646

It is possible to add a MaterialGap to a MergeableMaterial without its AnimationController for the separation animation being kicked off.
This leads to the gap instantly being removed again by _removeEmptyGaps() during the next build.

The fix adds an AnimationController.forward() call where the MaterialGap insertion happens.

I found this bug while using ExpansionPanelList as the issue describes. Here is a video of an ExpansionPanelList before and after the fix. Note the gaps between the panels:

Before
2023-06-11.03-12-09.mp4
After
2023-06-13.18-29-52.mp4

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 f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 13, 2023
@Snonky Snonky force-pushed the fix/mergeable-material branch 2 times, most recently from e3cb7cf to eddad96 Compare June 13, 2023 17:23
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution!! This looks good:) Just left one question and some nits below.

_insertChild(j, newChild);

if (newChild is MaterialGap) {
_animationTuples[newChild.key]!.controller.forward();
Copy link
Contributor

Choose a reason for hiding this comment

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

In _insertChild method, there is a check if (child is MaterialGap) {. So should we just add this line in that check and revert the code change here in this while loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not always AnimationController.forward() that is used when _inserChild is called for a MaterialGap. There are cases within didUpdateWidget where it is AnimationController.reverse() or the animation is not being touched at all like in these places:

https://github.com/flutter/flutter/blob/eddad960e992a10f5c9d893f2106d3696b1cb6b4/packages/flutter/lib/src/material/mergeable_material.dart#L437-L441

https://github.com/flutter/flutter/blob/eddad960e992a10f5c9d893f2106d3696b1cb6b4/packages/flutter/lib/src/material/mergeable_material.dart#L356-L362

The handling of a gap's animation is decided for each gap insertion case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. Thanks for the explanation!

Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks:)

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@QuncCccccc QuncCccccc merged commit e0a9ad1 into flutter:master Jul 5, 2023
72 checks passed
@Snonky Snonky deleted the fix/mergeable-material branch July 6, 2023 15:01
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 6, 2023
flutter/flutter@35085c3...bc49cd1

2023-07-06 ian@hixie.ch Allow long-press gestures to continue even if buttons change. (flutter/flutter#127877)
2023-07-06 goderbauer@google.com Enable unreachable_from_main lint - it is stable now!!1 (flutter/flutter#129854)
2023-07-06 ian@hixie.ch Update labeler to new label names (flutter/flutter#130040)
2023-07-05 47639380+Snonky@users.noreply.github.com MergeableMaterial: Fix adding a slice and separating it (flutter/flutter#128804)
2023-07-05 ian@hixie.ch Update infrastructure issue template for new priority scheme (flutter/flutter#129741)
2023-07-05 737941+loic-sharma@users.noreply.github.com Fix typo in canvas example (flutter/flutter#129879)
2023-07-05 hans.muller@gmail.com Reland Fix AnimatedList & AnimatedGrid doesn't apply MediaQuery padding #129556 (flutter/flutter#129860)
2023-07-05 ian@hixie.ch Change from "created via performance template" to "from: performance template" (flutter/flutter#130035)
2023-07-05 109253501+pdblasi-google@users.noreply.github.com Removes deprecated APIs from v2.6 in `binding.dart` and `widget_tester.dart` (flutter/flutter#129663)
2023-07-05 helinx@google.com Add new hot reload case string (flutter/flutter#130008)
2023-07-05 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 987b621eac4e to bd2e42b203e1 (32 revisions) (flutter/flutter#130023)
2023-07-05 69246223+moylanm@users.noreply.github.com Add simple unit tests for annotations.dart file (flutter/flutter#128902)
2023-07-05 gipcjs@gmail.com fix a bug when android uses CupertinoPageTransitionsBuilder... (flutter/flutter#114303)
2023-07-05 piotr.fleury@gmail.com Add .env file support for  option `--dart-define-from-file` (flutter/flutter#128668)

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 bmparr@google.com,rmistry@google.com,stuartmorgan@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

" '_children[j] is MaterialGap': is not true " when updating state of ExpansionPanelList
3 participants