Add setters for callbacks on StreamController.

sdk/lib/async/stream_controller.dart

Index: sdk/lib/async/stream_controller.dart
diff --git a/sdk/lib/async/stream_controller.dart b/sdk/lib/async/stream_controller.dart
index 5764ecdd2102e6dc975a6c1ca3d81a0ed4db3ad4..26bbb14706cb55d971db51bce691f7f41a0f6183 100644
--- a/sdk/lib/async/stream_controller.dart
+++ b/sdk/lib/async/stream_controller.dart
@@ -77,12 +77,6 @@ abstract class StreamController<T> implements StreamSink<T> {
                             void onResume(),
                             onCancel(),
                             bool sync: false}) {
-    if (onListen == null && onPause == null &&
-        onResume == null && onCancel == null) {
-      return sync
-          ? new _NoCallbackSyncStreamController<T>()
-          : new _NoCallbackAsyncStreamController<T>();
-    }
     return sync
          ? new _SyncStreamController<T>(onListen, onPause, onResume, onCancel)
          : new _AsyncStreamController<T>(onListen, onPause, onResume, onCancel);
@@ -139,6 +133,42 @@ abstract class StreamController<T> implements StreamSink<T> {
   }
 
   /**
+   * Sets the callback which is called when the stream is listened to.
+   *
+   * This overrides the previous callback, or clears it if the [onListenHandler]
+   * is `null`.
+   */
+  void set onListen(void onListenHandler());

[nweiz, 2015/07/17 20:40:16]

It's kind of weird that these are setters, whereas StreamSubscription uses plain
methods for the same thing. Even if we think getters are a better design,
inconsistency is yet worse. If we do make these setters, we should also
deprecate the StreamSubscription methods and add setters there.

[Lasse Reichstein Nielsen, 2015/08/05 09:08:45]

It is annoying that we didn't go for setters in StreamSubscription.
I don't want to keep doing the wrong thing for consistency (two wrongs won't
make a right).

It's hard to change now. We can have both, but dart2js doesn't support having a
setter and a method with the "same namne" in the same class, and it is specified
as giving a warning.

[nweiz, 2015/08/06 01:02:13]

I talked to Bob about this, and he pointed out that it's really confusing to
have setters without corresponding getters; properties are meant to mimic
fields, and when they're write-only that metaphor breaks down a lot.

I think if we were doing this from scratch I would advocate for "setOnListen()"
etc, but I don't think just "onListen()" is that different given that it's
consistent with StreamSubscription, so I still think that's what we should do.

[Lasse Reichstein Nielsen, 2015/08/06 07:00:41]

Well, it's quite symmetric - you have read-only fields so why not write-only :)
But it's true that it may be confusing, and I was considering adding
corresponding getters. I can't do that for StreamSubscription because it would
collide with the methods, but I can do it here.

I would never want "set" as part of the name. When you have that, it *is a
setter and you should use a setter for it. 
That may just be the Java speaking.

[floitsch, 2015/08/06 09:28:12]

In this case I wouldn't onListen to setOnListen. If we have a method called
"onListen", and a field "onListen", then we screwed up the names. In general, it
should be possible to know if something is a method or a property from the name.

[floitsch, 2015/08/06 09:28:12]

I actually disagree. Sometimes a "setX" method is perfectly fine. setters convey
a casual, you can set the value, whereas "setX" is much more explicit. In most
cases, a "setX" has more documentation and requirements. For example "only
invoke once, at a specific moment".

[Lasse Reichstein Nielsen, 2015/08/06 14:09:40]

I think we did screw up that particular type of name.
Here I made it a setter, because it really is a property (and I wouldn't mind
adding the getters).
On StreamSubscription it's a method. It should probably be a property as well,
with getters (no reason to hide the callback from the owner of the
subscription).
in HTML an "onClick" is a stream of events that you can listen to.
In other cases it's a parameter name.

The thing they have in common is that they are ways of adding callbacks.
For StreamSubscription and StreamController we can only have one callback of
each type. HTML allows multiple callbacks.
Maybe they should all be "events" (a simpler non-stream source of events). I'm
sure that's what they would be in C#, but they have good syntactic sugar for it.

As for fixing the current problem - I'd add getters here and setters on
StreamSubscription, and change the methods to getters in 2.0 (or whenever such a
breaking change can be done).

