From 2d9c0414f72c583e2894d81cc80ab1bad09e06c6 Mon Sep 17 00:00:00 2001
From: Scott Bell <scott@traclabs.com>
Date: Tue, 5 Dec 2023 04:12:24 +0100
Subject: [PATCH] Inconsistent behavior with multiple annotations in imagery
 (#7261)

* fix opacity issue

* wip, though selection still weird

* remove debugging

* plots still have issue with last tag

* add some better tests

* Apply suggestions from code review

Co-authored-by: David Tsay <3614296+davetsay@users.noreply.github.com>

* remove hardlined classnames

* case sensitivity

* good job tests finding issue

---------

Co-authored-by: David Tsay <3614296+davetsay@users.noreply.github.com>
Co-authored-by: John Hill <john.c.hill@nasa.gov>
---
 .../imagery/exampleImagery.e2e.spec.js        |  8 ++
 .../plugins/notebook/tags.e2e.spec.js         | 18 ++++
 .../plugins/plot/tagging.e2e.spec.js          | 23 +++++
 .../imagery/components/AnnotationsCanvas.vue  | 86 ++++++++++++++-----
 .../annotations/AnnotationsInspectorView.vue  |  4 +-
 .../annotations/tags/TagEditor.vue            |  9 +-
 .../annotations/tags/TagEditorClassNames.js   |  9 ++
 .../annotations/tags/TagSelection.vue         |  7 +-
 .../notebook/components/NotebookComponent.vue |  7 ++
 .../notebook/components/NotebookEntry.vue     |  8 +-
 src/plugins/plot/MctPlot.vue                  | 14 ++-
 11 files changed, 159 insertions(+), 34 deletions(-)
 create mode 100644 src/plugins/inspectorViews/annotations/tags/TagEditorClassNames.js

diff --git a/e2e/tests/functional/plugins/imagery/exampleImagery.e2e.spec.js b/e2e/tests/functional/plugins/imagery/exampleImagery.e2e.spec.js
index c7ae0f4235..a5204cdb94 100644
--- a/e2e/tests/functional/plugins/imagery/exampleImagery.e2e.spec.js
+++ b/e2e/tests/functional/plugins/imagery/exampleImagery.e2e.spec.js
@@ -247,6 +247,14 @@ test.describe('Example Imagery Object', () => {
     await page.mouse.click(canvasCenterX - 50, canvasCenterY - 50);
     await expect(page.getByText('Driving')).toBeVisible();
     await expect(page.getByText('Science')).toBeVisible();
+
+    // add another tag and expect it to appear without changing selection
+    await page.getByRole('button', { name: /Add Tag/ }).click();
+    await page.getByPlaceholder('Type to select tag').click();
+    await page.getByText('Drilling').click();
+    await expect(page.getByText('Driving')).toBeVisible();
+    await expect(page.getByText('Science')).toBeVisible();
+    await expect(page.getByText('Drilling')).toBeVisible();
   });
 
   test('Can use + - buttons to zoom on the image @unstable', async ({ page }) => {
diff --git a/e2e/tests/functional/plugins/notebook/tags.e2e.spec.js b/e2e/tests/functional/plugins/notebook/tags.e2e.spec.js
index b4c9270b00..b520bf1fb5 100644
--- a/e2e/tests/functional/plugins/notebook/tags.e2e.spec.js
+++ b/e2e/tests/functional/plugins/notebook/tags.e2e.spec.js
@@ -224,4 +224,22 @@ test.describe('Tagging in Notebooks @addInit', () => {
     // Verify the AutoComplete field is hidden
     await expect(page.locator('[placeholder="Type to select tag"]')).toBeHidden();
   });
+  test('Can start to add a tag, click away, and add a tag', async ({ page }) => {
+    await createNotebookEntryAndTags(page);
+
+    await page.getByRole('tab', { name: 'Annotations' }).click();
+
+    // Click on the body simulating a click outside the autocomplete)
+    await page.locator('body').click();
+    await page.locator(`[aria-label="Notebook Entry"]`).click();
+
+    await page.hover(`button:has-text("Add Tag")`);
+    await page.locator(`button:has-text("Add Tag")`).click();
+
+    // Click inside the tag search input
+    await page.locator('[placeholder="Type to select tag"]').click();
+    // Select the "Driving" tag
+    await page.locator('[aria-label="Autocomplete Options"] >> text=Drilling').click();
+    await expect(page.getByLabel('Notebook Entries').getByText('Drilling')).toBeVisible();
+  });
 });
diff --git a/e2e/tests/functional/plugins/plot/tagging.e2e.spec.js b/e2e/tests/functional/plugins/plot/tagging.e2e.spec.js
index 6b0fdd1176..84f65def9e 100644
--- a/e2e/tests/functional/plugins/plot/tagging.e2e.spec.js
+++ b/e2e/tests/functional/plugins/plot/tagging.e2e.spec.js
@@ -165,6 +165,29 @@ test.describe('Plot Tagging', () => {
 
     await expect(page.getByText('Science')).toBeVisible();
     await expect(page.getByText('Driving')).toBeHidden();
+
+    // click elsewhere
+    await page.locator('body').click();
+    //click on tagged plot point again
+    await canvas.click({
+      position: {
+        x: 100,
+        y: 100
+      }
+    });
+    // Add driving tag again
+    await page.getByText('Annotations').click();
+    await page.getByRole('button', { name: /Add Tag/ }).click();
+    await page.getByPlaceholder('Type to select tag').click();
+    await page.getByText('Driving').click();
+    await expect(page.getByText('Science')).toBeVisible();
+    await expect(page.getByText('Driving')).toBeVisible();
+
+    // Delete Driving again
+    await page.hover('[aria-label="Tag"]:has-text("Driving")');
+    await page.locator('[aria-label="Remove tag Driving"]').click();
+    await expect(page.getByText('Science')).toBeVisible();
+    await expect(page.getByText('Driving')).toBeHidden();
   }
 
   test.beforeEach(async ({ page }) => {
diff --git a/src/plugins/imagery/components/AnnotationsCanvas.vue b/src/plugins/imagery/components/AnnotationsCanvas.vue
index 66d99d6196..f5d03313dc 100644
--- a/src/plugins/imagery/components/AnnotationsCanvas.vue
+++ b/src/plugins/imagery/components/AnnotationsCanvas.vue
@@ -33,8 +33,11 @@
 
 <script>
 import Flatbush from 'flatbush';
+import isEqual from 'lodash/isEqual';
 import { toRaw } from 'vue';
 
+import TagEditorClassNames from '../../inspectorViews/annotations/tags/TagEditorClassNames';
+
 const EXISTING_ANNOTATION_STROKE_STYLE = '#D79078';
 const EXISTING_ANNOTATION_FILL_STYLE = 'rgba(202, 202, 142, 0.2)';
 const SELECTED_ANNOTATION_STROKE_COLOR = '#BD8ECC';
@@ -118,9 +121,22 @@ export default {
     document.body.removeEventListener('click', this.cancelSelection);
   },
   methods: {
-    onAnnotationChange(annotations) {
-      this.selectedAnnotations = annotations;
-      this.$emit('annotations-changed', annotations);
+    onAnnotationChange(updatedAnnotations) {
+      updatedAnnotations.forEach((updatedAnnotation) => {
+        // Try to find the annotation in the existing selected annotations
+        const existingIndex = this.selectedAnnotations.findIndex((annotation) =>
+          this.openmct.objects.areIdsEqual(annotation.identifier, updatedAnnotation.identifier)
+        );
+
+        // If found, update it
+        if (existingIndex > -1) {
+          this.selectedAnnotations[existingIndex] = updatedAnnotation;
+        } else {
+          // If not found, add it
+          this.selectedAnnotations.push(updatedAnnotation);
+        }
+      });
+      this.$emit('annotations-changed', this.selectedAnnotations);
     },
     transformAnnotationRectangleToFlatbushRectangle(annotationRectangle) {
       let { x, y, width, height } = annotationRectangle;
@@ -164,7 +180,13 @@ export default {
       const targetDetails = [];
       annotations.forEach((annotation) => {
         annotation.targets.forEach((target) => {
-          targetDetails.push(toRaw(target));
+          // only add targetDetails if we haven't added it before
+          const targetAlreadyAdded = targetDetails.some((targetDetail) => {
+            return isEqual(targetDetail, toRaw(target));
+          });
+          if (!targetAlreadyAdded) {
+            targetDetails.push(toRaw(target));
+          }
         });
       });
       this.selectedAnnotations = annotations;
@@ -296,9 +318,13 @@ export default {
     cancelSelection(event) {
       if (this.$refs.canvas) {
         const clickedInsideCanvas = this.$refs.canvas.contains(event.target);
+        // unfortunate side effect from possibly being detached from the DOM when
+        // adding/deleting tags, so closest() won't work
+        const clickedTagEditor = Object.values(TagEditorClassNames).some((className) => {
+          return event.target.classList.contains(className);
+        });
         const clickedInsideInspector = event.target.closest('.js-inspector') !== null;
-        const clickedOption = event.target.closest('.js-autocomplete-options') !== null;
-        if (!clickedInsideCanvas && !clickedInsideInspector && !clickedOption) {
+        if (!clickedInsideCanvas && !clickedTagEditor && !clickedInsideInspector) {
           this.newAnnotationRectangle = {};
           this.selectedAnnotations = [];
           this.drawAnnotations();
@@ -345,12 +371,13 @@ export default {
         const resultIndicies = this.annotationsIndex.search(x, y, x, y);
         resultIndicies.forEach((resultIndex) => {
           const foundAnnotation = this.indexToAnnotationMap[resultIndex];
-          if (foundAnnotation._deleted) {
-            return;
-          }
           nearbyAnnotations.push(foundAnnotation);
         });
-        //show annotations if some were found
+        //if everything has been deleted, don't bother with the selection
+        const allAnnotationsDeleted = nearbyAnnotations.every((annotation) => annotation._deleted);
+        if (allAnnotationsDeleted) {
+          nearbyAnnotations = [];
+        }
         const { targetDomainObjects, targetDetails } =
           this.prepareExistingAnnotationSelection(nearbyAnnotations);
         this.selectImageAnnotations({
@@ -419,6 +446,7 @@ export default {
     },
     drawAnnotations() {
       this.clearCanvas();
+      let drawnRectangles = [];
       this.imageryAnnotations.forEach((annotation) => {
         if (annotation._deleted) {
           return;
@@ -426,19 +454,31 @@ export default {
         const annotationRectangle = annotation.targets.find(
           (target) => target.keyString === this.keyString
         )?.rectangle;
-        const rectangleForPixelDensity = this.transformRectangleToPixelDense(annotationRectangle);
-        if (this.isSelectedAnnotation(annotation)) {
-          this.drawRectInCanvas(
-            rectangleForPixelDensity,
-            SELECTED_ANNOTATION_FILL_STYLE,
-            SELECTED_ANNOTATION_STROKE_COLOR
-          );
-        } else {
-          this.drawRectInCanvas(
-            rectangleForPixelDensity,
-            EXISTING_ANNOTATION_FILL_STYLE,
-            EXISTING_ANNOTATION_STROKE_STYLE
-          );
+
+        // Check if the rectangle has already been drawn
+        const hasBeenDrawn = drawnRectangles.some(
+          (drawnRect) =>
+            drawnRect.x === annotationRectangle.x &&
+            drawnRect.y === annotationRectangle.y &&
+            drawnRect.width === annotationRectangle.width &&
+            drawnRect.height === annotationRectangle.height
+        );
+        if (!hasBeenDrawn) {
+          const rectangleForPixelDensity = this.transformRectangleToPixelDense(annotationRectangle);
+          if (this.isSelectedAnnotation(annotation)) {
+            this.drawRectInCanvas(
+              rectangleForPixelDensity,
+              SELECTED_ANNOTATION_FILL_STYLE,
+              SELECTED_ANNOTATION_STROKE_COLOR
+            );
+          } else {
+            this.drawRectInCanvas(
+              rectangleForPixelDensity,
+              EXISTING_ANNOTATION_FILL_STYLE,
+              EXISTING_ANNOTATION_STROKE_STYLE
+            );
+          }
+          drawnRectangles.push(annotationRectangle);
         }
       });
     }
diff --git a/src/plugins/inspectorViews/annotations/AnnotationsInspectorView.vue b/src/plugins/inspectorViews/annotations/AnnotationsInspectorView.vue
index d28bb10397..1601bc149a 100644
--- a/src/plugins/inspectorViews/annotations/AnnotationsInspectorView.vue
+++ b/src/plugins/inspectorViews/annotations/AnnotationsInspectorView.vue
@@ -65,7 +65,7 @@ export default {
       }
 
       return this.loadedAnnotations.filter((annotation) => {
-        return !annotation.tags && !annotation._deleted;
+        return !annotation.tags;
       });
     },
     tagAnnotations() {
@@ -74,7 +74,7 @@ export default {
       }
 
       return this.loadedAnnotations.filter((annotation) => {
-        return !annotation.tags && !annotation._deleted;
+        return !annotation.tags;
       });
     },
     multiSelection() {
diff --git a/src/plugins/inspectorViews/annotations/tags/TagEditor.vue b/src/plugins/inspectorViews/annotations/tags/TagEditor.vue
index e6df4bf895..aae6e22ec5 100644
--- a/src/plugins/inspectorViews/annotations/tags/TagEditor.vue
+++ b/src/plugins/inspectorViews/annotations/tags/TagEditor.vue
@@ -35,10 +35,13 @@
     <button
       v-show="!userAddingTag && !maxTagsAdded"
       class="c-tag-applier__add-btn c-icon-button c-icon-button--major icon-plus"
+      :class="TagEditorClassNames.ADD_TAG_BUTTON"
       title="Add new tag"
       @click="addTag"
     >
-      <div class="c-icon-button__label c-tag-btn__label">Add Tag</div>
+      <div class="c-icon-button__label c-tag-btn__label" :class="TagEditorClassNames.ADD_TAG_LABEL">
+        Add Tag
+      </div>
     </button>
   </div>
 </template>
@@ -46,6 +49,7 @@
 <script>
 import { toRaw } from 'vue';
 
+import TagEditorClassNames from './TagEditorClassNames';
 import TagSelection from './TagSelection.vue';
 
 export default {
@@ -88,7 +92,8 @@ export default {
   data() {
     return {
       addedTags: [],
-      userAddingTag: false
+      userAddingTag: false,
+      TagEditorClassNames: TagEditorClassNames
     };
   },
   computed: {
diff --git a/src/plugins/inspectorViews/annotations/tags/TagEditorClassNames.js b/src/plugins/inspectorViews/annotations/tags/TagEditorClassNames.js
new file mode 100644
index 0000000000..3fe4a93abe
--- /dev/null
+++ b/src/plugins/inspectorViews/annotations/tags/TagEditorClassNames.js
@@ -0,0 +1,9 @@
+const TagEditorClassNames = Object.freeze({
+  REMOVE_TAG: 'js-remove-tag',
+  AUTOCOMPLETE_INPUT: 'js-autocomplete__input',
+  ADD_TAG_BUTTON: 'js-add-tag-button',
+  ADD_TAG_LABEL: 'js-add-tag-label',
+  TAG_OPTION: 'js-tag-option'
+});
+
+export default TagEditorClassNames;
diff --git a/src/plugins/inspectorViews/annotations/tags/TagSelection.vue b/src/plugins/inspectorViews/annotations/tags/TagSelection.vue
index 40d380b5d3..df0a80729e 100644
--- a/src/plugins/inspectorViews/annotations/tags/TagSelection.vue
+++ b/src/plugins/inspectorViews/annotations/tags/TagSelection.vue
@@ -29,7 +29,7 @@
         :model="availableTagModel"
         :place-holder-text="'Type to select tag'"
         class="c-tag-selection"
-        :item-css-class="'icon-circle'"
+        :item-css-class="`icon-circle ${TagEditorClassNames.TAG_OPTION}`"
         @on-change="tagSelected"
       />
     </template>
@@ -42,6 +42,7 @@
         <button
           v-show="!readOnly"
           class="c-completed-tag-deletion c-tag__remove-btn icon-x-in-circle"
+          :class="TagEditorClassNames.REMOVE_TAG"
           :style="{ textShadow: selectedBackgroundColor + ' 0 0 4px' }"
           :aria-label="`Remove tag ${selectedTagLabel}`"
           @click="removeTag"
@@ -54,6 +55,7 @@
 
 <script>
 import AutoCompleteField from '../../../../api/forms/components/controls/AutoCompleteField.vue';
+import TagEditorClassNames from './TagEditorClassNames';
 
 export default {
   components: {
@@ -88,7 +90,7 @@ export default {
   },
   emits: ['tag-removed', 'tag-added'],
   data() {
-    return {};
+    return { TagEditorClassNames: TagEditorClassNames };
   },
   computed: {
     availableTagModel() {
@@ -137,7 +139,6 @@ export default {
       }
     }
   },
-  mounted() {},
   methods: {
     getAvailableTagByID(tagID) {
       return this.openmct.annotation.getAvailableTags().find((tag) => {
diff --git a/src/plugins/notebook/components/NotebookComponent.vue b/src/plugins/notebook/components/NotebookComponent.vue
index 4b08974866..8489b149b7 100644
--- a/src/plugins/notebook/components/NotebookComponent.vue
+++ b/src/plugins/notebook/components/NotebookComponent.vue
@@ -139,6 +139,7 @@
             @editing-entry="startTransaction"
             @delete-entry="deleteEntry"
             @update-entry="updateEntry"
+            @update-annotations="loadAnnotations"
             @entry-selection="entrySelection(entry)"
           />
         </div>
@@ -298,6 +299,12 @@ export default {
     },
     showTime() {
       mutateObject(this.openmct, this.domainObject, 'configuration.showTime', this.showTime);
+    },
+    notebookAnnotations: {
+      handler() {
+        this.filterAndSortEntries();
+      },
+      deep: true
     }
   },
   beforeMount() {
diff --git a/src/plugins/notebook/components/NotebookEntry.vue b/src/plugins/notebook/components/NotebookEntry.vue
index c8a378a3da..3a983de509 100644
--- a/src/plugins/notebook/components/NotebookEntry.vue
+++ b/src/plugins/notebook/components/NotebookEntry.vue
@@ -274,7 +274,8 @@ export default {
     'change-section-page',
     'update-entry',
     'editing-entry',
-    'entry-selection'
+    'entry-selection',
+    'update-annotations'
   ],
   data() {
     return {
@@ -638,13 +639,16 @@ export default {
       this.entry.text = restoredQuoteBrackets;
       this.timestampAndUpdate();
     },
+    updateAnnotations(newAnnotations) {
+      this.$emit('update-annotations', newAnnotations);
+    },
     selectAndEmitEntry(event, entry) {
       selectEntry({
         element: this.$refs.entry,
         entryId: entry.id,
         domainObject: this.domainObject,
         openmct: this.openmct,
-        onAnnotationChange: this.timestampAndUpdate,
+        onAnnotationChange: this.updateAnnotations,
         notebookAnnotations: this.notebookAnnotations
       });
       event.stopPropagation();
diff --git a/src/plugins/plot/MctPlot.vue b/src/plugins/plot/MctPlot.vue
index 3f9d191223..5f908fa2e2 100644
--- a/src/plugins/plot/MctPlot.vue
+++ b/src/plugins/plot/MctPlot.vue
@@ -178,7 +178,9 @@
 import Flatbush from 'flatbush';
 import _ from 'lodash';
 import { useEventBus } from 'utils/useEventBus';
+import { toRaw } from 'vue';
 
+import TagEditorClassNames from '../inspectorViews/annotations/tags/TagEditorClassNames';
 import XAxis from './axis/XAxis.vue';
 import YAxis from './axis/YAxis.vue';
 import MctChart from './chart/MctChart.vue';
@@ -465,9 +467,14 @@ export default {
     cancelSelection(event) {
       if (this.$refs?.plot) {
         const clickedInsidePlot = this.$refs.plot.contains(event.target);
+        // unfortunate side effect from possibly being detached from the DOM when
+        // adding/deleting tags, so closest() won't work
+        const clickedTagEditor = Object.values(TagEditorClassNames).some((className) => {
+          return event.target.classList.contains(className);
+        });
         const clickedInsideInspector = event.target.closest('.js-inspector') !== null;
         const clickedOption = event.target.closest('.js-autocomplete-options') !== null;
-        if (!clickedInsidePlot && !clickedInsideInspector && !clickedOption) {
+        if (!clickedInsidePlot && !clickedInsideInspector && !clickedOption && !clickedTagEditor) {
           this.rectangles = [];
           this.annotationSelectionsBySeries = {};
           this.selectPlot();
@@ -937,7 +944,10 @@ export default {
       const targetDetails = [];
       const uniqueBoundsAnnotations = [];
       annotations.forEach((annotation) => {
-        targetDetails.push(annotation.targets);
+        // for each target, push toRaw
+        annotation.targets.forEach((target) => {
+          targetDetails.push(toRaw(target));
+        });
 
         const boundingBoxAlreadyAdded = uniqueBoundsAnnotations.some((existingAnnotation) => {
           const existingBoundingBox = Object.values(existingAnnotation.targets)[0];