diff --git a/src/annotation.js b/src/annotation.js index 9d28756bd6..2159bca396 100644 --- a/src/annotation.js +++ b/src/annotation.js @@ -45,11 +45,22 @@ var annotation = function (type, args) { return new annotation(type, args); } - annotationId += 1; var m_options = $.extend({}, {showLabel: true}, args || {}), - m_id = annotationId, - m_name = m_options.name || ( - type.charAt(0).toUpperCase() + type.substr(1) + ' ' + annotationId), + m_id = m_options.annotationId; + delete m_options.annotationId; + if (m_id === undefined || (m_options.layer && m_options.layer.annotationById(m_id))) { + annotationId += 1; + if (m_id !== undefined) { + console.warn('Annotation id ' + m_id + ' is in use; using ' + annotationId + ' instead.'); + } + m_id = annotationId; + } else { + if (m_id > annotationId) { + annotationId = m_id; + } + } + var m_name = m_options.name || ( + type.charAt(0).toUpperCase() + type.substr(1) + ' ' + m_id), m_label = m_options.label || null, m_description = m_options.description || undefined, m_type = type, @@ -273,7 +284,7 @@ var annotation = function (type, args) { } if (arg2 === undefined) { m_options = $.extend(true, m_options, arg1); - /* For style objects, reextend them without recursiion. This allows + /* For style objects, re-extend them without recursion. This allows * setting colors without an opacity field, for instance. */ ['style', 'editStyle', 'labelStyle'].forEach(function (key) { if (arg1[key] !== undefined) { diff --git a/src/annotationLayer.js b/src/annotationLayer.js index 8651234830..631ad327b9 100644 --- a/src/annotationLayer.js +++ b/src/annotationLayer.js @@ -420,16 +420,16 @@ var annotationLayer = function (args) { * the given geojson object. If `undefined`, return the current * annotations as geojson. This may be a JSON string, a javascript * object, or a File object. - * @param {boolean} [clear] If `true`, when adding annotations, first remove - * all existing objects. If `'update'`, update existing annotations and - * remove annotations that no longer exit, If falsy, update existing - * annotations and leave unchanged annotations. + * @param {boolean|string} [clear] If `true`, when adding annotations, first + * remove all existing objects. If `'update'`, update existing + * annotations and remove annotations that no longer exist. If falsy, + * update existing annotations and leave annotations that have not chaged. * @param {string|geo.transform|null} [gcs] `undefined` to use the interface * gcs, `null` to use the map gcs, or any other transform. * @param {boolean} [includeCrs] If truthy, include the coordinate system in * the output. * @returns {object|number|undefined} If `geojson` was undefined, the current - * annotations as a javascript object that can be converted to geojson + * annotations is a javascript object that can be converted to geojson * using JSON.stringify. If `geojson` is specified, either the number of * annotations now present upon success, or `undefined` if the value in * `geojson` was not able to be parsed. @@ -569,7 +569,9 @@ var annotationLayer = function (args) { }); if (options.annotationId !== undefined) { existing = m_this.annotationById(options.annotationId); - delete options.annotationId; + if (existing) { + delete options.annotationId; + } } if (existing && existing.type() === type && existing.state() === geo_annotation.state.done && existing.options('updated') === false) { /* We could change the state of the existing annotation if it differs diff --git a/tests/cases/annotation.js b/tests/cases/annotation.js index de274f99b2..17be841466 100644 --- a/tests/cases/annotation.js +++ b/tests/cases/annotation.js @@ -51,17 +51,28 @@ describe('geo.annotation', function () { layer = map.createLayer('annotation', { annotations: geo.listAnnotations() }); - ann = geo.annotation.annotation('test2', { + var params = { layer: layer, name: 'Annotation', state: geo.annotation.state.create - }); + }; + ann = geo.annotation.annotation('test2', params); expect(ann.type()).toBe('test2'); expect(ann.state()).toBe(geo.annotation.state.create); expect(ann.id()).toBeGreaterThan(0); expect(ann.name()).toBe('Annotation'); expect(ann.layer()).toBe(layer); expect(ann.coordinates()).toEqual([]); + + // check that reusing an annotationId throws a warning + sinon.stub(console, 'warn', function () {}); + params.annotationId = 10; + ann = geo.annotation.annotation('test2', params); + layer.addAnnotation(ann); + expect(console.warn.calledOnce).toBe(false); + geo.annotation.annotation('test2', params); + expect(console.warn.calledOnce).toBe(true); + console.warn.restore(); }); it('_exit', function () { var ann = geo.annotation.annotation('test'); diff --git a/tests/cases/annotationLayer.js b/tests/cases/annotationLayer.js index ec67f67fa3..fc7d129ecf 100644 --- a/tests/cases/annotationLayer.js +++ b/tests/cases/annotationLayer.js @@ -234,6 +234,10 @@ describe('geo.annotationLayer', function () { expect(layer.geojson(sampleGeojson)).toBe(5); expect(layer.geojson(sampleGeojson, 'update')).toBe(2); expect(layer.geojson(sampleGeojson, true)).toBe(2); + expect(layer.annotations()[1].id()).not.toBe(1000); + sampleGeojson.properties = {annotationId: 1000}; + expect(layer.geojson(sampleGeojson, true)).toBe(2); + expect(layer.annotations()[1].id()).toBe(1000); }); it('validateAttribute', function () { expect(layer.validateAttribute(undefined, 'other')).toBe(undefined);