fix(animations): generate aot code for animation trigger output events #12291

Merged
merged 1 commit into from Oct 19, 2016

Conversation

Projects
None yet
4 participants
@matsko
Member

matsko commented Oct 14, 2016

No description provided.

@googlebot googlebot added the cla: yes label Oct 14, 2016

@matsko

This comment has been minimized.

Show comment
Hide comment
@matsko

matsko Oct 14, 2016

Member

@tbosch non jit code fails for animation integration tests.

Also, we now explose player within animations (which I think is a useful feature to have since it allows for users to get access to the player to pause and cancel the animation programmatically).

Member

matsko commented Oct 14, 2016

@tbosch non jit code fails for animation integration tests.

Also, we now explose player within animations (which I think is a useful feature to have since it allows for users to get access to the player to pause and cancel the animation programmatically).

matsko added a commit to matsko/angular that referenced this pull request Oct 14, 2016

@matsko

This comment has been minimized.

Show comment
Hide comment
@matsko

matsko Oct 14, 2016

Member

Aot Sample:

    _View_App0.prototype.detectChangesInternal = function(throwOnChange) {
        var self = this;
        var transitionAnimation = null ;
        var currVal_4 = self.context.openClose;
        if (jit_checkBinding6(throwOnChange, self._expr_4, currVal_4)) {
            var oldRenderVar = self._expr_4;
            if ((oldRenderVar == jit_UNINITIALIZED5) ) {
                (oldRenderVar = 'void');
            }
            var newRenderVar = currVal_4;
            if ((newRenderVar == jit_UNINITIALIZED5) ) {
                (newRenderVar = 'void');
            }
            transitionAnimation = self.componentType.animations['myTrigger'](self, self._el_8, oldRenderVar, newRenderVar);
            self._expr_4 = currVal_4;
        }
        if (transitionAnimation) {
            transitionAnimation.player.onDone(self.eventHandler(self._handle_myTrigger_8_0.bind(self, transitionAnimation)));
        }
        if (transitionAnimation) {
            transitionAnimation.player.onStart(self.eventHandler(self._handle_myTrigger_8_1.bind(self, transitionAnimation)));
        }
        self.detectContentChildrenChanges(throwOnChange);
        self.detectViewChildrenChanges(throwOnChange);
    }
Member

matsko commented Oct 14, 2016

Aot Sample:

    _View_App0.prototype.detectChangesInternal = function(throwOnChange) {
        var self = this;
        var transitionAnimation = null ;
        var currVal_4 = self.context.openClose;
        if (jit_checkBinding6(throwOnChange, self._expr_4, currVal_4)) {
            var oldRenderVar = self._expr_4;
            if ((oldRenderVar == jit_UNINITIALIZED5) ) {
                (oldRenderVar = 'void');
            }
            var newRenderVar = currVal_4;
            if ((newRenderVar == jit_UNINITIALIZED5) ) {
                (newRenderVar = 'void');
            }
            transitionAnimation = self.componentType.animations['myTrigger'](self, self._el_8, oldRenderVar, newRenderVar);
            self._expr_4 = currVal_4;
        }
        if (transitionAnimation) {
            transitionAnimation.player.onDone(self.eventHandler(self._handle_myTrigger_8_0.bind(self, transitionAnimation)));
        }
        if (transitionAnimation) {
            transitionAnimation.player.onStart(self.eventHandler(self._handle_myTrigger_8_1.bind(self, transitionAnimation)));
        }
        self.detectContentChildrenChanges(throwOnChange);
        self.detectViewChildrenChanges(throwOnChange);
    }
@matsko

This comment has been minimized.

Show comment
Hide comment
@matsko

matsko Oct 14, 2016

Member

main refactorings:

  • generate events within onDone and onStart
  • return only player from animation factory
  • register inside of the if block
Member

matsko commented Oct 14, 2016

main refactorings:

  • generate events within onDone and onStart
  • return only player from animation factory
  • register inside of the if block

matsko added a commit to matsko/angular that referenced this pull request Oct 17, 2016

@matsko

This comment has been minimized.

Show comment
Hide comment
@matsko

matsko Oct 17, 2016

Member

Codegen looks like this now:

