Add Stream.periodic.

Add Stream.periodic.

BUG= http://dartbug.com/8477

Committed: https://code.google.com/p/dart/source/detail?r=20021

[floitsch, 2013-03-03 00:16:12]

Still missing tests.

Also: we need to decide on "periodic" and "repeating". Timer and Stream should
use the same named constructor.

[Lasse Reichstein Nielsen, 2013-03-04 08:43:04]

LGTM if we can agree on semantics.

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:83: * this and the last event. The duration can be
both longer than
last -> previous.
Add "and shorter" to match the "both" (and it is true).

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:86: * Stream had been paused.
had been -> was

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:88: * The first event is emitted after waiting for the
initial period.
After what? The stream being created or the first subscriber?

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:92: StreamController controller;
Consider making a specialized subclass of _SingleStreamImpl for this instead.

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:119: assert(timer == null);
Why not fire immediately if realWatch.elapsedMilliseconds >=
period.milliseconds, or set remaining timer based on that instead of userWatch?
You can drop userWatch then.

That is, if the receiver pauses, and then resumes because he's ready again,
before the period has elapsed, why should he not get the event on time? Imagine
something that asks for a period of 5 seconds, then pauses for one second while
executing the event (because it *might* take longer)?

Basically, I want a stream that gives me an event every period, or as soon as
possible after that (in case it's paused or busy at that time.If you pause for
more than period time, you can skip an event, but don't delay unless necessary.

[Sean Eagan, 2013-03-04 17:02:14]

https://chromiumcodereview.appspot.com/12381064/diff/1/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://chromiumcodereview.appspot.com/12381064/diff/1/sdk/lib/async/stream.d...
sdk/lib/async/stream.dart:82: * The sent event is a [Duration] object that
represents the time between
Why give the actual Duration for Stream.periodic but not for Future.delayed
which can also have a longer Duration if the system is busy?

Maybe give both an optional "compute()" argument which by default returns the
actual Duration waited.

[floitsch, 2013-03-13 19:26:55]

PTAL.
Brought the Stream.periodic interface in line with Future.delayed.

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:83: * this and the last event. The duration can be
both longer than
On 2013/03/04 08:43:04, Lasse Reichstein Nielsen wrote:
> last -> previous.
> Add "and shorter" to match the "both" (and it is true).

I don't think it can be shorter anymore. Removed the "both".

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:86: * Stream had been paused.
On 2013/03/04 08:43:04, Lasse Reichstein Nielsen wrote:
> had been -> was

Done.

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:88: * The first event is emitted after waiting for the
initial period.
On 2013/03/04 08:43:04, Lasse Reichstein Nielsen wrote:
> After what? The stream being created or the first subscriber?

reworded.

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:92: StreamController controller;
On 2013/03/04 08:43:04, Lasse Reichstein Nielsen wrote:
> Consider making a specialized subclass of _SingleStreamImpl for this instead.
> 

As discussed: keeping it as is. Do you want me to add a todo?

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:119: assert(timer == null);
On 2013/03/04 08:43:04, Lasse Reichstein Nielsen wrote:
> Why not fire immediately if realWatch.elapsedMilliseconds >=
> period.milliseconds, or set remaining timer based on that instead of
userWatch?
> You can drop userWatch then.
> 
> That is, if the receiver pauses, and then resumes because he's ready again,
> before the period has elapsed, why should he not get the event on time?
Imagine
> something that asks for a period of 5 seconds, then pauses for one second
while
> executing the event (because it *might* take longer)?
> 
> Basically, I want a stream that gives me an event every period, or as soon as
> possible after that (in case it's paused or busy at that time.If you pause for
> more than period time, you can skip an event, but don't delay unless
necessary.

As discussed: keeping the current semantics.

[Lasse Reichstein Nielsen, 2013-03-14 06:54:46]

lgtm

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:92: StreamController controller;
No, it's fine. We would only be saving some extra object creations, and it would
probably be premature optimization.

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:92: * By default, the computation always returns
`null`.
Above: The event values are computed ....
and here: If [computation] is omitted, the event values will all be `null`.

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:95: * [period]. After this duration has elapsed it
emits the first event.
When will the next event be emitted? [period] after the first one was emitted,
not counting time where the stream was paused?

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:142: if (timer != null) timer.cancel();
set timer to null here?
Otherwise unsubscribing and then resubscribing will hit the assert in
startPeriodicTimer.

Alternatively, call controller.close() which will prevent resubscription.

https://codereview.chromium.org/12381064/diff/8001/tests/lib/async/stream_per...
File tests/lib/async/stream_periodic4_test.dart (right):

https://codereview.chromium.org/12381064/diff/8001/tests/lib/async/stream_per...
tests/lib/async/stream_periodic4_test.dart:14: return (microsecs + 200) ~/ 1000;
Why + 200, and not +500. That would allow the same amount of slack on either
side of the expected value.
(It's perfectly reasonable, but explain it).

[Lasse Reichstein Nielsen, 2013-03-14 06:54:46]

lgtm

[floitsch, 2013-03-14 13:27:34]

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/12381064/diff/1/sdk/lib/async/stream.dart#new...
sdk/lib/async/stream.dart:82: * The sent event is a [Duration] object that
represents the time between
On 2013/03/04 17:02:14, seaneagan1 wrote:
> Why give the actual Duration for Stream.periodic but not for Future.delayed
> which can also have a longer Duration if the system is busy?
> 
> Maybe give both an optional "compute()" argument which by default returns the
> actual Duration waited.

Changed to be consistent. Both emit "null" now unless there is a compute
function.

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:92: * By default, the computation always returns
`null`.
On 2013/03/14 06:54:46, Lasse Reichstein Nielsen wrote:
> Above: The event values are computed ....
> and here: If [computation] is omitted, the event values will all be `null`.

Done.

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:95: * [period]. After this duration has elapsed it
emits the first event.
On 2013/03/14 06:54:46, Lasse Reichstein Nielsen wrote:
> When will the next event be emitted? [period] after the first one was emitted,
> not counting time where the stream was paused?

Removed the lines. Not adding anything.

https://codereview.chromium.org/12381064/diff/8001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:142: if (timer != null) timer.cancel();
On 2013/03/14 06:54:46, Lasse Reichstein Nielsen wrote:
> set timer to null here?
> Otherwise unsubscribing and then resubscribing will hit the assert in
> startPeriodicTimer.
> 
> Alternatively, call controller.close() which will prevent resubscription.

done.

https://codereview.chromium.org/12381064/diff/8001/tests/lib/async/stream_per...
File tests/lib/async/stream_periodic4_test.dart (right):

https://codereview.chromium.org/12381064/diff/8001/tests/lib/async/stream_per...
tests/lib/async/stream_periodic4_test.dart:14: return (microsecs + 200) ~/ 1000;
On 2013/03/14 06:54:46, Lasse Reichstein Nielsen wrote:
> Why + 200, and not +500. That would allow the same amount of slack on either
> side of the expected value.
> (It's perfectly reasonable, but explain it).

done.

[floitsch, 2013-03-14 13:43:50]

Committed patchset #4 manually as r20021 (presubmit successful).