When auto scale is turned off, handle user specified range correctly (#6258)

* Fix typo when saving the user specified range
* Ensure range is specified when autoscale is turned off
* Don't redraw unless absolutely necessary
* Add 'stats' to the handled attributes for redrawing plots
* Handle x axis displayRange, marker shape and size to redraw
* If there are is no closest data point for a plot, skip annotation gathering
* Ensure that min and max user defined ranges are valid when autoscale is disabled. Otherwise, enable autoscale to true.
* Fix autoscale e2e test
* updated snapshot
* Update e2e/README.md
Co-authored-by: Jesse Mazzella <ozyx@users.noreply.github.com>
Co-authored-by: John Hill <john.c.hill@nasa.gov>
Co-authored-by: Jesse Mazzella <ozyx@users.noreply.github.com>

e2e/README.md

@@ -89,7 +89,7 @@ Read more about [Playwright Snapshots](https://playwright.dev/docs/test-snapshot
 #### Open MCT's implementation
 
 - Our Snapshot tests receive a `@snapshot` tag.
-- Snapshots need to be executed within the official Playwright container to ensure we're using the exact rendering platform in CI and locally.
+- Snapshots need to be executed within the official Playwright container to ensure we're using the exact rendering platform in CI and locally. To do a valid comparison locally:
 
 ```sh
 docker run --rm --network host -v $(pwd):/work/ -w /work/ -it mcr.microsoft.com/playwright:[GET THIS VERSION FROM OUR CIRCLECI CONFIG FILE]-focal /bin/bash
@@ -97,9 +97,24 @@ npm install
 npx playwright test --config=e2e/playwright-ci.config.js --project=chrome --grep @snapshot
 ```
 
-### (WIP) Updating Snapshots
+### Updating Snapshots
 
-When the `@snapshot` tests fail, they will need to be evaluated to see if the failure is an acceptable change or
+When the `@snapshot` tests fail, they will need to be evaluated to determine if the failure is an acceptable and desireable or an unintended regression.
+
+To compare a snapshot, run a test and open the html report with the 'Expected' vs 'Actual' screenshot. If the actual screenshot is preferred, then the source-controlled 'Expected' snapshots will need to be updated with the following scripts.
+
+MacOS
+```
+npm run test:e2e:updatesnapshots
+```
+
+Linux/CI
+```sh
+// Replace {X.X.X} with the current Playwright version 
+// from our package.json or circleCI configuration file
+docker run --rm --network host -v $(pwd):/work/ -w /work/ -it mcr.microsoft.com/playwright:v{X.X.X}-focal /bin/bash
+npm install
+npm run test:e2e:updatesnapshots
 
 ## Performance Testing
 

e2e/tests/functional/plugins/plot/autoscale.e2e.spec.js

@@ -32,7 +32,7 @@ test.use({
     }
 });
 
-test.describe('ExportAsJSON', () => {
+test.describe('Autoscale', () => {
     test('User can set autoscale with a valid range @snapshot', async ({ page, openmctConfig }) => {
         const { myItemsFolderName } = openmctConfig;
 
@@ -47,16 +47,32 @@ test.describe('ExportAsJSON', () => {
 
         await testYTicks(page, ['-1.00', '-0.50', '0.00', '0.50', '1.00']);
 
+        // enter edit mode
+        await page.click('button[title="Edit"]');
+
         await turnOffAutoscale(page);
 
-        // Make sure that after turning off autoscale, the user selected range values start at the same values the plot had prior.
-        await testYTicks(page, ['-1.00', '-0.50', '0.00', '0.50', '1.00']);
+        await setUserDefinedMinAndMax(page, '-2', '2');
+
+        // save
+        await page.click('button[title="Save"]');
+        await Promise.all([
+            page.locator('li[title = "Save and Finish Editing"]').click(),
+            //Wait for Save Banner to appear
+            page.waitForSelector('.c-message-banner__message')
+        ]);
+        //Wait until Save Banner is gone
+        await page.locator('.c-message-banner__close-button').click();
+        await page.waitForSelector('.c-message-banner__message', { state: 'detached'});
+
+        // Make sure that after turning off autoscale, the user entered range values are reflexted in the ticks.
+        await testYTicks(page, ['-2.00', '-1.50', '-1.00', '-0.50', '0.00', '0.50', '1.00', '1.50', '2.00']);
 
         const canvas = page.locator('canvas').nth(1);
 
         await canvas.hover({trial: true});
 
-        expect(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-prepan.png', { animations: 'disabled' });
+        expect.soft(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-prepan.png', { animations: 'disabled' });
 
         //Alt Drag Start
         await page.keyboard.down('Alt');
@@ -76,11 +92,12 @@ test.describe('ExportAsJSON', () => {
         await page.keyboard.up('Alt');
 
         // Ensure the drag worked.
-        await testYTicks(page, ['0.00', '0.50', '1.00', '1.50', '2.00']);
+        await testYTicks(page, ['0.00', '0.50', '1.00', '1.50', '2.00', '2.50', '3.00', '3.50']);
 
+        //Wait for canvas to stablize.
         await canvas.hover({trial: true});
 
-        expect(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-panned.png', { animations: 'disabled' });
+        expect.soft(await canvas.screenshot()).toMatchSnapshot('autoscale-canvas-panned.png', { animations: 'disabled' });
     });
 });
 
@@ -152,22 +169,25 @@ async function createSinewaveOverlayPlot(page, myItemsFolderName) {
  * @param {import('@playwright/test').Page} page
  */
 async function turnOffAutoscale(page) {
-    // enter edit mode
-    await page.locator('text=Unnamed Overlay Plot Snapshot >> button').nth(3).click();
-
     // uncheck autoscale
     await page.getByRole('listitem').filter({ hasText: 'Auto scale' }).getByRole('checkbox').uncheck();
+}
 
-    // save
-    await page.locator('text=Snapshot Save and Finish Editing Save and Continue Editing >> button').nth(1).click();
-    await Promise.all([
-        page.locator('text=Save and Finish Editing').click(),
-        //Wait for Save Banner to appear
-        page.waitForSelector('.c-message-banner__message')
-    ]);
-    //Wait until Save Banner is gone
-    await page.locator('.c-message-banner__close-button').click();
-    await page.waitForSelector('.c-message-banner__message', { state: 'detached'});
+/**
+ * @param {import('@playwright/test').Page} page
+ * @param {string} min
+ * @param {string} max
+ */
+async function setUserDefinedMinAndMax(page, min, max) {
+    // set minimum value
+    const minRangeInput = page.getByRole('listitem').filter({ hasText: 'Minimum Value' }).locator('input[type="number"]');
+    await minRangeInput.click();
+    await minRangeInput.fill(min);
+
+    // set maximum value
+    const maxRangeInput = page.getByRole('listitem').filter({ hasText: 'Maximum Value' }).locator('input[type="number"]');
+    await maxRangeInput.click();
+    await maxRangeInput.fill(max);
 }
 
 /**
@@ -179,7 +199,7 @@ async function testYTicks(page, values) {
     let promises = [yTicks.count().then(c => expect(c).toBe(values.length))];
 
     for (let i = 0, l = values.length; i < l; i += 1) {
-        promises.push(expect(yTicks.nth(i)).toHaveText(values[i])); // eslint-disable-line
+        promises.push(expect.soft(yTicks.nth(i)).toHaveText(values[i])); // eslint-disable-line
     }
 
     await Promise.all(promises);

src/plugins/plot/MctPlot.vue

@@ -855,7 +855,7 @@ export default {
         gatherNearbyAnnotations() {
             const nearbyAnnotations = [];
             this.config.series.models.forEach(series => {
-                if (series.closest.annotationsById) {
+                if (series?.closest?.annotationsById) {
                     Object.values(series.closest.annotationsById).forEach(closeAnnotation => {
                         const addedAnnotationAlready = nearbyAnnotations.some(annotation => {
                             return _.isEqual(annotation.targets, closeAnnotation.targets)

src/plugins/plot/chart/MctChart.vue

@@ -52,6 +52,41 @@ const MARKER_SIZE = 6.0;
 const HIGHLIGHT_SIZE = MARKER_SIZE * 2.0;
 const ANNOTATION_SIZE = MARKER_SIZE * 3.0;
 const CLEARANCE = 15;
+// These attributes are changed in the plot model, but we don't need to react to the changes.
+const NO_HANDLING_NEEDED_ATTRIBUTES = {
+    label: 'label',
+    values: 'values',
+    format: 'format',
+    color: 'color',
+    name: 'name',
+    unit: 'unit'
+};
+// These attributes in turn set one of HANDLED_ATTRIBUTES, so we don't need specific listeners for them - this prevents excessive redraws.
+const IMPLICIT_HANDLED_ATTRIBUTES = {
+    range: 'range',
+    //series stats update y axis stats
+    stats: 'stats',
+    frozen: 'frozen',
+    autoscale: 'autoscale',
+    autoscalePadding: 'autoscalePadding',
+    logMode: 'logMode',
+    yKey: 'yKey'
+};
+// Attribute changes that we are specifically handling with listeners
+const HANDLED_ATTRIBUTES = {
+    //X and Y Axis attributes
+    key: 'key',
+    displayRange: 'displayRange',
+    //series attributes
+    xKey: 'xKey',
+    interpolate: 'interpolate',
+    markers: 'markers',
+    markerShape: 'markerShape',
+    markerSize: 'markerSize',
+    alarmMarkers: 'alarmMarkers',
+    limitLines: 'limitLines',
+    yAxisId: 'yAxisId'
+};
 
 export default {
     inject: ['openmct', 'domainObject', 'path'],
@@ -138,14 +173,16 @@ export default {
         this.offset = {
             [yAxisId]: {}
         };
-        this.listenTo(this.config.yAxis, 'change:key', this.resetYOffsetAndSeriesDataForYAxis.bind(this, yAxisId), this);
-        this.listenTo(this.config.yAxis, 'change', this.updateLimitsAndDraw);
+        this.listenTo(this.config.yAxis, `change:${HANDLED_ATTRIBUTES.displayRange}`, this.scheduleDraw);
+        this.listenTo(this.config.yAxis, `change:${HANDLED_ATTRIBUTES.key}`, this.resetYOffsetAndSeriesDataForYAxis.bind(this, yAxisId), this);
+        this.listenTo(this.config.yAxis, 'change', this.redrawIfNotAlreadyHandled);
         if (this.config.additionalYAxes.length) {
             this.config.additionalYAxes.forEach(yAxis => {
                 const id = yAxis.get('id');
                 this.offset[id] = {};
-                this.listenTo(yAxis, 'change', this.updateLimitsAndDraw);
-                this.listenTo(yAxis, 'change:key', this.resetYOffsetAndSeriesDataForYAxis.bind(this, id), this);
+                this.listenTo(yAxis, `change:${HANDLED_ATTRIBUTES.displayRange}`, this.scheduleDraw);
+                this.listenTo(yAxis, `change:${HANDLED_ATTRIBUTES.key}`, this.resetYOffsetAndSeriesDataForYAxis.bind(this, id), this);
+                this.listenTo(yAxis, 'change', this.redrawIfNotAlreadyHandled);
             });
         }
 
@@ -162,7 +199,8 @@ export default {
         this.listenTo(this.config.series, 'add', this.onSeriesAdd, this);
         this.listenTo(this.config.series, 'remove', this.onSeriesRemove, this);
 
-        this.listenTo(this.config.xAxis, 'change', this.updateLimitsAndDraw);
+        this.listenTo(this.config.xAxis, 'change:displayRange', this.scheduleDraw);
+        this.listenTo(this.config.xAxis, 'change', this.redrawIfNotAlreadyHandled);
         this.config.series.forEach(this.onSeriesAdd, this);
         this.$emit('chartLoaded');
     },
@@ -191,14 +229,15 @@ export default {
             this.changeLimitLines(mode, o, series);
         },
         onSeriesAdd(series) {
-            this.listenTo(series, 'change:xKey', this.reDraw, this);
-            this.listenTo(series, 'change:interpolate', this.changeInterpolate, this);
-            this.listenTo(series, 'change:markers', this.changeMarkers, this);
-            this.listenTo(series, 'change:alarmMarkers', this.changeAlarmMarkers, this);
-            this.listenTo(series, 'change:limitLines', this.changeLimitLines, this);
-            this.listenTo(series, 'change:yAxisId', this.resetAxisAndRedraw, this);
-            // TODO: Which other changes is the listener below reacting to?
-            this.listenTo(series, 'change', this.scheduleDraw);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.xKey}`, this.reDraw, this);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.interpolate}`, this.changeInterpolate, this);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.markers}`, this.changeMarkers, this);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.alarmMarkers}`, this.changeAlarmMarkers, this);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.limitLines}`, this.changeLimitLines, this);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.yAxisId}`, this.resetAxisAndRedraw, this);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.markerShape}`, this.scheduleDraw, this);
+            this.listenTo(series, `change:${HANDLED_ATTRIBUTES.markerSize}`, this.scheduleDraw, this);
+            this.listenTo(series, 'change', this.redrawIfNotAlreadyHandled);
             this.listenTo(series, 'add', this.onAddPoint);
             this.makeChartElement(series);
             this.makeLimitLines(series);
