OpenLayers OpenLayers

Ticket #902 (reopened feature)

Opened 1 year ago

Last modified 2 weeks ago

store evt on handler

Reported by: tschaub Assigned to: tschaub
Priority: minor Milestone: 2.8 Release
Component: Handler Version: 2.4
Keywords: Cc:
State: Needs Discussion

Description

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.

Attachments

handler.evt.patch (8.4 kB) - added by tschaub on 08/11/07 11:57:53.
add evt property to handlers
evt.patch (3.4 kB) - added by euzuro on 08/28/07 05:59:28.
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.

Change History

(follow-up: ↓ 2 ) 08/11/07 11:46:15 changed by tschaub

  • keywords set to review.
  • status changed from new to assigned.
  • component changed from general to Handler.

Tests pass in IE and FF. Please review.

08/11/07 11:57:53 changed by tschaub

  • attachment handler.evt.patch added.

add evt property to handlers

(in reply to: ↑ 1 ) 08/12/07 02:00:56 changed by 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?

08/27/07 13:37:27 changed by crschmidt

  • keywords changed from review to commit.

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.

08/27/07 15:55:05 changed by tschaub

  • keywords deleted.
  • status changed from assigned to closed.
  • resolution set to fixed.

(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)

(follow-up: ↓ 6 ) 08/28/07 05:46:08 changed by euzuro

  • status changed from closed to reopened.
  • resolution deleted.

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?

(in reply to: ↑ 5 ) 08/28/07 05:48:56 changed by 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

08/28/07 05:58:31 changed by 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.

08/28/07 05:59:28 changed by euzuro

  • attachment evt.patch added.

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.

08/28/07 12:14:28 changed by 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.

08/28/07 12:18:46 changed by 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.

08/28/07 12:30:59 changed by 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?

09/13/07 11:31:23 changed by tschaub

  • milestone changed from 2.5 Release to 2.6 Release.

Bumping to 2.6 to allow further discussion.

12/19/07 20:02:11 changed by tschaub

  • state set to Needs Discussion.

02/08/08 16:39:04 changed by tschaub

  • milestone changed from 2.6 Release to 2.7 Release.

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

07/03/08 19:03:44 changed by euzuro

  • milestone changed from 2.7 Release to 2.8 Release.

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.