Skip to content

fix(animations): change throw -> trace to avoid unnecessary app crash #5475

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.

Sign up for GitHub

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

Merged
SvetoslavTsenov merged 2 commits into from
Mar 8, 2018
Merged

fix(animations): change throw -> trace to avoid unnecessary app crash #5475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61937a2
fix(animations): change throw -> trace to avoid unnecessary app crash
NathanWalker Feb 27, 2018
60ff125
Fix fix animation throw (#1)
Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions tests/app/ui/animation/animation-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
21 changes: 12 additions & 9 deletions tns-core-modules/ui/animation/animation-common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 <AnimationPromiseDefinition>new Promise<void>((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 = <AnimationPromiseDefinition>new Promise<void>((resolve, reject) => {
Expand Down Expand Up @@ -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 {
Expand Down
19 changes: 13 additions & 6 deletions tns-core-modules/ui/animation/animation.android.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<android.animation.Animator>();
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down
11 changes: 9 additions & 2 deletions tns-core-modules/ui/animation/animation.ios.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
35 changes: 22 additions & 13 deletions tns-core-modules/ui/animation/keyframe-animation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
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<void> {
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<void>((resolve, reject) => {
reject(reason);
});
}

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