From 07179f7290ddca598906a5e86b93bea5c2285824 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 9 Nov 2015 15:42:19 -0800 Subject: [PATCH 01/13] [Action] Catch errors Catch errors during action instantiation, to avoid suppressing whole context menus on single failures; nasa/openmctweb#120. --- platform/core/bundle.json | 2 +- platform/core/src/actions/ActionProvider.js | 36 +++++++++++++++++---- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/platform/core/bundle.json b/platform/core/bundle.json index 330ac1c2b9..80b582d640 100644 --- a/platform/core/bundle.json +++ b/platform/core/bundle.json @@ -92,7 +92,7 @@ "provides": "actionService", "type": "provider", "implementation": "actions/ActionProvider.js", - "depends": [ "actions[]" ] + "depends": [ "actions[]", "$log" ] }, { "provides": "actionService", diff --git a/platform/core/src/actions/ActionProvider.js b/platform/core/src/actions/ActionProvider.js index dcb17eb6ce..b16db2e52a 100644 --- a/platform/core/src/actions/ActionProvider.js +++ b/platform/core/src/actions/ActionProvider.js @@ -39,9 +39,11 @@ define( * @imeplements {ActionService} * @constructor */ - function ActionProvider(actions) { + function ActionProvider(actions, $log) { var self = this; + this.$log = $log; + // Build up look-up tables this.actions = actions; this.actionsByKey = {}; @@ -74,6 +76,7 @@ define( var context = (actionContext || {}), category = context.category, key = context.key, + $log = this.$log, candidates; // Instantiate an action; invokes the constructor and @@ -103,12 +106,31 @@ define( // appliesTo method of given actions (if defined), and // instantiate those applicable actions. function createIfApplicable(actions, context) { - return (actions || []).filter(function (Action) { - return Action.appliesTo ? - Action.appliesTo(context) : true; - }).map(function (Action) { - return instantiateAction(Action, context); - }); + function isApplicable(Action) { + return Action.appliesTo ? Action.appliesTo(context) : true; + } + + function instantiate(Action) { + try { + return instantiateAction(Action, context); + } catch (e) { + $log.error([ + "Could not instantiate action", + Action.key, + "due to:", + e.message + ].join(" ")); + return undefined; + } + } + + function isDefined(action) { + return action !== undefined; + } + + return (actions || []).filter(isApplicable) + .map(instantiate) + .filter(isDefined); } // Match actions to the provided context by comparing "key" From 066fd5559091e20a2d823b2ad0511a256d31bd6c Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 9 Nov 2015 16:18:35 -0800 Subject: [PATCH 02/13] [Actions] Test error handling --- .../core/test/actions/ActionProviderSpec.js | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/platform/core/test/actions/ActionProviderSpec.js b/platform/core/test/actions/ActionProviderSpec.js index f059052139..14c719f0c2 100644 --- a/platform/core/test/actions/ActionProviderSpec.js +++ b/platform/core/test/actions/ActionProviderSpec.js @@ -30,7 +30,8 @@ define( "use strict"; describe("The action provider", function () { - var actions, + var mockLog, + actions, actionProvider; function SimpleAction() { @@ -62,6 +63,10 @@ define( MetadataAction.key = "metadata"; beforeEach(function () { + mockLog = jasmine.createSpyObj( + '$log', + ['error', 'warn', 'info', 'debug'] + ); actions = [ SimpleAction, CategorizedAction, @@ -137,6 +142,42 @@ define( expect(provided[0].getMetadata()).toEqual("custom metadata"); }); + describe("when actions throw errors during instantiation", function () { + var errorText, + provided; + + beforeEach(function () { + errorText = "some error text"; + + function BadAction() { + throw new Error(errorText); + } + + provided = new ActionProvider( + [ SimpleAction, BadAction ], + mockLog + ).getActions(); + }); + + it("logs an error", function () { + expect(mockLog.error) + .toHaveBeenCalledWith(jasmine.any(String)); + }); + + it("reports the error's message", function () { + expect( + mockLog.error.mostRecentCall.args[0].indexOf(errorText) + ).not.toEqual(-1); + }); + + it("still provides valid actions", function () { + expect(provided.length).toEqual(1); + expect(provided[0].perform()).toEqual("simple"); + }); + + }); + + }); } -); \ No newline at end of file +); From 2866574dc05e12bcf00c10e79e2ceb2340520ada Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 9 Nov 2015 16:28:04 -0800 Subject: [PATCH 03/13] [Actions] Define applicability Define applicability of Move/Copy/Link using appliesTo, to avoid errors being thrown due to lack of context during instantiation. Addresses immediate cause of nasa/openmctweb#120. --- .../src/actions/AbstractComposeAction.js | 8 +++++++ .../entanglement/src/actions/CopyAction.js | 11 ++++++---- .../entanglement/src/actions/LinkAction.js | 11 +++++----- .../entanglement/src/actions/MoveAction.js | 12 +++++----- .../test/actions/AbstractComposeActionSpec.js | 22 +++++++++++++++++++ 5 files changed, 50 insertions(+), 14 deletions(-) diff --git a/platform/entanglement/src/actions/AbstractComposeAction.js b/platform/entanglement/src/actions/AbstractComposeAction.js index f68391adc9..ef56c952b3 100644 --- a/platform/entanglement/src/actions/AbstractComposeAction.js +++ b/platform/entanglement/src/actions/AbstractComposeAction.js @@ -122,6 +122,14 @@ define( }); }; + AbstractComposeAction.appliesTo = function (context) { + var applicableObject = + context.selectedObject || context.domainObject; + + return !!(applicableObject && + applicableObject.hasCapability('context')); + }; + return AbstractComposeAction; } ); diff --git a/platform/entanglement/src/actions/CopyAction.js b/platform/entanglement/src/actions/CopyAction.js index cdcefeb935..161db1ce27 100644 --- a/platform/entanglement/src/actions/CopyAction.js +++ b/platform/entanglement/src/actions/CopyAction.js @@ -34,7 +34,7 @@ define( * @constructor * @memberof platform/entanglement */ - function CopyAction($log, locationService, copyService, dialogService, + function CopyAction($log, locationService, copyService, dialogService, notificationService, context) { this.dialog = undefined; this.notification = undefined; @@ -42,7 +42,7 @@ define( this.notificationService = notificationService; this.$log = $log; //Extend the behaviour of the Abstract Compose Action - AbstractComposeAction.call(this, locationService, copyService, + AbstractComposeAction.call(this, locationService, copyService, context, "Duplicate", "to a location"); } @@ -87,8 +87,8 @@ define( }; /** - * Executes the CopyAction. The CopyAction uses the default behaviour of - * the AbstractComposeAction, but extends it to support notification + * Executes the CopyAction. The CopyAction uses the default behaviour of + * the AbstractComposeAction, but extends it to support notification * updates of progress on copy. */ CopyAction.prototype.perform = function() { @@ -131,6 +131,9 @@ define( return AbstractComposeAction.prototype.perform.call(this) .then(success, error, notification); }; + + CopyAction.appliesTo = AbstractComposeAction.appliesTo; + return CopyAction; } ); diff --git a/platform/entanglement/src/actions/LinkAction.js b/platform/entanglement/src/actions/LinkAction.js index c791310886..f34e91f156 100644 --- a/platform/entanglement/src/actions/LinkAction.js +++ b/platform/entanglement/src/actions/LinkAction.js @@ -35,14 +35,15 @@ define( * @memberof platform/entanglement */ function LinkAction(locationService, linkService, context) { - return new AbstractComposeAction( - locationService, - linkService, - context, - "Link" + AbstractComposeAction.apply( + this, + [locationService, linkService, context, "Link"] ); } + LinkAction.prototype = Object.create(AbstractComposeAction.prototype); + LinkAction.appliesTo = AbstractComposeAction.appliesTo; + return LinkAction; } ); diff --git a/platform/entanglement/src/actions/MoveAction.js b/platform/entanglement/src/actions/MoveAction.js index 4fdd4b59df..1d090ce313 100644 --- a/platform/entanglement/src/actions/MoveAction.js +++ b/platform/entanglement/src/actions/MoveAction.js @@ -35,14 +35,16 @@ define( * @memberof platform/entanglement */ function MoveAction(locationService, moveService, context) { - return new AbstractComposeAction( - locationService, - moveService, - context, - "Move" + AbstractComposeAction.apply( + this, + [locationService, moveService, context, "Move"] ); + } + MoveAction.prototype = Object.create(AbstractComposeAction.prototype); + MoveAction.appliesTo = AbstractComposeAction.appliesTo; + return MoveAction; } ); diff --git a/platform/entanglement/test/actions/AbstractComposeActionSpec.js b/platform/entanglement/test/actions/AbstractComposeActionSpec.js index 5be0604ec3..6a0ddd7ebd 100644 --- a/platform/entanglement/test/actions/AbstractComposeActionSpec.js +++ b/platform/entanglement/test/actions/AbstractComposeActionSpec.js @@ -94,6 +94,28 @@ define( composeService = new MockCopyService(); }); + it("are only applicable to domain objects with a context", function () { + var noContextObject = domainObjectFactory({ + name: 'selectedObject', + model: { name: 'selectedObject' }, + capabilities: {} + }); + + expect(AbstractComposeAction.appliesTo({ + selectedObject: selectedObject + })).toBe(true); + expect(AbstractComposeAction.appliesTo({ + domainObject: selectedObject + })).toBe(true); + + expect(AbstractComposeAction.appliesTo({ + selectedObject: noContextObject + })).toBe(false); + expect(AbstractComposeAction.appliesTo({ + domainObject: noContextObject + })).toBe(false); + }); + describe("with context from context-action", function () { beforeEach(function () { From ae7a1618e819d7aea63bd43428f1feb4d02e4f27 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 10 Nov 2015 15:16:47 -0800 Subject: [PATCH 04/13] [Plot] Listen for domain/range changes nasa/openmctweb#218 --- platform/features/plot/src/PlotController.js | 50 ++++++++++++++------ 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/platform/features/plot/src/PlotController.js b/platform/features/plot/src/PlotController.js index cf6bfcf581..3cc5392049 100644 --- a/platform/features/plot/src/PlotController.js +++ b/platform/features/plot/src/PlotController.js @@ -78,6 +78,8 @@ define( cachedObjects = [], updater, lastBounds, + lastRange, + lastDomain, handle; // Populate the scope with axis information (specifically, options @@ -120,16 +122,16 @@ define( // Reinstantiate the plot updater (e.g. because we have a // new subscription.) This will clear the plot. function recreateUpdater() { - updater = new PlotUpdater( - handle, - ($scope.axes[0].active || {}).key, - ($scope.axes[1].active || {}).key, - PLOT_FIXED_DURATION - ); - self.limitTracker = new PlotLimitTracker( - handle, - ($scope.axes[1].active || {}).key - ); + var domain = ($scope.axes[0].active || {}).key, + range = ($scope.axes[1].active || {}).key, + duration = PLOT_FIXED_DURATION; + + updater = new PlotUpdater(handle, domain, range, duration); + lastDomain = domain; + lastRange = range; + + self.limitTracker = new PlotLimitTracker(handle, range); + // Keep any externally-provided bounds if (lastBounds) { setBasePanZoom(lastBounds); @@ -201,6 +203,24 @@ define( } } + function requery() { + self.pending = true; + releaseSubscription(); + subscribe($scope.domainObject); + } + + function domainRequery(newDomain) { + if (newDomain !== lastDomain) { + requery(); + } + } + + function rangeRequery(newRange) { + if (newRange !== lastRange) { + requery(); + } + } + // Respond to a display bounds change (requery for data) function changeDisplayBounds(event, bounds) { var domainAxis = $scope.axes[0]; @@ -208,13 +228,11 @@ define( domainAxis.chooseOption(bounds.domain); plotTelemetryFormatter .setDomainFormat(domainAxis.active.format); - - self.pending = true; - releaseSubscription(); - subscribe($scope.domainObject); setBasePanZoom(bounds); + requery(); } + function updateDomainFormat(format) { plotTelemetryFormatter.setDomainFormat(format); } @@ -237,6 +255,10 @@ define( new PlotAxis("ranges", [], AXIS_DEFAULTS[1]) ]; + // Watch for changes to the selected axis + $scope.$watch("axes[0].active.key", domainRequery); + $scope.$watch("axes[1].active.key", rangeRequery); + // Subscribe to telemetry when a domain object becomes available $scope.$watch('domainObject', subscribe); From a88fadcb491bb1a942c34501ca8dfbebb73903d4 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 10 Nov 2015 16:02:18 -0800 Subject: [PATCH 05/13] [Plot] Allow lookup of alternate ranges --- .../telemetry/src/TelemetrySubscription.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/platform/telemetry/src/TelemetrySubscription.js b/platform/telemetry/src/TelemetrySubscription.js index 5dcab54b94..71e1273dec 100644 --- a/platform/telemetry/src/TelemetrySubscription.js +++ b/platform/telemetry/src/TelemetrySubscription.js @@ -287,9 +287,12 @@ define( * @param {DomainObject} domainObject the object of interest * @returns the most recent domain value observed */ - TelemetrySubscription.prototype.getDomainValue = function (domainObject) { - var id = domainObject.getId(); - return (this.latestValues[id] || {}).domain; + TelemetrySubscription.prototype.getDomainValue = function (domainObject, key) { + var id = domainObject.getId(), + latestValue = this.latestValues[id]; + return latestValue && (key ? + latestValue.datum[key] : + latestValue.domain); }; /** @@ -304,9 +307,12 @@ define( * @param {DomainObject} domainObject the object of interest * @returns the most recent range value observed */ - TelemetrySubscription.prototype.getRangeValue = function (domainObject) { - var id = domainObject.getId(); - return (this.latestValues[id] || {}).range; + TelemetrySubscription.prototype.getRangeValue = function (domainObject, key) { + var id = domainObject.getId(), + latestValue = this.latestValues[id]; + return latestValue && (key ? + latestValue.datum[key] : + latestValue.range); }; /** From d60bf94501d6e5be971f9334c89437156a2092e9 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 10 Nov 2015 16:03:42 -0800 Subject: [PATCH 06/13] [Plot] Remove unnecessary fallback --- platform/features/plot/src/PlotController.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/platform/features/plot/src/PlotController.js b/platform/features/plot/src/PlotController.js index 3cc5392049..84350fe646 100644 --- a/platform/features/plot/src/PlotController.js +++ b/platform/features/plot/src/PlotController.js @@ -122,8 +122,8 @@ define( // Reinstantiate the plot updater (e.g. because we have a // new subscription.) This will clear the plot. function recreateUpdater() { - var domain = ($scope.axes[0].active || {}).key, - range = ($scope.axes[1].active || {}).key, + var domain = $scope.axes[0].active.key, + range = $scope.axes[1].active.key, duration = PLOT_FIXED_DURATION; updater = new PlotUpdater(handle, domain, range, duration); From b30e72081cc61c075a0ae0b7a1cddd801de6f053 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 10 Nov 2015 16:12:16 -0800 Subject: [PATCH 07/13] [Plot] Update domain format ...on change in user's domain selection. --- platform/features/plot/src/PlotController.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/platform/features/plot/src/PlotController.js b/platform/features/plot/src/PlotController.js index 84350fe646..4789f4c682 100644 --- a/platform/features/plot/src/PlotController.js +++ b/platform/features/plot/src/PlotController.js @@ -209,8 +209,15 @@ define( subscribe($scope.domainObject); } + function updateDomainFormat() { + var domainAxis = $scope.axes[0]; + plotTelemetryFormatter + .setDomainFormat(domainAxis.active.format); + } + function domainRequery(newDomain) { if (newDomain !== lastDomain) { + updateDomainFormat(); requery(); } } @@ -226,17 +233,11 @@ define( var domainAxis = $scope.axes[0]; domainAxis.chooseOption(bounds.domain); - plotTelemetryFormatter - .setDomainFormat(domainAxis.active.format); + updateDomainFormat(); setBasePanZoom(bounds); requery(); } - - function updateDomainFormat(format) { - plotTelemetryFormatter.setDomainFormat(format); - } - this.modeOptions = new PlotModeOptions([], subPlotFactory); this.updateValues = updateValues; From 7dc6f553ac22ab7fed73d844b4947b8fa5ff657e Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 10 Nov 2015 16:12:34 -0800 Subject: [PATCH 08/13] [Plot] Expose alternate domain values correctly ...from example telemetry (sine wave generator), to support testing of switching among domains within plot. --- example/generator/src/SinewaveTelemetrySeries.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/example/generator/src/SinewaveTelemetrySeries.js b/example/generator/src/SinewaveTelemetrySeries.js index fa47f8f59a..8643b0429b 100644 --- a/example/generator/src/SinewaveTelemetrySeries.js +++ b/example/generator/src/SinewaveTelemetrySeries.js @@ -62,7 +62,7 @@ define( // so it's not checked for here, just formatted for display // differently. return (i + offset) * 1000 + firstTime * 1000 - - (domain === 'yesterday' ? ONE_DAY : 0); + (domain === 'yesterday' ? (ONE_DAY * 1000) : 0); }; generatorData.getRangeValue = function (i, range) { From 93d969ad671636ff9b81d2ad7d71fb556dc4044f Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Tue, 10 Nov 2015 16:17:43 -0800 Subject: [PATCH 09/13] [Plot] Add tests ...to verify that data is requeried when user changes domain or range selection. --- .../features/plot/test/PlotControllerSpec.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/platform/features/plot/test/PlotControllerSpec.js b/platform/features/plot/test/PlotControllerSpec.js index addbdf5032..8edab2fc6d 100644 --- a/platform/features/plot/test/PlotControllerSpec.js +++ b/platform/features/plot/test/PlotControllerSpec.js @@ -53,6 +53,14 @@ define( }); } + function fireWatch(expr, value) { + mockScope.$watch.calls.forEach(function (call) { + if (call.args[0] === expr) { + call.args[1].apply(null, [value]); + } + }); + } + beforeEach(function () { mockScope = jasmine.createSpyObj( @@ -263,6 +271,20 @@ define( ]); expect(mockHandle.request.calls.length).toEqual(2); }); + + it("requeries when user changes domain selection", function () { + mockScope.$watch.mostRecentCall.args[1](mockDomainObject); + expect(mockHandle.request.calls.length).toEqual(1); + fireWatch("axes[0].active.key", 'someNewKey'); + expect(mockHandle.request.calls.length).toEqual(2); + }); + + it("requeries when user changes range selection", function () { + mockScope.$watch.mostRecentCall.args[1](mockDomainObject); + expect(mockHandle.request.calls.length).toEqual(1); + fireWatch("axes[1].active.key", 'someNewKey'); + expect(mockHandle.request.calls.length).toEqual(2); + }); }); } ); From 3a363898156bdd506d4c6e40de0e24c4ad6edf28 Mon Sep 17 00:00:00 2001 From: Henry Date: Tue, 17 Nov 2015 14:45:10 -0800 Subject: [PATCH 10/13] Fixed test --- platform/core/test/types/TypeImplSpec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/platform/core/test/types/TypeImplSpec.js b/platform/core/test/types/TypeImplSpec.js index f0a89875b3..ac5ee2b0d7 100644 --- a/platform/core/test/types/TypeImplSpec.js +++ b/platform/core/test/types/TypeImplSpec.js @@ -102,6 +102,8 @@ define( }); it("provides a fresh initial model each time", function () { + var model = type.getInitialModel(); + model.someKey = "some other value" expect(type.getInitialModel().someKey).toEqual("some value"); }); From 40b21e35fd9e861bcf561404cf715cb20a6f9a92 Mon Sep 17 00:00:00 2001 From: Henry Date: Tue, 17 Nov 2015 15:03:17 -0800 Subject: [PATCH 11/13] Fixed jslint error --- platform/core/test/types/TypeImplSpec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/platform/core/test/types/TypeImplSpec.js b/platform/core/test/types/TypeImplSpec.js index ac5ee2b0d7..0203058934 100644 --- a/platform/core/test/types/TypeImplSpec.js +++ b/platform/core/test/types/TypeImplSpec.js @@ -103,7 +103,7 @@ define( it("provides a fresh initial model each time", function () { var model = type.getInitialModel(); - model.someKey = "some other value" + model.someKey = "some other value"; expect(type.getInitialModel().someKey).toEqual("some value"); }); From a9518e9890cb53353da550b3cb9b32075a908833 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Wed, 18 Nov 2015 10:50:32 -0800 Subject: [PATCH 12/13] [Telemetry] Add missing JSDoc ...for new parameters added to support domain/range switching in plots, per code review on nasa/openmctweb#288 --- platform/telemetry/src/TelemetrySubscription.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/platform/telemetry/src/TelemetrySubscription.js b/platform/telemetry/src/TelemetrySubscription.js index 71e1273dec..3de00e4c37 100644 --- a/platform/telemetry/src/TelemetrySubscription.js +++ b/platform/telemetry/src/TelemetrySubscription.js @@ -285,6 +285,9 @@ define( * domain objects returned by `getTelemetryObjects()`. * * @param {DomainObject} domainObject the object of interest + * @param {string} [key] the symbolic identifier of the domain + * to look up; if omitted, the value for this object's + * default domain will be used * @returns the most recent domain value observed */ TelemetrySubscription.prototype.getDomainValue = function (domainObject, key) { @@ -305,6 +308,9 @@ define( * domain objects returned by `getTelemetryObjects()`. * * @param {DomainObject} domainObject the object of interest + * @param {string} [key] the symbolic identifier of the range + * to look up; if omitted, the value for this object's + * default range will be used * @returns the most recent range value observed */ TelemetrySubscription.prototype.getRangeValue = function (domainObject, key) { From 5ced8e655d46e1238de838f4e1855b57639bfd10 Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 18 Nov 2015 12:14:00 -0800 Subject: [PATCH 13/13] [bug] #317 Added jsdoc note about performance --- platform/core/src/types/TypeImpl.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/platform/core/src/types/TypeImpl.js b/platform/core/src/types/TypeImpl.js index 1a9854ea74..333f37663f 100644 --- a/platform/core/src/types/TypeImpl.js +++ b/platform/core/src/types/TypeImpl.js @@ -156,6 +156,13 @@ define( }); }; + /** + * Returns the default model for an object of this type. Note that + * this method returns a clone of the original model, so if using this + * method heavily, consider caching the result to optimize performance. + * + * @return {object} The default model for an object of this type. + */ TypeImpl.prototype.getInitialModel = function () { return JSON.parse(JSON.stringify(this.typeDef.model || {})); };