From 5527a1b1a4069c88f6eb62560c5a14ce887d64bb Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Thu, 2 Oct 2014 15:14:32 +0200 Subject: [PATCH] feature(change detection): implement barebone ChangeDetector fixes #39 --- .../change_detection/src/change_detection.js | 24 ---- .../change_detection/src/change_detector.js | 26 ++++ modules/change_detection/src/facade.dart | 11 ++ modules/change_detection/src/facade.es6 | 6 + modules/change_detection/src/record.js | 134 +++++++++--------- modules/change_detection/src/watch_group.js | 61 +++++--- .../test/change_detection_spec.js | 45 ------ .../test/change_detector_spec.js | 64 +++++++++ modules/core/src/core.js | 2 +- 9 files changed, 214 insertions(+), 159 deletions(-) delete mode 100644 modules/change_detection/src/change_detection.js create mode 100644 modules/change_detection/src/change_detector.js delete mode 100644 modules/change_detection/test/change_detection_spec.js create mode 100644 modules/change_detection/test/change_detector_spec.js diff --git a/modules/change_detection/src/change_detection.js b/modules/change_detection/src/change_detection.js deleted file mode 100644 index 636d17c4b2..0000000000 --- a/modules/change_detection/src/change_detection.js +++ /dev/null @@ -1,24 +0,0 @@ -import {ProtoWatchGrou, WatchGroup} from './watch_group'; -import {ProtoRecord, Record} from './record'; -import {FIELD} from 'facade/lang'; -export * from './record'; -export * from './watch_group' - -export class ChangeDetection { - - @FIELD('final _rootWatchGroup:WatchGroup') - constructor(watchGroup:WatchGroup) { - this._rootWatchGroup = watchGroup; - } - - detectChanges():int { - var current:Record = _rootWatchGroup.headRecord; - var count:number = 0; - while (current != null) { - if (current.check()) { - count++; - } - } - return count; - } -} diff --git a/modules/change_detection/src/change_detector.js b/modules/change_detection/src/change_detector.js new file mode 100644 index 0000000000..a81b707f5c --- /dev/null +++ b/modules/change_detection/src/change_detector.js @@ -0,0 +1,26 @@ +import {ProtoWatchGroup, WatchGroup} from './watch_group'; +import {ProtoRecord, Record} from './record'; +import {FIELD, int} from 'facade/lang'; +export * from './record'; +export * from './watch_group' + +export class ChangeDetector { + + @FIELD('final _rootWatchGroup:WatchGroup') + constructor(watchGroup:WatchGroup) { + this._rootWatchGroup = watchGroup; + } + + detectChanges():int { + var record:Record = this._rootWatchGroup.headRecord; + var count:int = 0; + for (record = this._rootWatchGroup.headRecord; + record != null; + record = record.checkNext) { + if (record.check()) { + count++; + } + } + return count; + } +} diff --git a/modules/change_detection/src/facade.dart b/modules/change_detection/src/facade.dart index 41f3bcc83d..25d71bc945 100644 --- a/modules/change_detection/src/facade.dart +++ b/modules/change_detection/src/facade.dart @@ -1,3 +1,14 @@ library change_detection.facade; +@MirrorsUsed(targets: const [FieldGetterFactory], metaTargets: const [] ) +import 'dart:mirrors'; + typedef SetterFn(Object obj, value); + +class FieldGetterFactory { + getter(Object object, String name) { + Symbol symbol = new Symbol(name); + InstanceMirror instanceMirror = reflect(object); + return (Object object) => instanceMirror.getField(symbol).reflectee; + } +} diff --git a/modules/change_detection/src/facade.es6 b/modules/change_detection/src/facade.es6 index 35d97b6d2e..30add7e745 100644 --- a/modules/change_detection/src/facade.es6 +++ b/modules/change_detection/src/facade.es6 @@ -1 +1,7 @@ export var SetterFn = Function; + +export class FieldGetterFactory { + getter(object, name:string) { + return new Function('o', 'return o["' + name + '"]'); + } +} diff --git a/modules/change_detection/src/record.js b/modules/change_detection/src/record.js index 5813604aca..2c5bfebb13 100644 --- a/modules/change_detection/src/record.js +++ b/modules/change_detection/src/record.js @@ -1,48 +1,28 @@ -//import * as wg from './watch_group'; +import {ProtoWatchGroup, WatchGroup} from './watch_group'; import {FIELD} from 'facade/lang'; +import {FieldGetterFactory} from './facade'; /** - * For now we are dropping expression coelescence. We can always add it later, but - * real world numbers should that it does not provide significant benefits. + * For now we are dropping expression coalescence. We can always add it later, but + * real world numbers show that it does not provide significant benefits. */ export class ProtoRecord { - @FIELD('final watchGroup:wg.ProtoWatchGroup') @FIELD('final fieldName:String') /// order list of all records. Including head/tail markers @FIELD('next:ProtoRecord') @FIELD('prev:ProtoRecord') - // Opeque data which will be the target of notification. - // If the object is instance of Record, than it it is directly procssed + // Opaque data which will be the target of notification. + // If the object is instance of Record, than it it is directly processed // Otherwise it is the context used by WatchGroupDispatcher. @FIELD('memento') - @FIELD('_clone') - constructor(watchGroup/*:wg.ProtoWatchGroup*/, fieldName:String, memento) { + constructor(watchGroup:ProtoWatchGroup, fieldName:string, dispatchMemento) { this.watchGroup = watchGroup; this.fieldName = fieldName; - this.memento = memento; + this.dispatchMemento = dispatchMemento; this.next = null; this.prev = null; - this.changeNotifier = null; - this._clone = null; - this.changeContext = null; - this.dispatcherContext = null; } - - instantiate(watchGroup/*:wg.WatchGroup*/):Record { - var record = this._clone = new Record(watchGroup, this); - record.prev = this.prev._clone; - record._checkPrev = this.prev._clone; - return _clone; - } - - instantiateComplete():Record { - var record = this._clone; - record.next = this.next._clone; - record._checkNext = this.next._clone; - this._clone = null; - return this.next; - } } @@ -59,11 +39,8 @@ export class ProtoRecord { * - Atomic watch operations * - Defaults to dirty checking * - Keep this object as lean as possible. (Lean in number of fields) - * - * MEMORY COST: 13 Words; */ export class Record { - @FIELD('final watchGroup:WatchGroup') @FIELD('final protoRecord:ProtoRecord') /// order list of all records. Including head/tail markers @@ -79,64 +56,93 @@ export class Record { @FIELD('_context') @FIELD('_getter') @FIELD('_arguments') - @FIELD('currentValue') @FIELD('previousValue') constructor(watchGroup/*:wg.WatchGroup*/, protoRecord:ProtoRecord) { this.protoRecord = protoRecord; this.watchGroup = watchGroup; this.next = null; this.prev = null; - this._checkNext = null; - this._checkPrev = null; - this._notifierNext = null; + this.checkNext = null; + this.checkPrev = null; + this.notifierNext = null; - this._mode = MODE_STATE_MARKER; - this._context = null; - this._getter = null; - this._arguments = null; - this.currentValue = null; + this.mode = MODE_STATE_MARKER; + this.context = null; + this.getter = null; + this.arguments = null; this.previousValue = null; + this.currentValue = null; } - check():bool { - var mode = this._mode; + check():boolean { + var mode = this.mode; var state = mode & MODE_MASK_STATE; var notify = mode & MODE_MASK_NOTIFY; - var currentValue; + var newValue; switch (state) { case MODE_STATE_MARKER: return false; case MODE_STATE_PROPERTY: - currentValue = this._getter(this._context); + newValue = this.getter(this.context); break; case MODE_STATE_INVOKE_CLOSURE: - currentValue = this._context(this._arguments); + newValue = this.context(this.arguments); break; case MODE_STATE_INVOKE_METHOD: - currentValue = this._getter(this._context, this._arguments); + newValue = this.getter(this.context, this.arguments); break; case MODE_STATE_MAP: + throw 'not implemented'; case MODE_STATE_LIST: + throw 'not implemented'; + default: + throw 'not implemented'; } - var previousValue = this.previousValue; - if (isSame(previousValue, currentValue)) return false; - if (previousValue instanceof String && currentValue instanceof String - && previousValue == currentValue) { - this.previousValue = currentValue; - return false - } - this.previousValue = currentValue; - if (this.protoRecord.changeContext instanceof ProtoRecord) { - // forward propaget to the next record + + + var previousValue = this.currentValue; + if (previousValue === this) { + // When the record is checked for the first time we should always notify + this.currentValue = newValue; + this.previousValue = previousValue = null; } else { - // notify throught dispatcher - this.watchGroup.dispatcher.onRecordChange(this, this.protoRecord.dispatcherContext); + this.currentValue = newValue; + this.previousValue = previousValue; + + if (isSame(previousValue, newValue)) return false; + + // In Dart, we can have `str1 !== str2` but `str1 == str2` + if (previousValue instanceof String && + newValue instanceof String && + previousValue == newValue) { + return false + } } + + + // todo(vicb): compute this info only once in ctor ? (add a bit in mode not to grow the mem req) + if (this.protoRecord.dispatchMemento === null) { + // forward propagate to the next record + } else { + // notify through dispatcher + this.watchGroup.dispatcher.onRecordChange(this, this.protoRecord.dispatchMemento); + } + return true; } + + setContext(context) { + // use `this` as a marker for a fresh record + this.currentValue = this; + this.mode = MODE_STATE_PROPERTY; + this.context = context; + var factory = new FieldGetterFactory(); + this.getter = factory.getter(context, this.protoRecord.fieldName); + } + } -// The mode is devided into two partes. Which notification mechanism +// The mode is divided into two parts. Which notification mechanism // to use and which dereference mode to execute. // We use dirty checking aka no notification @@ -160,11 +166,7 @@ const MODE_STATE_MAP = 0x0004; const MODE_STATE_LIST = 0x0005; function isSame(a, b) { - if (a === b) { - return true; - } else if ((a !== a) && (b !== b)) { - return true; - } else { - return false; - } + if (a === b) return true; + if ((a !== a) && (b !== b)) return true; + return false; } diff --git a/modules/change_detection/src/watch_group.js b/modules/change_detection/src/watch_group.js index 1159d5665e..9c202c50f8 100644 --- a/modules/change_detection/src/watch_group.js +++ b/modules/change_detection/src/watch_group.js @@ -13,38 +13,46 @@ export class ProtoWatchGroup { * Parses [expression] into [ProtoRecord]s and adds them to [ProtoWatchGroup]. * * @param expression The expression to watch - * @param memento an opeque object which will be bassed to WatchGroupDispatcher on + * @param memento an opaque object which will be passed to WatchGroupDispatcher on * detecting a change. * @param shallow Should collections be shallow watched */ - watch( - expression:String, - memento, - {shallow/*=false*/}:{shallow:bool}) + watch(expression:string, + memento, + shallow /*= false*/) // TODO(vicb): comment out when opt-params are supported { - /// IMPLEMENT + var protoRecord = new ProtoRecord(this, expression, memento); + + if (this.headRecord === null) { + this.headRecord = this.tailRecord = protoRecord; + } else { + this.tailRecord.next = protoRecord; + protoRecord.prev = this.tailRecord; + this.tailRecord = protoRecord; + } } instantiate(dispatcher:WatchGroupDispatcher):WatchGroup { var watchGroup:WatchGroup = new WatchGroup(this, dispatcher); - var head:Record = null; var tail:Record = null; - var proto:ProtoRecord = this.headRecord; + var proto:ProtoRecord; + var prevRecord:Record = null; - while(proto != null) { - tail = proto.instantiate(watchGroup); - if (head == null) head = tail; - proto = proto.next; + if (this.headRecord !== null) { + watchGroup.headRecord = tail = new Record(watchGroup, this.headRecord); + + for (proto = this.headRecord.next; proto != null; proto = proto.next) { + prevRecord = tail; + tail = new Record(watchGroup, proto); + tail.prev = prevRecord; + prevRecord.next = tail; + tail.checkPrev = prevRecord; + prevRecord.checkNext = tail; + } + + watchGroup.tailRecord = tail; } - proto = this.headRecord; - while(proto != null) { - proto.instantiateComplete(); - proto = proto.next; - } - - watchGroup.headRecord = head; - watchGroup.tailRecord = tail; return watchGroup; } @@ -60,14 +68,15 @@ export class WatchGroup { this.dispatcher = dispatcher; this.headRecord = null; this.tailRecord = null; + this.context = null; } insertChildGroup(newChild:WatchGroup, insertAfter:WatchGroup) { - /// IMPLEMENT + throw 'not implemented'; } remove() { - /// IMPLEMENT + throw 'not implemented'; } /** @@ -75,12 +84,18 @@ export class WatchGroup { * dereference themselves on. Since the WatchGroup can be reused the context * can be re-set many times during the lifetime of the WatchGroup. * - * @param context the new context for change dection for the curren WatchGroup + * @param context the new context for change detection for the current WatchGroup */ setContext(context) { + for (var record:Record = this.headRecord; + record != null; + record = record.next) { + record.setContext(context); + } } } export class WatchGroupDispatcher { + // The record holds the previous value at the time of the call onRecordChange(record:Record, context) {} } diff --git a/modules/change_detection/test/change_detection_spec.js b/modules/change_detection/test/change_detection_spec.js deleted file mode 100644 index 07da7050dd..0000000000 --- a/modules/change_detection/test/change_detection_spec.js +++ /dev/null @@ -1,45 +0,0 @@ -import {describe, it, xit, expect} from 'test_lib/test_lib'; -import {ProtoWatchGroup, WatchGroup, WatchGroupDispatcher, ChangeDetection} from 'change_detection/change_detection'; - - -export function main() { - describe('change_detection', function() { - describe('ChangeDetection', function() { - xit('should do simple watching', function() { - var person = new Person('misko', 38); - var pwg = new ProtoWatchGroup(); - pwg.watch('name', 'nameToken'); - pwg.watch('age', 'ageToken'); - var dispatcher = new LoggingDispatcher(); - var wg = pwg.instantiate(dispatcher); - wg.setContext(person); - var cd = new ChangeDetection(wg); - cd.detectChanges(); - expect(dispatcher.log).toEqual(['ageToken=38']); - dispatcher.clear(); - cd.detectChanges(); - expect(dispatcher.log).toEqual([]); - person.age=1; - person.name="Misko"; - cd.detectChanges(); - expect(dispatcher.log).toEqual(['nameToken=Misko', 'ageToken=1']); - }); - }); - }); -} - -class Person { - constructor(name:string, age:number) { - this.name = name; - this.age = age; - } -} - -class LoggingDispatcher extends WatchGroupDispatcher { - constructor() { - this.log = null; - } - clear() { - - } -} diff --git a/modules/change_detection/test/change_detector_spec.js b/modules/change_detection/test/change_detector_spec.js new file mode 100644 index 0000000000..91686c5fee --- /dev/null +++ b/modules/change_detection/test/change_detector_spec.js @@ -0,0 +1,64 @@ +import {describe, it, xit, expect} from 'test_lib/test_lib'; + +import {List, ListWrapper} from 'facade/collection'; + +import { + ChangeDetector, + ProtoWatchGroup, + WatchGroup, + WatchGroupDispatcher +} from 'change_detection/change_detector'; + +import {Record} from 'change_detection/record'; + +export function main() { + describe('change_detection', function() { + describe('ChangeDetection', function() { + it('should do simple watching', function() { + var person = new Person('misko', 38); + var pwg = new ProtoWatchGroup(); + pwg.watch('name', 'name', false); // TODO(vicb): remove opt shallow when supported + pwg.watch('age', 'age', false); + var dispatcher = new LoggingDispatcher(); + var wg = pwg.instantiate(dispatcher); + wg.setContext(person); + var cd = new ChangeDetector(wg); + cd.detectChanges(); + expect(dispatcher.log).toEqual(['name=misko', 'age=38']); + dispatcher.clear(); + cd.detectChanges(); + expect(dispatcher.log).toEqual([]); + person.age = 1; + person.name = "Misko"; + cd.detectChanges(); + expect(dispatcher.log).toEqual(['name=Misko', 'age=1']); + }); + }); + }); +} + +class Person { + constructor(name:string, age:number) { + this.name = name; + this.age = age; + } + + toString() { + return 'name=' + this.name + ' age=' + this.age.toString(); + } +} + +class LoggingDispatcher extends WatchGroupDispatcher { + constructor() { + this.log = null; + this.clear(); + } + + clear() { + this.log = ListWrapper.create(); + } + + onRecordChange(record:Record, context) { + ListWrapper.push(this.log, context + '=' + record.currentValue.toString()); + } +} diff --git a/modules/core/src/core.js b/modules/core/src/core.js index 9163021684..d4cea0732c 100644 --- a/modules/core/src/core.js +++ b/modules/core/src/core.js @@ -5,7 +5,7 @@ export * from './annotations/directive'; export * from './annotations/component'; export * from './annotations/template_config'; -export * from 'change_detection/change_detection'; +export * from 'change_detection/change_detector'; export * from 'change_detection/watch_group'; export * from 'change_detection/record';