OpenLayers OpenLayers

Ticket #1404 (closed feature: fixed)

Opened 9 months ago

Last modified 9 months ago

Specify event listeners at construction.

Reported by: tschaub Assigned to: tschaub
Priority: minor Milestone: 2.6 Release
Component: Events Version: 2.5
Keywords: Cc:
State: Complete

Description

As a final part of the event enhancements, I'd like to get in the ability to set event listeners at construction (for an object that has an events instance). This lets you write 

map = new OpenLayers.Map("map", {
    eventListeners: {
        "moveend": mapEvent,
        "zoomend": mapEvent,
        "changelayer": mapLayerChanged,
        "changebaselayer": mapBaseLayerChanged
    }
});

as an alternative to the current 

map = new OpenLayers.Map("map");
map.events.register("moveend", null, mapEvent);
map.events.register("zoomend", null, mapEvent);
map.events.register("changelayer", null, mapLayerChanged);
map.events.register("changebaselayer", null, mapBaseLayerChanged);

See this example.

Again, this was an oversight on my part not to make a ticket earlier. I'd like to kick this last change out of that events sandbox before 2.6 is out the door. Complaints welcome. Patch forthcoming.

Attachments

eventListeners.patch (13.6 kB) - added by tschaub on 02/29/08 01:48:04.
allow for setting event listeners at construction
null.patch (1.1 kB) - added by tschaub on 03/06/08 11:57:57.
null effect
increasing_likelyhood_of_typos.patch (1.5 kB) - added by tschaub on 03/06/08 12:44:40.
still no functional difference

Change History

02/29/08 00:36:40 changed by crschmidt

hm, didn't we kind of decide against something like this for layers or something? I don't know, my memory is failing me, but I thought we went through this a couple weeks ago.

02/29/08 01:48:04 changed by tschaub

  • attachment eventListeners.patch added.

allow for setting event listeners at construction

02/29/08 01:56:27 changed by crschmidt

Ah, I see, we decided we didn't want it in the events constructor. Got it.

02/29/08 01:58:18 changed by tschaub

Yeah, we decided not to put it in the Events constructor (checking for CLASS_NAME and eventsListener) - see #1353. I couldn't find that ticket when I created this one.

This patch is in response to Eric's comment 3.

02/29/08 02:02:10 changed by tschaub

  • state set to Review.

The other change included here (apologies) is that the event type is now accessible on the event. This completes the enhancements to events stuff, loading the event object with information related to the event.

Tests pass in FF & Safari. Checking elsewhere now.

02/29/08 02:14:08 changed by tschaub

Tests pass and example works in IE as well.

03/03/08 03:46:23 changed by elemoine

  • state changed from Review to Commit.

Looks good to me. Please commit.

03/04/08 19:07:46 changed by tschaub

  • status changed from new to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

(In [6435]) Adding eventListeners property to layer, control, and map. Setting this property at construction registers the given listeners based on event type key. r=elemoine (closes #1404)

03/05/08 11:40:54 changed by euzuro

some comments on this patch:

  • why are we keeping a reference to this 'eventListeners' property on the map, layer, and controls? I can't see anywhere else that it is used, so why keep a reference to it?
  • if we really want to keep a reference to it, why don't we then use it to unregister events on destroy? parallelism
  • if we do keep a reference to it, we should nullify/delete the 'eventListeners' property in the destroy(), right?

03/05/08 11:41:54 changed by euzuro

on further investigation, i realize why it has to be a property, because of the extend(this, options).

Ok. fine. but at least let's clean up after ourselves, no?

(follow-up: ↓ 13 ) 03/06/08 11:49:32 changed by tschaub

Agreed. Good to know I can get a review out of you by committing :).

(follow-up: ↓ 14 ) 03/06/08 11:54:57 changed by tschaub

Though events.destroy unregisters all event listeners (lest you thought there was room for leakage here) - and keeping those references to functions isn't like keeping references to dom elements.

03/06/08 11:57:57 changed by tschaub

  • attachment null.patch added.

null effect

(follow-up: ↓ 15 ) 03/06/08 11:58:34 changed by tschaub

You like I repoen and apply null.patch?

(in reply to: ↑ 10 ) 03/06/08 12:32:44 changed by euzuro

Replying to tschaub:

Agreed. Good to know I can get a review out of you by committing :).

guilty as charged

(in reply to: ↑ 11 ) 03/06/08 12:34:44 changed by euzuro

Replying to tschaub:

Though events.destroy unregisters all event listeners (lest you thought there was room for leakage here) - and keeping those references to functions isn't like keeping references to dom elements.

right... but i would like us to explicitly unregister them as well. the destroy on the events thing should be a last resort thing. i think it's important to be explicit about registering and unregistering, newing and deleting (nulling) or whatever... it makes it easier to make sure that we are taking care of things without relying on "magic" (as some say) functions.

plus... didn't you add some newfangled two letter function to do this automatically(unregister all those listeners)??

(in reply to: ↑ 12 ; follow-up: ↓ 16 ) 03/06/08 12:36:47 changed by euzuro

Replying to tschaub:

You like I repoen and apply null.patch?

sure, please do. i like this patch :-) could you maybe combine it with the unregistering bit too?

(in reply to: ↑ 15 ) 03/06/08 12:39:28 changed by tschaub

Replying to euzuro:

Replying to tschaub:

You like I repoen and apply null.patch?

sure, please do. i like this patch :-) could you maybe combine it with the unregistering bit too?

Unregistering happens just a few lines down in events.destroy.

(follow-up: ↓ 19 ) 03/06/08 12:40:57 changed by tschaub

ok, reading above now - seems to add confusion instead of clarity - but I'll abide

03/06/08 12:44:40 changed by tschaub

  • attachment increasing_likelyhood_of_typos.patch added.

still no functional difference

03/06/08 12:53:31 changed by tschaub

If this code were already in the trunk, I can imagine someone coming along and saying "hey, I can remove 12 redundant lines of code from thee files." I would consider that a good step.

All events.unregister does is splice out listeners from the listeners array one at a time. The events.destroy method removes the reference to the listeners array all at once. I don't see the point in doing a bunch of splicing just two lines before you ditch the reference to the array. No magic here that I can see.

(in reply to: ↑ 17 ) 03/06/08 12:56:52 changed by euzuro

  • status changed from closed to reopened.
  • state changed from Complete to Commit.
  • resolution deleted.

Replying to tschaub:

ok, reading above now - seems to add confusion instead of clarity - but I'll abide

Maybe I'm being absurd, but I really do think that this patch makes the code better. Readers can clearly see where events get registered and unregistered.ยจ

It's your call, though. I'm just one voice. I'll mark it as commit, but if you think it's foolish then don't put it in.

03/06/08 13:24:18 changed by tschaub

  • status changed from reopened to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

(In [6447]) Adding explicit eventListener unregistering in map, layer, and control destroy methods. Note that this only makes explicit what is already happening elsewhere, but it makes us feel complete. Also throwing in the example I missed in the previous change. r=euzuro (closes #1404)

03/06/08 13:27:23 changed by tschaub

What fun is coding without a good dose of absurdity?