Add method for retrieving features by id

[tschaub]

This adds a feature id index to the vector source. The getFeatureById method allows retrieval of features by id.

Note that the index does not differentiate string and numeric identifiers. So source.getFeatureById(2) and source.getFeatureById('2') both would return a feature with an id of 2 or '2'. This limitation should not affect applications that consistently use either strings or numbers as feature identifiers.

Fixes #2084.

[twpayne]

Note that this test will pass, even if no change event is dispatched.

[tschaub]

It fails after the 2000ms timeout (done is never called).

[elemoine]

But using done makes more sense for async call no? In that case wouldn't it make more sense to use a spy?

    it('dispatches the "change" event', function() {
      var spy = sinon.spy();
      var feature = new ol.Feature();
      feature.on('feature', spy);
      feature.setId('foo');
      expect(spy.calledOnce).to.be(true);
      expect(feature.getId()).to.be('foo');
    });

[tschaub]

Right, that is asserting two things at once: that the change event was fired and that it was fired synchronously. I'm partial to the idea of having change events fired asynchronously, and the tests reveal this bias.

[tschaub]

This is ready for review.

[tschaub]

As noted above the tests do fail without the lib changes.

[twpayne]

OK, I just wonder if:

var feature = new ol.Feature();
var callbackCalled = false;
feature.on('change', function() {
  callbackCalled = true;
});
feature.setId('foo');
expect(callbackCalled).to.be(true);

would be simpler and faster.

[tschaub]

That wouldn't pass any faster. But I grant that it would fail faster.

It is a pretty common pattern to call the done callback within an event listener (even within this same spec file). I'm surprised this one is drawing attention.

Perhaps we could separately go through the tests and modify things so we are essentially asserting that certain events are fired synchronously. It would be more consistent to do that as a separate effort though.

[elemoine]

Very minor but we use toString a lot elsewhere. AFAICT toString works on both numbers and strings.

[elemoine]

It looks good to me. I just added a minor, non-blocking, comments. Thanks.

[tschaub]

I'll rebase to get tests passing after #2017 is merged.

[elemoine]

Please merge when rebased (and when the build passes).