Add setters for callbacks on StreamController.

Add setters for callbacks on StreamController.

This allows you to create a stream controller at one point and
add or change the callbacks later. This can be useful if you
want to store a stream controller in a final instance field and
also want the controller to call instance methods as callbacks.

R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/4a8a844c345f8dff736f0b31f5371fd4571e1afa

[Lasse Reichstein Nielsen, 2015-07-17 06:33:08]

lrn@google.com changed reviewers:
+ floitsch@google.com, nweiz@google.com

[Lasse Reichstein Nielsen, 2015-07-17 06:33:08]

lrn@google.com changed required reviewers:
+ floitsch@google.com

[Lasse Reichstein Nielsen, 2015-07-17 06:33:09]

This prevents having a "no callback" version that is not wasting the four words
when they are not used. 
One alterantive could be to store the callbacks in a separate object so only one
pointer is wasted when there is no callback - but two extra words are needed
when there is a callback, and I think callbacks on stream controllers is used in
the majority of cases, even if it's only the onListen.

[floitsch, 2015-07-17 09:47:53]

I'm not a big fan of mutable classes, but I can see how this makes it easier to
write controllers.
LGTM.

https://codereview.chromium.org/1242023007/diff/1/sdk/lib/async/stream_contro...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/1/sdk/lib/async/stream_contro...
sdk/lib/async/stream_controller.dart:136: * Set the callback which is called
when the stream is listened to.
Sets

https://codereview.chromium.org/1242023007/diff/1/sdk/lib/async/stream_contro...
sdk/lib/async/stream_controller.dart:144: * Set the callback which is called
when the stream is paused.
ditto. and for the others.

[Lasse Reichstein Nielsen, 2015-07-17 10:03:18]

Committed patchset #2 (id:20001) manually as
4a8a844c345f8dff736f0b31f5371fd4571e1afa (presubmit successful).

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

https://codereview.chromium.org/1242023007/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1242023007/diff/20001/CHANGELOG.md#newcode6
CHANGELOG.md:6: * `StreamController`  added setters for the `onListen`,
`onPause`, `onResume`
Nit: extra space before "added".

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
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.

https://codereview.chromium.org/1242023007/diff/20001/tests/lib/async/stream_...
File tests/lib/async/stream_controller_test.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/tests/lib/async/stream_...
tests/lib/async/stream_controller_test.dart:732: controller..onListen = () {
state = running; }
Nit: Either make these bodies "=>"-style or make them multi-line.

https://codereview.chromium.org/1242023007/diff/20001/tests/lib/async/stream_...
tests/lib/async/stream_controller_test.dart:814: Expect.throws(() {
controller.onPause = (){}; },
Nit: "() {}"
(https://www.dartlang.org/articles/style-guide/#do-use-a-space-before--in-func...)

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

https://codereview.chromium.org/1242023007/diff/20001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/1242023007/diff/20001/CHANGELOG.md#newcode6
CHANGELOG.md:6: * `StreamController`  added setters for the `onListen`,
`onPause`, `onResume`
On 2015/07/17 20:40:16, nweiz wrote:
> Nit: extra space before "added".

Acknowledged.

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
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.

https://codereview.chromium.org/1242023007/diff/20001/tests/lib/async/stream_...
File tests/lib/async/stream_controller_test.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/tests/lib/async/stream_...
tests/lib/async/stream_controller_test.dart:732: controller..onListen = () {
state = running; }
No.
I will not make them => style. For onCancel that would be wrong (it should
return a Future if anything), for the rest it's just bad style to use => if you
don't actually care about returning the value.

As written, it's clear and readable, and adding more lines will not make it more
readable. The only change that could be considered is to align the end '}'s.

https://codereview.chromium.org/1242023007/diff/20001/tests/lib/async/stream_...
tests/lib/async/stream_controller_test.dart:814: Expect.throws(() {
controller.onPause = (){}; },
While I don't find it more readable (the space is actually pretty significant in
such a small and lightweight character sequence), I think I might not care too
much.

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

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
On 2015/08/05 09:08:45, Lasse Reichstein Nielsen wrote:
> 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.

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]

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
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]

DBC.
not adding much value, but since I already wrote it...

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
On 2015/08/06 07:00:41, Lasse Reichstein Nielsen wrote:
> 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.

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".

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
On 2015/08/06 01:02:13, nweiz wrote:
> On 2015/08/05 09:08:45, Lasse Reichstein Nielsen wrote:
> > 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.
> 
> 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.

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.

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

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
On 2015/08/06 09:28:12, floitsch wrote:

> 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.


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]

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/1242023007/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:141: void set onListen(void
onListenHandler());
On 2015/08/06 14:09:40, Lasse Reichstein Nielsen wrote:
> On 2015/08/06 09:28:12, floitsch wrote:
> 
> > 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.
> 
> 
> 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.
> 

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