Fix initialization of time in repeat on AnimationController #142887
Conversation
github-actions
bot
added
framework
|
Fly-by comment from triage: Could you take a look at the failing "Linux analyze" check and fix whatever it is complaining about? Thanks. |
paldepind
force-pushed
the
repeat-initial-value
branch
2 times, most recently
from
608d81b to
535259c
Compare
|
@goderbauer I've fixed the lint error. |
There was a problem hiding this comment.
Instead of changing an existing test case, could you add a new test case for this, please?
There was a problem hiding this comment.
Thanks for taking a look at the PR @goderbauer.
This existing test is supposed to test that min and max work as they should. However, it ends up only testing some very special cases and therefore doesn't really work as intended. I therefore thought it would be better to add more cases to the existing test, instead of adding a new test and leaving the insufficient test unchanged.
That being said if you still prefer a new test then I'll update the PR with that 👍
There was a problem hiding this comment.
We'd still want a test that tests .repeat when no lowerBound and upperBound is set (basically the old test). Therefore, I'd prefer a new test case for when lowerBound and upperBound are set.
There was a problem hiding this comment.
There is another test that tests repeat when lowerBound and upperBound is not set.
In this test, setting lowerBound and upperBound do not make a difference. The values are only used when min and max are not given to repeat and they are given in all the repeat invocations in this test. I've only set lowerBound and upperBound since there is an assert that max <= upperBound && min >= lowerBound which would be violated otherwise.
The changed test tests strictly more than it did before. Please consider if you don't think that's better practice than leaving a broken test in place. If not I'll create a new separate test.
There was a problem hiding this comment.
IMO, the test isn't broken. Doing what the test did before should still work after this change: you should still be able to call repeat with a max/min while the AnimationController has no lowerBound/upperBound configured. As far as I can tell, we lose test coverage for that case if we modify the test to give the AnimationController explicit bounds.
There was a problem hiding this comment.
I would say that the test is broken since it doesn't test what it claims to test. The description says: "calling repeat with specified min and max values makes the animation alternate between min and max values on each repeat", but it mistakenly only tests that in some corner cases that happened to work.
Doing what the test did before should still work after this change
Yes, absolutely. I've taken care both in the actual change and in the test to ensure that the existing behavior is preserved and still tested to the same extent. Here lowerBound and upperBound are only used in asserts so adding them doesn't affect what's being tested.
Anyway, I've made my case and I also understand that it can be easier to review a PR that doesn't modify existing tests. Let me know what to do and let's move the PR forward 👍
There was a problem hiding this comment.
I would say that the test is broken since it doesn't test what it claims to test.
My suggestion: Let's update the name of this test then so it matches what it really tests and leave it otherwise unchanged. Then, let's add a new test case for the behavior we want to test.
There was a problem hiding this comment.
Sounds good. I'll update the PR :)
paldepind
force-pushed
the
repeat-initial-value
branch
3 times, most recently
from
c9917f7 to
a5761f4
Compare
paldepind
force-pushed
the
repeat-initial-value
branch
from
a5761f4 to
115261c
Compare
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
LGTM
Thank you for the fix!
gspencergoog
added
the
autosubmit
|
You're welcome. Thanks for reviewing. |
auto-submit
bot
removed
the
autosubmit
|
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. |
goderbauer
added
the
autosubmit
Manual roll Flutter from 5129806 to efee280 (47 revisions) Manual roll requested by [email protected] flutter/flutter@5129806...efee280 2024-02-22 [email protected] Roll Flutter Engine from bf5c003085fd to 7eeb697687d5 (16 revisions) (flutter/flutter#143911) 2024-02-22 [email protected] Update PR template for dart fix (flutter/flutter#143879) 2024-02-22 [email protected] Re-use methods to calculate leading and trailing garbage in RenderSliverMultiBoxAdaptor (flutter/flutter#143884) 2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Make impeller goldens test blocking. (#143864)" (flutter/flutter#143896) 2024-02-21 [email protected] [Impeller] Make impeller goldens test blocking. (flutter/flutter#143864) 2024-02-21 [email protected] Disable color filter sepia test for Impeller. (flutter/flutter#143861) 2024-02-21 [email protected] Roll Flutter Engine from 52ffcaadea41 to bf5c003085fd (12 revisions) (flutter/flutter#143875) 2024-02-21 [email protected] Roll Flutter Engine from 4128895d79a1 to 52ffcaadea41 (1 revision) (flutter/flutter#143862) 2024-02-21 [email protected] Deprecate redundant itemExtent in RenderSliverFixedExtentBoxAdaptor methods (flutter/flutter#143412) 2024-02-21 [email protected] Add aab as alias for appbundle (flutter/flutter#143855) 2024-02-21 [email protected] Roll Flutter Engine from e16f43eeaaa4 to 4128895d79a1 (1 revision) (flutter/flutter#143856) 2024-02-21 [email protected] Roll Packages from 8bba41b to 48048f6 (2 revisions) (flutter/flutter#143853) 2024-02-21 [email protected] Update `hourMinuteTextStyle` defaults for Material 3 Time Picker (flutter/flutter#143749) 2024-02-21 [email protected] Roll Flutter Engine from 93063f61943a to e16f43eeaaa4 (2 revisions) (flutter/flutter#143827) 2024-02-21 [email protected] Roll Flutter Engine from ed49634486e9 to 93063f61943a (1 revision) (flutter/flutter#143826) 2024-02-21 [email protected] `CalendarDatePicker` doesn't announce selected date on desktop (flutter/flutter#143583) 2024-02-21 [email protected] Roll Flutter Engine from 9100d326475a to ed49634486e9 (2 revisions) (flutter/flutter#143824) 2024-02-21 [email protected] Add `timeSelectorSeparatorColor` and `timeSelectorSeparatorTextStyle` for Material 3 Time Picker (flutter/flutter#143739) 2024-02-21 [email protected] Roll Flutter Engine from efc69946cb1e to 9100d326475a (2 revisions) (flutter/flutter#143820) 2024-02-21 [email protected] Roll Flutter Engine from 700250436e3f to efc69946cb1e (2 revisions) (flutter/flutter#143816) 2024-02-21 [email protected] Roll Flutter Engine from 3557277c575c to 700250436e3f (1 revision) (flutter/flutter#143814) 2024-02-21 [email protected] more fixes to unstable impeller goldens. (flutter/flutter#143811) 2024-02-21 [email protected] Roll Flutter Engine from cb12a8cc97a1 to 3557277c575c (2 revisions) (flutter/flutter#143807) 2024-02-21 [email protected] [flutter_tools] enable wasm compile on beta channel (flutter/flutter#143779) 2024-02-21 [email protected] Fix initialization of time in repeat on AnimationController (flutter/flutter#142887) 2024-02-21 [email protected] Disable debug banner to stabilize impeller goldens. (flutter/flutter#143794) 2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (#143281)" (flutter/flutter#143801) 2024-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add UI Benchmarks (#143542)" (flutter/flutter#143798) 2024-02-20 [email protected] Avoid applying partial dartfixes on CI (flutter/flutter#143551) 2024-02-20 [email protected] Add UI Benchmarks (flutter/flutter#143542) 2024-02-20 [email protected] Roll Flutter Engine from 1ae2c10e8071 to cb12a8cc97a1 (1 revision) (flutter/flutter#143791) 2024-02-20 [email protected] Implement `_suspendedNode` fix (flutter/flutter#143556) 2024-02-20 [email protected] Change `ItemExtentBuilder`'s return value nullable (flutter/flutter#142428) 2024-02-20 [email protected] Roll Flutter Engine from 27828054f07a to 1ae2c10e8071 (6 revisions) (flutter/flutter#143783) 2024-02-20 [email protected] Roll Flutter Engine from e16a260265ad to 27828054f07a (1 revision) (flutter/flutter#143769) 2024-02-20 [email protected] [gold] Always provide host ABI to gold config (flutter/flutter#143621) 2024-02-20 [email protected] instead of exiting the tool, print a warning when using --flavor with an incompatible device (flutter/flutter#143735) 2024-02-20 [email protected] Implementing `switch` expressions: everything in `flutter/lib/src/` (flutter/flutter#143634) 2024-02-20 [email protected] Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (flutter/flutter#143281) 2024-02-20 [email protected] Delete local.properties that shouldn't have been pushed (flutter/flutter#143774) 2024-02-20 [email protected] Clean leaks. (flutter/flutter#142818) 2024-02-20 [email protected] Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#138521) 2024-02-20 [email protected] Explain when and why to use CrossAxisAlignment.baseline (flutter/flutter#143632) 2024-02-20 [email protected] Roll Flutter Engine from a41da3701923 to e16a260265ad (2 revisions) (flutter/flutter#143763) ...
) Manual roll Flutter from 5129806 to efee280 (47 revisions) Manual roll requested by [email protected] flutter/flutter@5129806...efee280 2024-02-22 [email protected] Roll Flutter Engine from bf5c003085fd to 7eeb697687d5 (16 revisions) (flutter/flutter#143911) 2024-02-22 [email protected] Update PR template for dart fix (flutter/flutter#143879) 2024-02-22 [email protected] Re-use methods to calculate leading and trailing garbage in RenderSliverMultiBoxAdaptor (flutter/flutter#143884) 2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Make impeller goldens test blocking. (#143864)" (flutter/flutter#143896) 2024-02-21 [email protected] [Impeller] Make impeller goldens test blocking. (flutter/flutter#143864) 2024-02-21 [email protected] Disable color filter sepia test for Impeller. (flutter/flutter#143861) 2024-02-21 [email protected] Roll Flutter Engine from 52ffcaadea41 to bf5c003085fd (12 revisions) (flutter/flutter#143875) 2024-02-21 [email protected] Roll Flutter Engine from 4128895d79a1 to 52ffcaadea41 (1 revision) (flutter/flutter#143862) 2024-02-21 [email protected] Deprecate redundant itemExtent in RenderSliverFixedExtentBoxAdaptor methods (flutter/flutter#143412) 2024-02-21 [email protected] Add aab as alias for appbundle (flutter/flutter#143855) 2024-02-21 [email protected] Roll Flutter Engine from e16f43eeaaa4 to 4128895d79a1 (1 revision) (flutter/flutter#143856) 2024-02-21 [email protected] Roll Packages from 8bba41b to 48048f6 (2 revisions) (flutter/flutter#143853) 2024-02-21 [email protected] Update `hourMinuteTextStyle` defaults for Material 3 Time Picker (flutter/flutter#143749) 2024-02-21 [email protected] Roll Flutter Engine from 93063f61943a to e16f43eeaaa4 (2 revisions) (flutter/flutter#143827) 2024-02-21 [email protected] Roll Flutter Engine from ed49634486e9 to 93063f61943a (1 revision) (flutter/flutter#143826) 2024-02-21 [email protected] `CalendarDatePicker` doesn't announce selected date on desktop (flutter/flutter#143583) 2024-02-21 [email protected] Roll Flutter Engine from 9100d326475a to ed49634486e9 (2 revisions) (flutter/flutter#143824) 2024-02-21 [email protected] Add `timeSelectorSeparatorColor` and `timeSelectorSeparatorTextStyle` for Material 3 Time Picker (flutter/flutter#143739) 2024-02-21 [email protected] Roll Flutter Engine from efc69946cb1e to 9100d326475a (2 revisions) (flutter/flutter#143820) 2024-02-21 [email protected] Roll Flutter Engine from 700250436e3f to efc69946cb1e (2 revisions) (flutter/flutter#143816) 2024-02-21 [email protected] Roll Flutter Engine from 3557277c575c to 700250436e3f (1 revision) (flutter/flutter#143814) 2024-02-21 [email protected] more fixes to unstable impeller goldens. (flutter/flutter#143811) 2024-02-21 [email protected] Roll Flutter Engine from cb12a8cc97a1 to 3557277c575c (2 revisions) (flutter/flutter#143807) 2024-02-21 [email protected] [flutter_tools] enable wasm compile on beta channel (flutter/flutter#143779) 2024-02-21 [email protected] Fix initialization of time in repeat on AnimationController (flutter/flutter#142887) 2024-02-21 [email protected] Disable debug banner to stabilize impeller goldens. (flutter/flutter#143794) 2024-02-21 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (#143281)" (flutter/flutter#143801) 2024-02-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add UI Benchmarks (#143542)" (flutter/flutter#143798) 2024-02-20 [email protected] Avoid applying partial dartfixes on CI (flutter/flutter#143551) 2024-02-20 [email protected] Add UI Benchmarks (flutter/flutter#143542) 2024-02-20 [email protected] Roll Flutter Engine from 1ae2c10e8071 to cb12a8cc97a1 (1 revision) (flutter/flutter#143791) 2024-02-20 [email protected] Implement `_suspendedNode` fix (flutter/flutter#143556) 2024-02-20 [email protected] Change `ItemExtentBuilder`'s return value nullable (flutter/flutter#142428) 2024-02-20 [email protected] Roll Flutter Engine from 27828054f07a to 1ae2c10e8071 (6 revisions) (flutter/flutter#143783) 2024-02-20 [email protected] Roll Flutter Engine from e16a260265ad to 27828054f07a (1 revision) (flutter/flutter#143769) 2024-02-20 [email protected] [gold] Always provide host ABI to gold config (flutter/flutter#143621) 2024-02-20 [email protected] instead of exiting the tool, print a warning when using --flavor with an incompatible device (flutter/flutter#143735) 2024-02-20 [email protected] Implementing `switch` expressions: everything in `flutter/lib/src/` (flutter/flutter#143634) 2024-02-20 [email protected] Changing `TextPainter.getOffsetForCaret` implementation to remove the logarithmic search (flutter/flutter#143281) 2024-02-20 [email protected] Delete local.properties that shouldn't have been pushed (flutter/flutter#143774) 2024-02-20 [email protected] Clean leaks. (flutter/flutter#142818) 2024-02-20 [email protected] Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#138521) 2024-02-20 [email protected] Explain when and why to use CrossAxisAlignment.baseline (flutter/flutter#143632) 2024-02-20 [email protected] Roll Flutter Engine from a41da3701923 to e16a260265ad (2 revisions) (flutter/flutter#143763) ...
3 participants
This PR fixes #142885.
The issue is that in
_RepeatingSimulationthe initial time is calculated as follows:This calculation does not work in general. For instance, if
maxis 300,minis 100, andinitialValueis 100 theninitialValue / (max - min)is 1/2 when it should be 0The 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
initialValueis 0, then the animation will always start fromminno matter what. For instance, in one of the tests, anAnimationControllerwith the value 0 is told torepeatbetween 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 #25125.
Pre-launch Checklist
///).