[Time Conductor] Addressed code review comments. Fixes #1287

platform/features/conductor-v2/conductor/res/templates/time-conductor.html

@@ -87,7 +87,7 @@
         <div class="l-data-visualization-holder l-row-elem flex-elem"
              ng-controller="ConductorTOIController as toi">
             <a class="l-page-button s-icon-button icon-pointer-left"></a>
-            <div class="l-data-visualization" ng-click="toi.click($event)">
+            <div class="l-data-visualization" ng-click="toi.setTOIFromPosition($event)">
                 <mct-include key="'time-of-interest'"
                              class="l-toi-holder show-val"
                              ng-class="{ pinned: toi.pinned, 'val-to-left': toi.left > 80 }"

platform/features/conductor-v2/conductor/src/ui/ConductorAxisController.js

@@ -28,9 +28,9 @@ define(
         var PADDING = 1;
 
         /**
-         * The mct-conductor-axis renders a horizontal axis with regular
+         * Controller that renders a horizontal time scale spanning the current bounds defined in the time conductor.
-         * labelled 'ticks'. It requires 'start' and 'end' integer values to
+         * Used by the mct-conductor-axis directive
-         * be specified as attributes.
+         * @constructor
          */
         function ConductorAxisController(openmct, formatService, conductorViewService, scope, element) {
             // Dependencies
@@ -54,6 +54,9 @@ define(
             this.initialize(element);
         }
 
+        /**
+         * @private
+         */
         ConductorAxisController.prototype.destroy = function () {
             this.conductor.off('timeSystem', this.changeTimeSystem);
             this.conductor.off('bounds', this.changeBounds);
@@ -62,9 +65,7 @@ define(
         };
 
         /**
-         * Set defaults, and apply d3 axis to the
+         * @private
-         * @param scope
-         * @param element
          */
         ConductorAxisController.prototype.initialize = function (element) {
             this.target = element[0].firstChild;
@@ -95,6 +96,9 @@ define(
             this.conductorViewService.on("zoom-stop", this.onZoomStop);
         };
 
+        /**
+         * @private
+         */
         ConductorAxisController.prototype.changeBounds = function (bounds) {
             this.bounds = bounds;
             if (!this.zooming) {
@@ -127,8 +131,7 @@ define(
         };
 
         /**
-         * When the time system changes, update the scale and formatter used
+         * When the time system changes, update the scale and formatter used for showing times.
-         * for showing times.
          * @param timeSystem
          */
         ConductorAxisController.prototype.changeTimeSystem = function (timeSystem) {
@@ -164,12 +167,27 @@ define(
             }
         };
 
+        /**
+         * The user has stopped panning the time conductor scale element.
+         * @event panStop
+         */
+        /**
+         * Called on release of mouse button after dragging the scale left or right.
+         * @fires platform.features.conductor.ConductorAxisController~panStop
+         */
         ConductorAxisController.prototype.panStop = function () {
             //resync view bounds with time conductor bounds
             this.conductorViewService.emit("pan-stop");
             this.conductor.bounds(this.bounds);
         };
 
+        /**
+         * Rescales the axis when the user zooms. Although zoom ultimately results in a bounds change once the user
+         * releases the zoom slider, dragging the slider will not immediately change the conductor bounds. It will
+         * however immediately update the scale and the bounds displayed in the UI.
+         * @private
+         * @param {ZoomLevel}
+         */
         ConductorAxisController.prototype.onZoom = function (zoom) {
             this.zooming = true;
 
@@ -177,15 +195,23 @@ define(
             this.setScale();
         };
 
+        /**
+         * @private
+         */
         ConductorAxisController.prototype.onZoomStop = function (zoom) {
             this.zooming = false;
         };
 
+        /**
+         * @event platform.features.conductor.ConductorAxisController~pan
+         * Fired when the time conductor is panned
+         */
         /**
          * Initiate panning via a click + drag gesture on the time conductor
          * scale. Panning triggers a "pan" event
          * @param {number} delta the offset from the original click event
          * @see TimeConductorViewService#
+         * @fires platform.features.conductor.ConductorAxisController~pan
          */
         ConductorAxisController.prototype.pan = function (delta) {
             if (!this.conductor.follow()) {
@@ -202,6 +228,9 @@ define(
             }
         };
 
+        /**
+         * Invoked on element resize. Will rebuild the scale based on the new dimensions of the element.
+         */
         ConductorAxisController.prototype.resize = function () {
             this.setScale();
         };

platform/features/conductor-v2/conductor/src/ui/ConductorTOIController.js

@@ -25,9 +25,9 @@ define(
     function ($) {
 
         /**
-         * The mct-conductor-axis renders a horizontal axis with regular
+         * Controller for the Time of Interest indicator in the conductor itself. Sets the horizontal position of the
-         * labelled 'ticks'. It requires 'start' and 'end' integer values to
+         * TOI indicator based on the current value of the TOI, and the width of the TOI conductor.
-         * be specified as attributes.
+         * @memberof platform.features.conductor
          */
         function ConductorTOIController($scope, openmct, conductorViewService) {
             this.conductor = openmct.conductor;
@@ -41,7 +41,7 @@ define(
             }.bind(this));
 
             this.conductor.on('timeOfInterest', this.changeTimeOfInterest);
-            this.conductorViewService.on('zoom', this.setOffsetFromBounds);
+            this.conductorViewService.on('zoom', this.setOffsetFromZoom);
             this.conductorViewService.on('pan', this.setOffsetFromBounds);
 
             var timeOfInterest = this.conductor.timeOfInterest();
@@ -50,12 +50,14 @@ define(
             }
 
             $scope.$on('$destroy', this.destroy);
-
         }
 
+        /**
+         * @private
+         */
         ConductorTOIController.prototype.destroy = function () {
             this.conductor.off('timeOfInterest', this.changeTimeOfInterest);
-            this.conductorViewService.off('zoom', this.setOffsetFromBounds);
+            this.conductorViewService.off('zoom', this.setOffsetFromZoom);
             this.conductorViewService.off('pan', this.setOffsetFromBounds);
         };
 
@@ -80,6 +82,17 @@ define(
             }
         };
 
+        /**
+         * @private
+         */
+        ConductorTOIController.prototype.setOffsetFromZoom = function (zoom) {
+            return this.setOffsetFromBounds(zoom.bounds);
+        };
+
+        /**
+         * Invoked when time of interest changes. Will set the horizontal offset of the TOI indicator.
+         * @private
+         */
         ConductorTOIController.prototype.changeTimeOfInterest = function () {
             var bounds = this.conductor.bounds();
             if (bounds) {
@@ -88,10 +101,11 @@ define(
         };
 
         /**
-         * Set time of interest
+         * On a mouse click event within the TOI element, convert position within element to a time of interest, and
+         * set the time of interest on the conductor.
          * @param e The angular $event object
          */
-        ConductorTOIController.prototype.click = function (e) {
+        ConductorTOIController.prototype.setTOIFromPosition = function (e) {
             //TOI is set using the alt key modified + primary click
             if (e.altKey) {
                 var element = $(e.currentTarget);

platform/features/conductor-v2/conductor/src/ui/ConductorTOIControllerSpec.js

@@ -27,14 +27,127 @@ define([
 ) {
     var mockConductor;
     var mockConductorViewService;
+    var mockScope;
+    var mockAPI;
+    var conductorTOIController;
+
+    function getNamedCallback(thing, name) {
+        return thing.calls.filter(function (call) {
+            return call.args[0] === name;
+        }).map(function (call) {
+            return call.args;
+        })[0][1];
+    }
 
     describe("The ConductorTOIController", function () {
-        mockConductor = jasmine.createSpyObj("conductor", [
+        beforeEach(function () {
-            "on"
+            mockConductor = jasmine.createSpyObj("conductor", [
-        ]);
+                "bounds",
-        mockConductorViewService = jasmine.createSpyObj("conductorViewService", [
+                "timeOfInterest",
-            "on"
+                "on",
-        ]);
+                "off"
+            ]);
+            mockAPI = {conductor: mockConductor};
 
+            mockConductorViewService = jasmine.createSpyObj("conductorViewService", [
+                "on",
+                "off"
+            ]);
+
+            mockScope = jasmine.createSpyObj("openMCT", [
+                "$on"
+            ]);
+
+            conductorTOIController = new ConductorTOIController(mockScope, mockAPI, mockConductorViewService);
+        });
+
+        it("listens to changes in the time of interest on the conductor", function () {
+            expect(mockConductor.on).toHaveBeenCalledWith("timeOfInterest", jasmine.any(Function));
+        });
+
+        describe("when responding to changes in the time of interest", function () {
+            var toiCallback;
+            beforeEach(function () {
+                var bounds = {
+                    start: 0,
+                    end: 200
+                };
+                mockConductor.bounds.andReturn(bounds);
+                toiCallback = getNamedCallback(mockConductor.on, "timeOfInterest");
+            });
+
+            it("calculates the correct horizontal offset based on bounds and current TOI", function () {
+                //Expect time of interest position to be 50% of element width
+                mockConductor.timeOfInterest.andReturn(100);
+                toiCallback();
+                expect(conductorTOIController.left).toBe(50);
+
+                //Expect time of interest position to be 25% of element width
+                mockConductor.timeOfInterest.andReturn(50);
+                toiCallback();
+                expect(conductorTOIController.left).toBe(25);
+
+                //Expect time of interest position to be 0% of element width
+                mockConductor.timeOfInterest.andReturn(0);
+                toiCallback();
+                expect(conductorTOIController.left).toBe(0);
+
+                //Expect time of interest position to be 100% of element width
+                mockConductor.timeOfInterest.andReturn(200);
+                toiCallback();
+                expect(conductorTOIController.left).toBe(100);
+            });
+
+            it("renders the TOI indicator visible", function () {
+                expect(conductorTOIController.pinned).toBeFalsy();
+                mockConductor.timeOfInterest.andReturn(100);
+                toiCallback();
+                expect(conductorTOIController.pinned).toBe(true);
+            });
+        });
+
+        it("responds to zoom events", function () {
+            var mockZoom = {
+                bounds: {
+                    start: 500,
+                    end: 1000
+                }
+            };
+            expect(mockConductorViewService.on).toHaveBeenCalledWith("zoom", jasmine.any(Function));
+
+            // Should correspond to horizontal offset of 50%
+            mockConductor.timeOfInterest.andReturn(750);
+            var zoomCallback = getNamedCallback(mockConductorViewService.on, "zoom");
+            zoomCallback(mockZoom);
+            expect(conductorTOIController.left).toBe(50);
+        });
+
+        it("responds to pan events", function () {
+            var mockPanBounds = {
+                start: 1000,
+                end: 3000
+            };
+
+            expect(mockConductorViewService.on).toHaveBeenCalledWith("pan", jasmine.any(Function));
+
+            // Should correspond to horizontal offset of 25%
+            mockConductor.timeOfInterest.andReturn(1500);
+            var panCallback = getNamedCallback(mockConductorViewService.on, "pan");
+            panCallback(mockPanBounds);
+            expect(conductorTOIController.left).toBe(25);
+        });
+
+
+        it("Cleans up all listeners when controller destroyed", function () {
+            var zoomCB = getNamedCallback(mockConductorViewService.on, "zoom");
+            var panCB = getNamedCallback(mockConductorViewService.on, "pan");
+            var toiCB = getNamedCallback(mockConductor.on, "timeOfInterest");
+
+            expect(mockScope.$on).toHaveBeenCalledWith("$destroy", jasmine.any(Function));
+            getNamedCallback(mockScope.$on, "$destroy")();
+            expect(mockConductorViewService.off).toHaveBeenCalledWith("zoom", zoomCB);
+            expect(mockConductorViewService.off).toHaveBeenCalledWith("pan", panCB);
+            expect(mockConductor.off).toHaveBeenCalledWith("timeOfInterest", toiCB);
+        });
     });
 });

platform/features/conductor-v2/conductor/src/ui/TimeConductorController.js

@@ -26,6 +26,12 @@ define(
     ],
     function (TimeConductorValidation) {
 
+        /**
+         * Controller for the Time Conductor UI element. The Time Conductor includes form fields for specifying time
+         * bounds and relative time deltas for queries, as well as controls for selection mode, time systems, and zooming.
+         * @memberof platform.features.conductor
+         * @constructor
+         */
         function TimeConductorController($scope, $window, openmct, conductorViewService, timeSystems, formatService) {
 
             var self = this;
@@ -106,6 +112,9 @@ define(
             this.$scope.$on('$destroy', this.destroy);
         };
 
+        /**
+         * @private
+         */
         TimeConductorController.prototype.destroy = function () {
             this.conductor.off('bounds', this.changeBounds);
             this.conductor.off('timeSystem', this.changeTimeSystem);
@@ -114,7 +123,13 @@ define(
             this.conductorViewService.off('pan-stop', this.onPanStop);
         };
 
+        /**
+         * When the conductor bounds change, set the bounds in the form.
+         * @private
+         * @param {TimeConductorBounds} bounds
+         */
         TimeConductorController.prototype.changeBounds = function (bounds) {
+            //If a zoom or pan is currently in progress, do not override form values.
             if (!this.zooming && !this.panning) {
                 this.setFormFromBounds(bounds);
             }
@@ -176,6 +191,10 @@ define(
                 });
         };
 
+        /**
+         * When the deltas change, update the values in the UI
+         * @private
+         */
         TimeConductorController.prototype.setFormFromDeltas = function (deltas) {
             this.$scope.boundsModel.startDelta = deltas.start;
             this.$scope.boundsModel.endDelta = deltas.end;
@@ -183,7 +202,7 @@ define(
 
         /**
          * Initialize the form when time system changes.
-         * @param timeSystem
+         * @param {TimeSystem} timeSystem
          */
         TimeConductorController.prototype.setFormFromTimeSystem = function (timeSystem) {
             var timeSystemModel = this.$scope.timeSystemModel;
@@ -328,6 +347,15 @@ define(
             }
         };
 
+        /**
+         * Fired when user has released the zoom slider
+         * @event platform.features.conductor.TimeConductorController~zoomStop
+         */
+        /**
+         * Invoked when zoom slider is released by user. Will update the time conductor with the new bounds, triggering
+         * a global bounds change event.
+         * @fires platform.features.conductor.TimeConductorController~zoomStop
+         */
         TimeConductorController.prototype.onZoomStop = function () {
             this.updateBoundsFromForm(this.$scope.boundsModel);
             this.updateDeltasFromForm(this.$scope.boundsModel);
@@ -342,7 +370,7 @@ define(
          * conductor. Panning updates the scale and bounds fields
          * immediately, but does not trigger a bounds change to other views
          * until the mouse button is released.
-         * @param bounds
+         * @param {TimeConductorBounds} bounds
          */
         TimeConductorController.prototype.onPan = function (bounds) {
             this.panning = true;
@@ -350,6 +378,9 @@ define(
             this.$scope.boundsModel.end = bounds.end;
         };
 
+        /**
+         * Called when the user releases the mouse button after panning.
+         */
         TimeConductorController.prototype.onPanStop = function () {
             this.panning = false;
         };

platform/features/conductor-v2/conductor/src/ui/TimeConductorControllerSpec.js

@@ -374,8 +374,6 @@ define(['./TimeConductorController'], function (TimeConductorController) {
                 expect(controller.$scope.boundsModel.start).not.toBe(testBounds.start);
                 expect(controller.$scope.boundsModel.end).not.toBe(testBounds.end);
 
-                // use registered CB instead
-                // controller.onPan(testBounds);
                 expect(controller.conductorViewService.on).toHaveBeenCalledWith("pan",
                     controller.onPan);
 

platform/features/conductor-v2/conductor/src/ui/TimeConductorMode.js

@@ -28,13 +28,14 @@ define(
          * Supports mode-specific time conductor behavior.
          *
          * @constructor
+         * @memberof platform.features.conductor
          * @param {TimeConductorMetadata} metadata
          */
         function TimeConductorMode(metadata, conductor, timeSystems) {
             this.conductor = conductor;
 
             this.mdata = metadata;
-            this.dlts = undefined;
+            this.deltasVal = undefined;
             this.source = undefined;
             this.sourceUnlisten = undefined;
             this.systems = timeSystems;
@@ -141,6 +142,9 @@ define(
             return this.source;
         };
 
+        /**
+         * @private
+         */
         TimeConductorMode.prototype.destroy = function () {
             this.conductor.off('timeSystem', this.changeTimeSystem);
 
@@ -178,25 +182,24 @@ define(
         TimeConductorMode.prototype.deltas = function (deltas) {
             if (arguments.length !== 0) {
                 var bounds = this.calculateBoundsFromDeltas(deltas);
-                this.dlts = deltas;
+                this.deltasVal = deltas;
                 if (this.metadata().key !== 'fixed') {
                     this.conductor.bounds(bounds);
                 }
             }
-            return this.dlts;
+            return this.deltasVal;
         };
 
         /**
-         *
          * @param deltas
          * @returns {TimeConductorBounds}
          */
         TimeConductorMode.prototype.calculateBoundsFromDeltas = function (deltas) {
             var oldEnd = this.conductor.bounds().end;
 
-            if (this.dlts && this.dlts.end !== undefined) {
+            if (this.deltasVal && this.deltasVal.end !== undefined) {
                 //Calculate the previous raw end value (without delta)
-                oldEnd = oldEnd - this.dlts.end;
+                oldEnd = oldEnd - this.deltasVal.end;
             }
 
             var bounds = {
@@ -207,9 +210,15 @@ define(
         };
 
         /**
-         * Calculates bounds and deltas based on a timeSpan. Collectively
+         * @typedef {Object} ZoomLevel
+         * @property {TimeConductorBounds} bounds The calculated bounds based on the zoom level
+         * @property {TimeConductorDeltas} deltas The calculated deltas based on the zoom level
+         */
+        /**
+         * Calculates bounds and deltas based on provided timeSpan. Collectively
          * the bounds and deltas will constitute the new zoom level.
          * @param {number} timeSpan time duration in ms.
+         * @return {ZoomLevel} The new zoom bounds and delta calculated for the provided time span
          */
         TimeConductorMode.prototype.calculateZoom = function (timeSpan) {
             var zoom = {};
@@ -219,7 +228,7 @@ define(
             if (this.tickSource()) {
                 zoom.deltas = {
                     start: timeSpan,
-                    end: this.dlts.end
+                    end: this.deltasVal.end
                 };
                 zoom.bounds = this.calculateBoundsFromDeltas(zoom.deltas);
                 // Calculate bounds based on deltas;

platform/features/conductor-v2/conductor/src/ui/TimeConductorViewService.js

@@ -151,7 +151,7 @@ define(
         };
 
         /**
-         * @typedef {object} Delta
+         * @typedef {object} TimeConductorDeltas
          * @property {number} start Used to set the start bound of the
          * TimeConductor on tick. A positive value that will be subtracted
          * from the value provided by a tick source to determine the start
@@ -178,7 +178,7 @@ define(
          *     tick
          *     - end: A time in ms after the timestamp of the last data received
          *     which will be used to determine the 'end' bound on tick
-         * @returns {Delta} current value of the deltas
+         * @returns {TimeConductorDeltas} current value of the deltas
          */
         TimeConductorViewService.prototype.deltas = function () {
             //Deltas stored on mode. Use .apply to preserve arguments
@@ -204,6 +204,11 @@ define(
             return this.currentMode.availableTimeSystems();
         };
 
+        /**
+         * An event to indicate that zooming is taking place
+         * @event platform.features.conductor.TimeConductorViewService~zoom
+         * @property {ZoomLevel} zoom the new zoom level.
+         */
         /**
          * Zoom to given time span. Will fire a zoom event with new zoom
          * bounds. Zoom bounds emitted in this way are considered ephemeral

platform/features/conductor-v2/conductor/src/ui/TimeOfInterestController.js

@@ -21,13 +21,14 @@
  *****************************************************************************/
 
 define(
-    ["zepto"],
+    [],
-    function ($) {
+    function () {
 
         /**
-         * The mct-conductor-axis renders a horizontal axis with regular
+         * Controller for the Time of Interest element used in various views to display the TOI. Responsible for setting
-         * labelled 'ticks'. It requires 'start' and 'end' integer values to
+         * the text label for the current TOI, and for toggling the (un)pinned state which determines whether the TOI
-         * be specified as attributes.
+         * indicator is visible.
+         * @constructor
          */
         function TimeOfInterestController($scope, openmct, formatService) {
             this.conductor = openmct.conductor;
@@ -56,6 +57,12 @@ define(
             $scope.$on('$destroy', this.destroy);
         }
 
+        /**
+         * Called when the time of interest changes on the conductor. Will pin (display) the TOI indicator, and set the
+         * text using the default formatter of the currently active Time System.
+         * @private
+         * @param {integer} toi Current time of interest in ms
+         */
         TimeOfInterestController.prototype.changeTimeOfInterest = function (toi) {
             if (toi !== undefined) {
                 this.$scope.pinned = true;
@@ -73,11 +80,18 @@ define(
             this.format = this.formatService.getFormat(timeSystem.formats()[0]);
         };
 
+        /**
+         * @private
+         */
         TimeOfInterestController.prototype.destroy = function () {
             this.conductor.off('timeOfInterest', this.changeTimeOfInterest);
             this.conductor.off('timeSystem', this.changeTimeSystem);
         };
 
+        /**
+         * Will unpin (hide) the TOI indicator. Has the effect of setting the time of interest to `undefined` on the
+         * Time Conductor
+         */
         TimeOfInterestController.prototype.dismiss = function () {
             this.conductor.timeOfInterest(undefined);
         };