Repository: DigitalSlideArchive/HistomicsUI (language: python)

Issue:
Default annotation element name
Annotation elements are named by their label.  If there is no label specified, the default to their type (polygon, line, rectangle, etc).  This leads to long lists of "polygon", etc.

It would be better to default to the annotation element style, possibly also with the type and a numerical counter.  If we add a counter needs to behave sensibly when annotations are added or removed and should be somewhat stable.


Discussion / hints:
So a default convention could look something like `<style>_<type>_<number>`?

What would sensible counter behavior look like? If I have elements

- default_point_1
- default_point_2
- default_point_3

And I delete the second entry, should default_point_3 change itself to default_point_2? It it stays default_point_3 should it be like that forever (i.e. the next time I load the annotation)? If that's the case, should we change the upload annotation process to fill in a default value for each element's label? 
I think it is perfectly fine to use spaces (e.g., I'd rather see "default point 1" than "default_point_1"). 

I'd rather not store the values into the labels -- that should be reserved for when the user has decided to have a label (plus it adds to the data volume).  I think that means that the elements end up getting renumbered.

I'm not sure about the element number, because it will be slow to have consistent behavior.  When we delete an element, we just remove the html without altering all the others (and, when we add an element, we just add it without modifying the other html).  This means that as long as the element list is not fully rerendered, we probably will have numbers that have apparent gaps (and new numbers will probably just be based on the last entry).  I think if the default counter was purely based on the initial element position (e.g., you would have "point 1", "polygon 2", "point 3" rather than "point 1", "polygon 1", "point 2"), and new elements just extend from where the last maximum was, this would look consistent (and be somewhat obvious when deleting similar entries), and when a rerender would reset the numbering, it wouldn't be bad.  But we'd have to try it to see if it actually makes sense.

Fix patch:
diff --git a/histomicsui/web_client/panels/DrawWidget.js b/histomicsui/web_client/panels/DrawWidget.js
index 16049bc4..3cb1b0c7 100644
--- a/histomicsui/web_client/panels/DrawWidget.js
+++ b/histomicsui/web_client/panels/DrawWidget.js
@@ -49,6 +49,7 @@ var DrawWidget = Panel.extend({
         this.image = settings.image;
         this.annotation = settings.annotation;
         this.collection = this.annotation.elements();
+        this.newElementDisplayIdStart = this.collection.length;
         this.viewer = settings.viewer;
         this.setViewer(settings.viewer);
         this.setAnnotationSelector(settings.annotationSelector);
@@ -114,7 +115,8 @@ var DrawWidget = Panel.extend({
                 opts: this._editOptions,
                 drawingType: this._drawingType,
                 collapsed: this.$('.s-panel-content.collapse').length && !this.$('.s-panel-content').hasClass('in'),
-                firstRender: true
+                firstRender: true,
+                displayIdStart: 0
             }));
             this.$('.h-dropdown-content').collapse({toggle: false});
         }
@@ -210,8 +212,17 @@ var DrawWidget = Panel.extend({
                     label = (obj.data.label || {}).value,
                     elemType = obj.element.get('type'),
                     group = obj.data.group;
-                label = label || (elemType === 'polyline' ? (obj.element.get('closed') ? 'polygon' : 'line') : elemType);
-                this.$(`.h-element[data-id="${id}"] .h-element-label`).text(label).attr('title', label);
+                let newLabel = '';
+                const labelElement = this.$(`.h-element[data-id="${id}"] .h-element-label`);
+                const oldLabel = labelElement.text().split(' ');
+                if (label) {
+                    newLabel = label;
+                } else if (['point', 'polyline', 'rectangle', 'ellipse', 'circle'].includes(elemType)) {
+                    newLabel = `${group || 'default'} ${elemType} ${parseInt(oldLabel[oldLabel.length - 1] || '')}`;
+                } else {
+                    newLabel = oldLabel;
+                }
+                this.$(`.h-element[data-id="${id}"] .h-element-label`).text(newLabel).attr('title', label);
                 if (origGroup !== group && ['point', 'polyline', 'rectangle', 'ellipse', 'circle'].includes(elemType)) {
                     this.updateCount(origGroup || 'default', -1);
                     this.updateCount(group || 'default', 1);
@@ -322,9 +333,11 @@ var DrawWidget = Panel.extend({
                 style: this._style.id,
                 highlighted: this._highlighted,
                 firstRender: false,
-                updateCount: this.updateCount
+                updateCount: this.updateCount,
+                displayIdStart: this.newElementDisplayIdStart
             })
         );
+        this.newElementDisplayIdStart += elements.length;
         if (this.$('.h-group-count-option.pixelmap').length > 0) {
             this.$('.h-group-count-option.pixelmap').remove();
             for (let element of this.collection.models) {
diff --git a/histomicsui/web_client/templates/panels/drawWidgetElement.pug b/histomicsui/web_client/templates/panels/drawWidgetElement.pug
index 8173fdcd..0e937964 100644
--- a/histomicsui/web_client/templates/panels/drawWidgetElement.pug
+++ b/histomicsui/web_client/templates/panels/drawWidgetElement.pug
@@ -1,21 +1,34 @@
 - var counts = {};
+- var displayIdOffset = 0;
 - var pixelmap = false;
+- var typeCounts = {};
 if elements.length
   each element in elements
     -
       var classes = highlighted[element.id] ? ['h-highlight-element'] : [];
       element = element.toJSON();
       var groupName = element['group'] ? element['group'] : 'default';
+      var type = element.type == 'polyline' ? (element.closed ? 'polygon' : 'line') : element.type;
+      var displayId = ++displayIdOffset + displayIdStart;
+      var label = ((element.label || {}).value || type);
       if (['point', 'polyline', 'rectangle', 'ellipse', 'circle'].includes(element.type)) {
         if (counts[groupName]) {
           counts[groupName]++;
         } else {
           counts[groupName] = 1;
         }
-      } else if (element.type === 'pixelmap') {
+        label = groupName + ' ' + label + ' ' + displayId;
+      } else {
+        if (typeCounts[type] > 0) {
+          label = label + ' ' + displayId;
+          typeCounts[type]++;
+        } else {
+          typeCounts[type] = 1;
+        }
+      }
+      if (element.type === 'pixelmap') {
         pixelmap = true;
       }
-      var label = (element.label || {}).value || (element.type === 'polyline' ? (element.closed ? 'polygon' : 'line') : element.type);
     .h-element(data-id=element.id, class=classes)
       span.icon-cog.h-edit-element(title='Change style')
       span.h-element-label(title=label) #{label}


Test patch:
diff --git a/tests/web_client_specs/annotationSpec.js b/tests/web_client_specs/annotationSpec.js
index dcf29ef3..e89833be 100644
--- a/tests/web_client_specs/annotationSpec.js
+++ b/tests/web_client_specs/annotationSpec.js
@@ -221,7 +221,7 @@ girderTest.promise.done(function () {
                     return $('.h-elements-container .h-element').length === 1;
                 }, 'point to be created');
                 runs(function () {
-                    expect($('.h-elements-container .h-element .h-element-label').text()).toBe('point');
+                    expect($('.h-elements-container .h-element .h-element-label').text()).toBe('default point 1');
                     expect($('.h-draw[data-type="point"]').hasClass('active')).toBe(true);
                     // turn off point drawing.
                     $('.h-draw[data-type="point"]').click();
@@ -275,7 +275,7 @@ girderTest.promise.done(function () {
                     return $('.h-elements-container .h-element').length === 2;
                 }, 'point to be created');
                 runs(function () {
-                    expect($('.h-elements-container .h-element:last .h-element-label').text()).toBe('point');
+                    expect($('.h-elements-container .h-element:last .h-element-label').text()).toBe('default point 2');
                 });
                 checkAutoSave('drawn 1', 2, annotationInfo);
             });
@@ -311,7 +311,7 @@ girderTest.promise.done(function () {
                     return $('.h-elements-container .h-element').length === 2;
                 }, 'point to be created');
                 runs(function () {
-                    expect($('.h-elements-container .h-element:last .h-element-label').text()).toBe('point');
+                    expect($('.h-elements-container .h-element:last .h-element-label').text()).toBe('default point 3');
                 });
                 checkAutoSave('drawn 1', 2, annotationInfo);
             });