Skip to content
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.

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

Partial flash.events.IEventDispatcher implementation #394

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Partial flash.events.IEventDispatcher implementation #394

wants to merge 8 commits into from

Conversation

joshtynjala
Copy link
Contributor

The following changes allow starling.events.EventDispatcher to implement flash.events.IEventDispatcher.

A few important warnings:

  1. The capture phase is not implemented. addEventListener() and removeEventListener() name the argument notSupported to try to make this clear. Currently, it is completely ignored, but we might consider throwing a runtime error if this argument is set to true.

  2. Priorities are not supported. addEventListener() and removeEventListener() name the argument notSupported2 to try to make this clear. Currently, it is completely ignored, but we might consider throwing a runtime error if this argument is set to any value but 0.

  3. Weak references are not supported. addEventListener() and removeEventListener() name the argument notSupported3 to try to make this clear. Currently, it is completely ignored, but we might consider throwing a runtime error if this argument is set to true.

For all three of the "notSupported" arguments, a quick line in the documentation should also help make it abundantly clear that they're not supported.

  1. dispatchEvent() requires a flash.events.Event instance. Since flash.events.Event has properties that starling.events.EventDispatcher cannot change, it must throw a runtime error if it receives an argument that isn't a starling.events.Event or a subclass of it. Similar to the items above, I feel that a note in the documentation should clear things up if the runtime error isn't clear enough.

The following new features have been added:

  1. Events may be cancelable. I added a cancelable argument at the end of the constructor, fromPool() function and reset() function. preventDefault() and isDefaultPrevented() are also implemented. dispatchEvent() and dispatchEventWith() return a Boolean to indicate if the event was cancelled, since IEventDispatcher defines that in the signature for dispatchEvent().

  2. I exposed the eventPhase getter to indicate if its "at target" or bubbling.

  3. Events are not cloned when they bubble, but starling.events.Event overrides the clone() function properly. I've heard that some people like to call clone() to redispatch events sometimes, so it makes sense to have it implemented since flash.events.Event defines it.

This change offers the following additional advantages:

  1. Unless they specifically need to reference a property of starling.events.Event that isn't on flash.events.Event (that's only the data property, and there's a good solution for accessing that without importing starling.events.Event), people can simply import flash.events.Event most of the time, which gets rid of the requirement to use the fully-qualified class names when flash.events.Event is needed with other dispatchers in the same class. This is probably the most common class name conflict with native Flash classes, so it should actually help a lot.

Event listeners can simply accept flash.events.Event instead of starling.events.Event. If someone needs the data property, they can use the two-argument signature on their listeners:

function listener(event:Event, data:Object):void

The data argument can be strongly typed too, so it's better than calling the data getter on the event since that's always typed as Object.

For dispatching pooled events with dispatchEventWith(), the type constants on flash.events.Event work just as well as the constants on starling.events.Event since they're simply Strings. There shouldn't be any reason to call dispatchEvent() with a starling.events.Event, since you lose the advantages of pooling, so no one should need to import starling.events.Event for that either.

If someone continues to import starling.events.Event in new or existing code, it will continue to work the same way as before. That change won't break anything, and new code can benefit from importing flash.events.Event instead.

  1. Starling and Feathers will work better in MXML since MXML requires IEventDispatcher to use some of its important features.

I feel like advantage 1 is the big one, but advantage 2 will make many Feathers users happy.

This should not break most existing code. If someone overrides addEventListener(), removeEventListener(), dispatchEvent(), or dispatchEventWith(), they will need to change the signature. I do this in a couple of places in Feathers, but it's an easy change. I doubt that many other people are adding overrides as such a low level.

What do you think?

@PrimaryFeather
Copy link
Contributor

Wow, that's an interesting idea! I want to look at all your changes properly before commenting on them. I'll look at it after Starling 1.4 is out (and I'm done with some other pressing tasks in my queue).

Thanks, Josh!

@joshtynjala
Copy link
Contributor Author

I realized that there are a couple important constants on starling.events.Event that aren't on flash.events.Event, like TRIGGERED for buttons. With that in mind, the conflict avoidance won't always work out as well as I hoped.

@k0t0vich
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants