Skip to content

Commit fa80355

Browse files
NathanWalkerSvetoslavTsenov
NathanWalker
authored and
SvetoslavTsenov
committed
fix(animations): change throw -> trace to avoid unnecessary app crash (NativeScript#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 (NativeScript#1) * chore(tests): Cleanup code snippets comments * refactor(animations): Plat-specific cancel and play methods refactored
1 parent 8141737 commit fa80355

File tree

5 files changed

+81
-30
lines changed

5 files changed

+81
-30
lines changed

‎tests/app/ui/animation/animation-tests.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,31 @@ export function test_AnimatingProperties(done) {
5454
// << animation-properties
5555
}
5656

57+
export function test_PlayRejectsWhenAlreadyPlayingAnimation(done) {
58+
let label = prepareTest();
59+
60+
var animation = label.createAnimation({ translate: { x: 100, y: 100 }, duration: 5 });
61+
62+
animation.play();
63+
animation.play().then(() => {
64+
// should never get here
65+
throw new Error("Already playing.");
66+
}, (e) => {
67+
TKUnit.assert(animation.isPlaying === true, "animation.isPlaying should be true since it's currently playing.");
68+
if (e === "Animation is already playing.") {
69+
done();
70+
}
71+
});
72+
}
73+
74+
export function test_CancelIgnoredWhenNotPlayingAnimation() {
75+
let label = prepareTest();
76+
77+
var animation = label.createAnimation({ translate: { x: 100, y: 100 }, duration: 5 });
78+
animation.cancel(); // should not throw
79+
TKUnit.assert(!animation.isPlaying, "animation.isPlaying should be falsey since it was never played.");
80+
}
81+
5782
export function test_CancellingAnimation(done) {
5883
let label = prepareTest();
5984

‎tns-core-modules/ui/animation/animation-common.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import { View } from "../core/view";
1010

1111
// Types.
1212
import { Color } from "../../color";
13-
import { isEnabled as traceEnabled, write as traceWrite, categories as traceCategories } from "../../trace";
13+
import { isEnabled as traceEnabled, write as traceWrite, categories as traceCategories, messageType as traceType } from "../../trace";
1414

15-
export { Color, traceEnabled, traceWrite, traceCategories };
15+
export { Color, traceEnabled, traceWrite, traceCategories, traceType };
1616
export { AnimationPromise } from ".";
1717

1818
export module Properties {
@@ -84,11 +84,16 @@ export abstract class AnimationBase implements AnimationBaseDefinition {
8484

8585
abstract _resolveAnimationCurve(curve: any): any;
8686

87-
public play(): AnimationPromiseDefinition {
88-
if (this.isPlaying) {
89-
throw new Error("Animation is already playing.");
90-
}
87+
protected _rejectAlreadyPlaying(): AnimationPromiseDefinition{
88+
const reason = "Animation is already playing.";
89+
traceWrite(reason, traceCategories.Animation, traceType.warn);
90+
91+
return <AnimationPromiseDefinition>new Promise<void>((resolve, reject) => {
92+
reject(reason);
93+
});
94+
}
9195

96+
public play(): AnimationPromiseDefinition {
9297
// We have to actually create a "Promise" due to a bug in the v8 engine and decedent promises
9398
// We just cast it to a animationPromise so that all the rest of the code works fine
9499
var animationFinishedPromise = <AnimationPromiseDefinition>new Promise<void>((resolve, reject) => {
@@ -123,9 +128,7 @@ export abstract class AnimationBase implements AnimationBaseDefinition {
123128
}
124129

125130
public cancel(): void {
126-
if (!this.isPlaying) {
127-
throw new Error("Animation is not currently playing.");
128-
}
131+
// Implemented in platform specific files
129132
}
130133

131134
public get isPlaying(): boolean {

‎tns-core-modules/ui/animation/animation.android.ts‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
import { AnimationDefinition } from ".";
33
import { View } from "../core/view";
44

5-
import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, Color, traceWrite, traceEnabled, traceCategories } from "./animation-common";
5+
import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, Color, traceWrite, traceEnabled, traceCategories, traceType } from "./animation-common";
66
import {
77
opacityProperty, backgroundColorProperty, rotateProperty,
88
translateXProperty, translateYProperty, scaleXProperty, scaleYProperty
@@ -135,6 +135,10 @@ export class Animation extends AnimationBase {
135135
}
136136

137137
public play(): AnimationPromise {
138+
if (this.isPlaying) {
139+
return this._rejectAlreadyPlaying();
140+
}
141+
138142
let animationFinishedPromise = super.play();
139143

140144
this._animators = new Array<android.animation.Animator>();
@@ -170,10 +174,13 @@ export class Animation extends AnimationBase {
170174
}
171175

172176
public cancel(): void {
173-
super.cancel();
174-
if (traceEnabled()) {
175-
traceWrite("Cancelling AnimatorSet.", traceCategories.Animation);
177+
if (!this.isPlaying) {
178+
traceWrite("Animation is not currently playing.", traceCategories.Animation, traceType.warn);
179+
return;
176180
}
181+
182+
traceWrite("Cancelling AnimatorSet.", traceCategories.Animation);
183+
177184
this._animatorSet.cancel();
178185
}
179186

@@ -301,7 +308,7 @@ export class Animation extends AnimationBase {
301308
} else {
302309
propertyAnimation.target.style[backgroundColorProperty.keyframe] = originalValue1;
303310
}
304-
311+
305312
if (propertyAnimation.target.nativeViewProtected && propertyAnimation.target[backgroundColorProperty.setNative]) {
306313
propertyAnimation.target[backgroundColorProperty.setNative](propertyAnimation.target.style.backgroundColor);
307314
}
@@ -414,7 +421,7 @@ export class Animation extends AnimationBase {
414421
} else {
415422
propertyAnimation.target.style[rotateProperty.keyframe] = originalValue1;
416423
}
417-
424+
418425
if (propertyAnimation.target.nativeViewProtected) {
419426
propertyAnimation.target[rotateProperty.setNative](propertyAnimation.target.style.rotate);
420427
}

‎tns-core-modules/ui/animation/animation.ios.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { AnimationDefinition } from ".";
22
import { View } from "../core/view";
33

4-
import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, traceWrite, traceEnabled, traceCategories } from "./animation-common";
4+
import { AnimationBase, Properties, PropertyAnimation, CubicBezierAnimationCurve, AnimationPromise, traceWrite, traceEnabled, traceCategories, traceType } from "./animation-common";
55
import {
66
opacityProperty, backgroundColorProperty, rotateProperty,
77
translateXProperty, translateYProperty, scaleXProperty, scaleYProperty
@@ -210,6 +210,10 @@ export class Animation extends AnimationBase {
210210
}
211211

212212
public play(): AnimationPromise {
213+
if (this.isPlaying) {
214+
return this._rejectAlreadyPlaying();
215+
}
216+
213217
let animationFinishedPromise = super.play();
214218
this._finishedAnimations = 0;
215219
this._cancelledAnimations = 0;
@@ -218,7 +222,10 @@ export class Animation extends AnimationBase {
218222
}
219223

220224
public cancel(): void {
221-
super.cancel();
225+
if (!this.isPlaying) {
226+
traceWrite("Animation is not currently playing.", traceCategories.Animation, traceType.warn);
227+
return;
228+
}
222229

223230
let i = 0;
224231
let length = this._mergedPropertyAnimations.length;

‎tns-core-modules/ui/animation/keyframe-animation.ts‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import { View, Color } from "../core/view";
1212

1313
import { AnimationCurve } from "../enums";
1414

15+
import { isEnabled as traceEnabled, write as traceWrite, categories as traceCategories, messageType as traceType } from "../../trace";
16+
1517
// Types.
1618
import { unsetValue } from "../core/properties";
1719
import { Animation } from "./animation";
@@ -143,25 +145,32 @@ export class KeyframeAnimation implements KeyframeAnimationDefinition {
143145
}
144146

145147
public cancel() {
146-
if (this._isPlaying) {
147-
this._isPlaying = false;
148-
for (let i = this._nativeAnimations.length - 1; i >= 0; i--) {
149-
let animation = this._nativeAnimations[i];
150-
if (animation.isPlaying) {
151-
animation.cancel();
152-
}
153-
}
154-
if (this._nativeAnimations.length > 0) {
155-
let animation = this._nativeAnimations[0];
156-
this._resetAnimationValues(this._target, animation);
148+
if (!this.isPlaying) {
149+
traceWrite("Keyframe animation is already playing.", traceCategories.Animation, traceType.warn);
150+
return;
151+
}
152+
153+
this._isPlaying = false;
154+
for (let i = this._nativeAnimations.length - 1; i >= 0; i--) {
155+
let animation = this._nativeAnimations[i];
156+
if (animation.isPlaying) {
157+
animation.cancel();
157158
}
158-
this._rejectAnimationFinishedPromise();
159159
}
160+
if (this._nativeAnimations.length > 0) {
161+
let animation = this._nativeAnimations[0];
162+
this._resetAnimationValues(this._target, animation);
163+
}
164+
this._rejectAnimationFinishedPromise();
160165
}
161166

162167
public play(view: View): Promise<void> {
163168
if (this._isPlaying) {
164-
throw new Error("Animation is already playing.");
169+
const reason = "Keyframe animation is already playing.";
170+
traceWrite(reason, traceCategories.Animation, traceType.warn);
171+
return new Promise<void>((resolve, reject) => {
172+
reject(reason);
173+
});
165174
}
166175

167176
let animationFinishedPromise = new Promise<void>((resolve, reject) => {

0 commit comments

Comments
 (0)