robotlegs / robotlegs-framework

An ActionScript 3 application framework for Flash and Flex
https://robotlegs.tenderapp.com/
MIT License
966 stars 261 forks source link

Possible bug when mapping two commands to the same event with diffrent guards #110

Closed jakubgondek closed 11 years ago

jakubgondek commented 11 years ago

Hi,

I'm not an experienced Robotlegs user, so code below may not follow best practices, but it certainly acts a bit weird.

/**
 * Author: Jakub Gondek
 */
package
{
import flash.events.Event;
import flash.events.IEventDispatcher;
import flash.utils.setTimeout;

import robotlegs.bender.extensions.eventCommandMap.api.IEventCommandMap;
import robotlegs.bender.framework.api.IConfig;
import robotlegs.bender.framework.api.IContext;
import robotlegs.bender.framework.api.LogLevel;

public class ApplicationConfig implements IConfig
{
    [Inject]
    public var context : IContext;

    [Inject]
    public var eventCommandMap : IEventCommandMap;

    [Inject]
    public var eventDispatcher : IEventDispatcher;

    public static const TEST : String = "test";

    public function configure() : void
    {
        context.logLevel = LogLevel.INFO;

        eventCommandMap
                .map(TEST, Event)
                .toCommand(Command1)
                .withGuards(
                        OnlyIfCondition1
                );

        eventCommandMap
                .map(TEST, Event)
                .toCommand(Command2)
                .withGuards(
                        OnlyIfCondition2
                );
        context.afterInitializing(function () : void
        {
            eventDispatcher.dispatchEvent(new Event(TEST));
            setTimeout(function () : void
            {
                TEST_RobotlegsBug.condition = !TEST_RobotlegsBug.condition;
                eventDispatcher.dispatchEvent(new Event(TEST));
            }, 5000);
        });
    }
}
}

internal class Command1
{
    public function execute() : void
    {
        trace("Command 1 executed");
    }
}

internal class Command2
{
    public function execute() : void
    {
        trace("Command 2 executed");
    }
}

internal class OnlyIfCondition1
{
    public function approve() : Boolean
    {
        trace("OnlyIfCondition1: " + TEST_RobotlegsBug.condition);
        return TEST_RobotlegsBug.condition;
    }
}

internal class OnlyIfCondition2
{
    public function approve() : Boolean
    {
        trace("OnlyIfCondition2: " + (!TEST_RobotlegsBug.condition));
        return !TEST_RobotlegsBug.condition;
    }
}

Here is the trace that I get:

OnlyIfCondition1: false OnlyIfCondition2: true Command 2 executed OnlyIfCondition1: true Command 1 executed OnlyIfCondition2: false Command 1 executed

and here is what I would expect:

OnlyIfCondition1: false OnlyIfCondition2: true Command 2 executed OnlyIfCondition1: true OnlyIfCondition2: false Command 1 executed

I'm using Robotlegs 2.0.0b4

Best regards Jakub Gondek

PS. Sory for the markdown issues if any.

Ondina commented 11 years ago

I think that’s because the same command gets executed more than once in EventCommandExecutor.execute(), even when guardsApprove returns false, in case there are 2 or more mappings for the same event type.

https://github.com/robotlegs/robotlegs-framework/blob/master/src/robotlegs/bender/extensions/eventCommandMap/impl/EventCommandExecutor.as

On line 81:

For each mapped guard, if guardsApprove returns true, a command is instantiated:

command = _injector.instantiateUnmapped(commandClass);

and then


if(command)
{
    mapping.fireOnce && _trigger.removeMapping(mapping);
    command.execute();
}

If there are 2 or more mappings for the same event, in case guardsApprove returns false for the subsequent mappings, the command should be null or flagged as executed.

I haven’t run any tests, I just did this:


if (mapping.guards.length == 0 || guardsApprove(mapping.guards, _injector))
{
    command = _injector.instantiateUnmapped(commandClass);
    //rest of the code…
}
else
{
    command = null;
}

And after that it worked as expected. Though, I’m not sure if this solves the issue, or if the code will explode because of the change. Shaun will know :)

darscan commented 11 years ago

@jakubgondek Thanks for catching that. @Ondina Thanks for pointing me in the right direction!