From 645bd5743fa382fd1a853cdd583541aeda7ecb83 Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 8 Feb 2016 23:05:48 -0800 Subject: [PATCH] [Plots] #638 Addressing feedback from code review [Plots] #638 Fixing failing tests [Plot] Changed PlotOptionsController to prototype form Fixed spacing Fixed jslint issue --- example/plotOptions/bundle.js | 22 +-- platform/commonUI/browse/bundle.js | 7 - .../templates/browse/inspector-region.html | 8 +- .../templates/browse/object-properties.html | 2 +- .../commonUI/browse/src/InspectorRegion.js | 10 +- .../browse/src/TypeRegionDecorator.js | 90 ----------- .../browse/test/InspectorRegionSpec.js | 2 +- .../browse/test/TypeRegionDecoratorSpec.js | 70 --------- platform/commonUI/regions/bundle.js | 8 +- ...onController.js => InspectorController.js} | 23 ++- platform/commonUI/regions/src/Region.js | 53 ++++--- ...llerSpec.js => InspectorControllerSpec.js} | 26 ++-- platform/commonUI/regions/test/RegionSpec.js | 71 +++++---- .../plot/src/PlotOptionsController.js | 147 +++++++++--------- .../features/plot/test/PlotOptionsFormSpec.js | 6 +- 15 files changed, 188 insertions(+), 357 deletions(-) delete mode 100644 platform/commonUI/browse/src/TypeRegionDecorator.js delete mode 100644 platform/commonUI/browse/test/TypeRegionDecoratorSpec.js rename platform/commonUI/regions/src/{RegionController.js => InspectorController.js} (69%) rename platform/commonUI/regions/test/{RegionControllerSpec.js => InspectorControllerSpec.js} (78%) diff --git a/example/plotOptions/bundle.js b/example/plotOptions/bundle.js index ea9be6495b..610b99ef2e 100644 --- a/example/plotOptions/bundle.js +++ b/example/plotOptions/bundle.js @@ -23,10 +23,12 @@ define([ 'legacyRegistry', - '../../platform/commonUI/browse/src/InspectorRegion' + '../../platform/commonUI/browse/src/InspectorRegion', + '../../platform/commonUI/regions/src/Region' ], function ( legacyRegistry, - InspectorRegion + InspectorRegion, + Region ) { "use strict"; @@ -45,22 +47,22 @@ define([ * to the same representation, but a different key could be used here to * include a customized representation for edit mode. */ - plotOptionsBrowsePart = { + plotOptionsBrowseRegion = new Region({ name: "plot-options", title: "Plot Options", modes: ['browse'], content: { key: "plot-options-browse" } - }, - plotOptionsEditPart = { + }), + plotOptionsEditRegion = new Region({ name: "plot-options", title: "Plot Options", modes: ['edit'], content: { key: "plot-options-browse" } - }; + }); /** * Both parts are added, and policies of type 'region' will determine @@ -68,8 +70,8 @@ define([ * provided which will check the 'modes' attribute of the region part * definition. */ - plotInspector.addPart(plotOptionsBrowsePart); - plotInspector.addPart(plotOptionsEditPart); + plotInspector.addRegion(plotOptionsBrowseRegion); + plotInspector.addRegion(plotOptionsEditRegion); legacyRegistry.register("example/plotType", { "name": "Plot Type", @@ -93,9 +95,7 @@ define([ "model": { "composition": [] }, - "regions": { - "inspector": plotInspector - }, + "inspector": plotInspector, "telemetry": { "source": "generator", "domains": [ diff --git a/platform/commonUI/browse/bundle.js b/platform/commonUI/browse/bundle.js index 9c08521f5e..d89d21a6e8 100644 --- a/platform/commonUI/browse/bundle.js +++ b/platform/commonUI/browse/bundle.js @@ -36,7 +36,6 @@ define([ "./src/creation/CreateActionProvider", "./src/creation/CreationService", "./src/windowing/WindowTitler", - "./src/TypeRegionDecorator", 'legacyRegistry' ], function ( BrowseController, @@ -53,7 +52,6 @@ define([ CreateActionProvider, CreationService, WindowTitler, - TypeRegionDecorator, legacyRegistry ) { "use strict"; @@ -290,11 +288,6 @@ define([ "$q", "$log" ] - }, - { - "provides": "typeService", - "type": "decorator", - "implementation": TypeRegionDecorator } ], "runs": [ diff --git a/platform/commonUI/browse/res/templates/browse/inspector-region.html b/platform/commonUI/browse/res/templates/browse/inspector-region.html index b946c06ab3..c04fb1fe91 100644 --- a/platform/commonUI/browse/res/templates/browse/inspector-region.html +++ b/platform/commonUI/browse/res/templates/browse/inspector-region.html @@ -19,12 +19,12 @@ this source code distribution or the Licensing information page available at runtime from the About dialog for additional information. --> -
-
+
+
-
+
diff --git a/platform/commonUI/browse/res/templates/browse/object-properties.html b/platform/commonUI/browse/res/templates/browse/object-properties.html index 9e9e21079e..a15efed6c9 100644 --- a/platform/commonUI/browse/res/templates/browse/object-properties.html +++ b/platform/commonUI/browse/res/templates/browse/object-properties.html @@ -57,4 +57,4 @@ -
\ No newline at end of file + \ No newline at end of file diff --git a/platform/commonUI/browse/src/InspectorRegion.js b/platform/commonUI/browse/src/InspectorRegion.js index accbac8fbd..cc47ae2db8 100644 --- a/platform/commonUI/browse/src/InspectorRegion.js +++ b/platform/commonUI/browse/src/InspectorRegion.js @@ -36,7 +36,7 @@ define( * @constructor */ function InspectorRegion() { - Region.call(this); + Region.call(this, {'name': 'Inspector'}); this.buildRegion(); } @@ -48,9 +48,9 @@ define( * @private */ InspectorRegion.prototype.buildRegion = function() { - var metadataPart = { - name: 'properties-location', - title: 'Properties and Location', + var metadataRegion = { + name: 'metadata', + title: 'Metadata Region', // Which modes should the region part be visible in? If // nothing provided here, then assumed that part is visible // in both. The visibility or otherwise of a region part @@ -61,7 +61,7 @@ define( key: 'object-properties' } }; - this.addPart(metadataPart, 0); + this.addRegion(new Region(metadataRegion), 0); }; return InspectorRegion; diff --git a/platform/commonUI/browse/src/TypeRegionDecorator.js b/platform/commonUI/browse/src/TypeRegionDecorator.js deleted file mode 100644 index 7c3f60994f..0000000000 --- a/platform/commonUI/browse/src/TypeRegionDecorator.js +++ /dev/null @@ -1,90 +0,0 @@ -/***************************************************************************** - * Open MCT Web, Copyright (c) 2014-2015, United States Government - * as represented by the Administrator of the National Aeronautics and Space - * Administration. All rights reserved. - * - * Open MCT Web is licensed under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0. - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations - * under the License. - * - * Open MCT Web includes source code licensed under additional open source - * licenses. See the Open Source Licenses file (LICENSES.md) included with - * this source code distribution or the Licensing information page available - * at runtime from the About dialog for additional information. - *****************************************************************************/ -/*global define,window*/ - -define( - [ - './InspectorRegion' - ], - function (InspectorRegion) { - "use strict"; - - /** - * Adds default browse screen regions to Type definitions. Screen - * regions are sections of the browse and edit view of an object - * that can be customized on a per-type basis. Within - * {@link Region}s are {@link RegionPart}s. Policies can be used to - * decide which parts are visible or not based on object state. - * @memberOf platform/commonUI/regions - * @see {@link Region}, {@link RegionPart}, {@link EditableRegionPolicy} - * @constructor - */ - function TypeRegionDecorator(typeService) { - this.typeService = typeService; - } - - /** - * Read Type bundle definition, and add default region definitions - * if none provided. - * @private - * @param type - * @returns {*} - */ - TypeRegionDecorator.prototype.decorateType = function (type) { - var regions = type.getDefinition().regions || {}; - - regions.inspector = regions.inspector || new InspectorRegion(); - - type.getDefinition().regions = regions; - - return type; - }; - - /** - * Override the provider functions in order to return decorated Type - * objects. - * @returns {Array|*} - */ - TypeRegionDecorator.prototype.listTypes = function() { - var self = this, - types = this.typeService.listTypes(); - - return types.map(function (type) { - return self.decorateType(type); - }); - }; - - /** - * Override the provider function in order to return decorated Type - * objects. - * @param key - */ - TypeRegionDecorator.prototype.getType = function(key) { - var self = this, - type = this.typeService.getType(key); - - return self.decorateType(type); - }; - - return TypeRegionDecorator; - } -); diff --git a/platform/commonUI/browse/test/InspectorRegionSpec.js b/platform/commonUI/browse/test/InspectorRegionSpec.js index 3eb9428518..e455a8df4d 100644 --- a/platform/commonUI/browse/test/InspectorRegionSpec.js +++ b/platform/commonUI/browse/test/InspectorRegionSpec.js @@ -37,7 +37,7 @@ define( }); it("creates default region parts", function () { - expect(inspectorRegion.parts.length).toBe(1); + expect(inspectorRegion.regions.length).toBe(1); }); }); diff --git a/platform/commonUI/browse/test/TypeRegionDecoratorSpec.js b/platform/commonUI/browse/test/TypeRegionDecoratorSpec.js deleted file mode 100644 index a525df5b9b..0000000000 --- a/platform/commonUI/browse/test/TypeRegionDecoratorSpec.js +++ /dev/null @@ -1,70 +0,0 @@ -/***************************************************************************** - * Open MCT Web, Copyright (c) 2014-2015, United States Government - * as represented by the Administrator of the National Aeronautics and Space - * Administration. All rights reserved. - * - * Open MCT Web is licensed under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0. - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations - * under the License. - * - * Open MCT Web includes source code licensed under additional open source - * licenses. See the Open Source Licenses file (LICENSES.md) included with - * this source code distribution or the Licensing information page available - * at runtime from the About dialog for additional information. - *****************************************************************************/ -/*global define,Promise,describe,it,expect,beforeEach,waitsFor,jasmine*/ - -/** - * MCTIncudeSpec. Created by vwoeltje on 11/6/14. - */ -define( - ["../src/TypeRegionDecorator"], - function (TypeRegionDecorator) { - "use strict"; - - describe("The type region decorator", function () { - var typeRegionDecorator, - mockTypeService, - mockType, - mockTypeDefinition; - - beforeEach(function () { - mockTypeDefinition = {}; - - mockType = jasmine.createSpyObj('type', [ - 'getDefinition' - ]); - mockType.getDefinition.andReturn(mockTypeDefinition); - - mockTypeService = jasmine.createSpyObj('typeService', [ - 'listTypes', - 'getType' - ]); - mockTypeService.getType.andReturn(mockType); - mockTypeService.listTypes.andReturn([mockType]); - - typeRegionDecorator = new TypeRegionDecorator(mockTypeService); - }); - - it("decorates individual type definitions with basic inspector" + - " region", function () { - typeRegionDecorator.getType('someType'); - expect(mockTypeDefinition.regions).toBeDefined(); - }); - - it("decorates all type definitions with basic inspector" + - " region", function () { - typeRegionDecorator.listTypes(); - expect(mockTypeDefinition.regions).toBeDefined(); - }); - - }); - } -); \ No newline at end of file diff --git a/platform/commonUI/regions/bundle.js b/platform/commonUI/regions/bundle.js index dce4543b60..d361347ad4 100644 --- a/platform/commonUI/regions/bundle.js +++ b/platform/commonUI/regions/bundle.js @@ -22,11 +22,11 @@ /*global define*/ define([ - './src/RegionController', + './src/InspectorController', './src/EditableRegionPolicy', 'legacyRegistry' ], function ( - RegionController, + InspectorController, EditableRegionPolicy, legacyRegistry ) { @@ -36,8 +36,8 @@ define([ "extensions": { "controllers": [ { - "key": "RegionController", - "implementation": RegionController, + "key": "InspectorController", + "implementation": InspectorController, "depends": [ "$scope", "policyService" diff --git a/platform/commonUI/regions/src/RegionController.js b/platform/commonUI/regions/src/InspectorController.js similarity index 69% rename from platform/commonUI/regions/src/RegionController.js rename to platform/commonUI/regions/src/InspectorController.js index 4641811672..7bc18fd342 100644 --- a/platform/commonUI/regions/src/RegionController.js +++ b/platform/commonUI/regions/src/InspectorController.js @@ -22,17 +22,17 @@ /*global define,Promise*/ define( - [], - function () { + ['../../browse/src/InspectorRegion'], + function (InspectorRegion) { "use strict"; /** - * The RegionController adds region data for a domain object's type + * The InspectorController adds region data for a domain object's type * to the scope. * * @constructor */ - function RegionController($scope, policyService) { + function InspectorController($scope, policyService) { var domainObject = $scope.domainObject, typeCapability = domainObject.getCapability('type'); @@ -41,21 +41,16 @@ define( * @param regions * @returns {{}} */ - function filterParts(regions) { + function filterRegions(inspector) { //Dupe so we're not modifying the type definition. - var filteredRegions = {}; - Object.keys(regions).forEach(function(regionName) { - filteredRegions[regionName] = Object.create(regions[regionName]); - filteredRegions[regionName].parts = (regions[regionName].parts || []).filter(function(part){ - return policyService.allow('region', part, domainObject); - }); + return inspector.regions && inspector.regions.filter(function(region) { + return policyService.allow('region', region, domainObject); }); - return filteredRegions; } - $scope.regions = filterParts(typeCapability.getDefinition().regions); + $scope.regions = filterRegions(typeCapability.getDefinition().inspector || new InspectorRegion()); } - return RegionController; + return InspectorController; } ); \ No newline at end of file diff --git a/platform/commonUI/regions/src/Region.js b/platform/commonUI/regions/src/Region.js index 3cc61c45ac..9e95590ca9 100644 --- a/platform/commonUI/regions/src/Region.js +++ b/platform/commonUI/regions/src/Region.js @@ -34,13 +34,13 @@ define( */ /** - * @typeDef {object} RegionPart + * @typeDef {object} RegionConfiguration * @property {string} name A unique name for this region part - * @property {PartContents} content the details of the region part - * being defined - * @property {Array} [modes] the modes that this region part + * @property {PartContents} [content] the details of the region being + * defined + * @property {Array} [modes] the modes that this region * should be included in. Options are 'edit' and 'browse'. By - * default, will be included in both. Inclusion of region parts is + * default, will be included in both. Inclusion of regions is * determined by policies of category 'region'. By default, the * {EditableRegionPolicy} will be applied. * @memberOf platform/commonUI/regions @@ -54,40 +54,45 @@ define( * @abstract * @constructor */ - function Region() { - this.parts = []; + function Region(configuration) { + configuration = configuration || {}; + this.name = configuration.name; + this.content = configuration.content; + this.modes = configuration.modes; + + this.regions = []; } /** - * Adds a part to this region. - * @param {RegionPart} part the part to add - * @param {number} [index] the position to insert the part. By default + * Adds a sub-region to this region. + * @param {Region} region the part to add + * @param {number} [index] the position to insert the region. By default * will add to the end */ - Region.prototype.addPart = function (part, index){ + Region.prototype.addRegion = function (region, index){ if (index) { - this.parts.splice(index, 0, part); + this.regions.splice(index, 0, region); } else { - this.parts.push(part); + this.regions.push(region); } }; /** - * Removes a part from this region. - * @param {RegionPart | number | strnig} part The region part to - * remove. If a number, will remove the part at that index. If a - * string, will remove the part with the matching name. If an + * Removes a sub-region from this region. + * @param {Region | number | strnig} region The region to + * remove. If a number, will remove the region at that index. If a + * string, will remove the region with the matching name. If an * object, will attempt to remove that object from the Region */ - Region.prototype.removePart = function (part){ - if (typeof part === 'number') { - this.parts.splice(part, 1); - } else if (typeof part === 'string'){ - this.parts = this.parts.filter(function(thisPart) { - return thisPart.name !== part; + Region.prototype.removeRegion = function (region){ + if (typeof region === 'number') { + this.regions.splice(region, 1); + } else if (typeof region === 'string'){ + this.regions = this.regions.filter(function(thisRegion) { + return thisRegion.name !== region; }); } else { - this.parts.splice(this.parts.indexOf(part), 1); + this.regions.splice(this.regions.indexOf(region), 1); } }; diff --git a/platform/commonUI/regions/test/RegionControllerSpec.js b/platform/commonUI/regions/test/InspectorControllerSpec.js similarity index 78% rename from platform/commonUI/regions/test/RegionControllerSpec.js rename to platform/commonUI/regions/test/InspectorControllerSpec.js index 352c5bd088..1fc230f01d 100644 --- a/platform/commonUI/regions/test/RegionControllerSpec.js +++ b/platform/commonUI/regions/test/InspectorControllerSpec.js @@ -22,11 +22,11 @@ /*global define,describe,it,expect,beforeEach,waitsFor,jasmine */ define( - ['../src/RegionController'], - function (RegionController) { + ['../src/InspectorController'], + function (InspectorController) { "use strict"; - describe("The region controller ", function () { + describe("The inspector controller ", function () { var mockScope, mockDomainObject, mockTypeCapability, @@ -36,14 +36,12 @@ define( beforeEach(function(){ mockTypeDefinition = { - regions: + inspector: { - 'regionOne': { - 'parts': [ - {'name': 'Part One'}, - {'name': 'Part Two'} - ] - } + 'regions': [ + {'name': 'Part One'}, + {'name': 'Part Two'} + ] } }; @@ -68,14 +66,14 @@ define( it("filters out regions disallowed by region policy", function() { mockPolicyService.allow.andReturn(false); - controller = new RegionController(mockScope, mockPolicyService); - expect(mockScope.regions.regionOne.parts.length).toBe(0); + controller = new InspectorController(mockScope, mockPolicyService); + expect(mockScope.regions.length).toBe(0); }); it("does not filter out regions allowed by region policy", function() { mockPolicyService.allow.andReturn(true); - controller = new RegionController(mockScope, mockPolicyService); - expect(mockScope.regions.regionOne.parts.length).toBe(2); + controller = new InspectorController(mockScope, mockPolicyService); + expect(mockScope.regions.length).toBe(2); }); }); } diff --git a/platform/commonUI/regions/test/RegionSpec.js b/platform/commonUI/regions/test/RegionSpec.js index 00ef1cb1e6..2e52508d04 100644 --- a/platform/commonUI/regions/test/RegionSpec.js +++ b/platform/commonUI/regions/test/RegionSpec.js @@ -29,77 +29,76 @@ define( describe("The region class ", function () { var region, - part2 = {'name': 'part2'}; + part2 = new Region({'name': 'part2'}); beforeEach(function(){ region = new Region(); - region.parts = [ - {name: 'part1'}, - {name: 'part3'}, - {name: 'part4'} + region.regions = [ + new Region({name: 'part1'}), + new Region({name: 'part3'}), + new Region({name: 'part4'}) ]; }); - it("adding a region part at a specified index adds it in that" + + it("adding a region at a specified index adds it in that" + " position", function() { - region.addPart(part2, 1); + region.addRegion(part2, 1); - expect(region.parts.length).toBe(4); - expect(region.parts[1]).toBe(part2); + expect(region.regions.length).toBe(4); + expect(region.regions[1]).toBe(part2); }); - it("adding a region part without an index adds it at the end", function() { - var partN = {'name': 'partN'}; + it("adding a region without an index adds it at the end", function() { + var partN = new Region({'name': 'partN'}); - region.addPart(partN); + region.addRegion(partN); - expect(region.parts.length).toBe(4); - expect(region.parts[region.parts.length-1]).toBe(partN); + expect(region.regions.length).toBe(4); + expect(region.regions[region.regions.length-1]).toBe(partN); }); - describe("removing a region part", function(){ + describe("removing a region", function(){ var partName = "part2"; beforeEach(function(){ - region.parts = [ - {name: 'part1'}, + region.regions = [ + new Region({name: 'part1'}), part2, - {name: 'part3'}, - {name: 'part4'} + new Region({name: 'part3'}), + new Region({name: 'part4'}) ]; }); - it("with a string matches on region part" + - " name", function() { - expect(region.parts.length).toBe(4); - expect(region.parts.indexOf(part2)).toBe(1); + it("with a string matches on region name", function() { + expect(region.regions.length).toBe(4); + expect(region.regions.indexOf(part2)).toBe(1); - region.removePart(partName); + region.removeRegion(partName); - expect(region.parts.length).toBe(3); - expect(region.parts.indexOf(part2)).toBe(-1); + expect(region.regions.length).toBe(3); + expect(region.regions.indexOf(part2)).toBe(-1); }); it("with a number removes by index", function() { - expect(region.parts.length).toBe(4); - expect(region.parts.indexOf(part2)).toBe(1); + expect(region.regions.length).toBe(4); + expect(region.regions.indexOf(part2)).toBe(1); - region.removePart(1); + region.removeRegion(1); - expect(region.parts.length).toBe(3); - expect(region.parts.indexOf(part2)).toBe(-1); + expect(region.regions.length).toBe(3); + expect(region.regions.indexOf(part2)).toBe(-1); }); it("with object matches that object", function() { - expect(region.parts.length).toBe(4); - expect(region.parts.indexOf(part2)).toBe(1); + expect(region.regions.length).toBe(4); + expect(region.regions.indexOf(part2)).toBe(1); - region.removePart(part2); + region.removeRegion(part2); - expect(region.parts.length).toBe(3); - expect(region.parts.indexOf(part2)).toBe(-1); + expect(region.regions.length).toBe(3); + expect(region.regions.indexOf(part2)).toBe(-1); }); }); }); diff --git a/platform/features/plot/src/PlotOptionsController.js b/platform/features/plot/src/PlotOptionsController.js index 7af8c23df3..4206799986 100644 --- a/platform/features/plot/src/PlotOptionsController.js +++ b/platform/features/plot/src/PlotOptionsController.js @@ -47,67 +47,26 @@ define( */ function PlotOptionsController($scope, topic) { - var self = this, - domainObject = $scope.domainObject, - composition, - mutationListener, - formListener, - configuration = domainObject.getModel().configuration || {}; - + var self = this; + this.$scope = $scope; + this.domainObject = $scope.domainObject; + this.configuration = this.domainObject.getModel().configuration || {}; this.plotOptionsForm = new PlotOptionsForm(topic); + this.composition = []; /* - * Determine whether the changes to the model that triggered a - * mutation event were purely compositional. + Listen for changes to the domain object and update the object's + children. */ - function hasCompositionChanged(oldComposition, newComposition){ - // Framed slightly strangely, but the boolean logic is - // easier to follow for the unchanged case. - var isUnchanged = oldComposition === newComposition || - ( - oldComposition.length === newComposition.length && - oldComposition.every( function (currentValue, index) { - return newComposition[index] && currentValue === newComposition[index]; - }) - ); - return !isUnchanged; - } + this.mutationListener = this.domainObject.getCapability('mutation').listen(function(model) { + if (self.hasCompositionChanged(self.composition, model.composition)) { + self.updateChildren(); + } + }); - /* - Default the plot options model - */ - function defaultConfiguration() { - configuration.plot = configuration.plot || {}; - configuration.plot.xAxis = configuration.plot.xAxis || {}; - configuration.plot.yAxis = configuration.plot.yAxis || {}; // y-axes will be associative array keyed on axis key - configuration.plot.series = configuration.plot.series || {}; // series will be associative array keyed on sub-object id - $scope.configuration = configuration; - } - - /* - When a child is added to, or removed from a plot, update the - plot options model - */ - function updateChildren() { - domainObject.useCapability('composition').then(function(children){ - $scope.children = children; - composition = domainObject.getModel().composition; - children.forEach(function(child){ - configuration.plot.series[child.getId()] = configuration.plot.series[child.getId()] || {}; - }); - }); - } - - /* - On changes to the form, update the configuration on the domain - object - */ - function updateConfiguration() { - domainObject.useCapability('mutation', function(model){ - model.configuration = model.configuration || {}; - model.configuration.plot = configuration.plot; - }); - } + this.formListener = this.plotOptionsForm.listen(function(value){ + self.updateConfiguration(value); + }); /* Set form structures on scope @@ -116,29 +75,71 @@ define( $scope.xAxisForm = this.plotOptionsForm.xAxisForm; $scope.yAxisForm = this.plotOptionsForm.yAxisForm; - /* - Listen for changes to the domain object and update the object's - children. - */ - mutationListener = domainObject.getCapability('mutation').listen(function(model) { - if (hasCompositionChanged(composition, model.composition)) { - updateChildren(); - } - }); - - formListener = this.plotOptionsForm.listen(updateConfiguration); - - defaultConfiguration(); - updateChildren(); - $scope.$on("$destroy", function() { //Clean up any listeners on destruction of controller - mutationListener(); - formListener(); + self.mutationListener(); + self.formListener(); }); + this.defaultConfiguration(); + this.updateChildren(); } + /* + * Determine whether the changes to the model that triggered a + * mutation event were purely compositional. + */ + PlotOptionsController.prototype.hasCompositionChanged = function(oldComposition, newComposition){ + // Framed slightly strangely, but the boolean logic is + // easier to follow for the unchanged case. + var isUnchanged = oldComposition === newComposition || + ( + oldComposition.length === newComposition.length && + oldComposition.every( function (currentValue, index) { + return newComposition[index] && currentValue === newComposition[index]; + }) + ); + return !isUnchanged; + }; + + /* + Default the plot options model + */ + PlotOptionsController.prototype.defaultConfiguration = function () { + this.configuration.plot = this.configuration.plot || {}; + this.configuration.plot.xAxis = this.configuration.plot.xAxis || {}; + this.configuration.plot.yAxis = this.configuration.plot.yAxis || {}; // y-axes will be associative array keyed on axis key + this.configuration.plot.series = this.configuration.plot.series || {}; // series will be associative array keyed on sub-object id + this.$scope.configuration = this.configuration; + }; + + /* + When a child is added to, or removed from a plot, update the + plot options model + */ + PlotOptionsController.prototype.updateChildren = function() { + var self = this; + this.domainObject.useCapability('composition').then(function(children){ + self.$scope.children = children; + self.composition = self.domainObject.getModel().composition; + children.forEach(function(child){ + self.configuration.plot.series[child.getId()] = self.configuration.plot.series[child.getId()] || {}; + }); + }); + }; + + /* + On changes to the form, update the configuration on the domain + object + */ + PlotOptionsController.prototype.updateConfiguration = function() { + var self = this; + this.domainObject.useCapability('mutation', function(model){ + model.configuration = model.configuration || {}; + model.configuration.plot = self.configuration.plot; + }); + }; + return PlotOptionsController; } ); diff --git a/platform/features/plot/test/PlotOptionsFormSpec.js b/platform/features/plot/test/PlotOptionsFormSpec.js index 2cd7b62390..3b5e1a990e 100644 --- a/platform/features/plot/test/PlotOptionsFormSpec.js +++ b/platform/features/plot/test/PlotOptionsFormSpec.js @@ -55,16 +55,16 @@ define( expect(plotOptionsForm.plotSeriesForm).toBeDefined(); }); - it("uses a topic to register listeners and inform them when a" + + it("uses a topic to register a listener and inform them when a" + " form value changes", function () { var changedValue = 'changedValue'; - expect(plotOptionsForm.xAxisForm.sections[0].rows[0].onchange).toBeDefined(); + expect(plotOptionsForm.xAxisForm.onchange).toBeDefined(); plotOptionsForm.listen(listener); expect(mockTopicObject.listen).toHaveBeenCalledWith(listener); - plotOptionsForm.xAxisForm.sections[0].rows[0].onchange(changedValue); + plotOptionsForm.xAxisForm.onchange(changedValue); expect(mockTopicObject.notify).toHaveBeenCalledWith(changedValue); });