From a45dfc38221e927349f58ccf891228de77a3a679 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Fri, 30 Oct 2015 15:06:51 -0700 Subject: [PATCH 1/6] [Representation] Don't reuse scopes Addresses nasa/openmctweb#227 --- platform/representation/src/TemplateLinker.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/platform/representation/src/TemplateLinker.js b/platform/representation/src/TemplateLinker.js index 426bbbe318..9a5e8a67f2 100644 --- a/platform/representation/src/TemplateLinker.js +++ b/platform/representation/src/TemplateLinker.js @@ -92,10 +92,19 @@ define( var activeElement = element, activeTemplateUrl, comment = this.$compile('')(scope), + activeScope, self = this; + function destroyScope() { + if (activeScope) { + activeScope.$destroy(); + activeScope = undefined; + } + } + function removeElement() { if (activeElement !== comment) { + destroyScope(); activeElement.replaceWith(comment); activeElement = comment; } @@ -110,8 +119,10 @@ define( } function populateElement(template) { + destroyScope(); + activeScope = scope.$new(false); element.empty(); - element.append(self.$compile(template)(scope)); + element.append(self.$compile(template)(activeScope)); } function badTemplate(templateUrl) { From 04594ea5362536a4f8072b4b1f5b8037f9d154aa Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Fri, 30 Oct 2015 15:32:20 -0700 Subject: [PATCH 2/6] [Representation] Rebuild scopes on every change Create new scopes on every changeTemplate request, even if the same template is being viewed; presume that we want a new instance of the same template. Avoids scope reuse for cases such as switching from a plot of one object to a plot of another object. --- platform/representation/src/TemplateLinker.js | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/platform/representation/src/TemplateLinker.js b/platform/representation/src/TemplateLinker.js index 9a5e8a67f2..dc19943ed8 100644 --- a/platform/representation/src/TemplateLinker.js +++ b/platform/representation/src/TemplateLinker.js @@ -131,22 +131,20 @@ define( } function changeTemplate(templateUrl) { - if (templateUrl !== activeTemplateUrl) { - if (templateUrl) { - addElement(); - self.load(templateUrl).then(function (template) { - // Avoid race conditions - if (templateUrl === activeTemplateUrl) { - populateElement(template); - } - }, function () { - badTemplate(templateUrl); - }); - } else { - removeElement(); - } - activeTemplateUrl = templateUrl; + if (templateUrl) { + addElement(); + self.load(templateUrl).then(function (template) { + // Avoid race conditions + if (templateUrl === activeTemplateUrl) { + populateElement(template); + } + }, function () { + badTemplate(templateUrl); + }); + } else { + removeElement(); } + activeTemplateUrl = templateUrl; } if (templateUrl) { From 560454e7c253b96c0bf461d0c2c7e336be17f560 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Fri, 30 Oct 2015 15:55:56 -0700 Subject: [PATCH 3/6] [Representation] Verify change before representing An mct-representation may have a refresh triggered either by a key change or a domain object change; both will typically happen in the same digest cycle. Track what prior state was an abort refreshes if nothing will change. --- .../representation/src/MCTRepresentation.js | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/platform/representation/src/MCTRepresentation.js b/platform/representation/src/MCTRepresentation.js index 10b6d3ccec..d1937389d2 100644 --- a/platform/representation/src/MCTRepresentation.js +++ b/platform/representation/src/MCTRepresentation.js @@ -31,7 +31,6 @@ define( function () { "use strict"; - /** * Defines the mct-representation directive. This may be used to * present domain objects as HTML (with event wiring), with the @@ -96,6 +95,9 @@ define( }), toClear = [], // Properties to clear out of scope on change counter = 0, + couldRepresent = false, + lastId, + lastKey, changeTemplate = templateLinker.link($scope, element); // Populate scope with any capabilities indicated by the @@ -141,6 +143,13 @@ define( }); } + function unchanged(canRepresent, id, key) { + return canRepresent && + couldRepresent && + id === lastId && + key === lastKey; + } + // General-purpose refresh mechanism; should set up the scope // as appropriate for current representation key and // domain object. @@ -149,7 +158,13 @@ define( representation = lookup($scope.key, domainObject), path = representation && getPath(representation), uses = ((representation || {}).uses || []), - canRepresent = !!(path && domainObject); + canRepresent = !!(path && domainObject), + id = domainObject && domainObject.getId(), + key = $scope.key; + + if (unchanged(canRepresent, id, key)) { + return; + } // Create an empty object named "representation", for this // representation to store local variables into. @@ -173,6 +188,11 @@ define( delete $scope[property]; }); + // To allow simplified change detection next time around + couldRepresent = canRepresent; + lastId = id; + lastKey = key; + // Populate scope with fields associated with the current // domain object (if one has been passed in) if (canRepresent) { From b487fa44385a61ab89b4a140617fb8d7280867c8 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Fri, 30 Oct 2015 16:01:24 -0700 Subject: [PATCH 4/6] [Representation] Update TemplateLinker spec ...to reflect creation of a new scope each time a template is changed. --- platform/representation/test/TemplateLinkerSpec.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/platform/representation/test/TemplateLinkerSpec.js b/platform/representation/test/TemplateLinkerSpec.js index bf762fba53..85d879f730 100644 --- a/platform/representation/test/TemplateLinkerSpec.js +++ b/platform/representation/test/TemplateLinkerSpec.js @@ -27,7 +27,8 @@ define( function (TemplateLinker) { 'use strict'; - var JQLITE_METHODS = [ 'replaceWith', 'empty', 'append' ]; + var JQLITE_METHODS = [ 'replaceWith', 'empty', 'append' ], + SCOPE_METHODS = [ '$on', '$new', '$destroy' ]; describe("TemplateLinker", function () { var mockTemplateRequest, @@ -38,6 +39,7 @@ define( mockElement, mockTemplates, mockElements, + mockNewScope, mockPromise, linker; @@ -46,7 +48,8 @@ define( mockSce = jasmine.createSpyObj('$sce', ['trustAsResourceUrl']); mockCompile = jasmine.createSpy('$compile'); mockLog = jasmine.createSpyObj('$log', ['error', 'warn']); - mockScope = jasmine.createSpyObj('$scope', ['$on']); + mockScope = jasmine.createSpyObj('$scope', SCOPE_METHODS); + mockNewScope = jasmine.createSpyObj('$scope', SCOPE_METHODS); mockElement = jasmine.createSpyObj('element', JQLITE_METHODS); mockPromise = jasmine.createSpyObj('promise', ['then']); mockTemplates = {}; @@ -63,6 +66,7 @@ define( mockSce.trustAsResourceUrl.andCallFake(function (url) { return { trusted: url }; }); + mockScope.$new.andReturn(mockNewScope); linker = new TemplateLinker( mockTemplateRequest, @@ -131,10 +135,10 @@ define( }, false); }); - it("compiles loaded templates with linked scope", function () { + it("compiles loaded templates with a new scope", function () { expect(mockCompile).toHaveBeenCalledWith(testTemplate); expect(mockTemplates[testTemplate]) - .toHaveBeenCalledWith(mockScope); + .toHaveBeenCalledWith(mockNewScope); }); it("replaces comments with specified element", function () { From 454a63e1f17b71055ce1bf31d9d3ccc8e4f27ee9 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Fri, 30 Oct 2015 16:18:01 -0700 Subject: [PATCH 5/6] [Representation] Destroy scopes before adding Destroy representation scope before adding elements back into the DOM; avoids having a momentary activation of watches and listeners on those scopes before they are replaced with newly compiled content for new scopes. --- platform/representation/src/TemplateLinker.js | 1 + 1 file changed, 1 insertion(+) diff --git a/platform/representation/src/TemplateLinker.js b/platform/representation/src/TemplateLinker.js index dc19943ed8..82cc1eb153 100644 --- a/platform/representation/src/TemplateLinker.js +++ b/platform/representation/src/TemplateLinker.js @@ -132,6 +132,7 @@ define( function changeTemplate(templateUrl) { if (templateUrl) { + destroyScope(); addElement(); self.load(templateUrl).then(function (template) { // Avoid race conditions From ae928a138cfc762e07ff26f6dd936468eb307551 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Fri, 30 Oct 2015 16:37:47 -0700 Subject: [PATCH 6/6] [Representation] Update compilation approach Update compilation approach for templateLinker to more closely resemble ng-include; minimizes likelihood of subtle behavioral differences (e.g. incorrect size selection for split pane) --- platform/representation/src/TemplateLinker.js | 4 +-- .../representation/test/TemplateLinkerSpec.js | 29 ++++++++++++++----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/platform/representation/src/TemplateLinker.js b/platform/representation/src/TemplateLinker.js index 82cc1eb153..14d0aa041d 100644 --- a/platform/representation/src/TemplateLinker.js +++ b/platform/representation/src/TemplateLinker.js @@ -121,8 +121,8 @@ define( function populateElement(template) { destroyScope(); activeScope = scope.$new(false); - element.empty(); - element.append(self.$compile(template)(activeScope)); + element.html(template); + self.$compile(element.contents())(activeScope); } function badTemplate(templateUrl) { diff --git a/platform/representation/test/TemplateLinkerSpec.js b/platform/representation/test/TemplateLinkerSpec.js index 85d879f730..726e4deab1 100644 --- a/platform/representation/test/TemplateLinkerSpec.js +++ b/platform/representation/test/TemplateLinkerSpec.js @@ -27,7 +27,7 @@ define( function (TemplateLinker) { 'use strict'; - var JQLITE_METHODS = [ 'replaceWith', 'empty', 'append' ], + var JQLITE_METHODS = [ 'replaceWith', 'empty', 'html', 'contents' ], SCOPE_METHODS = [ '$on', '$new', '$destroy' ]; describe("TemplateLinker", function () { @@ -39,6 +39,7 @@ define( mockElement, mockTemplates, mockElements, + mockContents, mockNewScope, mockPromise, linker; @@ -54,9 +55,12 @@ define( mockPromise = jasmine.createSpyObj('promise', ['then']); mockTemplates = {}; mockElements = {}; + mockContents = {}; mockTemplateRequest.andReturn(mockPromise); - mockCompile.andCallFake(function (html) { + mockCompile.andCallFake(function (toCompile) { + var html = typeof toCompile === 'string' ? + toCompile : toCompile.testHtml; mockTemplates[html] = jasmine.createSpy('template'); mockElements[html] = jasmine.createSpyObj('templateEl', JQLITE_METHODS); @@ -67,6 +71,16 @@ define( return { trusted: url }; }); mockScope.$new.andReturn(mockNewScope); + mockElement.html.andCallFake(function (html) { + mockContents[html] = + jasmine.createSpyObj('contentsEl', JQLITE_METHODS); + mockContents[html].testHtml = html; + }); + mockElement.contents.andCallFake(function () { + return mockContents[ + mockElement.html.mostRecentCall.args[0] + ]; + }); linker = new TemplateLinker( mockTemplateRequest, @@ -135,8 +149,9 @@ define( }, false); }); - it("compiles loaded templates with a new scope", function () { - expect(mockCompile).toHaveBeenCalledWith(testTemplate); + it("compiles element contents with a new scope", function () { + expect(mockCompile) + .toHaveBeenCalledWith(mockContents[testTemplate]); expect(mockTemplates[testTemplate]) .toHaveBeenCalledWith(mockNewScope); }); @@ -146,9 +161,9 @@ define( .toHaveBeenCalledWith(mockElement); }); - it("appends rendered content to the specified element", function () { - expect(mockElement.append) - .toHaveBeenCalledWith(mockElements[testTemplate]); + it("inserts HTML content into the specified element", function () { + expect(mockElement.html) + .toHaveBeenCalledWith(testTemplate); }); it("clears templates when called with undefined", function () {