From beef7f93728ce30e2cf1b1eef90907e7e6b8ffb9 Mon Sep 17 00:00:00 2001 From: Rossen Hristov Date: Mon, 21 Sep 2015 13:27:10 +0300 Subject: [PATCH 1/2] Fixed: If an Animation instance is played more than once, the same promise is resolved each time leading to unexpected results. #774 --- apps/animations/main-page.ts | 8 +- apps/tests/ui/animation/animation-tests.ts | 159 +++++++++++++++------ ui/animation/animation-common.ts | 19 +-- ui/animation/animation.android.ts | 12 +- ui/animation/animation.d.ts | 3 +- ui/animation/animation.ios.ts | 8 +- ui/core/view-common.ts | 2 +- 7 files changed, 132 insertions(+), 79 deletions(-) diff --git a/apps/animations/main-page.ts b/apps/animations/main-page.ts index 39c7d293f..c1773befd 100644 --- a/apps/animations/main-page.ts +++ b/apps/animations/main-page.ts @@ -38,8 +38,8 @@ export function onSlideOut(args: observable.EventData) { panelAnimation = panel.createAnimation({ opacity: 0, scale: { x: 0.5, y: 0.5 }, rotate: -360, backgroundColor: new colorModule.Color("red"), duration: vm.duration, iterations: vm.iterations }); - buttonAnimation.play().finished - .then(() => panelAnimation.play().finished) + buttonAnimation.play() + .then(() => panelAnimation.play()) .catch((e) => console.log(e.message)); } @@ -56,8 +56,8 @@ export function onSlideIn(args: observable.EventData) { ] buttonAnimation = new animationModule.Animation(buttonAnimations, vm.playSequentially); - panelAnimation.play().finished - .then(() => buttonAnimation.play().finished) + panelAnimation.play() + .then(() => buttonAnimation.play()) .catch((e) => console.log(e.message)); } diff --git a/apps/tests/ui/animation/animation-tests.ts b/apps/tests/ui/animation/animation-tests.ts index a7ffad6f6..3c06d3b01 100644 --- a/apps/tests/ui/animation/animation-tests.ts +++ b/apps/tests/ui/animation/animation-tests.ts @@ -81,7 +81,7 @@ export var test_CancellingAnimation = function (done) { // # Cancelling animation // ``` JavaScript var animation1 = label.createAnimation({ translate: { x: 100, y: 100 } }); - animation1.play().finished + animation1.play() .then(() => { ////console.log("Animation finished"); // @@ -145,51 +145,51 @@ export var test_ChainingAnimations = function (done) { // } -//export var test_ReusingAnimations = function (done) { -// var mainPage: pageModule.Page; -// var label: labelModule.Label; -// var pageFactory = function (): pageModule.Page { -// label = new labelModule.Label(); -// label.text = "label"; -// var stackLayout = new stackLayoutModule.StackLayout(); -// stackLayout.addChild(label); -// mainPage = new pageModule.Page(); -// mainPage.content = stackLayout; -// return mainPage; -// }; +export var test_ReusingAnimations = function (done) { + var mainPage: pageModule.Page; + var label: labelModule.Label; + var pageFactory = function (): pageModule.Page { + label = new labelModule.Label(); + label.text = "label"; + var stackLayout = new stackLayoutModule.StackLayout(); + stackLayout.addChild(label); + mainPage = new pageModule.Page(); + mainPage.content = stackLayout; + return mainPage; + }; -// helper.navigate(pageFactory); -// TKUnit.waitUntilReady(() => { return label.isLoaded }); + helper.navigate(pageFactory); + TKUnit.waitUntilReady(() => { return label.isLoaded }); -// // -// // # Reusing animations -// // ``` JavaScript -// var animation1 = label.createAnimation({ translate: { x: 100, y: 100 } }); -// var animation2 = label.createAnimation({ translate: { x: 0, y: 0 } }); + // + // # Reusing animations + // ``` JavaScript + var animation1 = label.createAnimation({ translate: { x: 100, y: 100 } }); + var animation2 = label.createAnimation({ translate: { x: 0, y: 0 } }); -// animation1.play().finished -// .then(() => animation1.play().finished) -// .then(() => animation1.play().finished) -// .then(() => animation2.play().finished) -// .then(() => animation1.play().finished) -// .then(() => animation2.play().finished) -// .then(() => { -// ////console.log("Animation finished"); -// // -// helper.goBack(); -// done(); -// // -// }) -// .catch((e) => { -// console.log(e.message); -// // -// helper.goBack(); -// done(e); -// // -// }); -// // ``` -// // -//} + animation1.play() + .then(() => animation2.play()) + .then(() => animation1.play()) + .then(() => animation2.play()) + .then(() => animation1.play()) + .then(() => animation2.play()) + .then(() => { + ////console.log("Animation finished"); + // + helper.goBack(); + done(); + // + }) + .catch((e) => { + console.log(e.message); + // + helper.goBack(); + done(e); + // + }); + // ``` + // +} export var test_AnimatingMultipleViews = function (done) { var mainPage: pageModule.Page; @@ -223,7 +223,7 @@ export var test_AnimatingMultipleViews = function (done) { { target: label3, translate: { x: 200, y: 200 }, duration: 1000, delay: 666 }, ]; var a = new animation.Animation(animations); - a.play().finished + a.play() .then(() => { ////console.log("Animations finished"); // @@ -404,10 +404,13 @@ export var test_AnimationsAreAlwaysPlayed = function (done) { var animation1 = label.createAnimation({ opacity: 0 }); var animation2 = label.createAnimation({ opacity: 1 }); - animation1.play().finished - .then(() => animation2.play().finished) + animation1.play() .then(() => { - TKUnit.assert(label.opacity === 1, `Label opacity expected vaue is 1, actual value is ${label.opacity}.`); + TKUnit.assert(label.opacity === 0, `Label opacity should be 0 after first animation, actual value is ${label.opacity}.`); + return animation2.play() + }) + .then(() => { + TKUnit.assert(label.opacity === 1, `Label opacity should be 1 after second animation, actual value is ${label.opacity}.`); helper.goBack(); done(); }) @@ -417,3 +420,65 @@ export var test_AnimationsAreAlwaysPlayed = function (done) { done(e); }); } + +export var test_PlayPromiseIsResolvedWhenAnimationFinishes = function (done) { + var mainPage: pageModule.Page; + var label: labelModule.Label; + var pageFactory = function (): pageModule.Page { + label = new labelModule.Label(); + label.text = "label"; + var stackLayout = new stackLayoutModule.StackLayout(); + stackLayout.addChild(label); + mainPage = new pageModule.Page(); + mainPage.content = stackLayout; + return mainPage; + }; + + helper.navigate(pageFactory); + TKUnit.waitUntilReady(() => { return label.isLoaded }); + + var animation = label.createAnimation({ opacity: 0, duration: 1000 }); + + animation.play() + .then(function onResolved() { + TKUnit.assert(animation.isPlaying === false, "Animation.isPlaying should be false when animation play promise is resolved."); + helper.goBack(); + done(); + }, function onRejected(e) { + TKUnit.assert(1 === 2, "Animation play promise should be resolved, not rejected."); + helper.goBack(); + done(e); + }); +} + +export var test_PlayPromiseIsRejectedWhenAnimationIsCancelled = function (done) { + var mainPage: pageModule.Page; + var label: labelModule.Label; + var pageFactory = function (): pageModule.Page { + label = new labelModule.Label(); + label.text = "label"; + var stackLayout = new stackLayoutModule.StackLayout(); + stackLayout.addChild(label); + mainPage = new pageModule.Page(); + mainPage.content = stackLayout; + return mainPage; + }; + + helper.navigate(pageFactory); + TKUnit.waitUntilReady(() => { return label.isLoaded }); + + var animation = label.createAnimation({ opacity: 0, duration: 1000 }); + + animation.play() + .then(function onResolved() { + TKUnit.assert(1 === 2, "Animation play promise should be rejected, not resolved."); + helper.goBack(); + done(); + }, function onRejected(e) { + TKUnit.assert(animation.isPlaying === false, "Animation.isPlaying should be false when animation play promise is rejected."); + helper.goBack(); + done(); + }); + + animation.cancel(); +} \ No newline at end of file diff --git a/ui/animation/animation-common.ts b/ui/animation/animation-common.ts index 906aac1c3..696039b24 100644 --- a/ui/animation/animation-common.ts +++ b/ui/animation/animation-common.ts @@ -26,15 +26,19 @@ export class Animation implements definition.Animation { private _isPlaying: boolean; private _resolve; private _reject; - private _animationFinishedPromise: Promise; - public play(): Animation { + public play(): Promise { if (this.isPlaying) { throw new Error("Animation is already playing."); } + var animationFinishedPromise = new Promise((resolve, reject) => { + this._resolve = resolve; + this._reject = reject; + }); + this._isPlaying = true; - return this; + return animationFinishedPromise; } public cancel(): void { @@ -43,10 +47,6 @@ export class Animation implements definition.Animation { } } - public get finished(): Promise { - return this._animationFinishedPromise; - } - public get isPlaying(): boolean { return this._isPlaying; } @@ -70,11 +70,6 @@ export class Animation implements definition.Animation { trace.write("Created " + this._propertyAnimations.length + " individual property animations.", trace.categories.Animation); this._playSequentially = playSequentially; - var that = this; - this._animationFinishedPromise = new Promise((resolve, reject) => { - that._resolve = resolve; - that._reject = reject; - }); } public _resolveAnimationFinishedPromise() { diff --git a/ui/animation/animation.android.ts b/ui/animation/animation.android.ts index 817500d7c..006629cd4 100644 --- a/ui/animation/animation.android.ts +++ b/ui/animation/animation.android.ts @@ -18,8 +18,8 @@ export class Animation extends common.Animation implements definition.Animation private _propertyUpdateCallbacks: Array; private _propertyResetCallbacks: Array; - public play(): Animation { - super.play(); + public play(): Promise { + var animationFinishedPromise = super.play(); var i: number; var length: number; @@ -34,12 +34,6 @@ export class Animation extends common.Animation implements definition.Animation this._createAnimators(this._propertyAnimations[i]); } - if (this._animators.length === 0) { - trace.write("Nothing to animate.", trace.categories.Animation); - this._resolveAnimationFinishedPromise(); - return this; - } - this._nativeAnimatorsArray = java.lang.reflect.Array.newInstance(android.animation.Animator.class, this._animators.length); i = 0; length = this._animators.length; @@ -58,7 +52,7 @@ export class Animation extends common.Animation implements definition.Animation trace.write("Starting " + this._nativeAnimatorsArray.length + " animations " + (this._playSequentially ? "sequentially." : "together."), trace.categories.Animation); this._animatorSet.start(); - return this; + return animationFinishedPromise; } public cancel(): void { diff --git a/ui/animation/animation.d.ts b/ui/animation/animation.d.ts index f1a0ac6f0..cb9c3379d 100644 --- a/ui/animation/animation.d.ts +++ b/ui/animation/animation.d.ts @@ -72,9 +72,8 @@ */ export class Animation { constructor(animationDefinitions: Array, playSequentially?: boolean); - public play: () => Animation; + public play: () => Promise; public cancel: () => void; - public finished: Promise; public isPlaying: boolean; } } \ No newline at end of file diff --git a/ui/animation/animation.ios.ts b/ui/animation/animation.ios.ts index d7998e0cc..6bd44ab00 100644 --- a/ui/animation/animation.ios.ts +++ b/ui/animation/animation.ios.ts @@ -46,14 +46,14 @@ export class Animation extends common.Animation implements definition.Animation private _cancelledAnimations: number; private _mergedPropertyAnimations: Array; - public play(): Animation { - super.play(); + public play(): Promise { + var animationFinishedPromise = super.play(); this._finishedAnimations = 0; this._cancelledAnimations = 0; - this._iOSAnimationFunction(); - return this; + this._iOSAnimationFunction(); + return animationFinishedPromise; } public cancel(): void { diff --git a/ui/core/view-common.ts b/ui/core/view-common.ts index daed3d05f..7baf6c72a 100644 --- a/ui/core/view-common.ts +++ b/ui/core/view-common.ts @@ -1099,7 +1099,7 @@ export class View extends proxy.ProxyObject implements definition.View { } public animate(animation: animationModule.AnimationDefinition): Promise { - return this.createAnimation(animation).play().finished; + return this.createAnimation(animation).play(); } public createAnimation(animation: animationModule.AnimationDefinition): animationModule.Animation { From 911e5fd72a6a1f74bb50fe0a0467928a3c91d985 Mon Sep 17 00:00:00 2001 From: Rossen Hristov Date: Mon, 21 Sep 2015 13:10:07 +0300 Subject: [PATCH 2/2] Added a breaking changes entry for the upcoming 1.4.0 release. --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fbd2e198..9883a9f9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,27 @@ Cross Platform Modules Changelog ============================== +##1.4.0 (planned for 2015, October 7) + +### Breaking changes +- [(#774)](https://github.com/NativeScript/NativeScript/issues/774) Animation class no longer has a **finished** property because an animation can be played multiple times. The **play** method now returns a new Promise each time it is invoked. Use this to listen for the animation finishing or being cancelled. When upgrading to version 1.4.0 or above simply remove **.finished** from your code. + +**Old Code (JavaScript)**: +```JavaScript +animation1.play().finished.then(function () { console.log("Finished"); }); +``` +**New Code (JavaScript)**: +```JavaScript +animation1.play().then(function () { console.log("Finished"); }); +``` +**Old Code (TypeScript)**: +```JavaScript +animation1.play().finished.then(()=>console.log("Finished")); +``` +**New Code (JavaScript)**: +```JavaScript +animation1.play().then(()=>console.log("Finished")); +``` + ##1.3.0 (2015, September 16) ### Fixed