implement dirty checking for @observable objects

pkg/observe/lib/src/path_observer.dart

Index: pkg/observe/lib/src/path_observer.dart
diff --git a/pkg/observe/lib/src/path_observer.dart b/pkg/observe/lib/src/path_observer.dart
index 7f26c3958afc60f6e0f57436bd0e56d931125899..978af7e525eeb610996fdefa8de8335223f69db6 100644
--- a/pkg/observe/lib/src/path_observer.dart
+++ b/pkg/observe/lib/src/path_observer.dart
@@ -10,24 +10,6 @@ part of observe;
 // ChangeSummary, we just implement what we need for data bindings.
 // This allows our implementation to be much simpler.
 
-// TODO(jmesserly): should we make these types stronger, and require
-// Observable objects? Currently, it is fine to say something like:
-//     var path = new PathObserver(123, '');
-//     print(path.value); // "123"
-//
-// Furthermore this degenerate case is allowed:
-//     var path = new PathObserver(123, 'foo.bar.baz.qux');
-//     print(path.value); // "null"
-//
-// Here we see that any invalid (i.e. not Observable) value will break the
-// path chain without producing an error or exception.
-//
-// Now the real question: should we do this? For the former case, the behavior
-// is correct but we could chose to handle it in the dart:html bindings layer.
-// For the latter case, it might be better to throw an error so users can find
-// the problem.
-
-
 /**
  * A data-bound path starting from a view-model or model object, for example
  * `foo.bar.baz`.
@@ -40,133 +22,151 @@ part of observe;
  * This class is used to implement [Node.bind] and similar functionality.
  */
 // TODO(jmesserly): find a better home for this type.
