From 61937a23a8410a345ba11845bcfdd56a6fa10159 Mon Sep 17 00:00:00 2001 From: Nathan Walker Date: Tue, 27 Feb 2018 10:30:29 -0800 Subject: [PATCH 1/2] fix(animations): change throw -> trace to avoid unnecessary app crash Fixes major cause of crashes/bugs in production apps using animation. --- tests/app/ui/animation/animation-tests.ts | 30 +++++++++++++++++++ .../ui/animation/animation-common.ts | 12 ++++++-- .../ui/animation/keyframe-animation.ts | 10 ++++++- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/tests/app/ui/animation/animation-tests.ts b/tests/app/ui/animation/animation-tests.ts index 7b4c7af3fc..41f89b142c 100644 --- a/tests/app/ui/animation/animation-tests.ts +++ b/tests/app/ui/animation/animation-tests.ts @@ -54,6 +54,36 @@ export function test_AnimatingProperties(done) { // << animation-properties } +export function test_PlayRejectsWhenAlreadyPlayingAnimation(done) { + let label = prepareTest(); + + // >> animation-play + 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(); + } + }); + // << animation-play +} + +export function test_CancelIgnoredWhenNotPlayingAnimation(done) { + let label = prepareTest(); + + // >> animation-cancel-ignore + 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."); + done(); + // << animation-cancel-ignore +} + 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 ded11ba508..900c6b7320 100644 --- a/tns-core-modules/ui/animation/animation-common.ts +++ b/tns-core-modules/ui/animation/animation-common.ts @@ -86,7 +86,13 @@ export abstract class AnimationBase implements AnimationBaseDefinition { public play(): AnimationPromiseDefinition { if (this.isPlaying) { - throw new Error("Animation is already playing."); + const reason = "Animation is already playing."; + if (traceEnabled()) { + traceWrite(reason, traceCategories.Animation, 2); + } + return new Promise((resolve, reject) => { + reject(reason); + }); } // We have to actually create a "Promise" due to a bug in the v8 engine and decedent promises @@ -124,7 +130,9 @@ export abstract class AnimationBase implements AnimationBaseDefinition { public cancel(): void { if (!this.isPlaying) { - throw new Error("Animation is not currently playing."); + if (traceEnabled()) { + traceWrite("Animation is not currently playing.", traceCategories.Animation, 2); + } } } diff --git a/tns-core-modules/ui/animation/keyframe-animation.ts b/tns-core-modules/ui/animation/keyframe-animation.ts index 578b7f220d..f8ad66ca2d 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 } from "../../trace"; + // Types. import { unsetValue } from "../core/properties"; import { Animation } from "./animation"; @@ -161,7 +163,13 @@ export class KeyframeAnimation implements KeyframeAnimationDefinition { public play(view: View): Promise { if (this._isPlaying) { - throw new Error("Animation is already playing."); + const reason = "Keyframe animation is already playing."; + if (traceEnabled()) { + traceWrite(reason, traceCategories.Animation, 2); + } + return new Promise((resolve, reject) => { + reject(reason); + }); } let animationFinishedPromise = new Promise((resolve, reject) => { From 60ff1254719a3773c160c1dfd0889a7f300b8630 Mon Sep 17 00:00:00 2001 From: Alexander Vakrilov Date: Fri, 2 Mar 2018 16:44:24 +0200 Subject: [PATCH 2/2] 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 | 7 +--- .../ui/animation/animation-common.ts | 29 +++++++--------- .../ui/animation/animation.android.ts | 19 +++++++---- .../ui/animation/animation.ios.ts | 11 +++++-- .../ui/animation/keyframe-animation.ts | 33 ++++++++++--------- 5 files changed, 52 insertions(+), 47 deletions(-) diff --git a/tests/app/ui/animation/animation-tests.ts b/tests/app/ui/animation/animation-tests.ts index 41f89b142c..905ddad1e2 100644 --- a/tests/app/ui/animation/animation-tests.ts +++ b/tests/app/ui/animation/animation-tests.ts @@ -57,7 +57,6 @@ export function test_AnimatingProperties(done) { export function test_PlayRejectsWhenAlreadyPlayingAnimation(done) { let label = prepareTest(); - // >> animation-play var animation = label.createAnimation({ translate: { x: 100, y: 100 }, duration: 5 }); animation.play(); @@ -70,18 +69,14 @@ export function test_PlayRejectsWhenAlreadyPlayingAnimation(done) { done(); } }); - // << animation-play } -export function test_CancelIgnoredWhenNotPlayingAnimation(done) { +export function test_CancelIgnoredWhenNotPlayingAnimation() { let label = prepareTest(); - // >> animation-cancel-ignore 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."); - done(); - // << animation-cancel-ignore } export function test_CancellingAnimation(done) { diff --git a/tns-core-modules/ui/animation/animation-common.ts b/tns-core-modules/ui/animation/animation-common.ts index 900c6b7320..0b86cbd452 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,17 +84,16 @@ export abstract class AnimationBase implements AnimationBaseDefinition { abstract _resolveAnimationCurve(curve: any): any; - public play(): AnimationPromiseDefinition { - if (this.isPlaying) { - const reason = "Animation is already playing."; - if (traceEnabled()) { - traceWrite(reason, traceCategories.Animation, 2); - } - return new Promise((resolve, reject) => { - reject(reason); - }); - } + 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) => { @@ -129,11 +128,7 @@ export abstract class AnimationBase implements AnimationBaseDefinition { } public cancel(): void { - if (!this.isPlaying) { - if (traceEnabled()) { - traceWrite("Animation is not currently playing.", traceCategories.Animation, 2); - } - } + // 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 2f5c75d461..8359c9cd48 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 3973096154..ce7ac6eef5 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 f8ad66ca2d..a35b21dbb8 100644 --- a/tns-core-modules/ui/animation/keyframe-animation.ts +++ b/tns-core-modules/ui/animation/keyframe-animation.ts @@ -12,7 +12,7 @@ import { View, Color } from "../core/view"; import { AnimationCurve } from "../enums"; -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"; // Types. import { unsetValue } from "../core/properties"; @@ -145,28 +145,29 @@ 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); + 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(); } - this._rejectAnimationFinishedPromise(); } + 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) { const reason = "Keyframe animation is already playing."; - if (traceEnabled()) { - traceWrite(reason, traceCategories.Animation, 2); - } + traceWrite(reason, traceCategories.Animation, traceType.warn); return new Promise((resolve, reject) => { reject(reason); });