OpenLayers.Event: new transversal method addListener.

[jorix]

Advantages of the addListener method:

• This ensures the call of "un" method and the destruction of events
• The developer will forget to call "un" or "unregister" methods
• No are required properties to store: Events instances or functions for call "un" method
• The code is shorter (and better)
• Can register listeners to another object. with correct unregister when object destroy.

Contents of the patch:

• The new methods:
  • Events.js

• Base classes which are implemented:
  • tests/Map.html
  • Map.js
  • Control.js
  • Popup.js

• How to use in a particular case:
  • MousePosition.js (to allow activation/deactivation)
  • examples/mouse-position.html (to regiter listeners to another object)

Note that in original MousePosition.js missing unregister of 'mouseout', would not have happened using addListener (therefore the code will be better).

This ticket contains only part of #2577, and is designed to facilitate the reading of the addListener methods within the patch. It has also modified some of the documentation.

SVN revision: 10299

[jorix]

Please review (or some feedback)

[ahocevar]

jorix: no review from me, but some feedback:

You have submitted lots of patches in the last couple of weeks. This is much appreciated. But most of them are huge and fix numerous problems at once, making it hard for potential reviewers to handle. It would be much easier if your tickets would concentrate on one small problem each, with a verbose description and a small patch with unit tests.

For architectural changes of big new features that you have in mind, you should discuss your ideas on the dev list before developing implementations.

[ahocevar]

@jorix: this ticket is a good example of how it should be done. Please also don't forget to close your huge tickets once you have split them up into small ones (e.g. #2577). Thanks! Also, make sure to discuss changes like this on the dev list first.

[ahocevar]

The addListener method should be added to classes using a Mixin, without the need to duplicate it in Map, Control and other classes. See e.g. Layer.Google, which mixes in Layer.EventPane.

[jorix]

As class

[jorix]

Replying to ahocevar:

The addListener method should be added to classes using a Mixin, without the need to duplicate it in Map, Control and other classes. See e.g. Layer.Google, which mixes in Layer.EventPane.

Yes, perfect. I've been working on it:

• Use inheritance or mixin to deploy methods addListener (I added the new class in Events.js).
• Remove code (also change the name of the "WhenActive" option by "WhenOn").
• Improve documentation of the code.
• New Units test for new methods in Map and Controls (for Popup would be the same)

To facilitate reading the patch does not contain documentation for Map, Control, and Popup.

Pending:

• Discuss the proposal.
• I think they should put the documentation for addListener in each class that inherits the method. For Control I think addListener should be a APIMethod as "events" property.
• Deploy in other classes? I think: Strategy, Tile, Marker and discard Layer that only have triggers. I think where there is more events "traffic" is in Control.

Please review or some feedback.

[jorix]

Replying to jorix:

... addListener should be a APIMethod as "events" property.

... addListener should be a APIMethod as "events" property in Map and eventListeners.

[jorix]

Is a proposal of the past, I close it.