Add addControl and removeControl methods to the map

[elemoine]

This PR adds addControl and removeControl methods to the map. And the map now has a collection of controls.

[NEED UPDATE I'll provide more details on why I'd like to introduce these changes]

[elemoine]

This PR suggests adding conveniences for control manipulation by application developers.

The map now has a collection of controls. This makes it possible for application developers to use the map as a place holder for controls. In this way, the map can be a central object in a ol3 application, from which map-related objects, like controls, can be retrieved.

The map now exposes addControl and removeControl methods. addControl allows to add a new control to the map after map creation. removeControl, as its name suggests, allows to remove a control that's currently in the map.

Application developers can still call setMap directly to "add" or "remove" a control. But in that case, the control won't be added to, or removed from, the map's controls collection.

I'm interested in opinions about this patch.

[adube]

This keeps them organized, easy to find and fetch in one place. Having "add" and "remove" method catches more the eye to what they do at first glance.

I like it.

[twpayne]

Should control.setMap(null) be called even if the control was not already added to the map (e.g. when controls.remove(control) returns undefined)?

[elemoine]

Good question. This function is about removing the control from the map, so I guess we should nothing (and not call setMap(null)) if the control is not in the map. Do you agree?

[twpayne]

Yes.

[twpayne]

I'd probably add an goog.asserts.fail() as well though, as this really should not happen.

[elemoine]

Yep, good suggestion. New commit to come.

[elemoine]

I'd probably add an goog.asserts.fail() as well though, as this really should not happen.

Thinking about it, are you sure that makes sense? It could be convenient that removeControl does nothing without complaining when it's passed a control that's not in the map, no?

[twpayne]

It could be a convention to silently ignore it, but I think in most cases this would indicate a bug in the application.

ol.control.Control has a getMap method, so users can always do

// remove control if has not already been removed
if (control.getMap()) {
  map.removeControl(control);
}

I'd prefer this to the option of being silent when a user mixes up which control is on which map.

[elemoine]

It could be a convention to silently ignore it, but I think in most cases this would indicate a bug in the application.

If we add goog.asserts.fail() then I should change the documentation of the removeControl function. Because right now the doc says that undefined is returned if the control is not the map, which suggests that it's not an abnormal case.

// remove control if has not already been removed
if (control.getMap()) {
 map.removeControl(control);
}

Note that getMap returning undefined doesn't necessarily mean that the control is not in the map. We can get out of sync if the application developer uses setMap directly.

map.addControl(control);
control.setMap(null);  # that doesn't remove the control from the map
if (control.getMap()) {
  map.removeControl(control);
}

Most users should just use addControl and removeControl. Advanced users who know what they're doing can use setMap directly.

[twpayne]

OK, that makes sense. So, to summarise, we should remain silent if map.removeControl(control) is called with a control which was not already on the map.

[twpayne]

LGTM

Should addInteraction and removeInteraction be added for consistency and completeness?

[elemoine]

Should addInteraction and removeInteraction be added for consistency and completeness?

Yes, I guess so. Can this go in a separate PR? (I'd create an issue if you agree.)

[twpayne]

Agree - addInteraction and removeInteraction should be in a separate PR.

[twpayne]

Perfect, +1 from me.