Stray / robotlegs-utilities-RelaxedEventMap

Just what it says - a robotlegs eventMap that lets you be a little more relaxed about race conditions and late-arriving views
17 stars 2 forks source link

Manually unmap your listeners? #2

Closed rickcr closed 13 years ago

rickcr commented 13 years ago

Been tracking down all kinds of odd behavior I was seeing today. (By the way States and RL are a tough beast, but more on that maybe later when I have the energy to explain.. I think I need to slash some beasts in DragonAge after this day:)...

In this current application, things are very dynamic. We add create views at runtime and add them to another view component and we destroy and remove them as well when the user picks some other option within the application.

In your docs you mention: "If you need to de-register for the event when your view leaves the stage, override onRemove and use relaxedCommandMap.unmapListener manually." (by the way I think you meant relaxedEventMap, no?)

I'm currently having to deal with this now, and I don't see that method on the RelaxedEventMap. I added one called unregisterListener to my local version of RelaxedEventMap. (Didn't want to override EventMap.unmapListener because then the user would have to provide this.eventDispatcher as the first param):

  public function unregisterListener(type:String, listener:Function, eventClass:Class = null,
                                           useCapture:Boolean = false):void {
        super.unmapListener(this.eventDispatcher, type, listener, eventClass, useCapture);
    }
Stray commented 13 years ago

Thanks Rick - I've added a symmetrical method - unmapRelaxedListener - that does just what your unregisterListener does.

The unmapListener method is inherited from EventMap, so it should have been fine to just do relaxedEventMap.unmapListener(eventDispatcher... etc) but it's much better to have it symmetrical as you say.

And yes.. relaxedCommandMap? What a load of nonsense!

Thanks for picking this up!

rickcr commented 13 years ago

I'm thinking of just patching base Mediator in my code to handle this...

The base Mediator could have the global instance of relaxedEventMap injected into it. Not even that big of a deal if it's not used.

You register your relaxed listeners on the mediator itself in your onRegister: onRegister { this.mapRelaxedListener(... ) } (In fact I'll probably just add a similar standard mapListener method to the base Mediator also.)

The mediator keeps track of the relaxed listeners added and obviously each call to the medaiotr's mapRelaxedListener also then does the real call to the mapRelaxedListener on relaxedEventMap.

When the mediator's preRemove is called it then also cycles over any listeners added for the relaxedEventMap and unmaps them.

one less thing the user has to worry about handling.

Stray commented 13 years ago

nice idea - in fact, so nice that I wonder whether this approach would be useful:

I'm not sure who should have responsibility for this - the mediator or the eventMap? What's your preference?

rickcr commented 13 years ago

I like where this is going. Since you'll have to modify core, might as well get rid of IRelaxedEventContext and then push all that set up junk directly into the core Context class. This cleans things up a lot right off the bat. (User would only need to define the dummy listener stuff in their Context or bootstrap.)

I thought about the param to mapRelaxedListener, but would you ever be mapping things outside of a Mediator? If not, this is where I think modifying the Mediator class a lot would help (the end user wouldn't even need to know about passing the owner)...

In regard to who has the responsibility, I'm leaning towards the eventMap with the hooks as above from the Mediator. Not sold one way or the other on that though.

It sounds like the above would clean things up a lot. Since maybe things(community?) isn't ready for this in core RL, could we leverage the dynamic nature of ActionScript and possibly dynamically add all these properties and methods to Context and Mediator, possibly in some bootstrapped helper class that is triggered early in the setup? I don't think that'll work though for things like Mediator. I'm don't think ActionScript lets you do things like Ruby does. In Ruby you could modify the core Mediator class at run time, so any new instances of the Mediator would then have your modifications (I've only seen the dynamic adding of things in ActionScript to instances of a class through the prototype not to the Class itself.)

rickcr commented 13 years ago

In regard to my last paragraph I guess we can probably just provide a new subclass of Mediator and Context although in this case I'd still prefer a ruby-like patch since later when it did become part of core, users wanting to upgrade wouldn't have to alter as much (they wouldn't need to change all their Mediators back to extending the original base Mediator.)

rickcr commented 13 years ago

Regardless, until core incorporates things, I think just having users extend a "RelaxedContext" (and dropping IRelaxedContext) is a bit cleaner?

public class RelaxedContext extends Context {

    public function RelaxedContext() {
        super();
    }

    protected var _relaxedEventMap:IRelaxedEventMap;

    public function get relaxedEventMap():IRelaxedEventMap
    {
        return _relaxedEventMap ||= new RelaxedEventMap(eventDispatcher);
    }

    public function set relaxedEventMap(value:IRelaxedEventMap):void
    {
        _relaxedEventMap = value;
    }

    override protected function mapInjections():void
    {
        super.mapInjections();
        injector.mapValue(IRelaxedEventMap, relaxedEventMap);
    }

}
Stray commented 13 years ago

Hi rick,

The extendable context is a nice idea but it comes unstuck when you need, for example, modular, and /or signal command map as well.

The reason we've stuck to doing interfaces for the context is that frequently people are mixing utils that adapt the context. Either way, the interface wouldn't be dropped - Interfaces over implementation etc etc.

Re dynamic additions - the problem with dynamic is that it kills all your compiler safety, so that's definitely not a solution I'm comfortable with. But also, robotlegs actually works with interfaces, not concrete instances, so that makes that approach tricky too, because the dynamic additions are to the concrete class, and not the interface.

The whole multi-context thing is the big, big issue that needs ironing out in the next major release of robotlegs.

I don't think editing core is going to fly with most people - though you're welcome to edit your own. Apart from anything else, editing core ties you to a specific release version of RL, where this should be version-agnostic (ie works with RL 1.0 and 1.4 and anything in between!)

What would be great would be to get Shaun, Joel etc starting to engage with this - to see whether they think it has potential for core, and if not then how they expect it to integrate.

I think the robotlegs 'big idea' is that the core release is super light weight, and the utils are always add ons, so I think where robotlegs is going is towards a more well defined approach to mixins for utils.

Meantime, I'll add the responsibility for clean-ups to the relaxedEventMap based on object and we'll see how that goes? I don't think there's any way to get the scope of a handler unfortunately... might double check that!

rickcr commented 13 years ago

ah good points on the modular aspects of things. I haven't worked with RL enough to leverage all that power (although actually coming up soon I will have to probably work with a way to load up different contexts at startup. I'll cross that bridge later though.)

Yea yea compiler safety:)

I've been a Java guy for years (and still am in my day job) , but now doing some stuff with Ruby on the side at home, and wow what a relief. Ruby handles all these issues we're having now beautifully, sure at the expense of compile time safety (but it's not a compiled language) - but come-on anyway - compile time safety - that's so 2000 :) That's what we have unit tests for:)

Yea I'm torn if I'll just modify my own version of RL or not. If I was the only one in the code base I would. But I want it to handle future updates of RL more easily (which will of course be more difficult if I start patching up too many things locally.) I still might though, we'll see.

Stray commented 13 years ago

I've added that extra parameter - you'll see a new section at the bottom of the readme - basically you can now pass 'this' from your mediator when you register and then unmap all the listeners you added for 'this' in one go.

I do like Ruby, mixins and all that jazz, but the compiler saves my sorry ass so many times a day I'm not sure I'm ready to give it up (though I do test the crap out of everything too).

I've brought up the whole patching-utils thing on the group to see whether anyone has any ideas brewing.

rickcr commented 13 years ago

Excellent! (now once it's part of core preRemove can do this for us:)