_View_App0.prototype.detectChangesInternal = function(throwOnChange) {
    var self = this;
    var currVal_4 = self.context.openClose;
    if (jit_checkBinding6(throwOnChange, self._expr_4, currVal_4)) {
        var oldRenderVar = self._expr_4;
        if ((oldRenderVar == jit_UNINITIALIZED5) ) {
            (oldRenderVar = 'void');
        }
        var newRenderVar = currVal_4;
        if ((newRenderVar == jit_UNINITIALIZED5) ) {
            (newRenderVar = 'void');
        }
        var animationTransition_myTrigger = self.componentType.animations['myTrigger'](self, self._el_8, oldRenderVar, newRenderVar);
        animationTransition_myTrigger.onDone(self._handle_myTrigger_8_0.bind(self));
        animationTransition_myTrigger.onStart(self._handle_myTrigger_8_1.bind(self));
        self._expr_4 = currVal_4;
    }
    self.detectContentChildrenChanges(throwOnChange);
    self.detectViewChildrenChanges(throwOnChange);
};
Member

matsko commented Oct 17, 2016

Codegen looks like this now:

_View_App0.prototype.detectChangesInternal = function(throwOnChange) {
    var self = this;
    var currVal_4 = self.context.openClose;
    if (jit_checkBinding6(throwOnChange, self._expr_4, currVal_4)) {
        var oldRenderVar = self._expr_4;
        if ((oldRenderVar == jit_UNINITIALIZED5) ) {
            (oldRenderVar = 'void');
        }
        var newRenderVar = currVal_4;
        if ((newRenderVar == jit_UNINITIALIZED5) ) {
            (newRenderVar = 'void');
        }
        var animationTransition_myTrigger = self.componentType.animations['myTrigger'](self, self._el_8, oldRenderVar, newRenderVar);
        animationTransition_myTrigger.onDone(self._handle_myTrigger_8_0.bind(self));
        animationTransition_myTrigger.onStart(self._handle_myTrigger_8_1.bind(self));
        self._expr_4 = currVal_4;
    }
    self.detectContentChildrenChanges(throwOnChange);
    self.detectViewChildrenChanges(throwOnChange);
};

matsko added a commit to matsko/angular that referenced this pull request Oct 17, 2016

matsko added a commit to matsko/angular that referenced this pull request Oct 17, 2016

@@ -172,12 +176,32 @@ function bindAndWriteToRenderer(
[newRenderVar.set(emptyStateValue).toStmt()]));

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Remove the variables oldRenderVar and newRenderVar, and use a ternary expression instead.

