From 75ae5ab3bbcccb2849b9674a78a7d199b54e3064 Mon Sep 17 00:00:00 2001 From: Pegah Sarram Date: Fri, 20 Apr 2018 08:45:29 -0700 Subject: [PATCH] [Toolbar] set selection initially in fixed controller and toolbar... (#1994) * [Toolbar] set selection initially in fixed controller and toolbar... ... to make the add button appear in the toolbar when a fixed position is created. Remove selection change listener on destroy. Start a digest cycle when handling selection in toolbar to avoid delays in toolbar. Fixes #1991, #1987 * fixed checkstyle and lint errors * Fix tests * Update comment --- .../representers/EditToolbarRepresenter.js | 38 ++++++++----------- .../src/representers/EditToolbarSelection.js | 25 +++++++++--- .../EditToolbarRepresenterSpec.js | 2 +- .../representers/EditToolbarSelectionSpec.js | 24 +++++++++++- .../features/layout/src/FixedController.js | 2 + .../layout/test/FixedControllerSpec.js | 4 +- 6 files changed, 61 insertions(+), 34 deletions(-) diff --git a/platform/commonUI/edit/src/representers/EditToolbarRepresenter.js b/platform/commonUI/edit/src/representers/EditToolbarRepresenter.js index 8fbfac292f..5393197f81 100644 --- a/platform/commonUI/edit/src/representers/EditToolbarRepresenter.js +++ b/platform/commonUI/edit/src/representers/EditToolbarRepresenter.js @@ -88,12 +88,6 @@ define( commit("Changes from toolbar."); } } - - // Avoid attaching scope to this; - // http://errors.angularjs.org/1.2.26/ng/cpws - this.setSelection = function (s) { - scope.selection = s; - }; this.clearExposedToolbar = function () { // Clear exposed toolbar state (if any) if (attrs.toolbar) { @@ -110,6 +104,7 @@ define( this.toolbar = undefined; this.toolbarObject = {}; this.openmct = openmct; + this.scope = scope; // If this representation exposes a toolbar, set up watches // to synchronize with it. @@ -130,26 +125,23 @@ define( // Represent a domain object using this definition EditToolbarRepresenter.prototype.represent = function (representation) { // Get the newest toolbar definition from the view - var definition = (representation || {}).toolbar || {}, - self = this; + var definition = (representation || {}).toolbar || {}; - // Initialize toolbar (expose object to parent scope) - function initialize(def) { - // If we have been asked to expose toolbar state... - if (self.attrs.toolbar) { - // Initialize toolbar object - self.toolbar = new EditToolbar(def, self.commit); - // Ensure toolbar state is exposed - self.exposeToolbar(); - } + // If we have been asked to expose toolbar state... + if (this.attrs.toolbar) { + // Initialize toolbar object + this.toolbar = new EditToolbar(definition, this.commit); + // Ensure toolbar state is exposed + this.exposeToolbar(); } - // Expose the toolbar object to the parent scope - initialize(definition); - // Create a selection scope - this.setSelection(new EditToolbarSelection(this.openmct)); - // Initialize toolbar to an empty selection - this.updateSelection([]); + // Add toolbar selection to scope. + this.scope.selection = new EditToolbarSelection( + this.scope, + this.openmct + ); + // Initialize toolbar to current selection + this.updateSelection(this.scope.selection.all()); }; // Destroy; remove toolbar object from parent scope diff --git a/platform/commonUI/edit/src/representers/EditToolbarSelection.js b/platform/commonUI/edit/src/representers/EditToolbarSelection.js index 282640d1da..1c12be49d9 100644 --- a/platform/commonUI/edit/src/representers/EditToolbarSelection.js +++ b/platform/commonUI/edit/src/representers/EditToolbarSelection.js @@ -38,24 +38,37 @@ define( * @memberof platform/commonUI/edit * @constructor */ - function EditToolbarSelection(openmct) { + function EditToolbarSelection($scope, openmct) { this.selection = [{}]; this.selecting = false; this.selectedObj = undefined; + this.openmct = openmct; + var self = this; - openmct.selection.on('change', function (selection) { + function setSelection(selection) { var selected = selection[0]; if (selected && selected.context.toolbar) { - this.select(selected.context.toolbar); + self.select(selected.context.toolbar); } else { - this.deselect(); + self.deselect(); } if (selected && selected.context.viewProxy) { - this.proxy(selected.context.viewProxy); + self.proxy(selected.context.viewProxy); } - }.bind(this)); + + setTimeout(function () { + $scope.$apply(); + }); + } + + $scope.$on("$destroy", function () { + self.openmct.selection.off('change', setSelection); + }); + + this.openmct.selection.on('change', setSelection); + setSelection(this.openmct.selection.get()); } /** diff --git a/platform/commonUI/edit/test/representers/EditToolbarRepresenterSpec.js b/platform/commonUI/edit/test/representers/EditToolbarRepresenterSpec.js index 34bcfb7af5..23650f44c0 100644 --- a/platform/commonUI/edit/test/representers/EditToolbarRepresenterSpec.js +++ b/platform/commonUI/edit/test/representers/EditToolbarRepresenterSpec.js @@ -36,7 +36,7 @@ define( beforeEach(function () { mockScope = jasmine.createSpyObj( '$scope', - ['$on', '$watch', '$watchCollection', "commit"] + ['$on', '$watch', '$watchCollection', "commit", "$apply"] ); mockElement = {}; testAttrs = { toolbar: 'testToolbar' }; diff --git a/platform/commonUI/edit/test/representers/EditToolbarSelectionSpec.js b/platform/commonUI/edit/test/representers/EditToolbarSelectionSpec.js index 27e3230145..f7851ce4ab 100644 --- a/platform/commonUI/edit/test/representers/EditToolbarSelectionSpec.js +++ b/platform/commonUI/edit/test/representers/EditToolbarSelectionSpec.js @@ -30,7 +30,8 @@ define( otherElement, selection, mockSelection, - mockOpenMCT; + mockOpenMCT, + mockScope; beforeEach(function () { testProxy = { someKey: "some value" }; @@ -46,7 +47,12 @@ define( mockOpenMCT = { selection: mockSelection }; - selection = new EditToolbarSelection(mockOpenMCT); + mockScope = jasmine.createSpyObj('$scope', [ + '$on', + '$apply' + ]); + + selection = new EditToolbarSelection(mockScope, mockOpenMCT); selection.proxy(testProxy); }); @@ -103,6 +109,20 @@ define( expect(selection.all()).toEqual([testProxy]); }); + it("cleans up selection on scope destroy", function () { + expect(mockScope.$on).toHaveBeenCalledWith( + '$destroy', + jasmine.any(Function) + ); + + mockScope.$on.mostRecentCall.args[1](); + + expect(mockOpenMCT.selection.off).toHaveBeenCalledWith( + 'change', + jasmine.any(Function) + ); + }); + }); } ); diff --git a/platform/features/layout/src/FixedController.js b/platform/features/layout/src/FixedController.js index 1a72fa3c0e..1c9e07ea92 100644 --- a/platform/features/layout/src/FixedController.js +++ b/platform/features/layout/src/FixedController.js @@ -315,6 +315,8 @@ define( this.openmct.time.on("bounds", updateDisplayBounds); this.openmct.selection.on('change', setSelection); this.$element.on('click', this.bypassSelection.bind(this)); + + setSelection(this.openmct.selection.get()); } /** diff --git a/platform/features/layout/test/FixedControllerSpec.js b/platform/features/layout/test/FixedControllerSpec.js index 798717038d..e2ca31432b 100644 --- a/platform/features/layout/test/FixedControllerSpec.js +++ b/platform/features/layout/test/FixedControllerSpec.js @@ -201,7 +201,7 @@ define( 'off', 'get' ]); - mockSelection.get.andCallThrough(); + mockSelection.get.andReturn([]); mockOpenMCT = { time: mockConductor, @@ -596,7 +596,7 @@ define( expect(controller.getSelectedElementStyle()).not.toEqual(oldStyle); }); - it("cleans up slection on scope destroy", function () { + it("cleans up selection on scope destroy", function () { expect(mockScope.$on).toHaveBeenCalledWith( '$destroy', jasmine.any(Function)