Merge pull request #747 from OpenGeoscience/preserve-annoation-id

Preserve annotation ids when reloading geojson.

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) {

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

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');

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);