From 847cefcb7b3bc0229b1daed6000ded8ed72f98d0 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Tue, 2 Dec 2014 17:09:46 -0800 Subject: [PATCH] feat(change_detector): notify directives on property changes --- .../change_detection/src/change_detector.js | 57 ++++++++- modules/change_detection/src/record.js | 46 ++++--- modules/change_detection/src/record_range.js | 20 +++- .../test/change_detector_spec.js | 85 +++++++++++-- .../test/record_range_spec.js | 2 +- modules/core/src/compiler/interfaces.js | 5 + modules/core/src/compiler/view.js | 112 ++++++++++++++---- modules/core/src/core.js | 1 + modules/core/test/compiler/view_spec.js | 54 ++++++++- 9 files changed, 320 insertions(+), 62 deletions(-) create mode 100644 modules/core/src/compiler/interfaces.js diff --git a/modules/change_detection/src/change_detector.js b/modules/change_detection/src/change_detector.js index 5d94d4d1c2..25887b1f0e 100644 --- a/modules/change_detection/src/change_detector.js +++ b/modules/change_detection/src/change_detector.js @@ -1,25 +1,70 @@ import {ProtoRecordRange, RecordRange} from './record_range'; import {ProtoRecord, Record} from './record'; -import {FIELD, int, isPresent} from 'facade/lang'; +import {int, isPresent, isBlank} from 'facade/lang'; +import {ListWrapper, List} from 'facade/collection'; export * from './record'; export * from './record_range' export class ChangeDetector { _rootRecordRange:RecordRange; + constructor(recordRange:RecordRange) { this._rootRecordRange = recordRange; } detectChanges():int { - var count:int = 0; - for (var record = this._rootRecordRange.findFirstEnabledRecord(); - isPresent(record); - record = record.nextEnabled) { + var count = 0; + var updatedRecords = null; + var record = this._rootRecordRange.findFirstEnabledRecord(); + + while (isPresent(record)) { + var currentRange = record.recordRange; + var currentGroup = record.groupMemento(); + + var nextEnabled = record.nextEnabled; + var nextRange = isPresent(nextEnabled) ? nextEnabled.recordRange : null; + var nextGroup = isPresent(nextEnabled) ? nextEnabled.groupMemento() : null; + if (record.check()) { - count++; + count ++; + if (record.terminatesExpression()) { + updatedRecords = this._addRecord(updatedRecords, record); + } } + + if (this._shouldNotifyDispatcher(currentRange, nextRange, currentGroup, nextGroup, updatedRecords)) { + currentRange.dispatcher.onRecordChange(currentGroup, updatedRecords); + updatedRecords = null; + } + + record = record.nextEnabled; } + return count; } + + _groupChanged(currentRange, nextRange, currentGroup, nextGroup) { + return currentRange != nextRange || currentGroup != nextGroup; + } + + _shouldNotifyDispatcher(currentRange, nextRange, currentGroup, nextGroup, updatedRecords) { + return this._groupChanged(currentRange, nextRange, currentGroup, nextGroup) && isPresent(updatedRecords); + } + + _addRecord(updatedRecords:List, record:Record) { + if (isBlank(updatedRecords)) { + updatedRecords = _singleElementList; + updatedRecords[0] = record; + + } else if (updatedRecords === _singleElementList) { + updatedRecords = [_singleElementList[0], record]; + + } else { + ListWrapper.push(updatedRecords, record); + } + return updatedRecords; + } } + +var _singleElementList = [null]; diff --git a/modules/change_detection/src/record.js b/modules/change_detection/src/record.js index b05e1277ac..4d55cb9a49 100644 --- a/modules/change_detection/src/record.js +++ b/modules/change_detection/src/record.js @@ -34,15 +34,19 @@ export class ProtoRecord { arity:int; name:string; dest:any; + groupMemento:any; + next:ProtoRecord; recordInConstruction:Record; + constructor(recordRange:ProtoRecordRange, mode:int, funcOrValue, arity:int, name:string, - dest) { + dest, + groupMemento) { this.recordRange = recordRange; this._mode = mode; @@ -50,6 +54,7 @@ export class ProtoRecord { this.arity = arity; this.name = name; this.dest = dest; + this.groupMemento = groupMemento; this.next = null; // The concrete Record instantiated from this ProtoRecord @@ -190,21 +195,20 @@ export class Record { check():boolean { if (this.isCollection) { - var changed = this._checkCollection(); - if (changed) { - this._notifyDispatcher(); - return true; - } - return false; + return this._checkCollection(); } else { - this.previousValue = this.currentValue; - this.currentValue = this._calculateNewValue(); - if (isSame(this.previousValue, this.currentValue)) return false; - this._updateDestination(); - return true; + return this._checkSingleRecord(); } } + _checkSingleRecord():boolean { + this.previousValue = this.currentValue; + this.currentValue = this._calculateNewValue(); + if (isSame(this.previousValue, this.currentValue)) return false; + this._updateDestination(); + return true; + } + _updateDestination() { // todo(vicb): compute this info only once in ctor ? (add a bit in mode not to grow the mem req) if (this.dest instanceof Record) { @@ -213,15 +217,9 @@ export class Record { } else { this.dest.updateContext(this.currentValue); } - } else { - this._notifyDispatcher(); } } - _notifyDispatcher() { - this.recordRange.dispatcher.onRecordChange(this, this.protoRecord.dest); - } - // return whether the content has changed _checkCollection():boolean { switch(this.type) { @@ -307,9 +305,21 @@ export class Record { } } + terminatesExpression():boolean { + return !(this.dest instanceof Record); + } + get isMarkerRecord() { return this.type == RECORD_TYPE_MARKER; } + + expressionMemento() { + return this.protoRecord.dest; + } + + groupMemento() { + return isPresent(this.protoRecord) ? this.protoRecord.groupMemento : null; + } } function isSame(a, b) { diff --git a/modules/change_detection/src/record_range.js b/modules/change_detection/src/record_range.js index cd882fa445..684ef3f7b8 100644 --- a/modules/change_detection/src/record_range.js +++ b/modules/change_detection/src/record_range.js @@ -45,21 +45,24 @@ export class ProtoRecordRange { * Parses [ast] into [ProtoRecord]s and adds them to [ProtoRecordRange]. * * @param ast The expression to watch - * @param memento an opaque object which will be passed to WatchGroupDispatcher on + * @param expressionMemento an opaque object which will be passed to WatchGroupDispatcher on * detecting a change. * @param content Wether to watch collection content (true) or reference (false, default) */ addRecordsFromAST(ast:AST, - memento, + expressionMemento, + groupMemento, content:boolean = false) { if (this.recordCreator === null) { this.recordCreator = new ProtoRecordCreator(this); } + if (content) { ast = new Collection(ast); } - this.recordCreator.createRecordsFromAST(ast, memento); + + this.recordCreator.createRecordsFromAST(ast, expressionMemento, groupMemento); } // TODO(rado): the type annotation should be dispatcher:WatchGroupDispatcher. @@ -85,6 +88,8 @@ export class ProtoRecordRange { for (var proto = this.recordCreator.headRecord; proto != null; proto = proto.next) { if (proto.dest instanceof Destination) { proto.recordInConstruction.dest = proto.dest.record.recordInConstruction; + } else { + proto.recordInConstruction.dest = proto.dest; } proto.recordInConstruction = null; } @@ -378,6 +383,8 @@ class ProtoRecordCreator { protoRecordRange:ProtoRecordRange; headRecord:ProtoRecord; tailRecord:ProtoRecord; + groupMemento:any; + constructor(protoRecordRange) { this.protoRecordRange = protoRecordRange; this.headRecord = null; @@ -491,12 +498,13 @@ class ProtoRecordCreator { visitTemplateBindings(ast, dest) {this._unsupported();} - createRecordsFromAST(ast:AST, memento){ - ast.visit(this, memento); + createRecordsFromAST(ast:AST, expressionMemento:any, groupMemento:any){ + this.groupMemento = groupMemento; + ast.visit(this, expressionMemento); } construct(recordType, funcOrValue, arity, name, dest) { - return new ProtoRecord(this.protoRecordRange, recordType, funcOrValue, arity, name, dest); + return new ProtoRecord(this.protoRecordRange, recordType, funcOrValue, arity, name, dest, this.groupMemento); } add(protoRecord:ProtoRecord) { diff --git a/modules/change_detection/test/change_detector_spec.js b/modules/change_detection/test/change_detector_spec.js index c799e69db3..8f5a18eb88 100644 --- a/modules/change_detection/test/change_detector_spec.js +++ b/modules/change_detection/test/change_detector_spec.js @@ -26,7 +26,7 @@ export function main() { function createChangeDetector(memo:string, exp:string, context = null, formatters = null, content = false) { var prr = new ProtoRecordRange(); - prr.addRecordsFromAST(ast(exp), memo, content); + prr.addRecordsFromAST(ast(exp), memo, memo, content); var dispatcher = new LoggingDispatcher(); var rr = prr.instantiate(dispatcher, formatters); @@ -97,21 +97,21 @@ export function main() { it("should support literal array", () => { var c = createChangeDetector('array', '[1,2]'); c["changeDetector"].detectChanges(); - expect(c["dispatcher"].loggedValues).toEqual([[1,2]]); + expect(c["dispatcher"].loggedValues).toEqual([[[1,2]]]); c = createChangeDetector('array', '[1,a]', new TestData(2)); c["changeDetector"].detectChanges(); - expect(c["dispatcher"].loggedValues).toEqual([[1,2]]); + expect(c["dispatcher"].loggedValues).toEqual([[[1,2]]]); }); it("should support literal maps", () => { var c = createChangeDetector('map', '{z:1}'); c["changeDetector"].detectChanges(); - expect(MapWrapper.get(c["dispatcher"].loggedValues[0], 'z')).toEqual(1); + expect(MapWrapper.get(c["dispatcher"].loggedValues[0][0], 'z')).toEqual(1); c = createChangeDetector('map', '{z:a}', new TestData(1)); c["changeDetector"].detectChanges(); - expect(MapWrapper.get(c["dispatcher"].loggedValues[0], 'z')).toEqual(1); + expect(MapWrapper.get(c["dispatcher"].loggedValues[0][0], 'z')).toEqual(1); }); it("should support binary operations", () => { @@ -242,6 +242,7 @@ export function main() { context.a = [0]; cd.detectChanges(); + expect(dsp.log).toEqual(["a=" + arrayChangesAsString({ collection: ['0[null->0]'], @@ -343,10 +344,69 @@ export function main() { }); } }); + + describe("onGroupChange", () => { + it("should notify the dispatcher when a group of records changes", () => { + var prr = new ProtoRecordRange(); + prr.addRecordsFromAST(ast("1 + 2"), "memo", 1); + prr.addRecordsFromAST(ast("10 + 20"), "memo", 1); + prr.addRecordsFromAST(ast("100 + 200"), "memo2", 2); + + var dispatcher = new LoggingDispatcher(); + var rr = prr.instantiate(dispatcher, null); + + var cd = new ChangeDetector(rr); + cd.detectChanges(); + + expect(dispatcher.loggedValues).toEqual([[3, 30], [300]]); + }); + + it("should update every instance of a group individually", () => { + var prr = new ProtoRecordRange(); + prr.addRecordsFromAST(ast("1 + 2"), "memo", "memo"); + + var dispatcher = new LoggingDispatcher(); + var rr = new RecordRange(null, dispatcher); + rr.addRange(prr.instantiate(dispatcher, null)); + rr.addRange(prr.instantiate(dispatcher, null)); + + var cd = new ChangeDetector(rr); + cd.detectChanges(); + + expect(dispatcher.loggedValues).toEqual([[3], [3]]); + }); + + it("should notify the dispatcher before switching to the next group", () => { + var prr = new ProtoRecordRange(); + prr.addRecordsFromAST(ast("a()"), "a", 1); + prr.addRecordsFromAST(ast("b()"), "b", 2); + prr.addRecordsFromAST(ast("c()"), "b", 2); + + var dispatcher = new LoggingDispatcher(); + var rr = prr.instantiate(dispatcher, null); + + var tr = new TestRecord(); + tr.a = () => {dispatcher.logValue('InvokeA'); return 'a'}; + tr.b = () => {dispatcher.logValue('InvokeB'); return 'b'}; + tr.c = () => {dispatcher.logValue('InvokeC'); return 'c'}; + rr.setContext(tr); + + var cd = new ChangeDetector(rr); + cd.detectChanges(); + + expect(dispatcher.loggedValues).toEqual(['InvokeA', ['a'], 'InvokeB', 'InvokeC', ['b', 'c']]); + }); + }); }); }); } +class TestRecord { + a; + b; + c; +} + class Person { name:string; address:Address; @@ -387,6 +447,7 @@ class TestData { class LoggingDispatcher extends WatchGroupDispatcher { log:List; loggedValues:List; + constructor() { this.log = null; this.loggedValues = null; @@ -397,10 +458,18 @@ class LoggingDispatcher extends WatchGroupDispatcher { this.log = ListWrapper.create(); this.loggedValues = ListWrapper.create(); } + + logValue(value) { + ListWrapper.push(this.loggedValues, value); + } - onRecordChange(record:Record, context) { - ListWrapper.push(this.loggedValues, record.currentValue); - ListWrapper.push(this.log, context + '=' + this._asString(record.currentValue)); + onRecordChange(group, records:List) { + var value = records[0].currentValue; + var dest = records[0].protoRecord.dest; + ListWrapper.push(this.log, dest + '=' + this._asString(value)); + + var values = ListWrapper.map(records, (r) => r.currentValue); + ListWrapper.push(this.loggedValues, values); } _asString(value) { diff --git a/modules/change_detection/test/record_range_spec.js b/modules/change_detection/test/record_range_spec.js index 82312d0c58..c4b7e6b9ad 100644 --- a/modules/change_detection/test/record_range_spec.js +++ b/modules/change_detection/test/record_range_spec.js @@ -46,7 +46,7 @@ export function main() { } function createRecord(rr) { - return new Record(rr, new ProtoRecord(null, 0, null, null, null, null), null); + return new Record(rr, new ProtoRecord(null, 0, null, null, null, null, null), null); } describe('record range', () => { diff --git a/modules/core/src/compiler/interfaces.js b/modules/core/src/compiler/interfaces.js new file mode 100644 index 0000000000..22b6f5ea2c --- /dev/null +++ b/modules/core/src/compiler/interfaces.js @@ -0,0 +1,5 @@ +export class OnChange { + onChange(changes) { + throw "not implemented"; + } +} \ No newline at end of file diff --git a/modules/core/src/compiler/view.js b/modules/core/src/compiler/view.js index 435c850370..544f2dd9db 100644 --- a/modules/core/src/compiler/view.js +++ b/modules/core/src/compiler/view.js @@ -1,5 +1,5 @@ import {DOM, Element, Node, Text, DocumentFragment, TemplateElement} from 'facade/dom'; -import {ListWrapper, MapWrapper, List} from 'facade/collection'; +import {ListWrapper, MapWrapper, StringMapWrapper, List} from 'facade/collection'; import {ProtoRecordRange, RecordRange, WatchGroupDispatcher} from 'change_detection/record_range'; import {Record} from 'change_detection/record'; import {AST} from 'change_detection/parser/ast'; @@ -12,6 +12,7 @@ import {FIELD, IMPLEMENTS, int, isPresent, isBlank} from 'facade/lang'; import {Injector} from 'di/di'; import {NgElement} from 'core/dom/element'; import {ViewPort} from './viewport'; +import {OnChange} from './interfaces'; const NG_BINDING_CLASS = 'ng-binding'; @@ -47,22 +48,55 @@ export class View { this.viewPorts = null; } - onRecordChange(record:Record, target) { + onRecordChange(groupMemento, records:List) { + this._invokeMementoForRecords(records); + if (groupMemento instanceof DirectivePropertyGroupMemento) { + this._notifyDirectiveAboutChanges(groupMemento, records); + } + } + + _invokeMementoForRecords(records:List) { + for(var i = 0; i < records.length; ++i) { + this._invokeMementoFor(records[i]); + } + } + + _notifyDirectiveAboutChanges(groupMemento, records:List) { + var dir = groupMemento.directive(this.elementInjectors); + if (dir instanceof OnChange) { + dir.onChange(this._collectChanges(records)); + } + } + // dispatch to element injector or text nodes based on context - if (target instanceof DirectivePropertyMemento) { + _invokeMementoFor(record:Record) { + var memento = record.expressionMemento(); + if (memento instanceof DirectivePropertyMemento) { // we know that it is DirectivePropertyMemento - var directiveMemento:DirectivePropertyMemento = target; + var directiveMemento:DirectivePropertyMemento = memento; directiveMemento.invoke(record, this.elementInjectors); - } else if (target instanceof ElementPropertyMemento) { - var elementMemento:ElementPropertyMemento = target; + + } else if (memento instanceof ElementPropertyMemento) { + var elementMemento:ElementPropertyMemento = memento; elementMemento.invoke(record, this.bindElements); + } else { // we know it refers to _textNodes. - var textNodeIndex:number = target; + var textNodeIndex:number = memento; DOM.setText(this.textNodes[textNodeIndex], record.currentValue); } } + _collectChanges(records:List) { + var changes = StringMapWrapper.create(); + for(var i = 0; i < records.length; ++i) { + var record = records[i]; + var propertyUpdate = new PropertyUpdate(record.currentValue, record.previousValue); + StringMapWrapper.set(changes, record.expressionMemento()._setterName, propertyUpdate); + } + return changes; + } + addViewPort(viewPort: ViewPort) { if (isBlank(this.viewPorts)) this.viewPorts = []; ListWrapper.push(this.viewPorts, viewPort); @@ -163,7 +197,8 @@ export class ProtoView { elBinder.textNodeIndices = ListWrapper.create(); } ListWrapper.push(elBinder.textNodeIndices, indexInParent); - this.protoRecordRange.addRecordsFromAST(expression, this.textNodesWithBindingCount++); + var memento = this.textNodesWithBindingCount++; + this.protoRecordRange.addRecordsFromAST(expression, memento, memento); } /** @@ -175,12 +210,8 @@ export class ProtoView { elBinder.hasElementPropertyBindings = true; this.elementsWithBindingCount++; } - this.protoRecordRange.addRecordsFromAST(expression, - new ElementPropertyMemento( - this.elementsWithBindingCount-1, - propertyName - ) - ); + var memento = new ElementPropertyMemento(this.elementsWithBindingCount-1, propertyName); + this.protoRecordRange.addRecordsFromAST(expression, memento, memento); } /** @@ -202,15 +233,15 @@ export class ProtoView { expression:AST, setterName:string, setter:SetterFn) { - this.protoRecordRange.addRecordsFromAST( - expression, - new DirectivePropertyMemento( - this.elementBinders.length-1, - directiveIndex, - setterName, - setter - ) + + var expMemento = new DirectivePropertyMemento( + this.elementBinders.length-1, + directiveIndex, + setterName, + setter ); + var groupMemento = DirectivePropertyGroupMemento.get(expMemento); + this.protoRecordRange.addRecordsFromAST(expression, expMemento, groupMemento, false); } static _createElementInjectors(elements, binders, hostElementInjector) { @@ -363,6 +394,43 @@ export class DirectivePropertyMemento { } } +var _groups = MapWrapper.create(); + +class DirectivePropertyGroupMemento { + _elementInjectorIndex:number; + _directiveIndex:number; + + constructor(elementInjectorIndex:number, directiveIndex:number) { + this._elementInjectorIndex = elementInjectorIndex; + this._directiveIndex = directiveIndex; + } + + static get(memento:DirectivePropertyMemento) { + var elementInjectorIndex = memento._elementInjectorIndex; + var directiveIndex = memento._directiveIndex; + var id = elementInjectorIndex * 100 + directiveIndex; + + if (! MapWrapper.contains(_groups, id)) { + return MapWrapper.set(_groups, id, new DirectivePropertyGroupMemento(elementInjectorIndex, directiveIndex)); + } + return MapWrapper.get(_groups, id); + } + + directive(elementInjectors:List) { + var elementInjector:ElementInjector = elementInjectors[this._elementInjectorIndex]; + return elementInjector.getAtIndex(this._directiveIndex); + } +} + +class PropertyUpdate { + currentValue; + previousValue; + + constructor(currentValue, previousValue) { + this.currentValue = currentValue; + this.previousValue = previousValue; + } +} //TODO(tbosch): I don't like to have done be called from a different place than notify diff --git a/modules/core/src/core.js b/modules/core/src/core.js index cd741494fb..024bc55b44 100644 --- a/modules/core/src/core.js +++ b/modules/core/src/core.js @@ -2,6 +2,7 @@ * Define public API for Angular here */ export * from './annotations/annotations'; +export * from './compiler/interfaces'; export * from './annotations/template_config'; export * from './application'; diff --git a/modules/core/test/compiler/view_spec.js b/modules/core/test/compiler/view_spec.js index 3c7940fc06..2fef685658 100644 --- a/modules/core/test/compiler/view_spec.js +++ b/modules/core/test/compiler/view_spec.js @@ -3,6 +3,7 @@ import {ProtoView, ElementPropertyMemento, DirectivePropertyMemento} from 'core/ import {ProtoElementInjector, ElementInjector} from 'core/compiler/element_injector'; import {DirectiveMetadataReader} from 'core/compiler/directive_metadata_reader'; import {Component, Decorator, Template} from 'core/annotations/annotations'; +import {OnChange} from 'core/core'; import {ProtoRecordRange} from 'change_detection/record_range'; import {ChangeDetector} from 'change_detection/change_detector'; import {TemplateConfig} from 'core/annotations/template_config'; @@ -281,7 +282,7 @@ export function main() { expect(view.bindElements[0].id).toEqual('buz'); }); - it('should consume directive watch expression change.', () => { + it('should consume directive watch expression change', () => { var pv = new ProtoView(createElement('
'), new ProtoRecordRange()); pv.bindElement(new ProtoElementInjector(null, 0, [SomeDirective])); @@ -292,6 +293,43 @@ export function main() { cd.detectChanges(); expect(view.elementInjectors[0].get(SomeDirective).prop).toEqual('buz'); }); + + it('should notify a directive about changes after all its properties have been set', () => { + var pv = new ProtoView(createElement('
'), + new ProtoRecordRange()); + + pv.bindElement(new ProtoElementInjector(null, 0, [DirectiveImplementingOnChange])); + pv.bindDirectiveProperty( 0, parser.parseBinding('a').ast, 'a', reflector.setter('a')); + pv.bindDirectiveProperty( 0, parser.parseBinding('b').ast, 'b', reflector.setter('b')); + createView(pv); + + ctx.a = 100; + ctx.b = 200; + cd.detectChanges(); + + var directive = view.elementInjectors[0].get(DirectiveImplementingOnChange); + expect(directive.c).toEqual(300); + }); + + it('should provide a map of updated properties', () => { + var pv = new ProtoView(createElement('
'), + new ProtoRecordRange()); + + pv.bindElement(new ProtoElementInjector(null, 0, [DirectiveImplementingOnChange])); + pv.bindDirectiveProperty( 0, parser.parseBinding('a').ast, 'a', reflector.setter('a')); + pv.bindDirectiveProperty( 0, parser.parseBinding('b').ast, 'b', reflector.setter('b')); + createView(pv); + ctx.a = 0; + ctx.b = 0; + cd.detectChanges(); + + ctx.a = 100; + cd.detectChanges(); + + var directive = view.elementInjectors[0].get(DirectiveImplementingOnChange); + expect(directive.changes["a"].currentValue).toEqual(100); + expect(directive.changes["b"]).not.toBeDefined(); + }); }); }); @@ -324,6 +362,18 @@ class SomeDirective { } } +class DirectiveImplementingOnChange extends OnChange { + a; + b; + c; + changes; + + onChange(changes) { + this.c = this.a + this.b; + this.changes = changes; + } +} + class SomeService {} @Component({ @@ -368,6 +418,8 @@ class AnotherDirective { class MyEvaluationContext { foo:string; + a; + b; constructor() { this.foo = 'bar'; };