Add a ResetTimer class.

Add a RestartableTimer class.

This is useful for heartbeat-style timeouts where a timeout is reset
when certain actions occur.

R=lrn@google.com

Committed: https://github.com/dart-lang/async/commit/d23c21e14f6e14cee5d02564d6a096f9ebc5d36e

[Lasse Reichstein Nielsen, 2015-10-27 09:45:14]

LGTM

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart
File lib/src/reset_timer.dart (right):

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:9: /// A non-periodic timer that can be reset.
"reset" isn't a very descriptive word, it doesn't tell me what it means to reset
a timer.
Maybe "restarted" or "reused" is a better description of what it does.
I'd prefer "ReuseTimer" or "ReusableTimer" as the class name as well.

Maybe:
/// A non-periodic timer that can be restarted.
///
/// The timer can be restarted both before and after it has fired.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:14: /// The callback to call when the timer fires.
Would it be useful to make the callback settable?

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:17: /// The inner timer.
Being "inner" doesn't say anything about the role of this timer.
Maybe "The timer for the current, or most recent, countdown".

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:19: /// This is canceled and re-created every time this
is reset.
Two uses of "this" referring to different things.
Maybe:
  This timer is canceled and overwritten every time the [ReuseTimer] is [reset].

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:22: bool get isActive => _timer.isActive;
Move getter below constructor.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:32: /// Resets the timer so that it starts counting
from its original duration again.
Resets -> restarts.
The "reset" name is fine for the method name, the documentation has to describe
what it means to "reset", so it can't use the same word.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:32: /// Resets the timer so that it starts counting
from its original duration again.
long line.

so that it starts ... -> and starts counting from the original duration again

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:34: /// This resets the timer even if it's already
fired or has been canceled.
resets -> restarts
it's -> it has

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart
File test/reset_timer_test.dart (right):

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart#...
test/reset_timer_test.dart:15: var timer = new ResetTimer(new Duration(seconds:
5), () => fired = true);
Use braces, not =>, for functions where the return value isn't relevant (dittos
below).


Consider having 

  const second = const Duration(seconds: 1);

declared somewhere, so you can just write "second * 5" for the duration.
Also makes it fit in a line when changing => to {;}.

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart#...
test/reset_timer_test.dart:34: async.elapse(new Duration(seconds: 4));
Interesting - this looks like it's synchronous, so how does it fire timer
callbacks (which should be called from a top-level event loop)?

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart#...
test/reset_timer_test.dart:90: });
Check that the timer is not periodic: Wait at least 10 seconds, and see that it
fires only once.

[nweiz, 2015-10-27 21:07:53]

Code review changes

[nweiz, 2015-10-27 21:08:12]

Waiting to commit for another round of feedback on naming.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart
File lib/src/reset_timer.dart (right):

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:9: /// A non-periodic timer that can be reset.
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> "reset" isn't a very descriptive word, it doesn't tell me what it means to
reset
> a timer.
> Maybe "restarted" or "reused" is a better description of what it does.
> I'd prefer "ReuseTimer" or "ReusableTimer" as the class name as well.
> 
> Maybe:
> /// A non-periodic timer that can be restarted.
> ///
> /// The timer can be restarted both before and after it has fired.

I chose "reset" because it's already used by a number of core library types,
most notably Stopwatch where it has very similar semantics to this. It seems
really confusing to have some methods named "reset" and some named "restart".
It's a lot like using Map.length rather than e.g. Map.size—"size" is slightly
more appropriate in the abstract, but "length" is better in the context of the
existing libraries.

I'm okay with ReusableTimer, but I do kind of feel like it emphasizes the wrong
thing. For most of the use-cases I have in mind for this, the major reason they
would use this rather than a normal Timer is because they want to reset the
countdown. A lot of the time (e.g. for timeout errors), it will still only fire
once.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:14: /// The callback to call when the timer fires.
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Would it be useful to make the callback settable?

I don't think so; if that's what you want, just hook it up to a completer or
something.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:17: /// The inner timer.
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Being "inner" doesn't say anything about the role of this timer.
> Maybe "The timer for the current, or most recent, countdown".

Done.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:19: /// This is canceled and re-created every time this
is reset.
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Two uses of "this" referring to different things.
> Maybe:
>   This timer is canceled and overwritten every time the [ReuseTimer] is
[reset].

