Fixing Node.addEventListener

[letmaik]

I just played around with events and I'm quite sure the implementation in KineticGWT isn't right yet.

The whole problem is the evt object. This object isn't coming from Kinetic but directly from the browser, this is why you see different event "types". They are the raw events and Kinetic is abstracting over them (and providing more semantic events like DRAG*). Of course it would've been a good idea in Kinetic to also create an abstraction for the evt object...

In my case I don't need the evt object (yet) so I just reduced the code to:

public final native void addEventListener(List<Event.Type> eventTypes, EventListener handler) /*-{  
    this.on(@net.edzard.kinetic.Node::createEventTypeString(Ljava/util/List;)(eventTypes), function() {
            handler.@net.edzard.kinetic.Node.EventListener::handle(Lnet/edzard/kinetic/Event;)(null);
    });
}-*/;

Which works without problems.

If you really want to make the evt object available then you probably need several types of those because the object is quite different for e.g. touch events.

[Dawied]

Even if the kineticjs event object (the abstraction) would be accessible, it still would be nice to be able to also get to the browser evt object. In my case I need it to query the shiftKey.

Anyway, I changed the code in the 0.9.2 version to pass the native event as Object, in this case I can cast it to NativeEvent in GWT and use getShiftKey.

Like so:

public final native void addEventListener(List<EventType> eventTypes, EventListener handler) /*-{   
    this.on(@net.edzard.kinetic.Node::createEventTypeString(Ljava/util/List;)(eventTypes), function(evt) {
        handler.@net.edzard.kinetic.Node.EventListener::handle(Ljava/lang/Object;)(evt);
    });
}-*/;

[letmaik]

Hmm doesn't sound too bad, I didn't know the NativeEvent class. What about the Event class?

(linking to d7e3f6977eec95921bc0f9054e3a63709af1bdad for reference)

[Dawied]

Event works also.

[letmaik]

Can you make the changes and submit a pull request?

@ehoefig Is it ok like that? I think we don't need any further abstraction to the Event class of GWT.

[Dawied]

After some more poking around I found a least one cause of the problem with Node.addEventListener:

evt.offsetX and evt.offsetY are undefined (at least in my testcase with FireFox), so the Event object is never created and the Javascript execution stops.

So it seems that the original approach from ehoefig is the right one after all, we just need to repair the Javascript.

I also suggest to add evt itself as a parameter to the Event Ctor and have a getNativeEvent function within Event.

