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.

Sign up for GitHub

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

Jump to bottom

Enhanced enum features for AnimationStatus #147801

Merged
merged 13 commits into from May 16, 2024
Merged

Enhanced enum features for AnimationStatus #147801

merged 13 commits into from May 16, 2024
+175 −3

Conversation

nate-thegrate
Copy link
Member

@nate-thegrate nate-thegrate commented May 3, 2024 •
edited

Based on issue #147799, this pull request adds two AnimationStatus getters.

bool get isRunning => switch (this) {
  forward   || reverse   => true,
  completed || dismissed => false,
};

bool get aimedForward => switch (this) {
  forward || completed => true,
  reverse || dismissed => false,
};

I also added a .toggle() method for animation controllers that makes use of aimedForward.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs labels May 3, 2024
@nate-thegrate nate-thegrate marked this pull request as ready for review May 3, 2024 20:12
nate-thegrate
nate-thegrate commented May 3, 2024
View reviewed changes
packages/flutter/test/animation/animation_controller_test.dart
Comment on lines +190 to +191
controller.dispose();

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that a controller was being reassigned here without a .dispose() call, so I added this as well.

@nate-thegrate nate-thegrate mentioned this pull request May 4, 2024
Open
@goderbauer goderbauer self-requested a review May 7, 2024 22:08
@nate-thegrate nate-thegrate marked this pull request as draft May 15, 2024 02:11
@nate-thegrate nate-thegrate marked this pull request as ready for review May 15, 2024 05:38
goderbauer
goderbauer reviewed May 15, 2024
View reviewed changes
packages/flutter/lib/src/animation/animation.dart Show resolved Hide resolved
packages/flutter/lib/src/animation/animation.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/animation.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/animation/animation.dart Outdated
///
/// Useful for toggling animations back and forth.
/// {@endtemplate}
bool get aimedForward => switch (this) {
Copy link
Member

Choose a reason for hiding this comment

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

I am not super in love with this name. I wish I had a better suggestion...

Copy link
Member Author

Choose a reason for hiding this comment

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

me too 🥲

Copy link
Member

Choose a reason for hiding this comment

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

I asked gemini, it came up with the very explicit isForwardOrCompleted name. Still not great, I like it slightly better then aimedForward, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm good with that!

packages/flutter/lib/src/animation/animation_controller.dart
/// The most recently returned [TickerFuture], if any, is marked as having been
/// canceled, meaning the future never completes and its [TickerFuture.orCancel]
/// derivative future completes with a [TickerCanceled] error.
TickerFuture toggle({ double? from }) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you document the parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure can!

I copied the documentation over from the forward/reverse functions, so I'll document the parameter for those as well.

goderbauer
goderbauer approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/animation/animation.dart Outdated
///
/// Useful for toggling animations back and forth.
/// {@endtemplate}
bool get aimedForward => switch (this) {
Copy link
Member

Choose a reason for hiding this comment

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

I asked gemini, it came up with the very explicit isForwardOrCompleted name. Still not great, I like it slightly better then aimedForward, though.

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label May 16, 2024
@auto-submit auto-submit bot merged commit 6b38361 into flutter:master May 16, 2024
73 checks passed
@nate-thegrate nate-thegrate deleted the aimed-forward branch May 16, 2024 17:58
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 17, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 17, 2024
@engine-flutter-autoroll
6b383615c Enhanced enum features for AnimationStatus (flutter/flutt…
e966360 
…er#147801)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 17, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 17, 2024
@engine-flutter-autoroll
6b383615c Enhanced enum features for AnimationStatus (flutter/flutt…
9520ef8 
…er#147801)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 17, 2024
@engine-flutter-autoroll
Roll Flutter from 0d22d9101aac to 00425ef1be26 (14 revisions) (#6753)
ae4dd32 
flutter/flutter@0d22d91...00425ef

2024-05-17 [email protected] Roll Flutter Engine from d575e511f9e5 to a19d3722922d (2 revisions) (flutter/flutter#148534)
2024-05-17 [email protected] Roll Flutter Engine from 8d1a1d8d7b48 to d575e511f9e5 (5 revisions) (flutter/flutter#148529)
2024-05-17 [email protected] Manual roll Flutter Engine from 6fa734d68688 to 8d1a1d8d7b48 (8 revisions) (flutter/flutter#148528)
2024-05-17 [email protected] Add test for material_banner.0.dart and material_banner.1.dart (flutter/flutter#148452)
2024-05-17 [email protected] `switch` statement cleanup (flutter/flutter#148382)
2024-05-16 [email protected] const vs. non-const widget build benchmark (flutter/flutter#148261)
2024-05-16 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.5 to 4.1.6 (flutter/flutter#148516)
2024-05-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 6fa734d68688 to 1850def2ccff (1 revision) (#148507)" (flutter/flutter#148517)
2024-05-16 [email protected] Add PrivacyInfo.xcprivacy to plugin template (flutter/flutter#148485)
2024-05-16 [email protected] Fix iOS reference in macOS Cocoapods error (flutter/flutter#148506)
2024-05-16 [email protected] Roll Flutter Engine from 6fa734d68688 to 1850def2ccff (1 revision) (flutter/flutter#148507)
2024-05-16 [email protected] Roll Flutter Engine from 460df6caef0e to 6fa734d68688 (4 revisions) (flutter/flutter#148500)
2024-05-16 [email protected] Enhanced enum features for `AnimationStatus` (flutter/flutter#147801)
2024-05-16 [email protected] [macOS] codesign native assets during embed (flutter/flutter#148310)

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
@nate-thegrate nate-thegrate mentioned this pull request May 17, 2024
Draft
@Piinks Piinks mentioned this pull request May 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees
No one assigned
Labels
a: animation Animation APIs autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nate-thegrate @goderbauer