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..07eb8a797d663daff88a35c29861e5929e1a1c8f 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 {

[Jennifer Messerly, 2013/07/19 01:32:58]

this is just a normal observable object now, instead of having a custom value
stream.

   /** 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;
+  List<Object> _values;

[Jennifer Messerly, 2013/07/19 01:32:58]

I completely redid this implementation using arrays instead of the linked list
of PropertyObservers. The old way had the downside of each link storing both its
object and cached value. Hopefully new impl is simpler and easier to follow!

+  List<Object> _segments;
+  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)
+  PathObserver(Object 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);
+    _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\$', caseSensitive: false);
+}();
 
 final _spacesRegExp = new RegExp(r'\s');