As a property name, it should probably have been onListenCallback or
onListenHandler. Nobody will thank us for making them write extra letters,
though.

[nweiz, 2015/08/06 19:56:12]

I'm fine with these being setters as long as there are getters as well.

+
+  /**
+   * Sets the callback which is called when the stream is paused.
+   *
+   * This overrides the previous callback, or clears it if the [onPauseHandler]
+   * is `null`.
+   *
+   * Pause related callbacks are not supported on broadcast stream controllers.
+   */
+  void set onPause(void onPauseHandler());
+
+  /**
+   * Sets the callback which is called when the stream is resumed.
+   *
+   * This overrides the previous callback, or clears it if the [onResumeHandler]
+   * is `null`.
+   *
+   * Pause related callbacks are not supported on broadcast stream controllers.
+   */
+  void set onResume(void onResumeHandler());
+
+  /**
+   * Sets the callback which is called when the stream is canceled.
+   *
+   * This overrides the previous callback, or clears it if the [onCancelHandler]
+   * is `null`.
+   */
+  void set onCancel(onCancelHandler());
+
+  /**
    * Returns a view of this object that only exposes the [StreamSink] interface.
    */
   StreamSink<T> get sink;
@@ -383,12 +413,23 @@ abstract class _StreamController<T> implements StreamController<T>,
   // accessed earlier, or if close is called before subscribing.
   _Future _doneFuture;
 
-  _StreamController();
+  _NotificationHandler _onListen;
+  _NotificationHandler _onPause;
+  _NotificationHandler _onResume;
+  _NotificationHandler _onCancel;
 
-  _NotificationHandler get _onListen;
-  _NotificationHandler get _onPause;
-  _NotificationHandler get _onResume;
-  _NotificationHandler get _onCancel;
+  _StreamController(void this._onListen(),
+                    void this._onPause(),
+                    void this._onResume(),
+                    this._onCancel());
+
+  void set onListen(void onListenHandler()) { _onListen = onListenHandler; }
+
+  void set onPause(void onPauseHandler()) { _onPause = onPauseHandler; }
+
+  void set onResume(void onResumeHandler()) { _onResume = onResumeHandler; }
+
+  void set onCancel(onCancelHandler()) { _onCancel = onCancelHandler; }
 
   // Return a new stream every time. The streams are equal, but not identical.
   Stream<T> get stream => new _ControllerStream<T>(this);
@@ -722,44 +763,11 @@ abstract class _AsyncStreamControllerDispatch<T>
 // TODO(lrn): Use common superclass for callback-controllers when VM supports
 // constructors in mixin superclasses.
 
-class _AsyncStreamController<T> extends _StreamController<T>
-                                   with _AsyncStreamControllerDispatch<T> {
-  final _NotificationHandler _onListen;
-  final _NotificationHandler _onPause;
-  final _NotificationHandler _onResume;
-  final _NotificationHandler _onCancel;
+class _AsyncStreamController<T> = _StreamController<T>
+                                  with _AsyncStreamControllerDispatch<T>;
 
-  _AsyncStreamController(void this._onListen(),
-                         void this._onPause(),
-                         void this._onResume(),
-                         this._onCancel());
-}
-
-class _SyncStreamController<T> extends _StreamController<T>
-                                  with _SyncStreamControllerDispatch<T> {
-  final _NotificationHandler _onListen;
-  final _NotificationHandler _onPause;
-  final _NotificationHandler _onResume;
-  final _NotificationHandler _onCancel;
-
-  _SyncStreamController(void this._onListen(),
-                        void this._onPause(),
-                        void this._onResume(),
-                        this._onCancel());
-}
-
-abstract class _NoCallbacks {
-  _NotificationHandler get _onListen => null;
-  _NotificationHandler get _onPause => null;
-  _NotificationHandler get _onResume => null;
-  _NotificationHandler get _onCancel => null;
-}
-
-class _NoCallbackAsyncStreamController<T> = _StreamController<T>
-       with _AsyncStreamControllerDispatch<T>, _NoCallbacks;
-
-class _NoCallbackSyncStreamController<T> = _StreamController<T>
-       with _SyncStreamControllerDispatch<T>, _NoCallbacks;
+class _SyncStreamController<T> = _StreamController<T>
+                                 with _SyncStreamControllerDispatch<T>;
 
 typedef _NotificationHandler();