From fa803554648cc7efffd199147471165f3ee96351 Mon Sep 17 00:00:00 2001 From: Nathan Walker Date: Thu, 8 Mar 2018 10:42:33 -0800 Subject: [PATCH] fix(animations): change throw -> trace to avoid unnecessary app crash (#5475) * fix(animations): change throw -> trace to avoid unnecessary app crash Fixes major cause of crashes/bugs in production apps using animation. * Fix fix animation throw (#1) * chore(tests): Cleanup code snippets comments * refactor(animations): Plat-specific cancel and play methods refactored --- tests/app/ui/animation/animation-tests.ts | 25 +++++++++++++ .../ui/animation/animation-common.ts | 21 ++++++----- .../ui/animation/animation.android.ts | 19 +++++++--- .../ui/animation/animation.ios.ts | 11 +++++- .../ui/animation/keyframe-animation.ts | 37 ++++++++++++------- 5 files changed, 82 insertions(+), 31 deletions(-) diff --git a/tests/app/ui/animation/animation-tests.ts b/tests/app/ui/animation/animation-tests.ts index 7b4c7af3f..905ddad1e 100644 --- a/tests/app/ui/animation/animation-tests.ts +++ b/tests/app/ui/animation/animation-tests.ts @@ -54,6 +54,31 @@ export function test_AnimatingProperties(done) { // << animation-properties } +export function test_PlayRejectsWhenAlreadyPlayingAnimation(done) { + let label = prepareTest(); + + var animation = label.createAnimation({ translate: { x: 100, y: 100 }, duration: 5 }); + + animation.play(); + animation.play().then(() => { + // should never get here + throw new Error("Already playing."); + }, (e) => { + TKUnit.assert(animation.isPlaying === true, "animation.isPlaying should be true since it's currently playing."); + if (e === "Animation is already playing.") { + done(); + } + }); +} + +export function test_CancelIgnoredWhenNotPlayingAnimation() { + let label = prepareTest(); + + var animation = label.createAnimation({ translate: { x: 100, y: 100 }, duration: 5 }); + animation.cancel(); // should not throw + TKUnit.assert(!animation.isPlaying, "animation.isPlaying should be falsey since it was never played."); +} + export function test_CancellingAnimation(done) { let label = prepareTest(); diff --git a/tns-core-modules/ui/animation/animation-common.ts b/tns-core-modules/ui/animation/animation-common.ts index ded11ba50..0b86cbd45 100644 --- a/tns-core-modules/ui/animation/animation-common.ts +++ b/tns-core-modules/ui/animation/animation-common.ts @@ -10,9 +10,9 @@ import { View } from "../core/view"; // Types. import { Color } from "../../color"; -import { isEnabled as traceEnabled, write as traceWrite, categories as traceCategories } from "../../trace"; +import { isEnabled as traceEnabled, write as traceWrite, categories as traceCategories, messageType as traceType } from "../../trace"; -export { Color, traceEnabled, traceWrite, traceCategories }; +export { Color, traceEnabled, traceWrite, traceCategories, traceType }; export { AnimationPromise } from "."; export module Properties { @@ -84,11 +84,16 @@ export abstract class AnimationBase implements AnimationBaseDefinition { abstract _resolveAnimationCurve(curve: any): any; - public play(): AnimationPromiseDefinition { - if (this.isPlaying) { - throw new Error("Animation is already playing."); - } + protected _rejectAlreadyPlaying(): AnimationPromiseDefinition{ + const reason = "Animation is already playing."; + traceWrite(reason, traceCategories.Animation, traceType.warn); + + return new Promise((resolve, reject) => { + reject(reason); + }); + } + public play(): AnimationPromiseDefinition { // We have to actually create a "Promise" due to a bug in the v8 engine and decedent promises // We just cast it to a animationPromise so that all the rest of the code works fine var animationFinishedPromise = new Promise((resolve, reject) => { @@ -123,9 +128,7 @@ export abstract class AnimationBase implements AnimationBaseDefinition { } public cancel(): void { - if (!this.isPlaying) { - throw new Error("Animation is not currently playing."); - } + // Implemented in platform specific files } public get isPlaying(): boolean { diff --git a/tns-core-modules/ui/animation/animation.android.ts b/tns-core-modules/ui/animation/animation.android.ts index 2f5c75d46..8359c9cd4 100644 --- a/tns-core-modules/ui/animation/animation.android.ts +++ b/tns-core-modules/ui/animation/animation.android.ts @@ -2,7 +2,7 @@ import { AnimationDefinition } from "."; import { View } from "../core/view"; -import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, Color, traceWrite, traceEnabled, traceCategories } from "./animation-common"; +import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, Color, traceWrite, traceEnabled, traceCategories, traceType } from "./animation-common"; import { opacityProperty, backgroundColorProperty, rotateProperty, translateXProperty, translateYProperty, scaleXProperty, scaleYProperty @@ -135,6 +135,10 @@ export class Animation extends AnimationBase { } public play(): AnimationPromise { + if (this.isPlaying) { + return this._rejectAlreadyPlaying(); + } + let animationFinishedPromise = super.play(); this._animators = new Array(); @@ -170,10 +174,13 @@ export class Animation extends AnimationBase { } public cancel(): void { - super.cancel(); - if (traceEnabled()) { - traceWrite("Cancelling AnimatorSet.", traceCategories.Animation); + if (!this.isPlaying) { + traceWrite("Animation is not currently playing.", traceCategories.Animation, traceType.warn); + return; } + + traceWrite("Cancelling AnimatorSet.", traceCategories.Animation); + this._animatorSet.cancel(); } @@ -301,7 +308,7 @@ export class Animation extends AnimationBase { } else { propertyAnimation.target.style[backgroundColorProperty.keyframe] = originalValue1; } - + if (propertyAnimation.target.nativeViewProtected && propertyAnimation.target[backgroundColorProperty.setNative]) { propertyAnimation.target[backgroundColorProperty.setNative](propertyAnimation.target.style.backgroundColor); } @@ -414,7 +421,7 @@ export class Animation extends AnimationBase { } else { propertyAnimation.target.style[rotateProperty.keyframe] = originalValue1; } - + if (propertyAnimation.target.nativeViewProtected) { propertyAnimation.target[rotateProperty.setNative](propertyAnimation.target.style.rotate); } diff --git a/tns-core-modules/ui/animation/animation.ios.ts b/tns-core-modules/ui/animation/animation.ios.ts index 397309615..ce7ac6eef 100644 --- a/tns-core-modules/ui/animation/animation.ios.ts +++ b/tns-core-modules/ui/animation/animation.ios.ts @@ -1,7 +1,7 @@ import { AnimationDefinition } from "."; import { View } from "../core/view"; -import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, traceWrite, traceEnabled, traceCategories } from "./animation-common"; +import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, traceWrite, traceEnabled, traceCategories, traceType } from "./animation-common"; import { opacityProperty, backgroundColorProperty, rotateProperty, translateXProperty, translateYProperty, scaleXProperty, scaleYProperty @@ -210,6 +210,10 @@ export class Animation extends AnimationBase { } public play(): AnimationPromise { + if (this.isPlaying) { + return this._rejectAlreadyPlaying(); + } + let animationFinishedPromise = super.play(); this._finishedAnimations = 0; this._cancelledAnimations = 0; @@ -218,7 +222,10 @@ export class Animation extends AnimationBase { } public cancel(): void { - super.cancel(); + if (!this.isPlaying) { + traceWrite("Animation is not currently playing.", traceCategories.Animation, traceType.warn); + return; + } let i = 0; let length = this._mergedPropertyAnimations.length; diff --git a/tns-core-modules/ui/animation/keyframe-animation.ts b/tns-core-modules/ui/animation/keyframe-animation.ts index 578b7f220..a35b21dbb 100644 --- a/tns-core-modules/ui/animation/keyframe-animation.ts +++ b/tns-core-modules/ui/animation/keyframe-animation.ts @@ -12,6 +12,8 @@ import { View, Color } from "../core/view"; import { AnimationCurve } from "../enums"; +import { isEnabled as traceEnabled, write as traceWrite, categories as traceCategories, messageType as traceType } from "../../trace"; + // Types. import { unsetValue } from "../core/properties"; import { Animation } from "./animation"; @@ -143,25 +145,32 @@ export class KeyframeAnimation implements KeyframeAnimationDefinition { } public cancel() { - if (this._isPlaying) { - this._isPlaying = false; - for (let i = this._nativeAnimations.length - 1; i >= 0; i--) { - let animation = this._nativeAnimations[i]; - if (animation.isPlaying) { - animation.cancel(); - } - } - if (this._nativeAnimations.length > 0) { - let animation = this._nativeAnimations[0]; - this._resetAnimationValues(this._target, animation); - } - this._rejectAnimationFinishedPromise(); + if (!this.isPlaying) { + traceWrite("Keyframe animation is already playing.", traceCategories.Animation, traceType.warn); + return; } + + this._isPlaying = false; + for (let i = this._nativeAnimations.length - 1; i >= 0; i--) { + let animation = this._nativeAnimations[i]; + if (animation.isPlaying) { + animation.cancel(); + } + } + if (this._nativeAnimations.length > 0) { + let animation = this._nativeAnimations[0]; + this._resetAnimationValues(this._target, animation); + } + this._rejectAnimationFinishedPromise(); } public play(view: View): Promise { if (this._isPlaying) { - throw new Error("Animation is already playing."); + const reason = "Keyframe animation is already playing."; + traceWrite(reason, traceCategories.Animation, traceType.warn); + return new Promise((resolve, reject) => { + reject(reason); + }); } let animationFinishedPromise = new Promise((resolve, reject) => {