From f192544be3062b60abff7f072787d8d20e48eb24 Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 29 Feb 2016 17:22:13 -0800 Subject: [PATCH] Addressing issues from code review --- platform/commonUI/browse/bundle.js | 2 + .../commonUI/browse/src/BrowseController.js | 25 +++++-- .../src/navigation/NavigationService.js | 46 +++---------- .../browse/test/BrowseControllerSpec.js | 13 ++++ .../test/navigation/NavigationServiceSpec.js | 18 ----- platform/commonUI/edit/bundle.js | 14 +++- .../edit/src/capabilities/EditorCapability.js | 1 - .../src/controllers/EditObjectController.js | 19 +++--- .../edit/src/directives/MCTBeforeUnload.js | 24 ++----- .../edit/src/policies/EditNavigationPolicy.js | 68 +++++++++++++++++++ .../edit/src/representers/EditRepresenter.js | 1 - .../test/controllers/EditControllerSpec.js | 7 ++ .../src/gestures/DropGesture.js | 7 +- 13 files changed, 147 insertions(+), 98 deletions(-) create mode 100644 platform/commonUI/edit/src/policies/EditNavigationPolicy.js diff --git a/platform/commonUI/browse/bundle.js b/platform/commonUI/browse/bundle.js index cf4ee5b7a0..388d484bb1 100644 --- a/platform/commonUI/browse/bundle.js +++ b/platform/commonUI/browse/bundle.js @@ -111,9 +111,11 @@ define([ "$scope", "$route", "$location", + "$window", "objectService", "navigationService", "urlService", + "policyService", "DEFAULT_PATH" ] }, diff --git a/platform/commonUI/browse/src/BrowseController.js b/platform/commonUI/browse/src/BrowseController.js index 6013772965..54a230682c 100644 --- a/platform/commonUI/browse/src/BrowseController.js +++ b/platform/commonUI/browse/src/BrowseController.js @@ -33,6 +33,7 @@ define( "use strict"; var ROOT_ID = "ROOT", + DEFAULT_PATH = "mine", CONFIRM_MSG = "Unsaved changes will be lost if you leave this page."; /** @@ -46,12 +47,14 @@ define( * @constructor */ function BrowseController( - $scope, - $route, - $location, - objectService, - navigationService, - urlService, + $scope, + $route, + $location, + $window, + objectService, + navigationService, + urlService, + policyService, defaultPath ) { var path = [ROOT_ID].concat( @@ -81,14 +84,22 @@ define( // Callback for updating the in-scope reference to the object // that is currently navigated-to. function setNavigation(domainObject) { + var navigationAllowed = true; + if (domainObject === $scope.navigatedObject){ //do nothing; return; } - if (navigationService.setNavigation(domainObject)) { + policyService.allow("navigation", $scope.navigatedObject, domainObject, function(message){ + navigationAllowed = $window.confirm(message + "\r\n\r\n" + + " Are you sure you want to continue?"); + }); + + if (navigationAllowed) { $scope.navigatedObject = domainObject; $scope.treeModel.selectedObject = domainObject; + navigationService.setNavigation(domainObject); updateRoute(domainObject); } else { //If navigation was unsuccessful (ie. blocked), reset diff --git a/platform/commonUI/browse/src/navigation/NavigationService.js b/platform/commonUI/browse/src/navigation/NavigationService.js index 858a5d80eb..3b4d266bd1 100644 --- a/platform/commonUI/browse/src/navigation/NavigationService.js +++ b/platform/commonUI/browse/src/navigation/NavigationService.js @@ -37,7 +37,7 @@ define( */ function NavigationService() { this.navigated = undefined; - this.callbacks = {}; + this.callbacks = []; } /** @@ -50,48 +50,27 @@ define( /** * Set the current navigation state. This will invoke listeners. - * Changing the navigation state will be blocked if any of the - * 'before' navigation state change listeners return 'false'. * @param {DomainObject} domainObject the domain object to navigate to */ NavigationService.prototype.setNavigation = function (value) { - var canNavigate = true; if (this.navigated !== value) { - canNavigate = (this.callbacks.before || []) - .reduce(function (previous, callback) { - //Check whether the callback returned a value of - // 'false' indicating that navigation should not - // continue. All other return values will allow - // navigation to continue - return (callback(value)!==false) && previous; - }, true); - if (canNavigate) { - this.navigated = value; - (this.callbacks.after || []).forEach(function (callback) { - callback(value); - }); - } + this.navigated = value; + this.callbacks.forEach(function (callback) { + callback(value); + }); } - return canNavigate; + return true; }; /** * Listen for changes in navigation. The passed callback will * be invoked with the new domain object of navigation when - * this changes. Callbacks can be registered to listen to pre or - * post-navigation events. The event to listen to is specified using - * the event parameter. In the case of pre-navigation events - * returning a false value will prevent the navigation event from - * going ahead. + * this changes. * @param {function} callback the callback to invoke when * navigation state changes - * @param {string} [event=after] the navigation event to listen to. - * One of 'before' or 'after'. */ - NavigationService.prototype.addListener = function (callback, event) { - event = event || 'after'; - this.callbacks[event] = this.callbacks[event] || []; - this.callbacks[event].push(callback); + NavigationService.prototype.addListener = function (callback) { + this.callbacks.push(callback); }; /** @@ -99,12 +78,9 @@ define( * @param {function} callback the callback which should * no longer be invoked when navigation state * changes - * @param {string} [event=after] the navigation event that the - * callback is registered to. One of 'before' or 'after'. */ - NavigationService.prototype.removeListener = function (callback, event) { - event = event || 'after'; - this.callbacks[event] = this.callbacks[event].filter(function (cb) { + NavigationService.prototype.removeListener = function (callback) { + this.callbacks = this.callbacks.filter(function (cb) { return cb !== callback; }); }; diff --git a/platform/commonUI/browse/test/BrowseControllerSpec.js b/platform/commonUI/browse/test/BrowseControllerSpec.js index caab0ed7d8..e40e39f6ef 100644 --- a/platform/commonUI/browse/test/BrowseControllerSpec.js +++ b/platform/commonUI/browse/test/BrowseControllerSpec.js @@ -39,6 +39,8 @@ define( mockUrlService, mockDomainObject, mockNextObject, + mockWindow, + mockPolicyService, testDefaultRoot, controller; @@ -55,14 +57,25 @@ define( mockScope, mockRoute, mockLocation, + mockWindow, mockObjectService, mockNavigationService, mockUrlService, + mockPolicyService, testDefaultRoot ); } beforeEach(function () { + mockWindow = jasmine.createSpyObj('$window', [ + "confirm" + ]); + mockWindow.confirm.andReturn(true); + + mockPolicyService = jasmine.createSpyObj('policyService', [ + 'allow' + ]); + testDefaultRoot = "some-root-level-domain-object"; mockScope = jasmine.createSpyObj( diff --git a/platform/commonUI/browse/test/navigation/NavigationServiceSpec.js b/platform/commonUI/browse/test/navigation/NavigationServiceSpec.js index 3a354dee78..410a5f1562 100644 --- a/platform/commonUI/browse/test/navigation/NavigationServiceSpec.js +++ b/platform/commonUI/browse/test/navigation/NavigationServiceSpec.js @@ -84,24 +84,6 @@ define( expect(callback).not.toHaveBeenCalled(); }); - it("adds listeners to the 'after' state by default", function(){ - expect(navigationService.callbacks.after).toBeUndefined(); - navigationService.addListener(function(){}); - expect(navigationService.callbacks.after).toBeDefined(); - expect(navigationService.callbacks.after.length).toBe(1); - }); - - it("allows navigationService events to be prevented", function(){ - var callback = jasmine.createSpy("callback"), - navigationResult; - callback.andReturn(false); - navigationService.addListener(callback, "before"); - navigationResult = navigationService.setNavigation({}); - expect(callback).toHaveBeenCalled(); - expect(navigationResult).toBe(false); - - }); - }); } ); diff --git a/platform/commonUI/edit/bundle.js b/platform/commonUI/edit/bundle.js index 2e0e0a93f3..7fbc5e7f31 100644 --- a/platform/commonUI/edit/bundle.js +++ b/platform/commonUI/edit/bundle.js @@ -34,6 +34,7 @@ define([ "./src/actions/SaveAction", "./src/actions/CancelAction", "./src/policies/EditActionPolicy", + "./src/policies/EditNavigationPolicy", "./src/representers/EditRepresenter", "./src/representers/EditToolbarRepresenter", "text!./res/templates/library.html", @@ -55,6 +56,7 @@ define([ SaveAction, CancelAction, EditActionPolicy, + EditNavigationPolicy, EditRepresenter, EditToolbarRepresenter, libraryTemplate, @@ -95,7 +97,8 @@ define([ "implementation": EditObjectController, "depends": [ "$scope", - "$location" + "$location", + "policyService" ] } ], @@ -183,7 +186,16 @@ define([ { "category": "action", "implementation": EditActionPolicy + }, + { + "category": "navigation", + "message": "There are unsaved changes.", + "implementation": EditNavigationPolicy, + "depends": [ + "$window" + ] } + ], "templates": [ { diff --git a/platform/commonUI/edit/src/capabilities/EditorCapability.js b/platform/commonUI/edit/src/capabilities/EditorCapability.js index f674880203..b48c1988e6 100644 --- a/platform/commonUI/edit/src/capabilities/EditorCapability.js +++ b/platform/commonUI/edit/src/capabilities/EditorCapability.js @@ -124,7 +124,6 @@ define( */ EditorCapability.prototype.cancel = function () { this.editableObject.getCapability("status").set("editing", false); - //TODO: Reset the cache as well here. this.cache.markClean(); return resolvePromise(undefined); }; diff --git a/platform/commonUI/edit/src/controllers/EditObjectController.js b/platform/commonUI/edit/src/controllers/EditObjectController.js index e46d5cffe1..d6121106ec 100644 --- a/platform/commonUI/edit/src/controllers/EditObjectController.js +++ b/platform/commonUI/edit/src/controllers/EditObjectController.js @@ -36,8 +36,9 @@ define( * @memberof platform/commonUI/edit * @constructor */ - function EditObjectController($scope, $location) { + function EditObjectController($scope, $location, policyService) { this.scope = $scope; + this.policyService = policyService; var navigatedObject; function setViewForDomainObject(domainObject) { @@ -73,16 +74,14 @@ define( */ EditObjectController.prototype.getUnloadWarning = function () { var navigatedObject = this.scope.domainObject, - editorCapability = navigatedObject && - navigatedObject.getCapability("editor"), - statusCapability = navigatedObject && - navigatedObject.getCapability("status"), - hasChanges = statusCapability && statusCapability.get('editing') - && editorCapability && editorCapability.dirty(); + policyMessage; + + this.policyService.allow("navigation", navigatedObject, undefined, function(message) { + policyMessage = message; + }); + + return policyMessage; - return hasChanges ? - "Unsaved changes will be lost if you leave this page." : - undefined; }; return EditObjectController; diff --git a/platform/commonUI/edit/src/directives/MCTBeforeUnload.js b/platform/commonUI/edit/src/directives/MCTBeforeUnload.js index 226e85f4a0..3e7501c788 100644 --- a/platform/commonUI/edit/src/directives/MCTBeforeUnload.js +++ b/platform/commonUI/edit/src/directives/MCTBeforeUnload.js @@ -35,7 +35,7 @@ define( * @constructor * @param $window the window */ - function MCTBeforeUnload($window, navigationService) { + function MCTBeforeUnload($window) { var unloads = [], oldBeforeUnload = $window.onbeforeunload; @@ -57,7 +57,6 @@ define( // Stop using this unload expression function removeUnload() { - navigationService.removeListener(checkNavigationEvent, "before"); unloads = unloads.filter(function (callback) { return callback !== unload; }); @@ -66,28 +65,17 @@ define( } } - function shouldAllowNavigation(){ + // Show a dialog before allowing a location change + function checkLocationChange(event) { // Get an unload message (if any) var warning = unload(); // Prompt the user if there's an unload message - return !warning || $window.confirm(warning); - } - - // Show a dialog before allowing a location change - function checkLocationChange(event) { - if (!shouldAllowNavigation()) { - // Prevent the route change if it was confirmed + if (warning && !$window.confirm(warning)) { + // ...and prevent the route change if it was confirmed event.preventDefault(); } } - // Show a dialog before allowing a location change - function checkNavigationEvent(event) { - // Return a false value to the navigationService to - // indicate that the navigation event should be prevented - return shouldAllowNavigation(); - } - // If this is the first active instance of this directive, // register as the window's beforeunload handler if (unloads.length === 0) { @@ -102,8 +90,6 @@ define( // Also handle route changes scope.$on("$locationChangeStart", checkLocationChange); - - navigationService.addListener(checkNavigationEvent, "before"); } return { diff --git a/platform/commonUI/edit/src/policies/EditNavigationPolicy.js b/platform/commonUI/edit/src/policies/EditNavigationPolicy.js new file mode 100644 index 0000000000..625f040a66 --- /dev/null +++ b/platform/commonUI/edit/src/policies/EditNavigationPolicy.js @@ -0,0 +1,68 @@ +/***************************************************************************** + * 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*/ + +define( + [], + function () { + "use strict"; + + /** + * Policy controlling whether navigation events should proceed + * when object is being edited. + * @memberof platform/commonUI/edit + * @constructor + * @implements {Policy.} + */ + function EditNavigationPolicy($window, policyService) { + this.window = $window; + this.policyService = policyService; + } + + /** + * @private + */ + EditNavigationPolicy.prototype.isDirty = function(domainObject) { + var navigatedObject = domainObject, + editorCapability = navigatedObject && + navigatedObject.getCapability("editor"), + statusCapability = navigatedObject && + navigatedObject.getCapability("status"); + + return statusCapability && statusCapability.get('editing') + && editorCapability && editorCapability.dirty(); + }; + + /** + * Allow navigation if an object is not dirty, or if the user elects + * to proceed anyway. + * @param currentNavigation + * @returns {boolean|*} true if the object model is clean; or if + * it's dirty and the user wishes to proceed anyway. + */ + EditNavigationPolicy.prototype.allow = function (currentNavigation) { + return !this.isDirty(currentNavigation); + }; + + return EditNavigationPolicy; + } +); diff --git a/platform/commonUI/edit/src/representers/EditRepresenter.js b/platform/commonUI/edit/src/representers/EditRepresenter.js index 5d96aeaf78..533c8031e0 100644 --- a/platform/commonUI/edit/src/representers/EditRepresenter.js +++ b/platform/commonUI/edit/src/representers/EditRepresenter.js @@ -147,7 +147,6 @@ define( // Respond to the destruction of the current representation. EditRepresenter.prototype.destroy = function destroy() { - // Nothing to clean up return this.listenHandle && this.listenHandle(); }; diff --git a/platform/commonUI/edit/test/controllers/EditControllerSpec.js b/platform/commonUI/edit/test/controllers/EditControllerSpec.js index 0a39874eb8..0af357f9c0 100644 --- a/platform/commonUI/edit/test/controllers/EditControllerSpec.js +++ b/platform/commonUI/edit/test/controllers/EditControllerSpec.js @@ -33,6 +33,7 @@ define( mockLocation, mockStatusCapability, mockCapabilities, + mockPolicyService, controller; // Utility function; look for a $watch on scope and fire it @@ -45,6 +46,12 @@ define( } beforeEach(function () { + mockPolicyService = jasmine.createSpyObj( + "policyService", + [ + "allow" + ] + ); mockScope = jasmine.createSpyObj( "$scope", [ "$on", "$watch" ] diff --git a/platform/representation/src/gestures/DropGesture.js b/platform/representation/src/gestures/DropGesture.js index 225211c3bd..a8ff9ac397 100644 --- a/platform/representation/src/gestures/DropGesture.js +++ b/platform/representation/src/gestures/DropGesture.js @@ -128,8 +128,7 @@ define( var typeKey = 'telemetry.panel', type = typeService.getType(typeKey), model = type.getInitialModel(), - newPanel, - composeAction; + newPanel; model.type = typeKey; newPanel = new EditableDomainObject(instantiate(model), $q); @@ -166,16 +165,12 @@ define( editableDomainObject = createVirtualPanel(domainObject, selectedObject); if (editableDomainObject) { editableDomainObject.getCapability('action').perform('edit'); - //navigationService.setNavigation(editableDomainObject); broadcastDrop(id, event); - //editableDomainObject.getCapability('status').set('editing', true); } } else { $q.when(action && action.perform()).then(function (result) { //Don't go into edit mode for folders if (domainObjectType!=='folder') { - // navigationService.setNavigation(editableDomainObject); - //editableDomainObject.getCapability('status').set('editing', true); editableDomainObject.getCapability('action').perform('edit'); } broadcastDrop(id, event);