Skip to content

Refactor animation_controller.dart#160005

Closed
nate-thegrate wants to merge 3 commits intoflutter:masterfrom
nate-thegrate:refactor-animation-controller
Closed

Refactor animation_controller.dart#160005
nate-thegrate wants to merge 3 commits intoflutter:masterfrom
nate-thegrate:refactor-animation-controller

Conversation

@nate-thegrate
Copy link
Contributor

@nate-thegrate nate-thegrate commented Dec 10, 2024 •
edited
Loading

This pull request aims to consolidate duplicated logic in the animation controller library.

A private field has been added to AnimationBehavior:

enum AnimationBehavior {
  normal,
  preserve;

  bool get _enableAnimations => switch (this) {
    normal => !SemanticsBinding.instance.disableAnimations,
    preserve => true,
  };
}

I'll make comments to describe the other changes.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs labels Dec 10, 2024
@nate-thegrate nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Dec 10, 2024
nate-thegrate
nate-thegrate commented Dec 10, 2024
View reviewed changes
@nate-thegrate nate-thegrate mentioned this pull request Dec 10, 2024
Closed
@nate-thegrate nate-thegrate marked this pull request as ready for review December 10, 2024 02:11
nate-thegrate
nate-thegrate commented Dec 11, 2024
View reviewed changes
@nate-thegrate
Copy link
Contributor Author

nate-thegrate commented Dec 11, 2024

I'm very happy with the improvement from #150727!
@gnprice if you're able to take a look here that'd be great, or if you don't have the bandwidth that's totally fine :)

@nate-thegrate nate-thegrate force-pushed the refactor-animation-controller branch 2 times, most recently from 5fe9276 to b2bfc20 Compare December 23, 2024 16:02
@flutter-dashboard

This comment was marked as resolved.

Sign in to view
goderbauer
goderbauer reviewed Jan 15, 2025
View reviewed changes
packages/flutter/lib/src/animation/animation_controller.dart Outdated

/// Whether animations should be enabled, based on the configured behavior
/// and the [AccessibilityFeatures.disableAnimations] flag.
bool get enableAnimations => switch (this) {
Copy link
Member

@goderbauer goderbauer Jan 15, 2025

Choose a reason for hiding this comment

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

This was designed as an enum so we could add additional behaviors later. enableAnimations pretends that it is just a boolean, which kinda defeats the purpose of that. If we add another value to AnimationBehavior we want the analyzer to tell us all the places we need to update with new behavior.

Copy link
Contributor Author

@nate-thegrate nate-thegrate Jan 16, 2025

Choose a reason for hiding this comment

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

That's fair. I could get rid of the getter, or it could be private (then in the future we could change it up without worrying about breaking people).

@nate-thegrate nate-thegrate force-pushed the refactor-animation-controller branch from b2bfc20 to b608521 Compare January 26, 2025 19:09
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jan 26, 2025

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@nate-thegrate nate-thegrate force-pushed the refactor-animation-controller branch from b608521 to ead611b Compare January 28, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer left review comments

@gnprice gnprice Awaiting requested review from gnprice

Assignees

No one assigned

Labels

a: animation Animation APIs framework flutter/packages/flutter repository. See also f: labels. refactor Improving readability/efficiency without behavioral changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nate-thegrate @goderbauer