(found this: http://bugs.jquery.com/ticket/8523)

[letmaik]

I know that the KineticGWT Event class/code was broken, but I don't think we need it. I think your suggested code is nearly right. I would have imagined that EventListener::handle(Event evt) (the GWT Event class) is the right thing to do. And I would also have thought that the Event class of GWT abstracts over the browser-specific behaviour of the native event data. So I don't quite understand what you mean by "evt.offsetX and evt.offsetY are undefined", when is this happening?

[Dawied]

This is done in the method Node.addEventListener when the Event object is created (code is commented out).

[letmaik]

Yes but that's the old code. In d7e3f69 Ed removed our own event class completely. And what I say is that we should use the GWT Event object as you originally suggested, like:

public final native void addEventListener(List<EventType> eventTypes, EventListener handler) /*-{   
    this.on(@net.edzard.kinetic.Node::createEventTypeString(Ljava/util/List;)(eventTypes), function(evt) {
        handler.@net.edzard.kinetic.Node.EventListener::handle(Lcom/google/gwt/user/client/Event;)(evt);
    });
}-*/;

Wouldn't this work?

[Dawied]

It would work (I actually have it like this right now) , what I don't know (and cannot test at the moment) if we would still be able to access the extra attributes that are added to the event object in KineticGWT, like Shape and maybe more.

[letmaik]

Probably not, but KineticJS doesn't allow it either, right?

[Dawied]

In kineticjs.js you have:

_handleEvent: function(eventType, evt, compareShape) {
        if(evt && this.nodeType === 'Shape') {
            evt.shape = this;
        }

I think that adds the shape property to the event object, right?

[letmaik]

Ah, I see, although it seems that the evt object is quite arbitrary, depending on the specific event:

 _fireChangeEvent: function(attr, oldVal, newVal) {
     this._handleEvent(attr + 'Change', {
         oldVal: oldVal,
         newVal: newVal
     });
 },

So we can't expect a native browser-event object in all cases.

[Dawied]

You're right, the object has different properties depending on the type of event. But still we could account for all of them within a specialized Event object like the one in version 0.9.1 We would need to check for undefined in the addEventListener Javascript code and set the properties accordingly. If you and ehoefig decide to go that way than two things to consider would be 1. performance impact and 2. having a way of getting hold of the original event object.

Last thought: maybe it is the best way to have the event object as a JavascriptObject. That way we do not need to check anything, and everybody can get to the properties he or she needs. Something like they do in GWTOpenlayers: https://bitbucket.org/gwtopenlayers/gwt-openlayers/src/dd35ea2b1f01/gwt-openlayers-client/src/main/java/org/gwtopenmaps/openlayers/client/event/EventObject.java?at=default

[ehoefig]

What would you think about deriving a custom KineticGwtEvent class from GWT's Event class? We might lose the ability to communicate special kineticjs events (e.g., the change event example discussed before), but we would be compatible with GWT and could accommodate for specific properties.

[letmaik]

Hmm, so basically something like the old class but inheriting from GWT's Event?

[ehoefig]

Yes, plus some code for kinetic.js specific stuff, like adding the shape.

[letmaik]

@asimov6 Using JavascriptObject is in my mind suboptimal, let's take the change event as an example. How would you access oldVal and newVal? The developer (or we) would have to create a class extending JavascriptObject which has JSNI methods getOldVal() and getNewVal() and then the user would have to cast to this new class. I don't like that at all.

I would rather introduce additional handler registration methods like public final native void addChangeEventListener(ChangeEventListener handler) as I guess there is a defined number of event types which have certain properties etc.

[Dawied]

@ehoefig I like the idea of the new KineticGwtEvent class

@neothemachine The idea was to have the orginal javascript object at your disposal. To get to the javascript properties you would have overloaded methods like int getProperty(String name) and String getProperty(String name), etc. that would take care of retieving the original javascript properties.

This way the users can allways get to the properties even if they are not yet available in the GWT implementation.

(have a look at the GWTOpenLayers Event class and it's parent class to see what I mean)

[letmaik]

@asimov6 I see, an approach like in GWTOpenLayers (example) would also allow users to develop their own event classes and then use the low-level generic event handler registration to make use of them. Or, as you say, without a new Event class, directly accessing stuff of the JS object, but I don't think it is wise to provide methods like int getIntProperty(String name) as we could only support primitive properties and not objects.

So in the end, it's a combination of your and my proposal (where you want a generic way of event registration with access to JS event object, and I want some high-level convenience methods like addChangeEventListener(ChangeEventListener handler)). Sounds good to me. Unfortunately I won't have time at the moment to implement it, but if you have more time than me, go ahead :)

[Dawied]

Hi, Unfortunalely I don't have the time to do that. Right now I am using a copy with the GWT Event, and it suffices:

public interface EventListener {

    /**
     * Handles an event.
     * Called by Kinetic.
     * @return True, if the event should be processed further by bubbling up in the hierarchy. False, if bubbling should stop. 
     */
    public boolean handle(Event evt);
}

public final native void addEventListener(List<EventType> eventTypes, EventListener handler) /*-{
    this.on(@net.edzard.kinetic.Node::createEventTypeString(Ljava/util/List;)(eventTypes), function(evt) {
        handler.@net.edzard.kinetic.Node.EventListener::handle(Lcom/google/gwt/user/client/Event;)(evt);
    });
}-*/;

[ehoefig]

Ok, I will give it some thought and will implement if time allows. Having proper event handling is necessary.