From 59c912498d0c6c0e617874e013ea2f78a4cd7cc2 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Sat, 11 Jul 2026 12:11:37 -0400 Subject: [PATCH 1/2] fix(mix): make animation transitions target-correct Repair curve, spring, phase, and keyframe driver transitions as one lifecycle-contract change so they always render the requested target. - Curve delay holds the currently displayed spec (tween begin) instead of snapping back to the driver's immutable initialSpec on interruption. - Removing a phase/keyframe config disposes the old driver and exposes the new target on the same update; only curve/spring are reused for an animate-out, centralized in _outgoingConfigFor. - Phase delay placement uses the destination transition's config for both the delay check and its weight (incl. last -> first). - Phase completion fires exactly once via PhaseAnimationConfig.onEnd, registered once on the controller; the leaking curveConfigs.last.onEnd path is removed and onEnd is exposed on phaseAnimation(). - Phase/keyframe configs validate inputs at construction (empty phases, count mismatch, negative durations/delays, zero-duration loops, duplicate track ids). Adds 41 regression tests (full old->new driver matrix, interrupted delays, listener accumulation, delay boundaries, validation) that fail on origin/main and pass with the fix. --- .../lib/src/animation/animation_config.dart | 57 +++- .../animation/style_animation_builder.dart | 18 +- .../src/animation/style_animation_driver.dart | 33 +- .../style/mixins/animation_style_mixin.dart | 6 + .../src/animation/animation_config_test.dart | 149 +++++++++ .../style_animation_builder_test.dart | 304 ++++++++++++++++++ .../style_animation_driver_test.dart | 155 +++++++++ 7 files changed, 707 insertions(+), 15 deletions(-) diff --git a/packages/mix/lib/src/animation/animation_config.dart b/packages/mix/lib/src/animation/animation_config.dart index a70f54e027..1efcaaa19b 100644 --- a/packages/mix/lib/src/animation/animation_config.dart +++ b/packages/mix/lib/src/animation/animation_config.dart @@ -847,12 +847,41 @@ class PhaseAnimationConfig, U extends Style> final Listenable? trigger; final VoidCallback? onEnd; - const PhaseAnimationConfig({ + // Not `const`: the asserts validate the supplied lists at construction time, + // which is the earliest point the invariants are known and well before any + // controller runs. These configs are never built in a const context. + PhaseAnimationConfig({ required this.styles, required this.curveConfigs, required this.trigger, this.onEnd, - }); + }) : assert( + styles.length == curveConfigs.length, + 'PhaseAnimationConfig requires one CurveAnimationConfig per phase ' + '(got ${styles.length} styles and ${curveConfigs.length} configs).', + ), + assert( + styles.isNotEmpty, + 'PhaseAnimationConfig requires at least one phase.', + ), + assert( + // Negative durations/delays produce out-of-order or backward tween + // weights and break TweenSequence timing. + curveConfigs.every((c) => c.duration >= .zero && c.delay >= .zero), + 'PhaseAnimationConfig durations and delays must be non-negative.', + ), + assert( + // A looping (untriggered) timeline is driven by `controller.repeat()`, + // which requires a positive period. + trigger != null || + curveConfigs.fold( + .zero, + (total, c) => total + c.totalDuration, + ) > + .zero, + 'A looping PhaseAnimationConfig (trigger == null) must have a positive ' + 'total duration.', + ); bool get isLooping => trigger == null; @@ -1042,7 +1071,11 @@ class KeyframeTrack with Equatable { this.segments, { required this.initial, TweenBuilder? tweenBuilder, - }) : tweenBuilder = tweenBuilder ?? Tween.new; + }) : tweenBuilder = tweenBuilder ?? Tween.new, + assert( + segments.every((s) => s.duration >= .zero), + 'KeyframeTrack "$id" segment durations must be non-negative.', + ); Duration get totalDuration { return segments.fold( @@ -1122,12 +1155,26 @@ class KeyframeAnimationConfig> extends AnimationConfig final KeyframeStyleBuilder> styleBuilder; final Style initialStyle; - const KeyframeAnimationConfig({ + // Not `const`: the asserts validate the timeline at construction time, the + // earliest point the invariants are known and before any controller runs. + KeyframeAnimationConfig({ required this.trigger, required this.timeline, required this.styleBuilder, required this.initialStyle, - }); + }) : assert( + // Track ids are the lookup keys for `KeyframeAnimationResult.get`; a + // duplicate silently shadows a track. + timeline.map((t) => t.id).toSet().length == timeline.length, + 'KeyframeAnimationConfig requires unique track ids; found a duplicate.', + ), + assert( + // A looping (untriggered) timeline is driven by `controller.repeat()`, + // which requires a positive period. + trigger != null || timeline.any((t) => t.totalDuration > .zero), + 'A looping KeyframeAnimationConfig (trigger == null) must have at least ' + 'one track with a positive duration.', + ); bool get isLooping => trigger == null; diff --git a/packages/mix/lib/src/animation/style_animation_builder.dart b/packages/mix/lib/src/animation/style_animation_builder.dart index 5a833d5ccf..53f70f3a8f 100644 --- a/packages/mix/lib/src/animation/style_animation_builder.dart +++ b/packages/mix/lib/src/animation/style_animation_builder.dart @@ -78,6 +78,22 @@ class _StyleAnimationBuilderState> }; } + /// Chooses the config that drives an animate-out transition when the new spec + /// removes its animation (`config == null`). + /// + /// Only implicit drivers (curve, spring) retarget toward the new spec through + /// [StyleAnimationDriver.didUpdateSpec], so only those may be reused. Phase and + /// keyframe drivers replay their own sequence and never retarget; reusing an + /// outgoing phase/keyframe config would keep the old animation running instead + /// of showing the new target, so return `null` to let a [NoAnimationDriver] + /// expose the target immediately on the same update. + AnimationConfig? _outgoingConfigFor(AnimationConfig? oldConfig) { + return switch (oldConfig) { + CurveAnimationConfig() || SpringAnimationConfig() => oldConfig, + PhaseAnimationConfig() || KeyframeAnimationConfig() || null => null, + }; + } + @override void dispose() { animationDriver.dispose(); @@ -96,7 +112,7 @@ class _StyleAnimationBuilderState> } else { animationDriver.dispose(); animationDriver = _createAnimationDriver( - config: config ?? oldConfig, + config: config ?? _outgoingConfigFor(oldConfig), initialSpec: oldWidget.spec, ); } diff --git a/packages/mix/lib/src/animation/style_animation_driver.dart b/packages/mix/lib/src/animation/style_animation_driver.dart index 578e43054b..88afde16a5 100644 --- a/packages/mix/lib/src/animation/style_animation_driver.dart +++ b/packages/mix/lib/src/animation/style_animation_driver.dart @@ -158,7 +158,11 @@ class CurveAnimationDriver> TweenSequence?> _createTweenSequence() => .new([ if (config.delay > .zero) TweenSequenceItem( - tween: ConstantTween(_initialSpec), + // Hold the currently displayed value during the delay. `_animateTo` + // sets `_tween.begin` to the current interpolated spec just before + // this runs, so an interrupted transition holds where it is visually + // instead of snapping back to the driver's original `_initialSpec`. + tween: ConstantTween(_tween.begin), weight: config.delay.inMilliseconds.toDouble(), ), TweenSequenceItem( @@ -239,6 +243,11 @@ class PhaseAnimationDriver> extends StyleAnimationDriver { required this.context, }) { _setUpAnimation(); + // Register the completion listener on the controller once, for the driver's + // whole lifetime. `_setUpAnimation` re-drives `_animation` on every + // `updateDriver`, so registering the listener there (as before) leaked a new + // listener per update and fired `onEnd` once per accumulated listener. + controller.addStatusListener(_onStatusChanged); if (config.isLooping) { _startLoopingAnimation(); } @@ -255,14 +264,15 @@ class PhaseAnimationDriver> extends StyleAnimationDriver { _animation = controller.drive(_PhasedSpecTween(_tweenSequence)); config.trigger?.addListener(_onTriggerChanged); + } - // Add status listener for onEnd callback - if (config.curveConfigs.last.onEnd != null) { - _animation.addStatusListener((status) { - if (status == .completed) { - config.curveConfigs.last.onEnd!(); - } - }); + // Phase completion is owned by `PhaseAnimationConfig.onEnd`. The per-phase + // `CurveAnimationConfig.onEnd` values are not a completion contract; they only + // carry timing/curve data for each transition. Reads the current `config` so + // an `updateDriver` that swaps the callback is honored. + void _onStatusChanged(AnimationStatus status) { + if (status == .completed) { + config.onEnd?.call(); } } @@ -279,7 +289,11 @@ class PhaseAnimationDriver> extends StyleAnimationDriver { final currentIndex = i % specs.length; final nextIndex = (i + 1) % specs.length; - if (configs[currentIndex].delay > .zero) { + // The transition into `nextIndex` is owned by `configs[nextIndex]`: its + // delay, duration, and curve all describe arriving at `specs[nextIndex]`. + // Use `nextIndex` for both the delay check and its weight so placement and + // duration always agree (previously the check read `currentIndex.delay`). + if (configs[nextIndex].delay > .zero) { items.add( TweenSequenceItem( tween: ConstantTween(specs[currentIndex]), @@ -320,6 +334,7 @@ class PhaseAnimationDriver> extends StyleAnimationDriver { @override void dispose() { config.trigger?.removeListener(_onTriggerChanged); + controller.removeStatusListener(_onStatusChanged); controller.stop(); super.dispose(); } diff --git a/packages/mix/lib/src/style/mixins/animation_style_mixin.dart b/packages/mix/lib/src/style/mixins/animation_style_mixin.dart index 256d9f28db..acb2a9169e 100644 --- a/packages/mix/lib/src/style/mixins/animation_style_mixin.dart +++ b/packages/mix/lib/src/style/mixins/animation_style_mixin.dart @@ -25,11 +25,16 @@ mixin AnimationStyleMixin, S extends Spec> on Style { } /// Creates a phase animation. It will animate through the given phases. + /// + /// [onEnd] fires once each time a triggered sequence completes; it is the + /// single phase-completion contract. The per-phase [CurveAnimationConfig]s + /// returned by [configBuilder] only carry timing/curve data. T phaseAnimation

({ Listenable? trigger, required List

phases, required T Function(P phase, T style) styleBuilder, required CurveAnimationConfig Function(P phase) configBuilder, + VoidCallback? onEnd, }) { final styles = []; final configs = []; @@ -44,6 +49,7 @@ mixin AnimationStyleMixin, S extends Spec> on Style { styles: styles, curveConfigs: configs, trigger: trigger, + onEnd: onEnd, ), ); } diff --git a/packages/mix/test/src/animation/animation_config_test.dart b/packages/mix/test/src/animation/animation_config_test.dart index d24b18c00b..52f54c9949 100644 --- a/packages/mix/test/src/animation/animation_config_test.dart +++ b/packages/mix/test/src/animation/animation_config_test.dart @@ -655,4 +655,153 @@ void main() { trigger2.dispose(); }); }); + + group('input validation', () { + group('PhaseAnimationConfig', () { + test('empty phase list throws', () { + expect( + () => PhaseAnimationConfig, MockStyle>( + styles: const [], + curveConfigs: const [], + trigger: null, + ), + throwsA(isA()), + ); + }); + + test('mismatched styles and curveConfigs throws', () { + expect( + () => PhaseAnimationConfig, MockStyle>( + styles: [MockStyle(0.0), MockStyle(1.0)], + curveConfigs: const [ + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + ], + trigger: null, + ), + throwsA(isA()), + ); + }); + + test('negative duration throws', () { + expect( + () => PhaseAnimationConfig, MockStyle>( + styles: [MockStyle(0.0)], + curveConfigs: const [ + CurveAnimationConfig( + duration: Duration(milliseconds: -100), + curve: Curves.linear, + ), + ], + trigger: null, + ), + throwsA(isA()), + ); + }); + + test('looping config with zero total duration throws', () { + expect( + () => PhaseAnimationConfig, MockStyle>( + styles: [MockStyle(0.0)], + curveConfigs: const [ + CurveAnimationConfig( + duration: Duration.zero, + curve: Curves.linear, + ), + ], + trigger: null, + ), + throwsA(isA()), + ); + }); + }); + + group('KeyframeTrack', () { + test('negative segment duration throws', () { + expect( + () => KeyframeTrack('t', const [ + Keyframe.linear(1.0, Duration(milliseconds: -50)), + ], initial: 0.0), + throwsA(isA()), + ); + }); + }); + + group('KeyframeAnimationConfig', () { + test('duplicate track ids throws', () { + final trigger = ValueNotifier(false); + addTearDown(trigger.dispose); + + expect( + () => KeyframeAnimationConfig>( + trigger: trigger, + timeline: [ + KeyframeTrack('dup', const [ + Keyframe.linear(1.0, Duration(milliseconds: 100)), + ], initial: 0.0), + KeyframeTrack('dup', const [ + Keyframe.linear(1.0, Duration(milliseconds: 100)), + ], initial: 0.0), + ], + styleBuilder: (result, style) => style, + initialStyle: MockStyle(0.0), + ), + throwsA(isA()), + ); + }); + + test('looping timeline with no positive duration throws', () { + expect( + () => KeyframeAnimationConfig>( + trigger: null, + timeline: [ + KeyframeTrack('t', const [ + Keyframe.linear(1.0, Duration.zero), + ], initial: 0.0), + ], + styleBuilder: (result, style) => style, + initialStyle: MockStyle(0.0), + ), + throwsA(isA()), + ); + }); + + test('triggered empty timeline is allowed', () { + final trigger = ValueNotifier(false); + addTearDown(trigger.dispose); + + expect( + () => KeyframeAnimationConfig>( + trigger: trigger, + timeline: const [], + styleBuilder: (result, style) => style, + initialStyle: MockStyle(0.0), + ), + returnsNormally, + ); + }); + }); + }); + + group('phaseAnimation onEnd contract', () { + test('fluent phaseAnimation forwards onEnd to PhaseAnimationConfig', () { + var called = false; + final style = BoxStyler().phaseAnimation( + phases: const [0, 1], + styleBuilder: (phase, s) => s, + configBuilder: (phase) => const CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + onEnd: () => called = true, + ); + + final config = style.$animation! as PhaseAnimationConfig; + expect(config.onEnd, isNotNull); + config.onEnd!(); + expect(called, isTrue); + }); + }); } diff --git a/packages/mix/test/src/animation/style_animation_builder_test.dart b/packages/mix/test/src/animation/style_animation_builder_test.dart index e22cbad7e2..cee12075c0 100644 --- a/packages/mix/test/src/animation/style_animation_builder_test.dart +++ b/packages/mix/test/src/animation/style_animation_builder_test.dart @@ -2,6 +2,8 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:mix/mix.dart'; +import '../../helpers/testing_utils.dart'; + Widget styleAnimationBuilderCapturingColor( StyleSpec spec, void Function(Color?) onColor, @@ -407,6 +409,308 @@ void main() { expect(find.byKey(const Key('test-container')), findsOneWidget); }); }); + + group('interrupted implicit animation (regression)', () { + testWidgets( + 'a delayed transition holds the interrupted value instead of snapping to ' + 'the start', + (tester) async { + Color? captured; + Widget build(StyleSpec spec) => + styleAnimationBuilderCapturingColor(spec, (c) => captured = c); + + const red = StyleSpec( + spec: TestSpec(color: Colors.red), + animation: CurveAnimationConfig( + duration: Duration(milliseconds: 200), + curve: Curves.linear, + ), + ); + const blue = StyleSpec( + spec: TestSpec(color: Colors.blue), + animation: CurveAnimationConfig( + duration: Duration(milliseconds: 200), + curve: Curves.linear, + ), + ); + const greenDelayed = StyleSpec( + spec: TestSpec(color: Colors.green), + animation: CurveAnimationConfig( + duration: Duration(milliseconds: 200), + curve: Curves.linear, + delay: Duration(milliseconds: 100), + ), + ); + + await tester.pumpWidget(build(red)); + await tester.pumpAndSettle(); + expect(captured, Colors.red); + + // Begin red -> blue and stop halfway. + await tester.pumpWidget(build(blue)); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 100)); + final interrupted = captured; + expect(interrupted, isNot(Colors.red)); + expect(interrupted, isNot(Colors.blue)); + + // Interrupt with a delayed transition to green. During the delay the + // value must hold where it was, not jump back to the driver's original + // red (`_initialSpec`). + await tester.pumpWidget(build(greenDelayed)); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 50)); + expect( + captured, + interrupted, + reason: 'the delay must hold the interrupted value', + ); + expect(captured, isNot(Colors.red)); + + // Once the delay elapses it animates to the new target. + await tester.pumpAndSettle(); + expect(captured, Colors.green); + }, + ); + }); + + group('implicit animation removal (regression)', () { + testWidgets( + 'spring removal reuses the previous config to animate to the new target', + (tester) async { + Color? captured; + Widget build(StyleSpec spec) => + styleAnimationBuilderCapturingColor(spec, (c) => captured = c); + + final withSpring = StyleSpec( + spec: const TestSpec(color: Colors.red), + animation: SpringAnimationConfig.standard(), + ); + const withoutAnimation = StyleSpec( + spec: TestSpec(color: Colors.blue), + animation: null, + ); + + await tester.pumpWidget(build(withSpring)); + await tester.pumpAndSettle(); + expect(captured, Colors.red); + + await tester.pumpWidget(build(withoutAnimation)); + await tester.pump(); + // The spring should animate out rather than jump straight to blue. + expect( + captured, + isNot(Colors.blue), + reason: 'spring removal should animate, not jump to the target', + ); + + await tester.pumpAndSettle(); + // A spring settles within its tolerance rather than exactly on the end + // control value, so assert channel closeness to the target instead of + // exact equality. + final settled = captured!; + expect(settled.r, closeTo(Colors.blue.r, 0.02)); + expect(settled.g, closeTo(Colors.blue.g, 0.02)); + expect(settled.b, closeTo(Colors.blue.b, 0.02)); + }, + ); + }); + + group('phase and keyframe removal (regression)', () { + testWidgets( + 'removing a looping phase animation shows the new target immediately and ' + 'stops the loop', + (tester) async { + double? captured; + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(1.0, _loopingPhaseConfig()), (v) { + captured = v; + }), + ); + await tester.pump(const Duration(milliseconds: 50)); + expect(tester.hasRunningAnimations, isTrue); + + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(42.0, null), (v) { + captured = v; + }), + ); + await tester.pump(); + + expect(captured, 42.0); + expect(tester.hasRunningAnimations, isFalse); + }, + ); + + testWidgets( + 'removing a looping keyframe animation shows the new target immediately ' + 'and stops the loop', + (tester) async { + double? captured; + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(1.0, _loopingKeyframeConfig()), (v) { + captured = v; + }), + ); + await tester.pump(const Duration(milliseconds: 50)); + expect(tester.hasRunningAnimations, isTrue); + + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(42.0, null), (v) { + captured = v; + }), + ); + await tester.pump(); + + expect(captured, 42.0); + expect(tester.hasRunningAnimations, isFalse); + }, + ); + }); + + group('driver transition matrix (regression)', () { + final families = { + 'null': () => null, + 'curve': _curveConfig, + 'spring': SpringAnimationConfig.standard, + 'phase': _loopingPhaseConfig, + 'keyframe': _loopingKeyframeConfig, + }; + // Configs that settle to the widget's target value (they retarget via + // `didUpdateSpec`), so we can assert the final rendered spec == target. + const settlingNew = ['null', 'curve', 'spring']; + // Looping configs never settle; we only assert the new loop took over. + const loopingNew = ['phase', 'keyframe']; + + for (final oldEntry in families.entries) { + for (final newKey in settlingNew) { + testWidgets( + '${oldEntry.key} -> $newKey disposes old and reaches target', + (tester) async { + double? captured; + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(1.0, oldEntry.value()), (v) { + captured = v; + }), + ); + await tester.pump(const Duration(milliseconds: 20)); + + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(2.0, families[newKey]!()), (v) { + captured = v; + }), + ); + await tester.pumpAndSettle(); + + expect(tester.takeException(), isNull); + expect( + captured, + 2.0, + reason: '${oldEntry.key} -> $newKey should render the new target', + ); + expect( + tester.hasRunningAnimations, + isFalse, + reason: 'the old ${oldEntry.key} ticker should be disposed', + ); + }, + ); + } + + for (final newKey in loopingNew) { + testWidgets( + '${oldEntry.key} -> $newKey disposes old and starts new loop', + (tester) async { + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(1.0, oldEntry.value()), (_) {}), + ); + await tester.pump(const Duration(milliseconds: 20)); + + await tester.pumpWidget( + _mockAnimationApp(_mockSpec(2.0, families[newKey]!()), (_) {}), + ); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 20)); + + expect(tester.takeException(), isNull); + expect( + tester.hasRunningAnimations, + isTrue, + reason: '${oldEntry.key} -> $newKey should run the new loop', + ); + + // Unmount so the repeating controller is disposed before teardown. + await tester.pumpWidget(const SizedBox()); + }, + ); + } + } + }); +} + +// Builds a MaterialApp hosting a StyleAnimationBuilder over a MockSpec and +// reports the resolved value on every rebuild. +Widget _mockAnimationApp( + StyleSpec> spec, + void Function(double?) onValue, +) { + return MaterialApp( + home: StyleAnimationBuilder>( + spec: spec, + builder: (context, resolved) { + onValue(resolved.spec.resolvedValue); + + return const SizedBox(); + }, + ), + ); +} + +StyleSpec> _mockSpec( + double value, + AnimationConfig? animation, +) { + return StyleSpec>( + spec: MockSpec(resolvedValue: value), + animation: animation, + ); +} + +AnimationConfig _curveConfig() => const CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, +); + +// A looping (untriggered) phase animation cycling between two phases. +AnimationConfig _loopingPhaseConfig() { + return PhaseAnimationConfig, MockStyle>( + styles: [MockStyle(0.0), MockStyle(1.0)], + curveConfigs: const [ + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + ], + trigger: null, + ); +} + +// A looping (untriggered) keyframe animation. +AnimationConfig _loopingKeyframeConfig() { + return KeyframeAnimationConfig>( + trigger: null, + timeline: [ + KeyframeTrack('value', const [ + Keyframe.linear(1.0, Duration(milliseconds: 100)), + ], initial: 0.0), + ], + styleBuilder: (result, style) => MockStyle(result.get('value')), + initialStyle: MockStyle(0.0), + ); } // Test helpers diff --git a/packages/mix/test/src/animation/style_animation_driver_test.dart b/packages/mix/test/src/animation/style_animation_driver_test.dart index f7263e9ced..95521f3b32 100644 --- a/packages/mix/test/src/animation/style_animation_driver_test.dart +++ b/packages/mix/test/src/animation/style_animation_driver_test.dart @@ -1037,4 +1037,159 @@ void main() { expect(driver.animation.value?.spec.resolvedValue, 1.0); }); }); + + group('PhaseAnimationDriver lifecycle (regression)', () { + PhaseAnimationDriver> makeDriver( + PhaseAnimationConfig, MockStyle> config, + ) { + return PhaseAnimationDriver>( + vsync: const TestVSync(), + config: config, + initialSpec: MockSpec(resolvedValue: 0.0).toStyleSpec(), + context: MockBuildContext(), + ); + } + + testWidgets( + 'delay belongs to the destination transition, including last -> first', + (tester) async { + final trigger = ValueNotifier(false); + final config = PhaseAnimationConfig, MockStyle>( + styles: [MockStyle(10.0), MockStyle(20.0)], + curveConfigs: const [ + // config[0]: no delay; owns the 1 -> 0 (last -> first) transition. + CurveAnimationConfig( + duration: Duration(milliseconds: 80), + curve: Curves.linear, + ), + // config[1]: 40ms delay; owns the 0 -> 1 transition. + CurveAnimationConfig( + duration: Duration(milliseconds: 80), + curve: Curves.linear, + delay: Duration(milliseconds: 40), + ), + ], + trigger: trigger, + ); + final driver = makeDriver(config); + addTearDown(() { + trigger.dispose(); + driver.dispose(); + }); + + double? valueNow() => driver.animation.value?.spec.resolvedValue; + + trigger.value = true; + await tester.pump(); // forward(from: 0) begins + + // [0, 40): the 0 -> 1 transition's 40ms delay holds phase 0 (10). + await tester.pump(const Duration(milliseconds: 20)); + expect(valueNow(), 10.0); + + // [40, 120): transition to phase 1 (20). + await tester.pump(const Duration(milliseconds: 40)); // t=60 + expect(valueNow(), 20.0); + + // [120, 200): last -> first transition (config[0], no delay) to phase 0. + await tester.pump(const Duration(milliseconds: 80)); // t=140 + expect(valueNow(), 10.0); + + await tester.pumpAndSettle(); + }, + ); + + testWidgets( + 'PhaseAnimationConfig.onEnd fires exactly once per run despite repeated ' + 'updates', + (tester) async { + var endCount = 0; + final trigger = ValueNotifier(false); + PhaseAnimationConfig, MockStyle> makeConfig() { + return PhaseAnimationConfig( + styles: [MockStyle(0.0), MockStyle(1.0)], + curveConfigs: const [ + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + ], + trigger: trigger, + onEnd: () => endCount++, + ); + } + + final driver = makeDriver(makeConfig()); + addTearDown(() { + trigger.dispose(); + driver.dispose(); + }); + + // Each updateDriver previously stacked another status listener, so a + // single completed run fired onEnd once per accumulated listener. + driver.updateDriver(makeConfig()); + driver.updateDriver(makeConfig()); + driver.updateDriver(makeConfig()); + + trigger.value = true; + await tester.pumpAndSettle(); + + expect(endCount, 1); + }, + ); + + testWidgets( + 'replacing the trigger and disposing remove the old trigger listeners', + (tester) async { + final triggerA = ValueNotifier(false); + final triggerB = ValueNotifier(false); + PhaseAnimationConfig, MockStyle> configFor( + ValueNotifier trigger, + ) { + return PhaseAnimationConfig( + styles: [MockStyle(0.0), MockStyle(1.0)], + curveConfigs: const [ + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + ], + trigger: trigger, + ); + } + + final driver = makeDriver(configFor(triggerA)); + addTearDown(() { + triggerA.dispose(); + triggerB.dispose(); + }); + + driver.updateDriver(configFor(triggerB)); + await tester.pump(); + + // The replaced trigger no longer drives the animation. + triggerA.value = true; + await tester.pump(); + expect(driver.animation.isAnimating, isFalse); + + // The current trigger does. + triggerB.value = true; + await tester.pump(); + expect(driver.animation.isAnimating, isTrue); + await tester.pumpAndSettle(); + + // After disposal, toggling the trigger must not reach the disposed + // controller. + driver.dispose(); + expect(() => triggerB.value = false, returnsNormally); + }, + ); + }); } From 349e613b2dd0b15159dd631d71f512ac435f8590 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Sat, 11 Jul 2026 12:51:08 -0400 Subject: [PATCH 2/2] refactor(mix): keep animation configs const, validate in drivers Restore `const` on PhaseAnimationConfig and KeyframeAnimationConfig by moving their invariant checks into a `validate()` method that the phase/keyframe drivers invoke in both their constructor and updateDriver. Invalid configs still fail before controller execution, while downstream const construction keeps compiling. KeyframeTrack keeps its own segment assert (it was never const). Adds driver-level tests proving each driver rejects an invalid config before running. Addresses code-review feedback on const compatibility. --- .../lib/src/animation/animation_config.dart | 106 ++++++++++-------- .../src/animation/style_animation_driver.dart | 8 ++ .../src/animation/animation_config_test.dart | 14 +-- .../style_animation_driver_test.dart | 53 +++++++++ 4 files changed, 127 insertions(+), 54 deletions(-) diff --git a/packages/mix/lib/src/animation/animation_config.dart b/packages/mix/lib/src/animation/animation_config.dart index 1efcaaa19b..98485f4922 100644 --- a/packages/mix/lib/src/animation/animation_config.dart +++ b/packages/mix/lib/src/animation/animation_config.dart @@ -847,44 +847,49 @@ class PhaseAnimationConfig, U extends Style> final Listenable? trigger; final VoidCallback? onEnd; - // Not `const`: the asserts validate the supplied lists at construction time, - // which is the earliest point the invariants are known and well before any - // controller runs. These configs are never built in a const context. - PhaseAnimationConfig({ + const PhaseAnimationConfig({ required this.styles, required this.curveConfigs, required this.trigger, this.onEnd, - }) : assert( - styles.length == curveConfigs.length, - 'PhaseAnimationConfig requires one CurveAnimationConfig per phase ' - '(got ${styles.length} styles and ${curveConfigs.length} configs).', - ), - assert( - styles.isNotEmpty, - 'PhaseAnimationConfig requires at least one phase.', - ), - assert( - // Negative durations/delays produce out-of-order or backward tween - // weights and break TweenSequence timing. - curveConfigs.every((c) => c.duration >= .zero && c.delay >= .zero), - 'PhaseAnimationConfig durations and delays must be non-negative.', - ), - assert( - // A looping (untriggered) timeline is driven by `controller.repeat()`, - // which requires a positive period. - trigger != null || - curveConfigs.fold( - .zero, - (total, c) => total + c.totalDuration, - ) > - .zero, - 'A looping PhaseAnimationConfig (trigger == null) must have a positive ' - 'total duration.', - ); + }); bool get isLooping => trigger == null; + /// Asserts the configuration invariants. Kept off the `const` constructor so + /// the public const construction path is preserved; [PhaseAnimationDriver] + /// calls this before the controller runs, so an invalid config still fails + /// before controller execution. + void validate() { + assert( + styles.length == curveConfigs.length, + 'PhaseAnimationConfig requires one CurveAnimationConfig per phase ' + '(got ${styles.length} styles and ${curveConfigs.length} configs).', + ); + assert( + styles.isNotEmpty, + 'PhaseAnimationConfig requires at least one phase.', + ); + assert( + // Negative durations/delays produce out-of-order or backward tween + // weights and break TweenSequence timing. + curveConfigs.every((c) => c.duration >= .zero && c.delay >= .zero), + 'PhaseAnimationConfig durations and delays must be non-negative.', + ); + assert( + // A looping (untriggered) timeline is driven by `controller.repeat()`, + // which requires a positive period. + trigger != null || + curveConfigs.fold( + .zero, + (total, c) => total + c.totalDuration, + ) > + .zero, + 'A looping PhaseAnimationConfig (trigger == null) must have a positive ' + 'total duration.', + ); + } + @override List get props => [styles, trigger, curveConfigs]; } @@ -1155,29 +1160,36 @@ class KeyframeAnimationConfig> extends AnimationConfig final KeyframeStyleBuilder> styleBuilder; final Style initialStyle; - // Not `const`: the asserts validate the timeline at construction time, the - // earliest point the invariants are known and before any controller runs. - KeyframeAnimationConfig({ + const KeyframeAnimationConfig({ required this.trigger, required this.timeline, required this.styleBuilder, required this.initialStyle, - }) : assert( - // Track ids are the lookup keys for `KeyframeAnimationResult.get`; a - // duplicate silently shadows a track. - timeline.map((t) => t.id).toSet().length == timeline.length, - 'KeyframeAnimationConfig requires unique track ids; found a duplicate.', - ), - assert( - // A looping (untriggered) timeline is driven by `controller.repeat()`, - // which requires a positive period. - trigger != null || timeline.any((t) => t.totalDuration > .zero), - 'A looping KeyframeAnimationConfig (trigger == null) must have at least ' - 'one track with a positive duration.', - ); + }); bool get isLooping => trigger == null; + /// Asserts the configuration invariants. Kept off the `const` constructor so + /// the public const construction path is preserved; [KeyframeAnimationDriver] + /// calls this before the controller runs, so an invalid config still fails + /// before controller execution. Per-track segment durations are validated by + /// [KeyframeTrack] itself. + void validate() { + assert( + // Track ids are the lookup keys for `KeyframeAnimationResult.get`; a + // duplicate silently shadows a track. + timeline.map((t) => t.id).toSet().length == timeline.length, + 'KeyframeAnimationConfig requires unique track ids; found a duplicate.', + ); + assert( + // A looping (untriggered) timeline is driven by `controller.repeat()`, + // which requires a positive period. + trigger != null || timeline.any((t) => t.totalDuration > .zero), + 'A looping KeyframeAnimationConfig (trigger == null) must have at least ' + 'one track with a positive duration.', + ); + } + @override List get props => [trigger, timeline, initialStyle]; } diff --git a/packages/mix/lib/src/animation/style_animation_driver.dart b/packages/mix/lib/src/animation/style_animation_driver.dart index 88afde16a5..1cbadcbcb4 100644 --- a/packages/mix/lib/src/animation/style_animation_driver.dart +++ b/packages/mix/lib/src/animation/style_animation_driver.dart @@ -242,6 +242,9 @@ class PhaseAnimationDriver> extends StyleAnimationDriver { required super.initialSpec, required this.context, }) { + // Validate before any controller work so invalid configs fail early. Kept + // here (not in the const constructor) to preserve const construction. + config.validate(); _setUpAnimation(); // Register the completion listener on the controller once, for the driver's // whole lifetime. `_setUpAnimation` re-drives `_animation` on every @@ -348,6 +351,7 @@ class PhaseAnimationDriver> extends StyleAnimationDriver { @override void updateDriver(covariant PhaseAnimationConfig config) { + config.validate(); this.config.trigger?.removeListener(_onTriggerChanged); if (config != this.config) { controller.reset(); @@ -376,6 +380,9 @@ class KeyframeAnimationDriver> required super.initialSpec, required this.context, }) : _config = config { + // Validate before any controller work so invalid configs fail early. Kept + // here (not in the const constructor) to preserve const construction. + config.validate(); _setUpAnimation(); if (config.isLooping) { _startLoopingAnimation(); @@ -433,6 +440,7 @@ class KeyframeAnimationDriver> @override void updateDriver(covariant KeyframeAnimationConfig config) { + config.validate(); _config.trigger?.removeListener(_onTriggerChanged); if (_config != config) { controller.reset(); diff --git a/packages/mix/test/src/animation/animation_config_test.dart b/packages/mix/test/src/animation/animation_config_test.dart index 52f54c9949..304f5bc0f8 100644 --- a/packages/mix/test/src/animation/animation_config_test.dart +++ b/packages/mix/test/src/animation/animation_config_test.dart @@ -664,7 +664,7 @@ void main() { styles: const [], curveConfigs: const [], trigger: null, - ), + ).validate(), throwsA(isA()), ); }); @@ -680,7 +680,7 @@ void main() { ), ], trigger: null, - ), + ).validate(), throwsA(isA()), ); }); @@ -696,7 +696,7 @@ void main() { ), ], trigger: null, - ), + ).validate(), throwsA(isA()), ); }); @@ -712,7 +712,7 @@ void main() { ), ], trigger: null, - ), + ).validate(), throwsA(isA()), ); }); @@ -747,7 +747,7 @@ void main() { ], styleBuilder: (result, style) => style, initialStyle: MockStyle(0.0), - ), + ).validate(), throwsA(isA()), ); }); @@ -763,7 +763,7 @@ void main() { ], styleBuilder: (result, style) => style, initialStyle: MockStyle(0.0), - ), + ).validate(), throwsA(isA()), ); }); @@ -778,7 +778,7 @@ void main() { timeline: const [], styleBuilder: (result, style) => style, initialStyle: MockStyle(0.0), - ), + ).validate(), returnsNormally, ); }); diff --git a/packages/mix/test/src/animation/style_animation_driver_test.dart b/packages/mix/test/src/animation/style_animation_driver_test.dart index 95521f3b32..22a94c7e9e 100644 --- a/packages/mix/test/src/animation/style_animation_driver_test.dart +++ b/packages/mix/test/src/animation/style_animation_driver_test.dart @@ -1192,4 +1192,57 @@ void main() { }, ); }); + + group('driver validates config before running (regression)', () { + test('PhaseAnimationDriver rejects an invalid config on construction', () { + // Mismatched styles/curveConfigs must fail via validate() before any + // controller work — proves the driver is wired to config.validate(). + expect( + () => PhaseAnimationDriver>( + vsync: const TestVSync(), + config: PhaseAnimationConfig, MockStyle>( + styles: [MockStyle(0.0), MockStyle(1.0)], + curveConfigs: const [ + CurveAnimationConfig( + duration: Duration(milliseconds: 100), + curve: Curves.linear, + ), + ], + trigger: null, + ), + initialSpec: MockSpec(resolvedValue: 0.0).toStyleSpec(), + context: MockBuildContext(), + ), + throwsA(isA()), + ); + }); + + test('KeyframeAnimationDriver rejects an invalid config on construction', () { + final trigger = ValueNotifier(false); + addTearDown(trigger.dispose); + + // Duplicate track ids must fail via validate() before any controller work. + expect( + () => KeyframeAnimationDriver>( + vsync: const TestVSync(), + config: KeyframeAnimationConfig>( + trigger: trigger, + timeline: [ + KeyframeTrack('dup', const [ + Keyframe.linear(1.0, Duration(milliseconds: 100)), + ], initial: 0.0), + KeyframeTrack('dup', const [ + Keyframe.linear(1.0, Duration(milliseconds: 100)), + ], initial: 0.0), + ], + styleBuilder: (result, style) => style, + initialStyle: MockStyle(0.0), + ), + initialSpec: MockSpec(resolvedValue: 0.0).toStyleSpec(), + context: MockBuildContext(), + ), + throwsA(isA()), + ); + }); + }); }