@@ -531,8 +570,25 @@ export default {
 
             return true;
         },
+        redrawIfNotAlreadyHandled(attribute, value, oldValue) {
+            if (Object.keys(HANDLED_ATTRIBUTES).includes(attribute) && oldValue) {
+                return;
+            }
+
+            if (Object.keys(IMPLICIT_HANDLED_ATTRIBUTES).includes(attribute) && oldValue) {
+                return;
+            }
+
+            if (Object.keys(NO_HANDLING_NEEDED_ATTRIBUTES).includes(attribute) && oldValue) {
+                return;
+            }
+
+            console.warn('Unhandled change:', attribute);
+            this.updateLimitsAndDraw();
+        },
         updateLimitsAndDraw() {
             this.drawLimitLines();
+            this.scheduleDraw();
         },
         scheduleDraw() {
             if (!this.drawScheduled) {

src/plugins/plot/configuration/PlotSeries.js

@@ -252,6 +252,7 @@ export default class PlotSeries extends Model {
         }
 
         const valueMetadata = this.metadata.value(newKey);
+        //TODO: Should we do this even if there is a persisted config?
         if (!this.persistedConfig || !this.persistedConfig.interpolate) {
             if (valueMetadata.format === 'enum') {
                 this.set('interpolate', 'stepAfter');

src/plugins/plot/configuration/YAxisModel.js

@@ -57,7 +57,14 @@ export default class YAxisModel extends Model {
         this.listenTo(this, 'change:logMode', this.onLogModeChange, this);
         this.listenTo(this, 'change:frozen', this.toggleFreeze, this);
         this.listenTo(this, 'change:range', this.updateDisplayRange, this);
-        this.updateDisplayRange(this.get('range'));
+        const range = this.get('range');
+        this.updateDisplayRange(range);
+        //This is an edge case and should not happen
+        const invalidRange = !range || (range?.min === undefined || range?.max === undefined);
+        const invalidAutoScaleOff = (options.model.autoscale === false) && invalidRange;
+        if (invalidAutoScaleOff) {
+            this.set('autoscale', true);
+        }
     }
     /**
      * @param {import('./SeriesCollection').default} seriesCollection
@@ -250,23 +257,6 @@ export default class YAxisModel extends Model {
             }
 
             this.set('displayRange', _range);
-        } else {
-            // Otherwise use the last known displayRange as the initial
-            // values for the user-defined range, so that we don't end up
-            // with any error from an undefined user range.
-
-            const _range = this.get('displayRange');
-
-            if (!_range) {
-                return;
-            }
-
-            if (this.get('logMode')) {
-                _range.min = antisymlog(_range.min, 10);
-                _range.max = antisymlog(_range.max, 10);
-            }
-
-            this.set('range', _range);
         }
     }
 
@@ -377,11 +367,8 @@ export default class YAxisModel extends Model {
             autoscale: true,
             logMode: options.model?.logMode ?? false,
             autoscalePadding: 0.1,
-            id: options.id
-
-            // 'range' is not specified here, it is undefined at first. When the
-            // user turns off autoscale, the current 'displayRange' is used for
-            // the initial value of 'range'.
+            id: options.id,
+            range: options.model?.range
         };
     }
 }

src/plugins/plot/inspector/PlotOptionsBrowse.vue

@@ -73,7 +73,7 @@
                 </div>
             </li>
             <li
-                v-if="!yAxis.autoscale && yAxis.rangeMin"
+                v-if="!yAxis.autoscale && yAxis.rangeMin !== ''"
                 class="grid-row"
             >
                 <div
@@ -83,7 +83,7 @@
                 <div class="grid-cell value">{{ yAxis.rangeMin }}</div>
             </li>
             <li
-                v-if="!yAxis.autoscale && yAxis.rangeMax"
+                v-if="!yAxis.autoscale && yAxis.rangeMax !== ''"
                 class="grid-row"
             >
                 <div
@@ -217,8 +217,8 @@ export default {
                     autoscale: this.config.yAxis.get('autoscale'),
                     logMode: this.config.yAxis.get('logMode'),
                     autoscalePadding: this.config.yAxis.get('autoscalePadding'),
-                    rangeMin: range ? range.min : '',
-                    rangeMax: range ? range.max : ''
+                    rangeMin: range?.min ?? '',
+                    rangeMax: range?.max ?? ''
                 });
                 this.config.additionalYAxes.forEach(yAxis => {
                     range = yAxis.get('range');
@@ -230,8 +230,8 @@ export default {
                         autoscale: yAxis.get('autoscale'),
                         logMode: yAxis.get('logMode'),
                         autoscalePadding: yAxis.get('autoscalePadding'),
-                        rangeMin: range ? range.min : '',
-                        rangeMax: range ? range.max : ''
+                        rangeMin: range?.min ?? '',
+                        rangeMax: range?.max ?? ''
                     });
                 });
             }

src/plugins/plot/inspector/forms/YAxisForm.vue

@@ -81,7 +81,7 @@
             >Minimum Value</div>
             <div class="grid-cell value">
                 <input
-                    v-model="rangeMin"
+                    v-model.lazy="rangeMin"
                     class="c-input--flex"
                     type="number"
                     @change="updateForm('range')"
@@ -94,7 +94,7 @@
                 title="Maximum Y axis value."
             >Maximum Value</div>
             <div class="grid-cell value"><input
-                v-model="rangeMax"
+                v-model.lazy="rangeMax"
                 class="c-input--flex"
                 type="number"
                 @change="updateForm('range')"
@@ -131,6 +131,12 @@ export default {
             loaded: false
         };
     },
+    beforeDestroy() {
+        if (this.autoscale === false && this.validationErrors.range) {
+            this.autoscale = true;
+            this.updateForm('autoscale');
+        }
+    },
     mounted() {
         eventHelpers.extend(this);
         this.getConfig();
@@ -172,12 +178,9 @@ export default {
                     objectPath: `${prefix}.logMode`
                 },
                 range: {
-                    objectPath: `${prefix}.range'`,
+                    objectPath: `${prefix}.range`,
                     coerce: function coerceRange(range) {
-                        const newRange = {
-                            min: -1,
-                            max: 1
-                        };
+                        const newRange = {};
 
                         if (range && typeof range.min !== 'undefined' && range.min !== null) {
                             newRange.min = Number(range.min);
@@ -222,9 +225,11 @@ export default {
             this.autoscale = this.yAxis.get('autoscale');
             this.logMode = this.yAxis.get('logMode');
             this.autoscalePadding = this.yAxis.get('autoscalePadding');
-            const range = this.yAxis.get('range') ?? this.yAxis.get('displayRange');
-            this.rangeMin = range?.min;
-            this.rangeMax = range?.max;
+            const range = this.yAxis.get('range');
+            if (range && range.min !== undefined && range.max !== undefined) {
+                this.rangeMin = range.min;
+                this.rangeMax = range.max;
+            }
         },
         getPrefix() {
             let prefix = 'yAxis';
@@ -311,6 +316,15 @@ export default {
                             });
                         }
                     }
+
+                    //If autoscale is turned off, we must know what the user defined min and max ranges are
+                    if (formKey === 'autoscale' && this.autoscale === false) {
+                        const rangeFormField = this.fields.range;
+                        this.validationErrors.range = rangeFormField.validate?.({
+                            min: this.rangeMin,
+                            max: this.rangeMax
+                        }, this.yAxis);
+                    }
                 }
             }
         }