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>
2024-12-19 21:27:52 +00:00 · 2023-12-05 04:12:24 +01:00 · 2023-12-05 04:12:24 +01:00 · 2d9c0414f7
commit 2d9c0414f7
parent a3e0a0f694
11 changed files with 159 additions and 34 deletions
--- 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 }) => {
--- 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();
+  });
 });
--- 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 }) => {
--- 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);
        }
      });
    }
--- 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() {
--- 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: {
--- a/src/plugins/inspectorViews/annotations/tags/TagEditorClassNames.js
+++ 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;
--- 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) => {
--- 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() {
--- 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();
--- 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];