-class PathObserver {
-  /** The object being observed. */
-  final object;
-
+class PathObserver extends ChangeNotifierBase {
   /** The path string. */
   final String path;
 
   /** True if the path is valid, otherwise false. */
   final bool _isValid;
 
-  // TODO(jmesserly): same issue here as ObservableMixin: is there an easier
-  // way to get a broadcast stream?
-  StreamController _values;
-  Stream _valueStream;
-
-  _PropertyObserver _observer, _lastObserver;
-
-  Object _lastValue;
-  bool _scheduled = false;
+  final List<Object> _segments;
+  List<Object> _values;
+  List<StreamSubscription> _subs;
 
   /**
    * Observes [path] on [object] for changes. This returns an object that can be
    * used to get the changes and get/set the value at this path.
-   * See [PathObserver.values] and [PathObserver.value].
+   * See [PathObserver.bindSync] and [PathObserver.value].
    */
-  PathObserver(this.object, String path)
-    : path = path, _isValid = _isPathValid(path) {
-
-    // TODO(jmesserly): if the path is empty, or the object is! Observable, we
-    // can optimize the PathObserver to be more lightweight.
-
-    _values = new StreamController.broadcast(sync: true,
-                                             onListen: _observe,
-                                             onCancel: _unobserve);
+  PathObserver(Object object, String path)
+      : path = path,
+        _isValid = _isPathValid(path),
+        _segments = <Object>[] {
 
     if (_isValid) {
-      var segments = [];
       for (var segment in path.trim().split('.')) {
         if (segment == '') continue;
-        var index = int.parse(segment, onError: (_) {});
-        segments.add(index != null ? index : new Symbol(segment));
+        var index = int.parse(segment, onError: (_) => null);
+        _segments.add(index != null ? index : new Symbol(segment));
       }
+    }
 
-      // Create the property observer linked list.
-      // Note that the structure of a path can't change after it is initially
-      // constructed, even though the objects along the path can change.
-      for (int i = segments.length - 1; i >= 0; i--) {
-        _observer = new _PropertyObserver(this, segments[i], _observer);
-        if (_lastObserver == null) _lastObserver = _observer;
-      }
+    // Initialize arrays.
+    // Note that the path itself can't change after it is initially
+    // constructed, even though the objects along the path can change.
+    _values = new List<Object>(_segments.length + 1);
+    _values[0] = object;
+    _subs = new List<StreamSubscription>(_segments.length);
+  }
+
+  /** The object being observed. */
+  get object => _values[0];
+
+  /** Gets the last reported value at this path. */
+  get value {
+    if (!_isValid) return null;
+    if (!hasObservers) _updateValues();
+    return _values.last;
+  }
+
+  /** Sets the value at this path. */
+  void set value(Object value) {
+    int len = _segments.length;
+
+    // TODO(jmesserly): throw if property cannot be set?
+    // MDV seems tolerant of these errors.
+    if (len == 0) return;
+    if (!hasObservers) _updateValues();
+
+    if (_setObjectProperty(_values[len - 1], _segments[len - 1], value)) {
+      // Technically, this would get updated asynchronously via a change record.
+      // However, it is nice if calling the getter will yield the same value
+      // that was just set. So we use this opportunity to update our cache.
+      _values[len] = value;
     }
   }
 
-  // TODO(jmesserly): we could try adding the first value to the stream, but
-  // that delivers the first record async.
   /**
-   * Listens to the stream, and invokes the [callback] immediately with the
-   * current [value]. This is useful for bindings, which want to be up-to-date
-   * immediately.
+   * Invokes the [callback] immediately with the current [value], and every time
+   * the value changes. This is useful for bindings, which want to be up-to-date
+   * immediately and stay bound to the value of the path.
    */
   StreamSubscription bindSync(void callback(value)) {
-    var result = values.listen(callback);
+    var result = changes.listen((records) { callback(value); });
     callback(value);
     return result;
   }
 
-  // TODO(jmesserly): should this be a change record with the old value?
-  // TODO(jmesserly): should this be a broadcast stream? We only need
-  // single-subscription in the bindings system, so single sub saves overhead.
-  /**
-   * Gets the stream of values that were observed at this path.
-   * This returns a single-subscription stream.
-   */
-  Stream get values => _values.stream;
-
-  /** Force synchronous delivery of [values]. */
-  void _deliverValues() {
-    _scheduled = false;
-
-    var newValue = value;
-    if (!identical(_lastValue, newValue)) {
-      _values.add(newValue);
-      _lastValue = newValue;
-    }
+  void _observed() {
+    super._observed();
+    _updateValues();
+    _observePath();
   }
 
-  void _observe() {
-    if (_observer != null) {
-      _lastValue = value;
-      _observer.observe();
+  void _unobserved() {
+    for (int i = 0; i < _subs.length; i++) {
+      if (_subs[i] != null) {
+        _subs[i].cancel();
+        _subs[i] = null;
+      }
     }
   }
 
-  void _unobserve() {
-    if (_observer != null) _observer.unobserve();
+  // TODO(jmesserly): should we be caching these values if not observing?
+  void _updateValues() {
+    for (int i = 0; i < _segments.length; i++) {
+      _values[i + 1] = _getObjectProperty(_values[i], _segments[i]);
+    }
   }
 
-  void _notifyChange() {
-    if (_scheduled) return;
-    _scheduled = true;
+  void _updateObservedValues([int start = 0]) {
+    bool changed = false;
+    for (int i = start; i < _segments.length; i++) {
+      final newValue = _getObjectProperty(_values[i], _segments[i]);
+      if (identical(_values[i + 1], newValue)) {
+        _observePath(start, i);
+        return;
+      }
+      _values[i + 1] = newValue;
+      changed = true;
+    }
 
-    // TODO(jmesserly): should we have a guarenteed order with respect to other
-    // paths? If so, we could implement this fairly easily by sorting instances
-    // of this class by birth order before delivery.
-    queueChangeRecords(_deliverValues);
+    _observePath(start);
+    if (changed) {
+      notifyChange(new PropertyChangeRecord(const Symbol('value')));
+    }
   }
 
-  /** Gets the last reported value at this path. */
-  get value {
-    if (!_isValid) return null;
-    if (_observer == null) return object;
-    _observer.ensureValue(object);
-    return _lastObserver.value;
+  void _observePath([int start = 0, int end]) {
+    if (end == null) end = _segments.length;
+
+    for (int i = start; i < end; i++) {
+      if (_subs[i] != null) _subs[i].cancel();
+      _observeIndex(i);
+    }
   }
 
-  /** Sets the value at this path. */
-  void set value(Object value) {
-    // TODO(jmesserly): throw if property cannot be set?
-    // MDV seems tolerant of these error.
-    if (_observer == null || !_isValid) return;
-    _observer.ensureValue(object);
-    var last = _lastObserver;
-    if (_setObjectProperty(last._object, last._property, value)) {
-      // Technically, this would get updated asynchronously via a change record.
-      // However, it is nice if calling the getter will yield the same value
-      // that was just set. So we use this opportunity to update our cache.
-      last.value = value;
+  void _observeIndex(int i) {
+    final object = _values[i];
+    if (object is Observable) {
+      // TODO(jmesserly): rather than allocating a new closure for each
+      // property, we could try and have one for the entire path. In that case,
+      // we would lose information about which object changed (note: unless
+      // PropertyChangeRecord is modified to includes the sender object), so
+      // we would need to re-evaluate the entire path. Need to evaluate perf.
+      _subs[i] = object.changes.listen((List<ChangeRecord> records) {
+        if (!identical(_values[i], object)) {
+          // Ignore this object if we're now tracking something else.
+          return;
+        }
+
+        for (var record in records) {
+          if (record.changes(_segments[i])) {
+            _updateObservedValues(i);
+            return;
+          }
+        }
+      });
     }
   }
 }
@@ -220,78 +220,18 @@ bool _setObjectProperty(object, property, value) {
   return false;
 }
 
-class _PropertyObserver {
-  final PathObserver _path;
-  final _property;
-  final _PropertyObserver _next;
-
-  // TODO(jmesserly): would be nice not to store both of these.
-  Object _object;
-  Object _value;
-  StreamSubscription _sub;
-
-  _PropertyObserver(this._path, this._property, this._next);
-
-  get value => _value;
-
-  void set value(Object newValue) {
-    _value = newValue;
-    if (_next != null) {
-      if (_sub != null) _next.unobserve();
-      _next.ensureValue(_value);
-      if (_sub != null) _next.observe();
-    }
-  }
-
-  void ensureValue(object) {
-    // If we're observing, values should be up to date already.
-    if (_sub != null) return;
-
-    _object = object;
-    value = _getObjectProperty(object, _property);
-  }
-
-  void observe() {
-    if (_object is Observable) {
-      assert(_sub == null);
-      _sub = (_object as Observable).changes.listen(_onChange);
-    }
-    if (_next != null) _next.observe();
-  }
-
-  void unobserve() {
-    if (_sub == null) return;
-
-    _sub.cancel();
-    _sub = null;
-    if (_next != null) _next.unobserve();
-  }
-
-  void _onChange(List<ChangeRecord> changes) {
-    for (var change in changes) {
-      // TODO(jmesserly): what to do about "new Symbol" here?
-      // Ideally this would only preserve names if the user has opted in to
-      // them being preserved.
-      // TODO(jmesserly): should we drop observable maps with String keys?
-      // If so then we only need one check here.
-      if (change.changes(_property)) {
-        value = _getObjectProperty(_object, _property);
-        _path._notifyChange();
-        return;
-      }
-    }
-  }
-}
 
 // From: https://github.com/rafaelw/ChangeSummary/blob/master/change_summary.js
 
-const _pathIndentPart = r'[$a-z0-9_]+[$a-z0-9_\d]*';
-final _pathRegExp = new RegExp('^'
-    '(?:#?' + _pathIndentPart + ')?'
-    '(?:'
-      '(?:\\.' + _pathIndentPart + ')'
-    ')*'
-    r'$', caseSensitive: false);
+final _pathRegExp = () {
+  const identStart = '[\$_a-zA-Z]';
+  const identPart = '[\$_a-zA-Z0-9]';
+  const ident = '$identStart+$identPart*';
+  const elementIndex = '(?:[0-9]|[1-9]+[0-9]+)';
+  const identOrElementIndex = '(?:$ident|$elementIndex)';
+  const path = '(?:$identOrElementIndex)(?:\\.$identOrElementIndex)*';
+  return new RegExp('^$path\$');
+}();
 
 final _spacesRegExp = new RegExp(r'\s');