Added override for system animations enabled behavior #1747
Conversation
| @@ -866,7 +866,7 @@ public boolean isAnimating() { | |||
| return animator.isRunning(); | |||
| } | |||
|
|
|||
| void setSystemAnimationsAreEnabled(Boolean areEnabled) { | |||
| public void setSystemAnimationsAreEnabled(Boolean areEnabled) { | |||
There was a problem hiding this comment.
Can you leave this visibility the same as before?
There was a problem hiding this comment.
I guess it is possible, but then this functionality will be accessible only via XML attribute. I think it's a good idea to give ability to override this behaviour from the code (conditionally, for instance) as well.
Alternatively, I can add setSystemAnimationsAreEnabled to LottieAnimationView and leave setSystemAnimationsAreEnabled of LottieDrawable as it was.
@gpeal thoughts?
There was a problem hiding this comment.
@IljaKosynkin If you want a programmatic API (not a bad idea), then there should be one on LottieAnimationView as well (in addition to the attr).
And in that case, it's important to make all 3 have similar names. Let's go with lottie_ignoreDisabledSystemAnimations and setIgnoreDisabledSystemAnimations(boolean)
It's a bit verbose but it's fairly clear I think.
There was a problem hiding this comment.
Ok, just to make sure @gpeal, you suggest to rename attribute to lottie_ignoreDisabledSystemAnimations and adding two public functions to both LottieAnimationView and LottieDrawable with name setIgnoreDisabledSystemAnimations, which will set a flag to ignore system settings in LottieDrawable.
I also to leave visibility of setSystemAnimationsAreEnabled in LottieDrawable as package-private.
Is everything correct?
There was a problem hiding this comment.
Yes, and add a javadoc to both new APIs.
There was a problem hiding this comment.
@gpeal updated the PR, sorry for the delay!
I'm not sure I got right splitting of flags in LottieDrawable, it spanned a little bit further than I originally wanted it to, so let me know if everything is alright.
| @@ -59,7 +59,9 @@ | |||
| private LottieComposition composition; | |||
| private final LottieValueAnimator animator = new LottieValueAnimator(); | |||
| private float scale = 1f; | |||
| private boolean animationsEnabled = true; | |||
There was a problem hiding this comment.
Let's make animationsEnabled() a private function and in the javadoc for systemAnimationsEnabled and ignoreSystemAnimationsDisabled, let's say Call animationsEnabled() instead of using these fields directly
Then we don't manually have to update a third field which could theoretically get out of sync with the other two.
There was a problem hiding this comment.
@gpeal good idea, updated PR with function and comment on fields.
…enabled based off flags
Good morning or afternoon, Lottie!
I created this small PR for a use case I currently have in my project. Essentially, we have a fullscreen animation that have loading and success states. When loading finishes, we play out final state of the animation and listen for Animator.AnimatorListener#onAnimationEnd to dismiss the screen.
Obviously, if user opted to disable animations the screen is stuck, because the listener method is never invoked (we add it just before playing the final state of the animation) and therefore screen is never dismissed.
This PR provides a possibility to override current behaviour (which is just looking if animations are disabled in Settings) with custom flag in XML (false by default, so behaviour won't change) and makes LottieDrawable#setSystemAnimationsAreEnabled public so it can be overridden from the code.
I haven't found any test cases that would use API I've changed and didn't see docs I should modify, however if I missed that please do tell me.