Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added override for system animations enabled behavior #1747

Conversation

IljaKosynkin
Copy link
Contributor

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.

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@@ -866,7 +866,7 @@ public boolean isAnimating() {
return animator.isRunning();
}

void setSystemAnimationsAreEnabled(Boolean areEnabled) {
public void setSystemAnimationsAreEnabled(Boolean areEnabled) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you leave this visibility the same as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@IljaKosynkin IljaKosynkin Mar 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and add a javadoc to both new APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer!

@@ -59,7 +59,9 @@
private LottieComposition composition;
private final LottieValueAnimator animator = new LottieValueAnimator();
private float scale = 1f;
private boolean animationsEnabled = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gpeal good idea, updated PR with function and comment on fields.

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gpeal gpeal merged commit 0809f91 into airbnb:master Mar 13, 2021
Copy link

@richardsonmark316 richardsonmark316 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

20201223_105608
😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants