diff --git a/platform/commonUI/edit/src/capabilities/EditableCompositionCapability.js b/platform/commonUI/edit/src/capabilities/EditableCompositionCapability.js new file mode 100644 index 0000000000..9fb364441d --- /dev/null +++ b/platform/commonUI/edit/src/capabilities/EditableCompositionCapability.js @@ -0,0 +1,36 @@ +/*global define*/ + + +define( + ['./EditableLookupCapability'], + function (EditableLookupCapability) { + 'use strict'; + + /** + * Wrapper for the "composition" capability; + * ensures that any domain objects reachable in Edit mode + * are also wrapped as EditableDomainObjects. + * + * Meant specifically for use by EditableDomainObject and the + * associated cache; the constructor signature is particular + * to a pattern used there and may contain unused arguments. + */ + return function EditableCompositionCapability( + contextCapability, + editableObject, + domainObject, + cache + ) { + // This is a "lookup" style capability (it looks up other + // domain objects), but we do not want to return the same + // specific value every time (composition may change) + return new EditableLookupCapability( + contextCapability, + editableObject, + domainObject, + cache, + false // Not idempotent + ); + }; + } +); \ No newline at end of file diff --git a/platform/commonUI/edit/src/capabilities/EditableContextCapability.js b/platform/commonUI/edit/src/capabilities/EditableContextCapability.js index f1b442f85a..b8658aa19a 100644 --- a/platform/commonUI/edit/src/capabilities/EditableContextCapability.js +++ b/platform/commonUI/edit/src/capabilities/EditableContextCapability.js @@ -2,12 +2,12 @@ define( - [], - function () { + ['./EditableLookupCapability'], + function (EditableLookupCapability) { 'use strict'; /** - * Wrapper for both "context" and "composition" capabilities; + * Wrapper for the "context" capability; * ensures that any domain objects reachable in Edit mode * are also wrapped as EditableDomainObjects. * @@ -21,55 +21,15 @@ define( domainObject, cache ) { - var capability = Object.create(contextCapability); - - // Check for domain object interface. If something has these - // three methods, we assume it's a domain object. - function isDomainObject(obj) { - return obj !== undefined && - typeof obj.getId === 'function' && - typeof obj.getModel === 'function' && - typeof obj.getCapability === 'function'; - } - - // Check an object returned by the wrapped capability; if it - // is a domain object, we want to make it editable and/or get - // it from the cache of editable domain objects. This will - // prevent changes made in edit mode from modifying the actual - // underlying domain object. - function makeEditableObject(obj) { - return isDomainObject(obj) ? - cache.getEditableObject(obj) : - obj; - } - - // Wrap a returned value (see above); if it's an array, wrap - // all elements. - function makeEditable(returnValue) { - return Array.isArray(returnValue) ? - returnValue.map(makeEditableObject) : - makeEditableObject(returnValue); - } - - // Wrap a returned value (see above); if it's a promise, wrap - // the resolved value. - function wrapResult(result) { - return result.then ? // promise-like - result.then(makeEditable) : - makeEditable(result); - } - - // Wrap all methods; return only editable domain objects. - Object.keys(contextCapability).forEach(function (k) { - capability[k] = function () { - return wrapResult(contextCapability[k].apply( - capability, - arguments - )); - }; - }); - - return capability; + // This is a "lookup" style capability (it looks up other + // domain objects), and it should be idempotent + return new EditableLookupCapability( + contextCapability, + editableObject, + domainObject, + cache, + true // Not idempotent + ); }; } ); \ No newline at end of file diff --git a/platform/commonUI/edit/src/capabilities/EditableLookupCapability.js b/platform/commonUI/edit/src/capabilities/EditableLookupCapability.js new file mode 100644 index 0000000000..632f4cde39 --- /dev/null +++ b/platform/commonUI/edit/src/capabilities/EditableLookupCapability.js @@ -0,0 +1,97 @@ +/*global define*/ + + +define( + [], + function () { + 'use strict'; + + /** + * Wrapper for both "context" and "composition" capabilities; + * ensures that any domain objects reachable in Edit mode + * are also wrapped as EditableDomainObjects. + * + * Meant specifically for use by EditableDomainObject and the + * associated cache; the constructor signature is particular + * to a pattern used there and may contain unused arguments. + */ + return function EditableLookupCapability( + contextCapability, + editableObject, + domainObject, + cache, + idempotent + ) { + var capability = Object.create(contextCapability); + + // Check for domain object interface. If something has these + // three methods, we assume it's a domain object. + function isDomainObject(obj) { + return obj !== undefined && + typeof obj.getId === 'function' && + typeof obj.getModel === 'function' && + typeof obj.getCapability === 'function'; + } + + // Check an object returned by the wrapped capability; if it + // is a domain object, we want to make it editable and/or get + // it from the cache of editable domain objects. This will + // prevent changes made in edit mode from modifying the actual + // underlying domain object. + function makeEditableObject(obj) { + return isDomainObject(obj) ? + cache.getEditableObject(obj) : + obj; + } + + // Wrap a returned value (see above); if it's an array, wrap + // all elements. + function makeEditable(returnValue) { + return Array.isArray(returnValue) ? + returnValue.map(makeEditableObject) : + makeEditableObject(returnValue); + } + + // Wrap a returned value (see above); if it's a promise, wrap + // the resolved value. + function wrapResult(result) { + return result.then ? // promise-like + result.then(makeEditable) : + makeEditable(result); + } + + // Return a wrapped version of a function, which ensures + // all results are editable domain objects. + function wrapFunction(fn) { + return function () { + return wrapResult(contextCapability[fn].apply( + capability, + arguments + )); + }; + } + + // Wrap a method such that it only delegates once. + function oneTimeFunction(fn) { + return function () { + var result = wrapFunction(fn).apply(this, arguments); + capability[fn] = function () { + return result; + }; + return result; + }; + } + + // Wrap a method of this capability + function wrapMethod(fn) { + capability[fn] = + (idempotent ? oneTimeFunction : wrapFunction)(fn); + } + + // Wrap all methods; return only editable domain objects. + Object.keys(contextCapability).forEach(wrapMethod); + + return capability; + }; + } +); \ No newline at end of file diff --git a/platform/commonUI/edit/src/objects/EditableDomainObject.js b/platform/commonUI/edit/src/objects/EditableDomainObject.js index 14aa5435a0..a6b3d503d7 100644 --- a/platform/commonUI/edit/src/objects/EditableDomainObject.js +++ b/platform/commonUI/edit/src/objects/EditableDomainObject.js @@ -13,12 +13,14 @@ define( [ '../capabilities/EditablePersistenceCapability', '../capabilities/EditableContextCapability', + '../capabilities/EditableCompositionCapability', '../capabilities/EditorCapability', './EditableDomainObjectCache' ], function ( EditablePersistenceCapability, EditableContextCapability, + EditableCompositionCapability, EditorCapability, EditableDomainObjectCache ) { @@ -27,7 +29,7 @@ define( var capabilityFactories = { persistence: EditablePersistenceCapability, context: EditableContextCapability, - composition: EditableContextCapability, + composition: EditableCompositionCapability, editor: EditorCapability }; @@ -53,9 +55,8 @@ define( // Constructor for EditableDomainObject, which adheres // to the same shared cache. - function EditableDomainObjectImpl(domainObject) { - var model = JSON.parse(JSON.stringify(domainObject.getModel())), - editableObject = Object.create(domainObject); + function EditableDomainObjectImpl(domainObject, model) { + var editableObject = Object.create(domainObject); // Only provide the cloned model. editableObject.getModel = function () { return model; }; diff --git a/platform/commonUI/edit/src/objects/EditableDomainObjectCache.js b/platform/commonUI/edit/src/objects/EditableDomainObjectCache.js index 0b7354dc84..6673e31998 100644 --- a/platform/commonUI/edit/src/objects/EditableDomainObjectCache.js +++ b/platform/commonUI/edit/src/objects/EditableDomainObjectCache.js @@ -15,7 +15,8 @@ * @module editor/object/editable-domain-object-cache */ define( - function () { + ["./EditableModelCache"], + function (EditableModelCache) { 'use strict'; /** @@ -32,7 +33,7 @@ define( * @memberof module:editor/object/editable-domain-object-cache */ function EditableDomainObjectCache(EditableDomainObject) { - var cache = {}, + var cache = new EditableModelCache(), dirty = {}; return { @@ -44,9 +45,10 @@ define( * @returns {DomainObject} the domain object in an editable form */ getEditableObject: function (domainObject) { - var id = domainObject.getId(); - return (cache[id] = - cache[id] || new EditableDomainObject(domainObject)); + return new EditableDomainObject( + domainObject, + cache.getCachedModel(domainObject) + ); }, /** * Mark an editable domain object (presumably already cached) diff --git a/platform/commonUI/edit/src/objects/EditableModelCache.js b/platform/commonUI/edit/src/objects/EditableModelCache.js new file mode 100644 index 0000000000..bcda83eab3 --- /dev/null +++ b/platform/commonUI/edit/src/objects/EditableModelCache.js @@ -0,0 +1,42 @@ +/*global define*/ + +define( + [], + function () { + "use strict"; + + /** + * An editable model cache stores domain object models that have been + * made editable, to support a group that can be saved all-at-once. + * This is useful in Edit mode, which is launched for a specific + * object but may contain changes across many objects. + * @constructor + */ + function EditableModelCache() { + var cache = {}; + + // Deep-copy a model. Models are JSONifiable, so this can be + // done by stringification then destringification + function clone(model) { + return JSON.parse(JSON.stringify(model)); + } + + return { + /** + * Get this domain object's model from the cache (or + * place it in the cache if it isn't in the cache yet) + * @returns a clone of the domain object's model + */ + getCachedModel: function (domainObject) { + var id = domainObject.getId(); + + return (cache[id] = + cache[id] || clone(domainObject.getModel())); + } + }; + + } + + return EditableModelCache; + } +); \ No newline at end of file diff --git a/platform/commonUI/edit/test/capabilities/EditableCompositionCapabilitySpec.js b/platform/commonUI/edit/test/capabilities/EditableCompositionCapabilitySpec.js new file mode 100644 index 0000000000..ded1a0c30e --- /dev/null +++ b/platform/commonUI/edit/test/capabilities/EditableCompositionCapabilitySpec.js @@ -0,0 +1,54 @@ +/*global define,describe,it,expect,beforeEach,jasmine*/ + +define( + ["../../src/capabilities/EditableCompositionCapability"], + function (EditableCompositionCapability) { + "use strict"; + + describe("An editable composition capability", function () { + var mockContext, + mockEditableObject, + mockDomainObject, + mockTestObject, + someValue, + mockFactory, + capability; + + beforeEach(function () { + // EditableContextCapability should watch ALL + // methods for domain objects, so give it an + // arbitrary interface to wrap. + mockContext = + jasmine.createSpyObj("context", [ "getDomainObject" ]); + mockTestObject = jasmine.createSpyObj( + "domainObject", + [ "getId", "getModel", "getCapability" ] + ); + mockFactory = + jasmine.createSpyObj("factory", ["getEditableObject"]); + + someValue = { x: 42 }; + + mockContext.getDomainObject.andReturn(mockTestObject); + mockFactory.getEditableObject.andReturn(someValue); + + capability = new EditableCompositionCapability( + mockContext, + mockEditableObject, + mockDomainObject, + mockFactory + ); + + }); + + // Most behavior is tested for EditableLookupCapability, + // so just verify that this isse + it("presumes non-idempotence of its wrapped capability", function () { + expect(capability.getDomainObject()) + .toEqual(capability.getDomainObject()); + expect(mockContext.getDomainObject.calls.length).toEqual(2); + }); + + }); + } +); \ No newline at end of file diff --git a/platform/commonUI/edit/test/capabilities/EditableContextCapabilitySpec.js b/platform/commonUI/edit/test/capabilities/EditableContextCapabilitySpec.js index d79e9e5029..7beeb7c548 100644 --- a/platform/commonUI/edit/test/capabilities/EditableContextCapabilitySpec.js +++ b/platform/commonUI/edit/test/capabilities/EditableContextCapabilitySpec.js @@ -11,63 +11,40 @@ define( mockDomainObject, mockTestObject, someValue, - factory, + mockFactory, capability; beforeEach(function () { // EditableContextCapability should watch ALL // methods for domain objects, so give it an // arbitrary interface to wrap. - mockContext = jasmine.createSpyObj( - "context", - [ - "getSomething", - "getDomainObject", - "getDomainObjectArray" - ] - ); + mockContext = + jasmine.createSpyObj("context", [ "getDomainObject" ]); mockTestObject = jasmine.createSpyObj( "domainObject", [ "getId", "getModel", "getCapability" ] ); - factory = { - getEditableObject: function (v) { - return { - isFromTestFactory: true, - calledWith: v - }; - } - }; + mockFactory = + jasmine.createSpyObj("factory", ["getEditableObject"]); someValue = { x: 42 }; - mockContext.getSomething.andReturn(someValue); mockContext.getDomainObject.andReturn(mockTestObject); - mockContext.getDomainObjectArray.andReturn([mockTestObject]); + mockFactory.getEditableObject.andReturn(someValue); capability = new EditableContextCapability( mockContext, mockEditableObject, mockDomainObject, - factory + mockFactory ); }); - it("wraps retrieved domain objects", function () { - var object = capability.getDomainObject(); - expect(object.isFromTestFactory).toBe(true); - expect(object.calledWith).toEqual(mockTestObject); - }); - - it("wraps retrieved domain object arrays", function () { - var object = capability.getDomainObjectArray()[0]; - expect(object.isFromTestFactory).toBe(true); - expect(object.calledWith).toEqual(mockTestObject); - }); - - it("does not wrap non-domain-objects", function () { - expect(capability.getSomething()).toEqual(someValue); + it("presumes idempotence of its wrapped capability", function () { + expect(capability.getDomainObject()) + .toEqual(capability.getDomainObject()); + expect(mockContext.getDomainObject.calls.length).toEqual(1); }); }); diff --git a/platform/commonUI/edit/test/capabilities/EditableLookupCapabilitySpec.js b/platform/commonUI/edit/test/capabilities/EditableLookupCapabilitySpec.js new file mode 100644 index 0000000000..12046dbedd --- /dev/null +++ b/platform/commonUI/edit/test/capabilities/EditableLookupCapabilitySpec.js @@ -0,0 +1,102 @@ +/*global define,describe,it,expect,beforeEach,jasmine*/ + +define( + ["../../src/capabilities/EditableLookupCapability"], + function (EditableLookupCapability) { + "use strict"; + + describe("An editable lookup capability", function () { + var mockContext, + mockEditableObject, + mockDomainObject, + mockTestObject, + someValue, + factory, + capability; + + beforeEach(function () { + // EditableContextCapability should watch ALL + // methods for domain objects, so give it an + // arbitrary interface to wrap. + mockContext = jasmine.createSpyObj( + "context", + [ + "getSomething", + "getDomainObject", + "getDomainObjectArray" + ] + ); + mockTestObject = jasmine.createSpyObj( + "domainObject", + [ "getId", "getModel", "getCapability" ] + ); + factory = { + getEditableObject: function (v) { + return { + isFromTestFactory: true, + calledWith: v + }; + } + }; + + someValue = { x: 42 }; + + mockContext.getSomething.andReturn(someValue); + mockContext.getDomainObject.andReturn(mockTestObject); + mockContext.getDomainObjectArray.andReturn([mockTestObject]); + + capability = new EditableLookupCapability( + mockContext, + mockEditableObject, + mockDomainObject, + factory, + false + ); + + }); + + it("wraps retrieved domain objects", function () { + var object = capability.getDomainObject(); + expect(object.isFromTestFactory).toBe(true); + expect(object.calledWith).toEqual(mockTestObject); + }); + + it("wraps retrieved domain object arrays", function () { + var object = capability.getDomainObjectArray()[0]; + expect(object.isFromTestFactory).toBe(true); + expect(object.calledWith).toEqual(mockTestObject); + }); + + it("does not wrap non-domain-objects", function () { + expect(capability.getSomething()).toEqual(someValue); + }); + + it("caches idempotent lookups", function () { + capability = new EditableLookupCapability( + mockContext, + mockEditableObject, + mockDomainObject, + factory, + true // idempotent + ); + expect(capability.getDomainObject()) + .toEqual(capability.getDomainObject()); + expect(mockContext.getDomainObject.calls.length).toEqual(1); + }); + + it("does not cache non-idempotent lookups", function () { + capability = new EditableLookupCapability( + mockContext, + mockEditableObject, + mockDomainObject, + factory, + false // Not idempotent + ); + expect(capability.getDomainObject()) + .toEqual(capability.getDomainObject()); + expect(mockContext.getDomainObject.calls.length).toEqual(2); + }); + + }); + } +); \ No newline at end of file diff --git a/platform/commonUI/edit/test/objects/EditableDomainObjectCacheSpec.js b/platform/commonUI/edit/test/objects/EditableDomainObjectCacheSpec.js index 4a606606fc..bdc8cd0d3f 100644 --- a/platform/commonUI/edit/test/objects/EditableDomainObjectCacheSpec.js +++ b/platform/commonUI/edit/test/objects/EditableDomainObjectCacheSpec.js @@ -24,9 +24,10 @@ define( }; } - function WrapObject(domainObject) { + function WrapObject(domainObject, model) { var result = Object.create(domainObject); result.wrapped = true; + result.wrappedModel = model; captured.wraps = (captured.wraps || 0) + 1; return result; } @@ -49,24 +50,30 @@ define( expect(wrappedObject.getId()).toEqual(domainObject.getId()); }); - it("only wraps objects once", function () { + it("wraps objects repeatedly, wraps models once", function () { var domainObject = new TestObject('test-id'), - wrappedObject; + wrappedObjects = []; // Verify precondition expect(captured.wraps).toBeUndefined(); // Invoke a few more times; expect count not to increment - wrappedObject = cache.getEditableObject(domainObject); - expect(captured.wraps).toEqual(1); - wrappedObject = cache.getEditableObject(domainObject); - expect(captured.wraps).toEqual(1); - wrappedObject = cache.getEditableObject(domainObject); + wrappedObjects.push(cache.getEditableObject(domainObject)); expect(captured.wraps).toEqual(1); + wrappedObjects.push(cache.getEditableObject(domainObject)); + expect(captured.wraps).toEqual(2); + wrappedObjects.push(cache.getEditableObject(domainObject)); + expect(captured.wraps).toEqual(3); // Verify that the last call still gave us a wrapped object - expect(wrappedObject.wrapped).toBeTruthy(); - expect(wrappedObject.getId()).toEqual(domainObject.getId()); + expect(wrappedObjects[0].wrapped).toBeTruthy(); + expect(wrappedObjects[0].getId()).toEqual(domainObject.getId()); + + // Verify that objects are distinct but models are identical + expect(wrappedObjects[0].wrappedModel) + .toBe(wrappedObjects[1].wrappedModel); + expect(wrappedObjects[0]).not + .toBe(wrappedObjects[1]); }); it("saves objects that have been marked dirty", function () { diff --git a/platform/commonUI/edit/test/objects/EditableModelCacheSpec.js b/platform/commonUI/edit/test/objects/EditableModelCacheSpec.js new file mode 100644 index 0000000000..85fad1ae70 --- /dev/null +++ b/platform/commonUI/edit/test/objects/EditableModelCacheSpec.js @@ -0,0 +1,60 @@ +/*global define,describe,it,expect,beforeEach,jasmine*/ + +define( + ["../../src/objects/EditableModelCache"], + function (EditableModelCache) { + "use strict"; + + describe("The editable model cache", function () { + var mockObject, + mockOtherObject, + testModel, + testId, + otherModel, + otherId, + cache; + + beforeEach(function () { + testId = "test"; + testModel = { someKey: "some value" }; + otherId = "other"; + otherModel = { someKey: "some other value" }; + + mockObject = jasmine.createSpyObj( + "domainObject", + [ "getId", "getModel" ] + ); + mockOtherObject = jasmine.createSpyObj( + "domainObject", + [ "getId", "getModel" ] + ); + + mockObject.getId.andReturn(testId); + mockObject.getModel.andReturn(testModel); + mockOtherObject.getId.andReturn(otherId); + mockOtherObject.getModel.andReturn(otherModel); + + cache = new EditableModelCache(); + }); + + it("provides clones of domain object models", function () { + var model = cache.getCachedModel(mockObject); + // Should be identical... + expect(model).toEqual(testModel); + // ...but not pointer-identical + expect(model).not.toBe(testModel); + }); + + it("provides only one clone per object", function () { + var model = cache.getCachedModel(mockObject); + expect(cache.getCachedModel(mockObject)).toBe(model); + }); + + it("maintains separate caches per-object", function () { + expect(cache.getCachedModel(mockObject)) + .not.toEqual(cache.getCachedModel(mockOtherObject)); + }); + }); + + } +); \ No newline at end of file diff --git a/platform/commonUI/edit/test/suite.json b/platform/commonUI/edit/test/suite.json index 98a0d3de00..e50fb236d4 100644 --- a/platform/commonUI/edit/test/suite.json +++ b/platform/commonUI/edit/test/suite.json @@ -8,9 +8,12 @@ "actions/PropertiesDialog", "actions/RemoveAction", "actions/SaveAction", + "capabilities/EditableCompositionCapability", "capabilities/EditableContextCapability", + "capabilities/EditableLookupCapability", "capabilities/EditablePersistenceCapability", "capabilities/EditorCapability", "objects/EditableDomainObject", - "objects/EditableDomainObjectCache" + "objects/EditableDomainObjectCache", + "objects/EditableModelCache" ] \ No newline at end of file