Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ol.layer.Vector.getSource return type specialisation. #2651

Merged

Conversation

gberaudo
Copy link
Member

Avoid having to cast to ol.source.Vector.

@fredj
Copy link
Member

fredj commented Aug 29, 2014

+1

@elemoine
Copy link
Member

What about Tile and Image? We need to do the same there as well.

@fredj
Copy link
Member

fredj commented Aug 29, 2014

ol.layer.Tile#getSource() and ol.layer.Image#getSource() return type is already a ol.source.Source so there's nothing to do.

@elemoine
Copy link
Member

ol.layer.Tile#getSource() and ol.layer.Image#getSource() return type is already a ol.source.Source so there's nothing to do.

My understanding is that @gberaudo wants getSource to be annotated with more specific return types. This is to avoid type-casts in his compiled code. His patch changes ol.layer.Vector#getSource's return type from ol.source.Source to ol.source.Vector. Likewise, ol.layer.Image#getSource's return type should be changed to ol.source.Image.

Also, with that I think we also can remove a number of type-casts in the ol3 code. Here are two example for ol.layer.Vector#getSource: https://github.com/openlayers/ol3/blob/master/examples/gpx.js#L99 and https://github.com/openlayers/ol3/blob/master/src/ol/renderer/canvas/canvasvectorlayerrenderer.js#L171.

@fredj
Copy link
Member

fredj commented Aug 29, 2014

But ol.layer.Image constructor take a ol.source.Source as argument (param type is olx.layer.LayerOptions)

@elemoine
Copy link
Member

That's a mistake I think. An ol.layer.Image instance should be configured with an ol.source.Image instance.

@fredj
Copy link
Member

fredj commented Aug 29, 2014

That's a mistake I think. An ol.layer.Image instance should be configured with an ol.source.Image instance.

New PR to come

@fredj
Copy link
Member

fredj commented Aug 29, 2014

Unnecessary type cast removed here: fredj@b88e52c

@gberaudo gberaudo force-pushed the layer_vector_source_specialisation branch from 84f2d1e to 4a21ad5 Compare September 2, 2014 13:18
@fredj
Copy link
Member

fredj commented Sep 3, 2014

PR updated, thanks for any review

@elemoine
Copy link
Member

elemoine commented Sep 3, 2014

This looks good. +1 for merging. We should also merge #2656 after this one.

fredj added a commit that referenced this pull request Sep 3, 2014
…tion

ol.layer.Vector.getSource return type specialisation.
@fredj fredj merged commit aa618e3 into openlayers:master Sep 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants