From cb242d8efba3d2dbe26638be9fdc52bfd2b5483a Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Mon, 3 Jul 2017 10:30:32 -0700 Subject: [PATCH 1/3] [TimeAPI] check for change before triggering Update the TimeSettingsURLHandler to check for changes to the search parameters and only trigger a bounds change when there is a change. Added integration tests between Time API and settings url handler. Prevents extraneous bounds events. Fixes https://github.com/nasa/openmct/issues/1636 --- src/adapter/runs/TimeSettingsURLHandler.js | 97 ++- .../runs/TimeSettingsURLHandlerSpec.js | 624 ++++++++++++++---- 2 files changed, 555 insertions(+), 166 deletions(-) diff --git a/src/adapter/runs/TimeSettingsURLHandler.js b/src/adapter/runs/TimeSettingsURLHandler.js index 0e15567854..f97165ff1e 100644 --- a/src/adapter/runs/TimeSettingsURLHandler.js +++ b/src/adapter/runs/TimeSettingsURLHandler.js @@ -20,7 +20,11 @@ * at runtime from the About dialog for additional information. *****************************************************************************/ -define([], function () { +define([ + 'lodash' +], function ( + _ +) { // Parameter names in query string var SEARCH = { MODE: 'tc.mode', @@ -73,43 +77,70 @@ define([], function () { this.$location.search(SEARCH.END_DELTA, deltas.end); }; + TimeSettingsURLHandler.prototype.parseQueryParams = function () { + var searchParams = _.pick(this.$location.search(), _.values(SEARCH)); + var parsedParams = { + clock: searchParams[SEARCH.MODE], + timeSystem: searchParams[SEARCH.TIME_SYSTEM] + }; + if (!isNaN(parseInt(searchParams[SEARCH.START_DELTA], 0xA)) && + !isNaN(parseInt(searchParams[SEARCH.END_DELTA], 0xA))) { + parsedParams.clockOffsets = { + start: -searchParams[SEARCH.START_DELTA], + end: +searchParams[SEARCH.END_DELTA] + }; + } + if (!isNaN(parseInt(searchParams[SEARCH.START_BOUND], 0xA)) && + !isNaN(parseInt(searchParams[SEARCH.END_BOUND], 0xA))) { + parsedParams.bounds = { + start: +searchParams[SEARCH.START_BOUND], + end: +searchParams[SEARCH.END_BOUND] + }; + } + return parsedParams; + }; + TimeSettingsURLHandler.prototype.updateTime = function () { - var searchParams = this.$location.search(); - var mode = searchParams[SEARCH.MODE]; - var timeSystem = searchParams[SEARCH.TIME_SYSTEM]; - var clockOffsets = { - start: -searchParams[SEARCH.START_DELTA], - end: +searchParams[SEARCH.END_DELTA] - }; - var bounds = { - start: +searchParams[SEARCH.START_BOUND], - end: +searchParams[SEARCH.END_BOUND] - }; - var fixed = (mode === 'fixed'); - var clock = fixed ? undefined : mode; - var hasDeltas = - !isNaN(parseInt(searchParams[SEARCH.START_DELTA], 0xA)) && - !isNaN(parseInt(searchParams[SEARCH.END_DELTA], 0xA)); - var hasBounds = - !isNaN(parseInt(searchParams[SEARCH.START_BOUND], 0xA)) && - !isNaN(parseInt(searchParams[SEARCH.END_BOUND], 0xA)); - - if (fixed && timeSystem && hasBounds) { - this.time.timeSystem(timeSystem, bounds); - this.time.stopClock(); + var params = this.parseQueryParams(); + if (_.isEqual(params, this.last)) { + return; // Do nothing; } + this.last = params; - if (!fixed && clock && hasDeltas) { - this.time.clock(clock, clockOffsets); - this.time.timeSystem(timeSystem); - } + if (params.bounds) { + if (!this.time.timeSystem() || + this.time.timeSystem().key !== params.timeSystem) { - if (hasDeltas && !fixed) { - this.time.clockOffsets(clockOffsets); - } + this.time.timeSystem( + params.timeSystem, + params.bounds + ); + } else if (!_.isEqual(this.time.bounds(), params.bounds)) { + this.time.bounds(params.bounds); + } + if (this.time.clock()) { + this.time.stopClock(); + } + } else if (params.clockOffsets) { + if (params.clock === 'fixed') { + this.time.stopClock(); + return; + } + if (!this.time.clock() || + this.time.clock().key !== params.clock) { - if (hasBounds && fixed) { - this.time.bounds(bounds); + this.time.clock(params.clock, params.clockOffsets); + } else if (!_.isEqual(this.time.clockOffsets(), params.clockOffsets)) { + this.time.clockOffsets(params.clockOffsets); + } + if (!this.time.timeSystem() || + this.time.timeSystem().key !== params.timeSystem) { + + this.time.timeSystem(params.timeSystem); + } + } else { + // Neither found, update from timeSystem. + this.updateQueryParams(); } }; diff --git a/src/adapter/runs/TimeSettingsURLHandlerSpec.js b/src/adapter/runs/TimeSettingsURLHandlerSpec.js index 1c403d1422..a46ed89881 100644 --- a/src/adapter/runs/TimeSettingsURLHandlerSpec.js +++ b/src/adapter/runs/TimeSettingsURLHandlerSpec.js @@ -20,23 +20,74 @@ * at runtime from the About dialog for additional information. *****************************************************************************/ -define(['./TimeSettingsURLHandler'], function (TimeSettingsURLHandler) { +define([ + './TimeSettingsURLHandler', + '../../api/time/TimeAPI' +], function ( + TimeSettingsURLHandler, + TimeAPI +) { describe("TimeSettingsURLHandler", function () { var time; var $location; var $rootScope; var search; var handler; + var clockA; + var clockB; + var timeSystemA; + var timeSystemB; + var boundsA; + var boundsB; + var offsetsA; + var offsetsB; + var initialize; + var triggerLocationChange; + beforeEach(function () { - time = jasmine.createSpyObj('time', [ + clockA = jasmine.createSpyObj('clockA', ['on', 'off']); + clockA.key = 'clockA'; + clockA.currentValue = function () { return 1000; } + clockB = jasmine.createSpyObj('clockB', ['on', 'off']); + clockB.key = 'clockB'; + clockB.currentValue = function () { return 2000; } + timeSystemA = {key: 'timeSystemA'}; + timeSystemB = {key: 'timeSystemB'}; + boundsA = { + start: 10, + end: 20 + }; + boundsB = { + start: 120, + end: 360 + }; + offsetsA = { + start: -100, + end: 0 + }; + offsetsB = { + start: -50, + end: 50 + }; + + time = new TimeAPI(); + + [ 'on', 'bounds', 'clockOffsets', 'timeSystem', 'clock', 'stopClock' - ]); + ].forEach(function (method) { + spyOn(time, method).andCallThrough(); + }); + time.addTimeSystem(timeSystemA); + time.addTimeSystem(timeSystemB); + time.addClock(clockA); + time.addClock(clockB); + $location = jasmine.createSpyObj('$location', [ 'search' ]); @@ -44,8 +95,6 @@ define(['./TimeSettingsURLHandler'], function (TimeSettingsURLHandler) { '$on' ]); - time.timeSystem.andReturn({ key: 'test-time-system' }); - search = {}; $location.search.andCallFake(function (key, value) { if (arguments.length === 0) { @@ -59,142 +108,451 @@ define(['./TimeSettingsURLHandler'], function (TimeSettingsURLHandler) { return this; }); - handler = new TimeSettingsURLHandler( - time, - $location, - $rootScope + expect(time.timeSystem()).toBeUndefined(); + expect(time.bounds()).toEqual({}); + expect(time.clockOffsets()).toBeUndefined(); + expect(time.clock()).toBeUndefined(); + + initialize = function () { + handler = new TimeSettingsURLHandler( + time, + $location, + $rootScope + ); + expect($rootScope.$on).toHaveBeenCalledWith( + '$locationChangeSuccess', + jasmine.any(Function) + ); + triggerLocationChange = $rootScope.$on.mostRecentCall.args[1]; + + }; + }); + + it("can initalize fixed mode from location", function () { + search['tc.mode'] = 'fixed'; + search['tc.timeSystem'] = 'timeSystemA'; + search['tc.startBound'] = '123'; + search['tc.endBound'] = '456'; + + initialize(); + + expect(time.timeSystem).toHaveBeenCalledWith( + 'timeSystemA', + { + start: 123, + end: 456 + } ); }); - ['bounds', 'timeSystem', 'clock', 'clockOffsets'].forEach(function (event) { - it("listens for " + event + " time events", function () { - expect(time.on) - .toHaveBeenCalledWith(event, jasmine.any(Function)); - }); + it("can initialize clock mode from location", function () { + search['tc.mode'] = 'clockA'; + search['tc.timeSystem'] = 'timeSystemA'; + search['tc.startDelta'] = '123'; + search['tc.endDelta'] = '456'; - describe("when " + event + " time event occurs with no clock", function () { - var expected; + initialize(); - beforeEach(function () { - expected = { - 'tc.mode': 'fixed', - 'tc.timeSystem': 'test-time-system', - 'tc.startBound': '123', - 'tc.endBound': '456' - }; - time.clock.andReturn(undefined); - time.bounds.andReturn({ start: 123, end: 456 }); - - time.on.calls.forEach(function (call) { - if (call.args[0] === event) { - call.args[1](); - } - }); - }); - - it("updates query parameters for fixed mode", function () { - expect(search).toEqual(expected); - }); - }); - - describe("when " + event + " time event occurs with no time system", function () { - beforeEach(function () { - time.timeSystem.andReturn(undefined); - time.on.calls.forEach(function (call) { - if (call.args[0] === event) { - call.args[1](); - } - }); - }); - - it("clears the time system from the URL", function () { - expect(search['tc.timeSystem']).toBeUndefined(); - }); - }); - - describe("when " + event + " time event occurs with a clock", function () { - var expected; - - beforeEach(function () { - expected = { - 'tc.mode': 'clocky', - 'tc.timeSystem': 'test-time-system', - 'tc.startDelta': '123', - 'tc.endDelta': '456' - }; - time.clock.andReturn({ key: 'clocky' }); - time.clockOffsets.andReturn({ start: -123, end: 456 }); - - time.on.calls.forEach(function (call) { - if (call.args[0] === event) { - call.args[1](); - } - }); - }); - - it("updates query parameters for realtime mode", function () { - expect(search).toEqual(expected); - }); - }); - }); - - it("listens for location changes", function () { - expect($rootScope.$on) - .toHaveBeenCalledWith('$locationChangeSuccess', jasmine.any(Function)); - }); - - [false, true].forEach(function (fixed) { - var name = fixed ? "fixed-time" : "real-time"; - var suffix = fixed ? 'Bound' : 'Delta'; - describe("when " + name + " location changes occur", function () { - beforeEach(function () { - search['tc.mode'] = fixed ? 'fixed' : 'clocky'; - search['tc.timeSystem'] = 'some-time-system'; - search['tc.start' + suffix] = '12321'; - search['tc.end' + suffix] = '32123'; - $rootScope.$on.mostRecentCall.args[1](); - }); - - if (fixed) { - var bounds = { start: 12321, end: 32123 }; - it("stops the clock", function () { - expect(time.stopClock).toHaveBeenCalled(); - }); - - it("sets the bounds", function () { - expect(time.bounds).toHaveBeenCalledWith(bounds); - }); - - it("sets the time system with bounds", function () { - expect(time.timeSystem).toHaveBeenCalledWith( - search['tc.timeSystem'], - bounds - ); - }); - } else { - var clockOffsets = { start: -12321, end: 32123 }; - - it("sets the clock", function () { - expect(time.stopClock).not.toHaveBeenCalled(); - expect(time.clock).toHaveBeenCalledWith( - search['tc.mode'], - clockOffsets - ); - }); - - it("sets clock offsets", function () { - expect(time.clockOffsets) - .toHaveBeenCalledWith(clockOffsets); - }); - - it("sets the time system without bounds", function () { - expect(time.timeSystem).toHaveBeenCalledWith( - search['tc.timeSystem'] - ); - }); + expect(time.clock).toHaveBeenCalledWith( + 'clockA', + { + start: -123, + end: 456 } + ); + expect(time.timeSystem).toHaveBeenCalledWith( + 'timeSystemA' + ); + }); + + it("can initialize fixed mode from time API", function () { + time.timeSystem(timeSystemA.key, boundsA); + initialize(); + expect($location.search) + .toHaveBeenCalledWith('tc.mode', 'fixed'); + expect($location.search) + .toHaveBeenCalledWith('tc.timeSystem', 'timeSystemA'); + expect($location.search) + .toHaveBeenCalledWith('tc.startBound', 10); + expect($location.search) + .toHaveBeenCalledWith('tc.endBound', 20); + expect($location.search) + .toHaveBeenCalledWith('tc.startDelta', null); + expect($location.search) + .toHaveBeenCalledWith('tc.endDelta', null); + }); + + it("can initialize clock mode from time API", function () { + time.clock(clockA.key, offsetsA); + time.timeSystem(timeSystemA.key); + initialize(); + expect($location.search) + .toHaveBeenCalledWith('tc.mode', 'clockA'); + expect($location.search) + .toHaveBeenCalledWith('tc.timeSystem', 'timeSystemA'); + expect($location.search) + .toHaveBeenCalledWith('tc.startBound', null); + expect($location.search) + .toHaveBeenCalledWith('tc.endBound', null); + expect($location.search) + .toHaveBeenCalledWith('tc.startDelta', 100); + expect($location.search) + .toHaveBeenCalledWith('tc.endDelta', 0); + }); + + describe('location changes in fixed mode', function () { + + beforeEach(function () { + time.timeSystem(timeSystemA.key, boundsA); + initialize(); + time.timeSystem.reset(); + time.bounds.reset(); + time.clock.reset(); + time.stopClock.reset(); + }); + + it("does not change on spurious location change", function () { + triggerLocationChange(); + expect(time.timeSystem).not.toHaveBeenCalledWith( + 'timeSystemA', + jasmine.any(Object) + ); + expect(time.bounds).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + expect(time.stopClock).not.toHaveBeenCalled(); + }); + + it("updates timeSystem changes", function () { + search['tc.timeSystem'] = 'timeSystemB'; + triggerLocationChange(); + expect(time.timeSystem).toHaveBeenCalledWith( + 'timeSystemB', + { + start: 10, + end: 20 + } + ); + }); + + it("updates bounds changes", function () { + search['tc.startBound'] = '100'; + search['tc.endBound'] = '200'; + triggerLocationChange(); + expect(time.timeSystem).not.toHaveBeenCalledWith( + jasmine.any(), jasmine.any() + ); + expect(time.bounds).toHaveBeenCalledWith({ + start: 100, + end: 200 + }); + search['tc.endBound'] = '300'; + triggerLocationChange(); + expect(time.timeSystem).not.toHaveBeenCalledWith( + jasmine.any(), jasmine.any() + ); + expect(time.bounds).toHaveBeenCalledWith({ + start: 100, + end: 300 + }); + }); + + it("updates clock mode w/o timeSystem change", function () { + search['tc.mode'] = 'clockA'; + search['tc.startDelta'] = '50'; + search['tc.endDelta'] = '50'; + delete search['tc.endBound']; + delete search['tc.startBound']; + triggerLocationChange(); + expect(time.clock).toHaveBeenCalledWith( + 'clockA', + { + start: -50, + end: 50 + } + ); + expect(time.timeSystem).not.toHaveBeenCalledWith( + jasmine.any(), jasmine.any() + ); + }); + + it("updates clock mode and timeSystem", function () { + search['tc.mode'] = 'clockA'; + search['tc.startDelta'] = '50'; + search['tc.endDelta'] = '50'; + search['tc.timeSystem'] = 'timeSystemB'; + delete search['tc.endBound']; + delete search['tc.startBound']; + triggerLocationChange(); + expect(time.clock).toHaveBeenCalledWith( + 'clockA', + { + start: -50, + end: 50 + } + ); + expect(time.timeSystem).toHaveBeenCalledWith('timeSystemB'); }); }); + describe('location changes in clock mode', function () { + + beforeEach(function () { + time.clock(clockA.key, offsetsA); + time.timeSystem(timeSystemA.key); + initialize(); + time.timeSystem.reset(); + time.bounds.reset(); + time.clock.reset(); + time.clockOffsets.reset(); + time.stopClock.reset(); + }); + + it("does not change on spurious location change", function () { + triggerLocationChange(); + expect(time.timeSystem).not.toHaveBeenCalledWith( + 'timeSystemA', + jasmine.any(Object) + ); + expect(time.clockOffsets).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + expect(time.clock).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + expect(time.bounds).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + }); + + it("changes time system", function () { + search['tc.timeSystem'] = 'timeSystemB'; + triggerLocationChange(); + expect(time.timeSystem).toHaveBeenCalledWith( + 'timeSystemB' + ); + expect(time.clockOffsets).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + expect(time.clock).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + expect(time.stopClock).not.toHaveBeenCalled(); + expect(time.bounds).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + }); + + it("changes offsets", function () { + search['tc.startDelta'] = '50'; + search['tc.endDelta'] = '50'; + triggerLocationChange(); + expect(time.timeSystem).not.toHaveBeenCalledWith( + 'timeSystemA', + jasmine.any(Object) + ); + expect(time.clockOffsets).toHaveBeenCalledWith( + { + start: -50, + end: 50 + } + ); + expect(time.clock).not.toHaveBeenCalledWith( + jasmine.any(Object) + ); + }); + + it("updates to fixed w/o timeSystem change", function () { + search['tc.mode'] = 'fixed'; + search['tc.startBound'] = '234'; + search['tc.endBound'] = '567'; + delete search['tc.endDelta']; + delete search['tc.startDelta']; + + triggerLocationChange(); + expect(time.stopClock).toHaveBeenCalled(); + expect(time.bounds).toHaveBeenCalledWith({ + start: 234, + end: 567 + }); + expect(time.timeSystem).not.toHaveBeenCalledWith( + jasmine.any(), jasmine.any() + ); + }); + + it("updates fixed and timeSystem", function () { + search['tc.mode'] = 'fixed'; + search['tc.startBound'] = '234'; + search['tc.endBound'] = '567'; + search['tc.timeSystem'] = 'timeSystemB'; + delete search['tc.endDelta']; + delete search['tc.startDelta']; + + triggerLocationChange(); + expect(time.stopClock).toHaveBeenCalled(); + expect(time.timeSystem).toHaveBeenCalledWith( + 'timeSystemB', + { + start: 234, + end: 567 + } + ); + }); + + it("updates clock", function () { + search['tc.mode'] = 'clockB'; + triggerLocationChange(); + expect(time.clock).toHaveBeenCalledWith( + 'clockB', + { + start: -100, + end: 0 + } + ); + expect(time.timeSystem).not.toHaveBeenCalledWith(jasmine.any()); + }); + + it("updates clock and timeSystem", function () { + search['tc.mode'] = 'clockB'; + search['tc.timeSystem'] = 'timeSystemB'; + triggerLocationChange(); + expect(time.clock).toHaveBeenCalledWith( + 'clockB', + { + start: -100, + end: 0 + } + ); + expect(time.timeSystem).toHaveBeenCalledWith( + 'timeSystemB' + ); + }); + + it("updates clock and timeSystem and offsets", function () { + search['tc.mode'] = 'clockB'; + search['tc.timeSystem'] = 'timeSystemB'; + search['tc.startDelta'] = '50'; + search['tc.endDelta'] = '50'; + triggerLocationChange(); + expect(time.clock).toHaveBeenCalledWith( + 'clockB', + { + start: -50, + end: 50 + } + ); + expect(time.timeSystem).toHaveBeenCalledWith( + 'timeSystemB' + ); + }); + + it("stops the clock", function () { + // this is a robustness test, unsure if desired, requires + // user to be manually editing location strings. + search['tc.mode'] = 'fixed'; + triggerLocationChange(); + expect(time.stopClock).toHaveBeenCalled(); + }); + }); + + + describe("location updates from time API in fixed", function () { + beforeEach(function () { + time.timeSystem(timeSystemA.key, boundsA); + initialize(); + }); + + it("updates on bounds change", function () { + time.bounds(boundsB); + expect(search).toEqual({ + 'tc.mode': 'fixed', + 'tc.startBound': '120', + 'tc.endBound': '360', + 'tc.timeSystem': 'timeSystemA' + }); + }); + + it("updates on timeSystem change", function () { + time.timeSystem(timeSystemB, boundsA); + expect(search).toEqual({ + 'tc.mode': 'fixed', + 'tc.startBound': '10', + 'tc.endBound': '20', + 'tc.timeSystem': 'timeSystemB' + }); + time.timeSystem(timeSystemA, boundsB); + expect(search).toEqual({ + 'tc.mode': 'fixed', + 'tc.startBound': '120', + 'tc.endBound': '360', + 'tc.timeSystem': 'timeSystemA' + }); + }); + + it("Updates to clock", function () { + time.clock(clockA, offsetsA); + expect(search).toEqual({ + 'tc.mode': 'clockA', + 'tc.startDelta': '100', + 'tc.endDelta': '0', + 'tc.timeSystem': 'timeSystemA' + }); + }); + }); + + describe("location updates from time API in fixed", function () { + beforeEach(function () { + time.clock(clockA.key, offsetsA); + time.timeSystem(timeSystemA.key); + initialize(); + }); + + it("updates offsets", function () { + time.clockOffsets(offsetsB); + expect(search).toEqual({ + 'tc.mode': 'clockA', + 'tc.startDelta': '50', + 'tc.endDelta': '50', + 'tc.timeSystem': 'timeSystemA' + }); + }); + + it("updates clocks", function () { + time.clock(clockB, offsetsA); + expect(search).toEqual({ + 'tc.mode': 'clockB', + 'tc.startDelta': '100', + 'tc.endDelta': '0', + 'tc.timeSystem': 'timeSystemA' + }); + time.clock(clockA, offsetsB); + expect(search).toEqual({ + 'tc.mode': 'clockA', + 'tc.startDelta': '50', + 'tc.endDelta': '50', + 'tc.timeSystem': 'timeSystemA' + }); + }); + + it("updates timesystems", function () { + time.timeSystem(timeSystemB); + expect(search).toEqual({ + 'tc.mode': 'clockA', + 'tc.startDelta': '100', + 'tc.endDelta': '0', + 'tc.timeSystem': 'timeSystemB' + }); + }); + + it("stops the clock", function () { + time.stopClock(); + expect(search).toEqual({ + 'tc.mode': 'fixed', + 'tc.startBound': '900', + 'tc.endBound': '1000', + 'tc.timeSystem': 'timeSystemA' + }); + }); + }); }); }); From 4e7e5bb7839b6edd769e4f694b8cc109a86d8e1a Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Tue, 4 Jul 2017 15:34:37 -0700 Subject: [PATCH 2/3] [Time] Conductor changes based on click not scope Update time conductor so that it triggers changes when the user selects them instead of when the scope is updated. Prevents spurious changes from being triggered by the conductor when it updates in response to a time API change. Fixes #1636. --- .../templates/mode-selector/mode-menu.html | 2 +- .../core/src/ui/TimeConductorController.js | 43 ++++++++++--------- .../runs/TimeSettingsURLHandlerSpec.js | 8 +++- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/platform/features/conductor/core/res/templates/mode-selector/mode-menu.html b/platform/features/conductor/core/res/templates/mode-selector/mode-menu.html index 32c026de9d..b0689ea4c9 100644 --- a/platform/features/conductor/core/res/templates/mode-selector/mode-menu.html +++ b/platform/features/conductor/core/res/templates/mode-selector/mode-menu.html @@ -23,7 +23,7 @@