flutter_animate icon indicating copy to clipboard operation
flutter_animate copied to clipboard

Animate.autoPlay

Open gskinner opened this issue 2 years ago • 10 comments

Currently, to create an animation that doesn't play automatically, you need to use onInit:

foo.animate(onInit: (controller) => controller.stop()).fade()

This works, but less semantically clear than having an autoPlay param.

However, implementation of such a feature has some ambiguities / concerns:

  1. Should Animate.delay be respected? It is a delay before the animation starts playing, so it's confusing either way.
  2. If the answer to (1) is yes, then how would you start it playing? A method like Animate.play? Thus far all playback control is delegated to the AnimationController, so this would be a possibly confusing departure.
  3. Because onInit runs when the animation plays, you wouldn't have a mechanism to access to controller if there was a delay.
  4. Even with autoPlay you still need to save off the Animate or AnimationController in order to play it later, so I'm not certain this is providing a ton of value.
  5. AnimateList uses Animate.delay for interval, which could cause confusionautoPlay if didn't respect delay

Something to consider, but for now I'm leaning towards not adding this. It's an advanced convenience feature, that may add more confusion than value.

gskinner avatar Jun 27 '22 19:06 gskinner

Thoughts:

  1. I don't think it make sense to use a delay with autoPlay = false. The entire intent of autoPlay=false, is "I will handle the play call myself, at a later pt in time". In such a situation, it's trivial to add any delay I need. ie:
onPressed: () => Future.delayed(100.ms).then(() => if(mounted) _fade.play())
  1. Non issues if we ignore delay when autoPlay:false
  2. ''
  3. True, its not a LOC saving, its more readable as you can parse it down the left-hand side of the widget config.
  • Also you might pass in your own controller? In which case, its quite a bit nicer to not have to use the closure.
  1. This seems like an implementation detail of AnimateList to work around... which itself is only syntactic sugar. I wouldn't let it block improvements to the core API.

esDotDev avatar Sep 22 '22 05:09 esDotDev

Just started trying this package out today - I immediately tried to look for an bool autoplay sort of flag. Is there some reason onInit doesn't exist on the Animate(child:) widget as well as the .animate() call? Just wanted to mention that I would've found this feature useful - thanks!

Edit: I see there isn't an onInit at all even on .animate() - is onPlay supposed to be the substitute now?

bdlindsay avatar Oct 17 '22 21:10 bdlindsay

Thanks for the feedback @bdlindsay.

To answer your question: Yes, onInit was renamed onPlay because that's actually when it was firing. I had added an onInit as well, but never committed it because it gets a bit messy with delay and controller initialization.

Thinking out loud a bit here, but one way to think about autoPlay=false is simply as an infinite delay. In fact, one of the things I don't like about autoPlay is that it would require that delay is ignored if it's set to false. We could:

  • simply document that users could use delay: Duration(days: 0xFFFFFFFF) to prevent autoplay
  • we could add an extension Duration.eon (or just eon keyword?) to make that even easier
  • we could even recognize that value and just not run the delay

Just a thought.

gskinner avatar Oct 17 '22 22:10 gskinner

Alternate option, just document that delay is ignored if autoPlay = false, it doesn't really make semantic sense anyways, if there is no autoplay, what is delayed.... Once the user has taken responsibility to call play it's trivial for them to add their own delay.

esDotDev avatar Oct 17 '22 22:10 esDotDev

Totally, I'm always just a little hesitant to add params that are basically just a shortcut to another param. Not a huge issue though.

gskinner avatar Oct 17 '22 23:10 gskinner

Is there something wrong with just respecting the delay when the user has taken the responsibility to call play themselves if if they could add their own at that time? If going the direction of passing some sort of infinite duration to delay, I wonder if we could make the intended purpose of the extension more explicit. Something like Duration.untilPlayed or the like.

