Add setters for callbacks on StreamController.

sdk/lib/async/stream_controller.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of dart.async;
 
 // -------------------------------------------------------------------
 // Controller for creating and adding events to a stream.
 // -------------------------------------------------------------------
 
@@ skipping 59 matching lines @@
    * is done.
    *
    * If the stream is canceled before the controller needs new data the
    * [onResume] call might not be executed.
    */
   factory StreamController({void onListen(),
                             void onPause(),
                             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);
   }
 
   /**
    * A controller where [stream] can be listened to more than once.
    *
    * The [Stream] returned by [stream] is a broadcast stream.
    * It can be listened to more than once.
@@ skipping 36 matching lines @@
    */
   factory StreamController.broadcast({void onListen(),
                                       void onCancel(),
                                       bool sync: false}) {
     return sync
         ? new _SyncBroadcastStreamController<T>(onListen, onCancel)
         : new _AsyncBroadcastStreamController<T>(onListen, onCancel);
   }
 
   /**
+   * 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;
 
   /**
    * Whether the stream controller is closed for adding more events.
    *
    * The controller becomes closed by calling the [close] method.
    * New events cannot be added, by calling [add] or [addError],
    * to a closed controller.
@@ skipping 224 matching lines @@
   /**
    * Future completed when the stream sends its last event.
    *
    * This is also the future returned by [close].
    */
   // TODO(lrn): Could this be stored in the varData field too, if it's not
   // accessed until the call to "close"? Then we need to special case if it's
   // 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);
 
   /**
    * Returns a view of this object that only exposes the [StreamSink] interface.
    */
   StreamSink<T> get sink => new _StreamSinkWrapper<T>(this);
 
   /**
@@ skipping 313 matching lines @@
   }
 
   void _sendDone() {
     _subscription._addPending(const _DelayedDone());
   }
 }
 
 // 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();
 
 Future _runGuarded(_NotificationHandler notificationHandler) {
   if (notificationHandler == null) return null;
   try {
     var result = notificationHandler();
     if (result is Future) return result;
     return null;
   } catch (e, s) {
@@ skipping 126 matching lines @@
   _StreamControllerAddStreamState(_StreamController controller,
                                   this.varData,
                                   Stream source,
                                   bool cancelOnError)
       : super(controller, source, cancelOnError) {
     if (controller.isPaused) {
       addSubscription.pause();
     }
   }
 }