...animationFnExpr.callFn([o.THIS_EXPR,
  oldRenderValue.equals(o.importExpr(resolveIdentifier(Identifiers.UNINITIALIZED))
    .conditional(oldRenderVar, emptyStateValue)),
  newRenderValue.equals(o.importExpr(resolveIdentifier(Identifiers.UNINITIALIZED))
    .conditional(newRenderVar, emptyStateValue)),
...
@tbosch

tbosch Oct 18, 2016

Member

Remove the variables oldRenderVar and newRenderVar, and use a ternary expression instead.

...animationFnExpr.callFn([o.THIS_EXPR,
  oldRenderValue.equals(o.importExpr(resolveIdentifier(Identifiers.UNINITIALIZED))
    .conditional(oldRenderVar, emptyStateValue)),
  newRenderValue.equals(o.importExpr(resolveIdentifier(Identifiers.UNINITIALIZED))
    .conditional(newRenderVar, emptyStateValue)),
...
@@ -290,7 +292,7 @@ class _AnimationBuilder implements AnimationAstVisitor {
new o.FnParam(_ANIMATION_CURRENT_STATE_VAR.name, o.DYNAMIC_TYPE),
new o.FnParam(_ANIMATION_NEXT_STATE_VAR.name, o.DYNAMIC_TYPE)
],
- statements);
+ statements, o.DYNAMIC_TYPE);

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

This should be o.importType(resolveIdentifier(Identifiers.AnimationTransition)), as the factory returns an AnimationTransition, right?

@tbosch

tbosch Oct 18, 2016

Member

This should be o.importType(resolveIdentifier(Identifiers.AnimationTransition)), as the factory returns an AnimationTransition, right?

@@ -10,6 +10,7 @@ import {CompileDirectiveMetadata} from '../compile_metadata';
import {isPresent} from '../facade/lang';
import {identifierToken} from '../identifiers';
import * as o from '../output/output_ast';
+import {ViewType} from '../private_import_core';

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Not needed

@tbosch

tbosch Oct 18, 2016

Member

Not needed

- listener.listenToAnimation();
- } else {
+ // the animation listeners are handled within property_binder.ts to
+ // allow then to be placed next to the animation factory statements

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

typo: allow them...

@tbosch

tbosch Oct 18, 2016

Member

typo: allow them...

compileMethod = view.animationBindingsMethod;
+ const _ANIMATION_TRANSITION_VAR = o.variable('animationTransition_' + animationName);

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

remove _ prefix, and don't upper case everything. This is just a variable here, not a constant defined at the top leve.

@tbosch

tbosch Oct 18, 2016

Member

remove _ prefix, and don't upper case everything. This is just a variable here, not a constant defined at the top leve.

compileMethod = view.animationBindingsMethod;
+ const _ANIMATION_TRANSITION_VAR = o.variable('animationTransition_' + animationName);

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Move this further down, right before you declare the variable (as it is not used before that).

@tbosch

tbosch Oct 18, 2016

Member

Move this further down, right before you declare the variable (as it is not used before that).

- view.detachMethod.addStmt(
- animationFnExpr.callFn([o.THIS_EXPR, renderNode, oldRenderValue, emptyStateValue])
- .toStmt());
+ detachStmts.push(_ANIMATION_TRANSITION_VAR

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Why are we always creating the animation again on view detach?

@tbosch

tbosch Oct 18, 2016

Member

Why are we always creating the animation again on view detach?

+ eventListeners.forEach(listener => {
+ if (listener.isAnimation && listener.eventName === animationName) {
+ let callbackMethod = listener.eventPhase == 'start' ? 'onStart' : 'onDone';
+ let transitionEvent = o.variable('e');

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

this variable is not used any more.

@tbosch

tbosch Oct 18, 2016

Member

this variable is not used any more.

+ _ANIMATION_TRANSITION_VAR
+ .callMethod(
+ callbackMethod,
+ [o.THIS_EXPR.prop(listener.methodName).callMethod('bind', [o.THIS_EXPR])])

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Use .callMethod(o.BuiltinMethod.Bind, ..., don't pass in bind as a string.

@tbosch

tbosch Oct 18, 2016

Member

Use .callMethod(o.BuiltinMethod.Bind, ..., don't pass in bind as a string.

+
+ eventListeners.forEach(listener => {
+ if (listener.isAnimation && listener.eventName === animationName) {
+ let callbackMethod = listener.eventPhase == 'start' ? 'onStart' : 'onDone';

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Move the logic here into CompileEventListener.listenToAnimation.

@tbosch

tbosch Oct 18, 2016

Member

Move the logic here into CompileEventListener.listenToAnimation.

+ private _totalTime: number;
+
+ constructor(
+ public player: AnimationPlayer,

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Why does the player need to be public?

@tbosch

tbosch Oct 18, 2016

Member

Why does the player need to be public?

+
+ constructor(
+ public player: AnimationPlayer,
+ {fromState, toState, totalTime}: {fromState: string, toState: string, totalTime: number}) {

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Don't use named arguments here, just positional ones and declare the fields as well. E.g.

constructor(private _player: AnimationPlayer, private _fromState: string, ...) {}
@tbosch

tbosch Oct 18, 2016

Member

Don't use named arguments here, just positional ones and declare the fields as well. E.g.

constructor(private _player: AnimationPlayer, private _fromState: string, ...) {}
fromState: string;
toState: string;
totalTime: number;
+ phaseName: string;

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

Please ping @IgorMinar whether this is a breaking change, and we need to make this field optional. I don't expect users to new up their own AnimationTransitionEvent.

@tbosch

tbosch Oct 18, 2016

Member

Please ping @IgorMinar whether this is a breaking change, and we need to make this field optional. I don't expect users to new up their own AnimationTransitionEvent.

+ private _totalTime: number) {}
+
+ /** @internal */
+ _createEvent(phaseName: string): AnimationTransitionEvent {

This comment has been minimized.

@tbosch

tbosch Oct 18, 2016

Member

why not private

@tbosch

tbosch Oct 18, 2016

Member

why not private

matsko added a commit to matsko/angular that referenced this pull request Oct 18, 2016

@alxhub alxhub merged commit 6e5f8b5 into angular:master Oct 19, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed

@matsko matsko deleted the matsko:codegen_animation_outputs branch Oct 19, 2016

btrigueiro added a commit to btrigueiro/angular that referenced this pull request Oct 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment