[Plots] #638 Addressing feedback from code review

[Plots] #638 Fixing failing tests

[Plot] Changed PlotOptionsController to prototype form

Fixed spacing

Fixed jslint issue
This commit is contained in:
Henry 2016-02-08 23:05:48 -08:00
parent be031285b9
commit 645bd5743f
15 changed files with 188 additions and 357 deletions

View File

@ -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": [

View File

@ -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": [

View File

@ -19,12 +19,12 @@
this source code distribution or the Licensing information page available
at runtime from the About dialog for additional information.
-->
<div ng-controller="RegionController as regionController">
<div ng-repeat="part in regions.inspector.parts">
<div ng-controller="InspectorController">
<div ng-repeat="region in regions">
<mct-representation
key="part.content.key"
key="region.content.key"
mct-object="domainObject"
ng-model="ngModel">
</mct-representation>
</div>
</div><!--/ PaneController -->
</div>

View File

@ -57,4 +57,4 @@
</span>
</li>
</ul>
</div><!--/ holder-inspector -->
</div>

View File

@ -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;

View File

@ -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;
}
);

View File

@ -37,7 +37,7 @@ define(
});
it("creates default region parts", function () {
expect(inspectorRegion.parts.length).toBe(1);
expect(inspectorRegion.regions.length).toBe(1);
});
});

View File

@ -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();
});
});
}
);

View File

@ -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"

View File

@ -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;
}
);

View File

@ -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<string>} [modes] the modes that this region part
* @property {PartContents} [content] the details of the region being
* defined
* @property {Array<string>} [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);
}
};

View File

@ -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);
});
});
}

View File

@ -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);
});
});
});

View File

@ -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;
}
);

View File

@ -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);
});