I think I would lean towards an autoPlay parameter where the specified delay could still be respected. It is easiest to discover and understand for new users of the package where an infinite duration delay seems like a bit of a hack. Delay still serves a purpose in the API to let someone specify how they expect that animation to be used, and the delay would not have to be remembered if there happened to be multiple call sites for playing the animation. It would be more confusing to know delay is ignored than to know you could remove always the delay yourself if you want to control it else where.

bdlindsay avatar Oct 18 '22 03:10 bdlindsay

In order to do that, I believe a custom controller would have to be introduced. It could then define a play method, and internally apply the delay when user calls play.

When I made a similar tween lib, this use case specifically was what tipped the scales for me to implement a custom controller, rather than just passing along the raw AnimationController as animate does now.

esDotDev avatar Oct 18 '22 03:10 esDotDev

The Animate.delay is not part of the animation, it is a delay before the animation begins playing (ie. AnimationController.forward() is called), and your ability to control the animation is via the same AnimationController. It is already fairly trivial to just add some delay to the effects (and thus the actual animation):

foo.animate().fadeIn(delay: 1.second).blur(); // all delayed 1s at the animation level.

Just to add more info here, @bdlindsay could you share what you are trying to accomplish with autoPlay, including a code snippet of how you'd see yourself using it? I want to be sure we're trying to fix the right problem here.

gskinner avatar Oct 18 '22 15:10 gskinner

Animate(
  onPlay: (controller) {
    _controller ??= controller;
    if (!_isLoading) {
      controller.stop();
    }
  },
  controller: _controller,
  effects: const [RotateEffect(duration: Duration(milliseconds: 600))],
  child: const Icon(Icons.refresh, color: activeColor),
),

I'm just looking to spin a refresh icon as a network call is finishing, so I don't want the animation to play on initial load, just when the refresh button is pressed. In the onPress handler of the button this is contained in, I'm calling _controller?.repeat(); to start the animation and _controller?.stop() when the call completes.

Right now, I have the work-around of using a state variable to essentially just immediately stop the animation on the first page build.

bdlindsay avatar Oct 18 '22 15:10 bdlindsay

The Animate.delay is not part of the animation, it is a delay before the animation begins playing (ie. AnimationController.forward() is called), and your ability to control the animation is via the same AnimationController

But it could be, if _AnimateState were passed instead of the raw controller, or a small wrapper for the AC were created, then you could respect the defiend delay at a later call to 'play`.

The benefit to having the lib do this, is that not checking mounted is a potential land-mine. Proper user code needs to do something like:

onPressed: (){
  await Future.delay(1.seconds);
  if(mounted) {  // most people will forget this important check
    _controller.forward(); 
  }
}

This is error prone. It would be nice if I could just do:

Animate(delay: 1.seconds, autoPlay: false, onInit: (c) => _controller = c, ...);

// then
_controller.play(); /// uses a 1.s delay, but won't fire if widget is unmounted in the meantime
_controller.play(delay: 0.seconds); // plays immediately
_controller.animation.forward(...) // take full control if you need

esDotDev avatar Oct 18 '22 18:10 esDotDev

how to disable autoplay right now? Guys, I want to trigger only using controller

dokind avatar Feb 06 '23 04:02 dokind

since onInit has been removed and we only have onPlay now, there does not appear to be a way to stop the animation from auto-playing anymore. This means, we can't use flutter_animate for animations which arent automatically played on render. this feels like an oversight.

clragon avatar Feb 09 '23 13:02 clragon

I also agree autoPlay is an obvious feature to have, but for now the recommended approach is to stop the animation within the onPlay callback:

onPlay: (c) => _controller = c..stop();

The main downside of this, since Animate directly hands us the animation controller and not some higher level abstraction, you will need to manually check mounted before calling forward, or you could get an error.

I believe you can also use the new .target value to do this:

Animate(
  effects: ...
  target: _playAnimation? 0 : 1,
)

In that case, the animation starts, but has nowhere to go. When you change the 0 to a 1, the animation will restart, and run it's full cycle.

Still not as semantically clear as a simple autoPlay obviously.

esDotDev avatar Feb 09 '23 15:02 esDotDev

but for now the recommended approach is to stop the animation within the onPlay callback

how so? isn't onPlay called everytime the animation is played? writing that will cause the animation to not play ever again, even if we manually pass the controller and call animateTo. the only way to accomplish this would therefore be with the target value, I believe.

clragon avatar Feb 13 '23 13:02 clragon

No, onPlay is called once when the tween initially starts, or when the animation is re-started due to widget props (or target) changing. But you it won't be called if you manually call controller.forward() yourself. https://github.com/gskinner/flutter_animate/blob/main/lib/src/animate.dart

imo, it would be more clear and useful if there was a simple onInit callback only ever called once, and then onPlay could actually be dispatched any time the animation starts playing, which might be handy. But that's not how it works now.

With all that said, target is probably the better option, as you don't need the mounted check, and its probably going to be less lines of code and easier to read.

esDotDev avatar Feb 13 '23 14:02 esDotDev

Now that v4.0 is released, this issue is going to be my next focus.

Here's a quick summary of my current thoughts based on the above:

  • add autoPlay — this will prevent the internal call to controller.forward() in _play. It would also ignore Animate.delay entirely. This would not block adapter or target functionality.
  • add onInit (or possibly onInitController) — this would fire whenever the controller is inited. Most of the time, this would only happen once, but could happen again if you pass a new controller via controller, or change the overall duration of the animation.
  • onPlay — unchanged. Fires immediately after the internal controller.forward() call. For clarity, this wouldn't fire if autoPlay=false.

With regards to onInit, I'd love feedback on this. I also considered whether it should only fire in the case where a new internal controller is created. That would mean it would almost never re-init (only in extreme edge cases), but it would also make it difficult to write external logic that needs an accurate controller.duration. I expect that's rare, but I'm not totally convinced it's not worth supporting.

Feedback invited.

gskinner avatar Feb 14 '23 17:02 gskinner

With regards to onInit I would expect to be basically the equivalent to initState, called once and only once. But in the case that you do re-create a controller, I suppose it would make sense to call again.

I don't understand the controller.duration problem tbh.

esDotDev avatar Feb 14 '23 18:02 esDotDev

I'm mostly just thinking there may be cases where you want to react to the total duration of the animation. I honestly can't think of a great reason for this, but it seems like something someone might need. I might just be over thinking it though.

You may also want to use it as a way to get the total duration when you supply your own controller, for some reason.

foo.animate(
  controller: myController,
  onInit: (controller) => doStuff(controller.duration)
)

gskinner avatar Feb 14 '23 18:02 gskinner

I just pushed a commit that adds autoPlay and onInit. I'd love to have people try it out before I do a public release so I can be sure it's meeting everyone's expectations.

It also adds some new assert checks to warn you when you're using incompatible features (ex. autoPlay=false with onPlay or delay with adapter).

gskinner avatar Feb 14 '23 23:02 gskinner

That error looks like it's coming from an Adapter, specifically a ValueNotifierAdapter, but I don't see one in your code. Is it perhaps something else in your app throwing that error? If so, I'd still be interested to know if it's due to a bug in the library — if so, could you start a new issue for it?

gskinner avatar Feb 18 '23 17:02 gskinner

Hi @gskinner, it seems to be indeed an issue with another Animate widget higher up the widget tree, showing up since I updated to the latest git version. I will make a new issue.

rienkkk avatar Feb 19 '23 20:02 rienkkk

Closing as resolved via 0aecdacb5418f03fe13012bc173e78daabf670d8

gskinner avatar Feb 20 '23 17:02 gskinner

Great addition! I just started using this library and needed autoPlay. I was happy to see that it was just added. Works like a charm!

benank avatar Feb 24 '23 20:02 benank