Change how events are handled in Effen

sky/framework/fn.dart

Index: sky/framework/fn.dart
diff --git a/sky/framework/fn.dart b/sky/framework/fn.dart
index 4a0f980a049295cd7f4bc2d7866185907ed38b4a..8288d83862620abbfe8f53252b2f94bf66a2d6aa 100644
--- a/sky/framework/fn.dart
+++ b/sky/framework/fn.dart
@@ -22,26 +22,6 @@ bool _initIsInCheckedMode() {
 final bool _isInCheckedMode = _initIsInCheckedMode();
 final bool _shouldLogRenderDuration = false;
 
-class EventHandler {
-  final String type;
-  final sky.EventListener listener;
-
-  EventHandler(this.type, this.listener);
-}
-
-class EventMap {
-  final List<EventHandler> _handlers = new List<EventHandler>();
-
-  void listen(String type, sky.EventListener listener) {
-    assert(listener != null);
-    _handlers.add(new EventHandler(type, listener));
-  }
-
-  void addAll(EventMap events) {
-    _handlers.addAll(events._handlers);
-  }
-}
-
 class Style {
   final String _className;
   static final Map<String, Style> _cache = new HashMap<String, Style>();
@@ -91,10 +71,6 @@ abstract class Node {
   sky.Node _root;
   bool _defunct = false;
 
-  // TODO(abarth): Both Elements and Components have |events| but |Text|
-  // doesn't. Should we add a common base class to contain |events|?
-  final EventMap events = new EventMap();
-
   Node({ Object key }) {
     _key = key == null ? "$runtimeType" : "$runtimeType-$key";
   }
@@ -138,51 +114,6 @@ abstract class Node {
     assert(node._root is sky.Node);
     return node;
   }
-
-  void _syncEvents(EventMap oldEventMap) {
-    List<EventHandler> newHandlers = events._handlers;
-    int newStartIndex = 0;
-    int newEndIndex = newHandlers.length;
-
-    List<EventHandler> oldHandlers = oldEventMap._handlers;
-    int oldStartIndex = 0;
-    int oldEndIndex = oldHandlers.length;
-
-    // Skip over leading handlers that match.
-    while (newStartIndex < newEndIndex && oldStartIndex < oldEndIndex) {
-      EventHandler newHandler = newHandlers[newStartIndex];
-      EventHandler oldHandler = oldHandlers[oldStartIndex];
-      if (newHandler.type != oldHandler.type
-          || newHandler.listener != oldHandler.listener)
-        break;
-      ++newStartIndex;
-      ++oldStartIndex;
-    }
-
-    // Skip over trailing handlers that match.
-    while (newStartIndex < newEndIndex && oldStartIndex < oldEndIndex) {
-      EventHandler newHandler = newHandlers[newEndIndex - 1];
-      EventHandler oldHandler = oldHandlers[oldEndIndex - 1];
-      if (newHandler.type != oldHandler.type
-          || newHandler.listener != oldHandler.listener)
-        break;
-      --newEndIndex;
-      --oldEndIndex;
-    }
-
-    sky.Element root = _root as sky.Element;
-
-    for (int i = oldStartIndex; i < oldEndIndex; ++i) {
-      EventHandler oldHandler = oldHandlers[i];
-      root.removeEventListener(oldHandler.type, oldHandler.listener);
-    }
-
-    for (int i = newStartIndex; i < newEndIndex; ++i) {
-      EventHandler newHandler = newHandlers[i];
-      root.addEventListener(newHandler.type, newHandler.listener);
-    }
-  }
-
 }
 
 /*
@@ -193,6 +124,11 @@ abstract class Node {
  */
 abstract class RenderNode extends Node {
 
+  static Map<sky.Node, RenderNode> _nodeMap =

[abarth-chromium, 2015/03/19 14:19:59]

final

[rafaelw, 2015/03/19 15:03:50]

Done.

+      new HashMap<sky.Node, RenderNode>();
+
+  static RenderNode _getMounted(sky.Node node) => _nodeMap[node];
+
   RenderNode({ Object key }) : super(key: key);
 
   RenderNode get _emptyNode;
@@ -208,6 +144,7 @@ abstract class RenderNode extends Node {
       _root = old._root;
     }
 
+    _nodeMap[_root] = this;
     _syncNode(old);
   }
 
@@ -216,6 +153,132 @@ abstract class RenderNode extends Node {
   void _remove() {
     assert(_root != null);
     _root.remove();
+    _nodeMap.remove(_root);
+    super._remove();
+  }
+}
+
+typedef GestureEventListener(sky.GestureEvent e);
+typedef PointerEventListener(sky.PointerEvent e);
+typedef EventListener(sky.Event e);
+
+class EventTarget extends Node  {
+  Node content;
+  final Map<String, sky.EventListener> listeners;
+  bool _docListenersRegistered = false;

[abarth-chromium, 2015/03/19 14:19:59]

style nit: _docListenersRegistered -> _haveRegisteredListenersOnDocument ?

[rafaelw, 2015/03/19 15:03:50]

Removed this.

+
+  static final Set<String> _registeredEvents = new HashSet<String>();
+
+  static Map<String, sky.EventListener> _createListeners({
+    EventListener onWheel,
+    GestureEventListener onGestureFlingCancel,
+    GestureEventListener onGestureFlingStart,
+    GestureEventListener onGestureScrollStart,
+    GestureEventListener onGestureScrollUpdate,
+    GestureEventListener onGestureTap,
+    PointerEventListener onPointerCancel,
+    PointerEventListener onPointerDown,
+    PointerEventListener onPointerMove,
+    PointerEventListener onPointerUp,
+    Map<String, sky.EventListener> custom
+  }) {
+    var listeners = custom != null ? custom
+        : new HashMap<String, sky.EventListener>();
+
+    if (onWheel != null)
+      listeners['wheel'] = onWheel;

[abarth-chromium, 2015/03/19 14:19:59]

This if-cascade is kind of ugly, but I think you're right that it's valuable to
have the shorthands for the common events.

[rafaelw, 2015/03/19 15:03:50]

Yeah. We could also do this with reflection (i.e. create an annotation with the
event names on the handlers), but I'm not sure that's much better.

+    if (onGestureFlingCancel != null)
+      listeners['gestureflingcancel'] = onGestureFlingCancel;
+    if (onGestureFlingStart != null)
+      listeners['gestureflingstart'] = onGestureFlingStart;
+    if (onGestureScrollStart != null)
+      listeners['gesturescrollstart'] = onGestureScrollStart;
+    if (onGestureScrollUpdate != null)
+      listeners['gesturescrollupdate'] = onGestureScrollUpdate;
+    if (onGestureTap != null)
+      listeners['gesturetap'] = onGestureTap;
+    if (onPointerCancel != null)
+      listeners['pointercancel'] = onPointerCancel;
+    if (onPointerDown != null)
+      listeners['pointerdown'] = onPointerDown;
+    if (onPointerMove != null)
+      listeners['pointermove'] = onPointerMove;
+    if (onPointerUp != null)
+      listeners['pointerup'] = onPointerUp;

[abarth-chromium, 2015/03/19 14:19:59]

Do you actually want to mutate the |custom| argument?  You might want to make a
copy of the map before mutating it.

[rafaelw, 2015/03/19 15:03:50]

Done.

+
+    return listeners;
+  }
+
+  EventTarget(Node content, {
+    EventListener onWheel,
+    GestureEventListener onGestureFlingCancel,
+    GestureEventListener onGestureFlingStart,
+    GestureEventListener onGestureScrollStart,
+    GestureEventListener onGestureScrollUpdate,
+    GestureEventListener onGestureTap,
+    PointerEventListener onPointerCancel,
+    PointerEventListener onPointerDown,
+    PointerEventListener onPointerMove,
+    PointerEventListener onPointerUp,
+    Map<String, sky.EventListener> custom
+  }) : this.content = content,

[abarth-chromium, 2015/03/19 14:19:59]

You can just write |this.content| in the argument list to do this automatically.

[rafaelw, 2015/03/19 15:03:50]

Not if you want to use content in the initializer (passed into super:key below).
You'll get an error that you can't access |this| in the initializer.

+       listeners = _createListeners(
+         onWheel: onWheel,
+         onGestureFlingCancel: onGestureFlingCancel,
+         onGestureFlingStart: onGestureFlingStart,
+         onGestureScrollUpdate: onGestureScrollUpdate,
+         onGestureScrollStart: onGestureScrollStart,
+         onGestureTap: onGestureTap,
+         onPointerCancel: onPointerCancel,
+         onPointerDown: onPointerDown,
+         onPointerMove: onPointerMove,
+         onPointerUp: onPointerUp,
+         custom: custom
+       ),
+       super(key: content._key);
+
+  void _handleEvent(sky.Event e) {
+    sky.EventListener listener = listeners[e.type];
+    if (listener != null) {
+      listener(e);
+    }
+  }
+
+  static void _dispatchEvent(sky.Event e) {
+    Node target = RenderNode._getMounted(e.target);
+
+    // TODO(rafaelw): StopPropagation?
+    while (target != null) {
+      if (target is EventTarget) {
+        (target as EventTarget)._handleEvent(e);
+      }
+
+      target = target._parent;
+    }
+  }
+
+  static void _ensureDocListener(String eventType) {

[abarth-chromium, 2015/03/19 14:19:59]

style nit: s/Doc/Document/

[rafaelw, 2015/03/19 15:03:50]

Done.

+    if (_registeredEvents.add(eventType)) {
+        sky.document.addEventListener(eventType, _dispatchEvent);
+      }
+    }

[abarth-chromium, 2015/03/19 14:19:59]

Something is off with your indentation here.

[rafaelw, 2015/03/19 15:03:50]

Done.

+
+  void _sync(Node old, sky.ParentNode host, sky.Node insertBefore) {
+    if (!_docListenersRegistered) {
+      for (var type in listeners.keys) {
+        _ensureDocListener(type);
+      }
+
+      _docListenersRegistered = true;

[abarth-chromium, 2015/03/19 14:19:59]

Do we ever sync the same Node twice?  It seems like we always get the "new"
version in the sync...  Will this flag ever cut off work?

[rafaelw, 2015/03/19 15:03:50]

No, I think you're right. This buys us very little. I'm just going to remove it.
Presumably this work will show up in some future profile and we'll just create a
way to register for all events.

+    }
+
+    Node oldContent = old == null ? null : (old as EventTarget).content;
+    content = _syncChild(content, oldContent , host, insertBefore);

[abarth-chromium, 2015/03/19 14:19:59]

s/ ,/,/

[rafaelw, 2015/03/19 15:03:51]

Done.

+    _root = content._root;
+  }
+
+  void _remove() {
+    content._remove();
     super._remove();
   }
 }
@@ -299,8 +362,6 @@ abstract class Element extends RenderNode {
     Element oldElement = old as Element;
     sky.Element root = _root as sky.Element;
 
-    _syncEvents(oldElement.events);
-
     if (_class != oldElement._class)
       root.setAttribute('class', _class);
 
@@ -581,8 +642,6 @@ void _scheduleComponentForRender(Component c) {
   }
 }
 
-EventMap _emptyEventMap = new EventMap();
-
 abstract class Component extends Node {
   bool get _isBuilding => _currentlyBuilding == this;
   bool _dirty = true;
@@ -666,9 +725,6 @@ abstract class Component extends Node {
     _built = _syncChild(_built, oldBuilt, host, insertBefore);
     _dirty = false;
     _root = _built._root;
-
-    _built.events.addAll(events);
-    _syncEvents(oldComponent != null ? oldComponent.events : _emptyEventMap);
   }
 
   void _buildIfDirty() {