Done.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:22: bool get isActive => _timer.isActive;
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Move getter below constructor.

Done.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:32: /// Resets the timer so that it starts counting
from its original duration again.
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Resets -> restarts.
> The "reset" name is fine for the method name, the documentation has to
describe
> what it means to "reset", so it can't use the same word.

Done.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:32: /// Resets the timer so that it starts counting
from its original duration again.
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> long line.
> 
> so that it starts ... -> and starts counting from the original duration again

Done.

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:34: /// This resets the timer even if it's already
fired or has been canceled.
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> resets -> restarts
> it's -> it has

Done.

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart
File test/reset_timer_test.dart (right):

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart#...
test/reset_timer_test.dart:15: var timer = new ResetTimer(new Duration(seconds:
5), () => fired = true);
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Use braces, not =>, for functions where the return value isn't relevant
(dittos
> below).

Done.

> Consider having 
> 
>   const second = const Duration(seconds: 1);
> 
> declared somewhere, so you can just write "second * 5" for the duration.

I'm not a huge fan of creating an idiosyncratic way of declaring durations just
to avoid a few characters.

> Also makes it fit in a line when changing => to {;}.

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart#...
test/reset_timer_test.dart:34: async.elapse(new Duration(seconds: 4));
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Interesting - this looks like it's synchronous, so how does it fire timer
> callbacks (which should be called from a top-level event loop)?

I believe it just creates an entire simulated event loop using a
ZoneSpecification.

https://codereview.chromium.org/1417373004/diff/1/test/reset_timer_test.dart#...
test/reset_timer_test.dart:90: });
On 2015/10/27 09:45:14, Lasse Reichstein Nielsen wrote:
> Check that the timer is not periodic: Wait at least 10 seconds, and see that
it
> fires only once.

Done.

[Lasse Reichstein Nielsen, 2015-10-28 09:51:03]

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart
File lib/src/reset_timer.dart (right):

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:9: /// A non-periodic timer that can be reset.
I have no problem with "reset" as the name of the method, but by itself it's not
descriptive enough to say what a "ResetTimer" or "ResettableTimer" does. It's
too generic a name to use as the description of what it does since it's already
used in many settings with different meanings.

That means that the method should be called reset, but the documentation and
class name should be more descriptive, giving the context that explains what
"reset" means in this case.

I'm not sure exactly what the name should be. "ReusableTimer" isn't very good,
for the reasons you state. "RestartableTimer" is better but still doesn't feel
perfect to me. "RepeatTimer" has potential, but may sound too much like a
periodic timer. "ReTimer" is cute, but flawed :)

I think "RestartableTimer" is the best bet so far.

[nweiz, 2015-10-28 21:10:38]

Code review changes

[nweiz, 2015-10-28 21:10:43]

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart
File lib/src/reset_timer.dart (right):

https://codereview.chromium.org/1417373004/diff/1/lib/src/reset_timer.dart#ne...
lib/src/reset_timer.dart:9: /// A non-periodic timer that can be reset.
On 2015/10/28 09:51:02, Lasse Reichstein Nielsen wrote:
> I have no problem with "reset" as the name of the method, but by itself it's
not
> descriptive enough to say what a "ResetTimer" or "ResettableTimer" does. It's
> too generic a name to use as the description of what it does since it's
already
> used in many settings with different meanings.
> 
> That means that the method should be called reset, but the documentation and
> class name should be more descriptive, giving the context that explains what
> "reset" means in this case.
> 
> I'm not sure exactly what the name should be. "ReusableTimer" isn't very good,
> for the reasons you state. "RestartableTimer" is better but still doesn't feel
> perfect to me. "RepeatTimer" has potential, but may sound too much like a
> periodic timer. "ReTimer" is cute, but flawed :)
> 
> I think "RestartableTimer" is the best bet so far.
> 

Done.

[nweiz, 2015-10-28 21:11:00]

Description was changed from

==========
Add a ResetTimer class.

This is useful for heartbeat-style timeouts where a timeout is reset
when certain actions occur.

R=lrn@google.com
==========

to

==========
Add a RestartableTimer class.

This is useful for heartbeat-style timeouts where a timeout is reset
when certain actions occur.

R=lrn@google.com
==========

[nweiz, 2015-10-28 21:11:33]

Committed patchset #3 (id:40001) manually as
d23c21e14f6e14cee5d02564d6a096f9ebc5d36e (presubmit successful).