store evt on handler

[tschaub]

The main purpose of the handlers is to abstract the detail of browser event handling so controls don't have to repeat the same detail in many places.

However, we shouldn't prevent controls from knowing the detail if they need it. This can be improved by storing a reference to the browser event on the handler. Because controls can reference their handlers, if they need to they can access the browser event.

This is particularly useful in determining any key modifiers associated with an event. A control should be able to modify its behavior depending on something about the browser event (altKey, shiftKey, ctrlKey, xy).

Eventually, we could store a keyDown object on the map - this would allow things like repeat pans while a key is held down. Until then, this improves the situation with handlers.

[tschaub]

Tests pass in IE and FF. Please review.

[tschaub]

add evt property to handlers

[elemoine]

Replying to tschaub:

Tests pass in IE and FF. Please review.

One question: why not "declaring" the evt property at the head of the Handler class?

[crschmidt]

Tim --

I'm happy with this patch if you want to change it, as Eric suggests, to include the 'evt' in the docs, as a Property.

[tschaub]

(In [4062]) Give handlers a non-API evt property - this to be used by other controls in the library only. Eventually, we may decide to restructure this. (Closes #902)

[euzuro]

caveat emptor: My understanding of the Handler classes is nil or next to nil at best.

Question that comes to mind as I see this patch go in. What if I do this:

    var func = function() {
        //pescaturismo
    };
    var func2 = function() {
        //showdrinkees
    };
    var h = new OpenLayers.Handler();
    h.register("load", func);
    h.register("load", func2);

I know have two setEvent()s registered to the 'load' function... no?

now what if I

    h.unregister("load", func2);

Will it remove *both* of the setEvent() which are registered to the 'load' event? just one? (answer I think is just one, but it still makes me uneasy)

Can I depend on the setEvent() event getting called *before* func() and/or func2() ?

What if func() does this:

    this.evt = null;

Will func2() still have something for this.evt?

[euzuro]

Replying to euzuro:

caveat emptor: My understanding of the Handler classes is nil or next to nil at best. Question that comes to mind as I see this patch go in. What if I do this: {{{ var func = function() { //pescaturismo }; var func2 = function() { //showdrinkees }; var h = new OpenLayers.Handler(); h.register("load", func); h.register("load", func2); }}} I know have two setEvent()s registered to the 'load' function... no? now what if I {{{ h.unregister("load", func2); }}} Will it remove *both* of the setEvent() which are registered to the 'load' event? just one? (answer I think is just one, but it still makes me uneasy) Can I depend on the setEvent() event getting called *before* func() and/or func2() ? What if func() does this: {{{ this.evt = null; }}} Will func2() still have something for this.evt?

know -> now

[euzuro]

I understand that the solution I'm suggesting here is much more complicated, but I think maybe it's more directly what we want to do.

The problem is that every time an event is triggered, we want to copy the evt as a property to the handler -- seems to me like the precise way to do this is by adding a wrapper function which first sets the 'evt' property on the handler and then goes ahead and calls the desired method.

This way we are guaranteed that there will always be a freshly set 'evt' property on the handler, and we do not convolute the map's 'events' object with an extra listener for every registered event handler.

Note that the patch I've made is *not* complete: tests are for obvious reasons failing, though running examples seems to indicate that things are working correctly. Running the dragfeatures.html everything is working fine. I'm guessing that tests are just broken because they are expecting certain things to be getting registered but the anonymous wrapper function screws that up.

If people agree that this is a more complete approach, I can take some time and update the tests so that they all pass.

[euzuro]

Possible reworking of the functionality to give the handler an 'evt' property on events. This is not a complete patch, only a suggested attack plan. Tests do not pass.

[tschaub]

I agree, the version in the trunk has issues. Specifically, if a control checks control.handler.evt it cannot be certain that the event that it gets is the one that triggered the sequence of calls that lead to the currently executing code.

I think the attached patch is getting things a bit confused. The binding in wrapperFunction does nothing to change the uncertainty mentioned above - which I think is the only issue to be concerned with. The registerPriority method was put in to deal with ordering of event listeners. I think we'll need to talk in person before I can understand your concerns.

[tschaub]

On a moments reflection - I do like what the new patch is getting at. I think registering two listeners for each event is dumb. I'd still like to talk about a better overall solution.

[euzuro]

im down to talk it through and see if we can get to a better solution. remember my caveat which is that i have no idea what this code is actually doing or why someone wants access to that evt. if we can chat and you can give a brother some context, i'll do my best to bind() and together we make something better.

unfortunately, i have already used up all my OL time for this week and given that i'm sort of on holiday in sicilia, im trying to spend my free time doing things like drinking becks and fighting forest fires.

next week?

[tschaub]

Bumping to 2.6 to allow further discussion.

[tschaub]

Lets bump this to 2.7, huh? I'm good with doing it before, but don't think it's critical.

[euzuro]

Mass ticket move out of 2.7 in preparation for a release plan.

If you are actively working on this task, and think that you can help this ticket to get finished and closed by September 1, please move it back to 2.7.

[crschmidt]

Not touched since 2.7, bumping.

[tschaub]

Pull back if you want this before 3.0.