flutter icon indicating copy to clipboard operation
flutter copied to clipboard

Fix initialization of time in repeat on AnimationController

Open paldepind opened this issue 1 year ago • 2 comments

This PR fixes #142885.

The issue is that in _RepeatingSimulation the initial time is calculated as follows:

(initialValue / (max - min)) * (period.inMicroseconds / Duration.microsecondsPerSecond)

This calculation does not work in general. For instance, if max is 300, min is 100, and initialValue is 100 then initialValue / (max - min) is 1/2 when it should be 0

The current tests work by happenstance because the numbers used happen to work. To reveal the bug I've added some more tests similar to the existing ones but with different numbers.

A "side-effect" of the incorrect calculation is that if initialValue is 0, then the animation will always start from min no matter what. For instance, in one of the tests, an AnimationController with the value 0 is told to repeat between 0.5 and 1.0, and this starts the animation from 0.5. To preserve this behavior, and to more generally handle the case where the initial value is out of bounds, this PR clamps the initial value to be within the lower and upper bounds of the repetition.

Just for reference, this calculation was introduced at https://github.com/flutter/flutter/pull/25125.

Pre-launch Checklist

  • [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • [ ] I signed the [CLA].
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is [test-exempt].
  • [x] All existing and new tests are passing.

paldepind avatar Feb 04 '24 13:02 paldepind

Fly-by comment from triage: Could you take a look at the failing "Linux analyze" check and fix whatever it is complaining about? Thanks.

goderbauer avatar Feb 06 '24 23:02 goderbauer

@goderbauer I've fixed the lint error.

paldepind avatar Feb 12 '24 10:02 paldepind

You're welcome. Thanks for reviewing.

paldepind avatar Feb 20 '24 20:02 paldepind

auto label is removed for flutter/flutter/142887, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit[bot] avatar Feb 20 '24 20:02 auto-submit[bot]