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

Adding safe event listeners, [Event] metadata autowiring, and a basic phase validation implementation. #7

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

Conversation

trxcllnt
Copy link

Points of interest/conflict (maybe):

  1. changed addListenerToTarget() method signature, promoting the useWeakListener argument above others. Some people may have strong feelings or existing code which could hinder the acceptance of this change, but the discussion is started.
  2. safe listeners:
    • function wrappers to allow the real callback to optionally implement the event argument.
    • generated function kept in a global dictionary, keyed off the function they wrap.
    • when the callback is invoked, no reflection type checking is done on the wrapped Function's argument, only a lookup on the Function.length property.
    • can introduce major memory leaks if not used carefully/correctly (this is due to Dictionary's lack of ability to use Functions as weakly referenced keys).
  3. event auto-wiring simply adds the event listener on the target object. I didn't bother to implement targeting or routing from other objects, though you're free to if you wanna.
  4. there's no way to remove phases after you've declared them. is this a feature you need? also, phases are based on the existing wait() function, may want to change this in the future to work with Event.RENDER also.

guyinthechair added 5 commits March 27, 2011 20:49
…n signature, why not promote the importance of the useWeakListener argument? Also, drops the dependency on DisplayObject in favor of IEventDispatcher.
…te event callbacks that optionally accept the event argument.
…("eventName")] metadata. Autowires using safe listeners, so it's ok to annotate functions that don't take arguments.
…e phases and their order, then use invalidate to add instances to a phase at a certain depth.

A phase expects each object to be validated to have a public method with the same name as the phase's name. For example:
<code>
addPhase("render", 1, true);
invalidate({render: function(){ trace("render was called"); }}, 0, "render");
</code>
Will trace out "render was called" on the next frame.
@mimshwright
Copy link
Contributor

Hey Guy,

Here're my thoughts on these changes for what they're worth.

  1. changed addListenerToTarget() method signature, promoting the useWeakListener argument above others. Some people may have strong feelings or existing code which could hinder the acceptance of this change, but the discussion is started.

I personally like strong links and manual removal but regardless of that, I think this should mirror what the addEventListener() function does by default which is useWeakReferences = false

Actually, these two functions seem kinda useless to me. Do we really need a separate function just to suppress a potentially helpful error?

  1. safe listeners:
  2. function wrappers to allow the real callback to optionally implement the event argument.
  3. generated function kept in a global dictionary, keyed off the function they wrap.
  4. when the callback is invoked, no reflection type checking is done on the wrapped Function's argument, only a lookup on the Function.length property.
  5. can introduce major memory leaks if not used carefully/correctly (this is due to Dictionary's lack of ability to use Functions as weakly referenced keys).

Cool idea. Needs an example use case. Seems like it could be too complex for an ad hoc utils lib but maybe not.
Also, can this not be done by just using (event:Event = null) as a param in your event handler?

  1. event auto-wiring simply adds the event listener on the target object. I didn't bother to implement targeting or routing from other objects, though you're free to if you wanna.

Cool! Needs example usage since it's not very clear from looking.

  1. there's no way to remove phases after you've declared them. is this a feature you need? also, phases are based on the existing wait() function, may want to change this in the future to work with Event.RENDER also.

Again, looks like a micro-framework, possibly too complex for this library. Needs a good use case example.
Also, typically, it's not a good idea to subclass base classes like Array but rather use proxies or composition.

Sorry if these notes seem nit-picky! Thanks for contributing.

Mims

@trxcllnt
Copy link
Author

  1. The functions already existed, only they were typed against DO instead of IED. I said the same thing as you, but while we're rewriting the method anyway, might as well make its signature more useful. I don't mind defaulting to strong listeners, the current method was already defaulted to a weak listener. I'm sure that's why the method was written in the first place, to get weak listeners without typing "false, 0, true" all over the place.
  2. Defining methods with significant optional inputs increases the complexity of testing.
  3. example:
    [Event("render")]
    public function onRender(){
    //do something on the render event here.
    }
    It's just reflection, and the Event metadata keyword is pretty standard. Note, this uses safe listeners.
  4. I think it fits in well here. It's a micro implementation of a pattern people are forced to rewrite all the time, especially in AS3-only projects, only better since it supports bidirectional traversal and phase injection.
    I think it's fine to extend Array here. It's the proper application of inheritance, it keeps the code succinct. If we weren't meant to extend it in the right places, Array would be sealed ;).

Thanks for taking the time to review this Mims.

@mimshwright
Copy link
Contributor

I merged 1. and switched it back to the default for addEventListener.
For 2. Anyone else have an opinion?
3 & 4. I think they need to have an explanation for why & how they are used added to the code before they are merged.

guyinthechair added 2 commits March 29, 2011 23:34
…object doesn't have a public function implemented for the phase name. Fixed a bug where phases wouldn't validate a second time. oops.
@trxcllnt
Copy link
Author

  1. I agree.
  2. this turns any function into an event callback, whether it accepts an event object or not.
  3. reflects on an object's public methods, looking for any annotated with [Event("eventName")] metadata. It then registers those functions as event handlers on the object for the given "eventName".
  4. implements invalidation and syncs with the display list on Event.ENTER_FRAME, only slightly abstracted so you can instantiate any number of invalidation phases you want, with directionality thrown in for good measure. Easy for subclasses of Sprite to implement invalidation/validation without a lot of hubbub.

Here are all 3 used together to draw pretty boxes when you mouse down, mouse up, and click. rendering is handled efficiently with invalidation, packaging calls made in quick succession into one validation loop.

package
{
    import flash.display.*;
    import flash.geom.Point;
    import utils.metadata.wireEvents;
    import utils.syncro.*;

    [SWF(width="600", height="600")]
    public class Main extends Sprite
    {
        addphase("render", 0);

        private var child:Sprite;

        public function Main()
        {
            super();

            child = new Sprite();
            var g:Graphics = child.graphics;
            g.beginFill(0, 0.1);
            g.drawRect(0, 0, 600, 600);
            g.endFill();
            addChild(child);

            wireEvents(this);
        }

        [Event("mouseDown")]
        [Event("mouseUp")]
        [Event("click")]
        public function onMouseDown():void
        {
            invalidate(this, 0, "render");
        }

        [Event("render")]
        public function drawSquare():void
        {
            var loc:Point = new Point(Math.random() * 600, Math.random() * 600);
            var g:Graphics = child.graphics;

            g.beginFill(
                Math.random() * 255 << 16 | 
                Math.random() * 255 << 8 | 
                Math.random() * 255, 
                Math.random());
            g.drawRect(loc.x, loc.y, Math.random() * 100, Math.random() * 100);
            g.endFill();
        }
    }
}

Useful for anybody developing AS3 only display stuff, and things I've written over and over. They always end up in some utils package, I hoped this would be the last time.

@mimshwright
Copy link
Contributor

Looks pretty cool. I still think we need docs in the code itself. I'm going to defer to some of the other admins for their input before adding. Thanks for your patience.

@slackmage
Copy link
Contributor

Hey Paul, can you write some passing tests around these?

As far as the weak listener issue, I think if we're going to go against the default of AS3 then the method itself should indicate that the listeners will be weak by default. I wouldn't want people to use it without looking at the behavior and falsely assume that they're going to be non-weak.

Perhaps two methods?

addListenerToTarget()
addWeakListenerToTarget()

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

Successfully merging this pull request may close these issues.

3 participants