port TemplateBinding to ed3266266e751b5ab1f75f8e0509d0d5f0ef35d8

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 1b6e1a34dc1a80b7638bf918a0897b763616e81a..4d9cb330b261773a031cc7cb3237d02ae76cbf67 100644
--- a/pkg/observe/lib/src/path_observer.dart
+++ b/pkg/observe/lib/src/path_observer.dart
@@ -21,6 +21,9 @@ part of observe;
  *
  * This class is used to implement [Node.bind] and similar functionality.
  */
+// TODO(jmesserly): consider specialized subclasses for:
+// * single token in path, e.g. "foo"
+// * specifically: "value"

[Siggi Cherem (dart-lang), 2013/10/29 21:00:07]

+1

[Jennifer Messerly, 2013/10/29 22:35:07]

Done.

 class PathObserver extends ChangeNotifier {
   /** The path string. */
   final String path;
@@ -32,13 +35,19 @@ class PathObserver extends ChangeNotifier {
   List<Object> _values;
   List<StreamSubscription> _subs;
 
+  // TODO(jmesserly): I'm not sure about these APIs. They do increase
+  // performance but at the cost of complexity and perhaps having too much
+  // functionality inside PathObserver.

[Siggi Cherem (dart-lang), 2013/10/29 21:00:07]

I think it's worth it. IIUC, if we have a compile-time pass that creates
functions to read values without mirrors, we can plug them here?

[Jennifer Messerly, 2013/10/29 22:35:07]

agree. removed the TODO. original it was getValue/setValue and valueCallback
which felt too much. but just getValue isn't so bad.

+  final Function _getValue;
+
   /**
    * 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.bindSync] and [PathObserver.value].

[Siggi Cherem (dart-lang), 2013/10/29 21:00:07]

add short description for getValue?

[Jennifer Messerly, 2013/10/29 22:35:07]

Done.

    */
-  PathObserver(Object object, String path)
+  PathObserver(Object object, String path, {getValue(x)})
       : path = path,
+        _getValue = getValue,
         _isValid = _isPathValid(path),
         _segments = <Object>[] {
 
@@ -54,6 +63,8 @@ class PathObserver extends ChangeNotifier {
     // 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);
+
+    if (_segments.isEmpty && getValue != null) object = getValue(object);

[Siggi Cherem (dart-lang), 2013/10/29 21:00:07]

reading this and the lines below it's a bit confusing. Might help to add a
typedef for the type of getValue to clarify a bit. For example, is the argument
the old value? 

[Jennifer Messerly, 2013/10/29 22:35:07]

tried to clarify in a comment. changed argument name to newValue

     _values[0] = object;
     _subs = new List<StreamSubscription>(_segments.length);
   }
@@ -69,7 +80,7 @@ class PathObserver extends ChangeNotifier {
   }
 
   /** Sets the value at this path. */
-  @reflectable void set value(Object value) {
+  @reflectable void set value(Object newValue) {
     int len = _segments.length;
 
     // TODO(jmesserly): throw if property cannot be set?
@@ -77,11 +88,11 @@ class PathObserver extends ChangeNotifier {
     if (len == 0) return;
     if (!hasObservers) _updateValues();
 
-    if (_setObjectProperty(_values[len - 1], _segments[len - 1], value)) {
+    if (_setObjectProperty(_values[len - 1], _segments[len - 1], newValue)) {
       // 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;
+      _values[len] = newValue;
     }
   }
 
@@ -113,16 +124,19 @@ class PathObserver extends ChangeNotifier {
 
   // 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]);
+    for (int i = 0, last = _segments.length - 1; i <= last; i++) {
+      var newValue = _getObjectProperty(_values[i], _segments[i]);
+      if (i == last && _getValue != null) newValue = _getValue(newValue);
+      _values[i + 1] = newValue;
     }
   }
 
   void _updateObservedValues([int start = 0]) {
     var oldValue, newValue;
-    for (int i = start; i < _segments.length; i++) {
+    for (int i = start, last = _segments.length - 1; i <= last; i++) {
       oldValue = _values[i + 1];
       newValue = _getObjectProperty(_values[i], _segments[i]);
+      if (i == last && _getValue != null) newValue = _getValue(newValue);
       if (identical(oldValue, newValue)) {
         _observePath(start, i);
         return;
@@ -147,16 +161,11 @@ class PathObserver extends ChangeNotifier {
     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.
+      // property, we could try and have one for the entire path. However we'd
+      // need to do a linear scan to find the index as soon as we got a change.
+      // Also we need to fix ListChangeRecord and MapChangeRecord to contain
+      // the target. Not sure if it's worth it.
       _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 (_changeRecordMatches(record, _segments[i])) {
             _updateObservedValues(i);
@@ -201,11 +210,14 @@ _getObjectProperty(object, property) {
     if (result != null) return result.reflectee;
   }
 
+  // TODO(jmesserly): need to fix NoSuchMethodErrors from _tryGetField; right
+  // now it's painful to debug (and probably really slow) to use maps.
   if (object is Map) {
     if (property is Symbol) property = MirrorSystem.getName(property);
     return object[property];
   }
 
+  // TODO(jmesserly): log a binding that fails?
   return null;
 }