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.

Sign up for GitHub

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

When adding annotations, add a gcs options. #654

Merged
merged 2 commits into from
Dec 13, 2016
+20 −3
Merged

When adding annotations, add a gcs options. #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c344c7
When adding annotations, add a gcs options.
manthey Dec 12, 2016
ee94db7
Merge branch 'master' into annotation-gcs
manthey Dec 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/annotationLayer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,14 +200,24 @@ var annotationLayer = function (args) {
* Add an annotation to the layer. The annotation could be in any state.
*
* @param {object} annotation the annotation to add.
* @param {string|geo.transform} [gcs] undefined to use the interface gcs,
* null to use the map gcs, or any other transform
*/
this.addAnnotation = function (annotation) {
this.addAnnotation = function (annotation, gcs) {
var pos = $.inArray(annotation, m_annotations);
if (pos < 0) {
m_this.geoTrigger(geo_event.annotation.add_before, {
annotation: annotation
});
m_annotations.push(annotation);
annotation.layer(this);
var map = this.map();
gcs = (gcs === null ? map.gcs() : (
gcs === undefined ? map.ingcs() : gcs));
if (gcs !== map.gcs()) {
annotation._coordinates(transform.transformCoordinates(
gcs, map.gcs(), annotation._coordinates()));
}
this.modified();
this._update();
this.draw();
Expand Down Expand Up @@ -348,7 +358,7 @@ var annotationLayer = function (args) {
state: geo_annotation.state.create,
layer: this
});
this.addAnnotation(m_this.currentAnnotation);
this.addAnnotation(m_this.currentAnnotation, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be little confusing in the way we are using null or undefined. I would suggest that if it is null or undefined, we fall back to map.gcs(). Should we create keys perhaps? map_gcs, map_ingcs? Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done throughout the other modules, so that none is the special value that can be used internally to mean use the map.gcs() while not forcing a user to constantly pass gcs parameters when what they mostly want is the map.ingcs().

I would not make the undefined value go to map.gcs(), as that is certainly not what a user wants (they want the interface gcs, not the display gcs).

Using map_gcs instead of none would be okay, but this really should be a constant defined somewhere. In which case, I lament that we don't have one constants module. The user already has to import events and actions to get constants.

Copy link
Member

@aashish24 aashish24 Dec 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with all of what you said above. May we can add constants and use map_gcs as a replacement for none in a separate PR. Care to create a issue for it? I would think that having a entry for map_ingcs will be useful too.

actions = this.currentAnnotation.actions(geo_annotation.state.create);
$.each(actions, function (idx, action) {
m_this.map().interactor().addAction(action);
Expand Down Expand Up @@ -528,7 +538,7 @@ var annotationLayer = function (args) {
options.state = geo_annotation.state.done;
options.layer = m_this;
options.updated = 'new';
m_this.addAnnotation(registry.createAnnotation(type, options));
m_this.addAnnotation(registry.createAnnotation(type, options), null);
}
});
};
Expand Down
7 changes: 7 additions & 0 deletions tests/cases/annotationLayer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,13 @@ describe('geo.annotationLayer', function () {
expect(addAnnotationBeforeEvent).toBe(2);
expect(addAnnotationEvent).toBe(2);
expect(lastAddAnnotationEvent.annotation).toBe(rect);
expect(layer.annotations()[1]._coordinates()[1]).not.toEqual({x: 1, y: 0});
layer.removeAnnotation(rect);
rect = geo.annotation.rectangleAnnotation({
layer: layer,
corners: [{x: 0, y: 0}, {x: 1, y: 0}, {x: 1, y: 1}, {x: 0, y: 1}]});
layer.addAnnotation(rect, map.gcs());
expect(layer.annotations()[1]._coordinates()[1]).toEqual({x: 1, y: 0});
});
it('annotationById', function () {
expect(layer.annotationById()).toBe(undefined);
Expand Down