Bugfix in the absorbTicker method#181706
Conversation
There was a problem hiding this comment.
Code Review
This pull request aims to fix a bug in the absorbTicker method where an assertion would incorrectly fail if resync was called on an AnimationController immediately after starting an animation. A test case is added to cover this scenario.
While the change correctly handles the reported case, the new assertion logic appears to be flawed. It would cause an assertion failure when trying to absorb a ticker that is already actively ticking, which is a valid use case. It also fails to detect a disposed ticker, which is the original purpose of the assertion. I've left a comment with a suggested correction to the assertion logic that should cover all cases correctly.
| assert(_animationId == null); | ||
| assert( | ||
| (originalTicker._future == null) == (originalTicker._startTime == null), | ||
| (originalTicker._future != null) || (originalTicker._startTime == null), |
There was a problem hiding this comment.
This makes sense to me, _future can be null (so the ticker is disposed and so _startTime must be null). But _startTime can be null even when _future is non-null.
There was a problem hiding this comment.
The logical difference before/after is that after, this case does not trigger the assertion:
originalTicker._future != null && originalTicker._startTime == null
That seems to make sense because that's not the case after dispose, it's the case when you create the Ticker and then pass it to absorbTicker before starting it, which seems to be exactly what @nate-thegrate describes in the issue.
| assert(_animationId == null); | ||
| assert( | ||
| (originalTicker._future == null) == (originalTicker._startTime == null), | ||
| (originalTicker._future != null) || (originalTicker._startTime == null), |
There was a problem hiding this comment.
The logical difference before/after is that after, this case does not trigger the assertion:
originalTicker._future != null && originalTicker._startTime == null
That seems to make sense because that's not the case after dispose, it's the case when you create the Ticker and then pass it to absorbTicker before starting it, which seems to be exactly what @nate-thegrate describes in the issue.
|
autosubmit label was removed for flutter/flutter/181706, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
Currently, `absorbTicker` assumes that between `_future` and `_startTime`, if one of them is null and the other is not then the ticker must have been disposed of. https://github.com/flutter/flutter/blob/9b6f15e67d181fcbc4e3ccc7387aa0da8a3d19e6/packages/flutter/lib/src/scheduler/ticker.dart#L329-L337 I discovered that starting an animation and then immediately calling `resync()` leads to a false positive, so this PR tweaks the `absorbTicker` method accordingly. <br> resolves flutter#181699 --------- Co-authored-by: Victor Sanni <victorsanniay@gmail.com>
Currently,
absorbTickerassumes that between_futureand_startTime, if one of them is null and the other is not then the ticker must have been disposed of.flutter/packages/flutter/lib/src/scheduler/ticker.dart
Lines 329 to 337 in 9b6f15e
I discovered that starting an animation and then immediately calling
resync()leads to a false positive, so this PR tweaks theabsorbTickermethod accordingly.resolves #181699