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

Author: akhenry

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
-         * labelled 'ticks'. It requires 'start' and 'end' integer values to
-         * be specified as attributes.
+         * Controller that renders a horizontal time scale spanning the current bounds defined in the time conductor.
+         * Used by the mct-conductor-axis directive
+         * @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
-         * @param scope
-         * @param element
+         * @private
          */
         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
-         * for showing times.
+         * When the time system changes, update the scale and formatter used for showing times.
          * @param timeSystem
          */
         ConductorAxisController.prototype.changeTimeSystem = function (timeSystem) {
@@ -164,28 +167,51 @@ 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;
 
             this.bounds = zoom.bounds;
             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
-         * labelled 'ticks'. It requires 'start' and 'end' integer values to
-         * be specified as attributes.
+         * Controller for the Time of Interest indicator in the conductor itself. Sets the horizontal position of the
+         * TOI indicator based on the current value of the TOI, and the width of the TOI conductor.
+         * @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", [
-            "on"
-        ]);
-        mockConductorViewService = jasmine.createSpyObj("conductorViewService", [
-            "on"
-        ]);
+        beforeEach(function () {
+            mockConductor = jasmine.createSpyObj("conductor", [
+                "bounds",
+                "timeOfInterest",
+                "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);
+        });
     });
 });