2
\$\begingroup\$

I'm working on a WPF project where a timer with Pause/Resume functionality is required. The standard Windows.Threading.DispatcherTimer doesn't offer that, so I decided to inherit the class and add those features myself.

The way it works is pretty simple, there is a Stopwatch running internally, whenever Pause() is called base.Stop() is invoked along with Stopwatch.Stop().

Invoking Resume() would initiate an asynchronous Task, which calculates how much time should be waited before firing the next event based on the Stopwatch.Elapsed property. Once this time interval has passed, all the event handlers from the InvocationList of the Tick event are being invoked. After this is done the Stopwatch and the class itself are "resumed".

Example:

Let's say we have a timer with Interval 10 seconds. You Start() the timer and wait 6 seconds. You Stop() the timer, wait another 2 seconds and than Start() the timer again. The next firing of the tick event would be after 10 seconds, if you replace Stop and Play with Pause and Resume respectively, the next firing of the tick event would be after 4 seconds only.

This is the code:

public class IntermissionTimer : DispatcherTimer
{
    private readonly Stopwatch _sw;

    private MulticastDelegate _tickEventHandlers;
    private MulticastDelegate tickEventHandlers =>
        _tickEventHandlers ?? (_tickEventHandlers = (MulticastDelegate)typeof(DispatcherTimer)
            .GetField("Tick", BindingFlags.Instance | BindingFlags.NonPublic)
            .GetValue(this));

    private CancellationTokenSource _cancelationToken;
    private Task _task;

    public TimeSpan Elapsed => _sw.Elapsed;

    private bool _hasScheduledIntermission;

    public IntermissionTimer()
    {
        _sw = new Stopwatch();
        Cancelation();
    }

    public new void Start()
    {
        base.Start();
        _sw.Start();
    }

    public new void Stop()
    {
        base.Stop();
        _sw.Reset();
    }

    private void Cancelation()
    {
        _cancelationToken = new CancellationTokenSource();
        _cancelationToken.Token.Register(() =>
        {
            _hasScheduledIntermission = false;
            _sw.Stop();
        });
    }

    public void Pause()
    {
        if (_task != null && !_task.IsCompleted)
        {
            _cancelationToken.Cancel();
        }
        else
        {
            Cancelation();
        }
        base.Stop();
        _sw.Stop();
        _hasScheduledIntermission = true;
    }

    public void Resume()
    {
        if ((_task == null || _task.IsCompleted) && (_hasScheduledIntermission || _sw.ElapsedTicks == 0))
        {
            _task = Task.Run(async () =>
            {
                _sw.Start();
                var value = (int)(_sw.Elapsed.TotalMilliseconds / Interval.TotalMilliseconds);
                var scheduledIntermissionTime = Math.Abs((int)((value + 1) * Interval.TotalMilliseconds -
                                                               _sw.Elapsed.TotalMilliseconds));
                if (_cancelationToken.IsCancellationRequested)
                {
                    Cancelation();
                    return;
                }
                await Task.Delay(scheduledIntermissionTime, _cancelationToken.Token).ContinueWith(t => { });
                if (_cancelationToken.IsCancellationRequested)
                {
                    Cancelation();
                    return;
                }
                foreach (var handler in tickEventHandlers.GetInvocationList())
                {
                    Dispatcher.Invoke(() => handler.Method.Invoke(handler.Target,
                        new object[] { this, EventArgs.Empty }));
                }
                Start();
                _hasScheduledIntermission = false;
            }, _cancelationToken.Token);
            Cancelation();
        }
    }
}

It's not pretty but it does the job as far as I'm concerned. I'm looking for any code improvements, I haven't worked with Tasks a lot and that's the main part of this class, so I'd like to put the focus on that (especially the cancellation).

\$\endgroup\$
  • \$\begingroup\$ @t3chb0t If you call Stop and than Play the next tick will be fired after DispatcherTimer.Interval time, regardless of when you stopped it, any information on how much time was actually left for the next tick is deleted. Pause and Resume are doing the opposite. \$\endgroup\$ – Denis Oct 31 '17 at 12:14
  • \$\begingroup\$ @t3chb0t I've updated that, I wrote it late last night, so I forgot to change the pseudo names. I also elaborated a bit more on the differences. \$\endgroup\$ – Denis Oct 31 '17 at 12:50
  • 1
    \$\begingroup\$ Great, and thx for adding the example. Has it only been there form the beginning... now everything is clear ;-) \$\endgroup\$ – t3chb0t Oct 31 '17 at 12:54
1
\$\begingroup\$

I know you are primarily interested in your use of Task. Sadly my remarks are not about that.

I think _hasScheduledIntermission would be better named as _isPauseRequested. For one, you really haven't scheduled it. Two, the phrase "Pause" is bonded more closely to your Pause() method. And three, it seems like a natural counterpart to IsCancellationRequested. You may even consider having IsPauseRequested be a public settable property.

The top of your Pause() method has this:

if (_hasScheduledIntermission && !_cancelationToken.IsCancellationRequested)
{
    _cancelationToken?.Cancel();
}

I would expect to see a ?. in the conditional, or not have it in the true block. And the way your code currently exists, the ?. is not needed there.

I think your Cancelation method does not adequately separate concerns. It does 2 things. It gets a new cancellation token source, and its stops the stopwatch. Perhaps these would be better if separated.

Perhaps the cancellation token source should be created when Start() or Resume() runs.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I do agree on the naming, and the conditional access is really weird there haha. \$\endgroup\$ – Denis Oct 31 '17 at 13:39
1
\$\begingroup\$

I would count your solution as hacking the DispatcherTimer and the biggest issue I see there is that you don't dispose the CancellationTokenSource.

Your strategy seems to be mostly ok and I can't complain much about it. One thing that I don't like about is the lengthy if condition inside the Resume method. A helper variable for this condition would be nice.


I think this is overengineered and all the reflection stuff is scarry - but sometimes inevitable - not this time. It's a pitty that the DispatcherTimer does not allow to easily override its methods even though it's not sealed.

Well, I find you actually don't need it to be so complex and you actually don't need any reflection after all - call me the alternative-guy becuase yet again I'll post an alternative version ;-)

It's very similar to your current solution but there is no reflection involved.

In order to maintain the Interval you can subscribe yourself to the Tick event and set it to the rest of the time that is left until next tick. Then reset it back to the original interval and restart so it ticks in it original intervals.

When you Pause the time you start the stopwatch and save the interval and start the timer. On Resume you claculate how much time is left and update the Interval accordingly and start the timer again. When it ticks and the Interval is not equal baseInterval you reset it and restart the timer. Now it ticks as before.

public class IntermissionTimer : DispatcherTimer
{
    private readonly Stopwatch _pauseStopwatch = new Stopwatch();
    private TimeSpan _baseInterval;

    public IntermissionTimer()
    {
        Tick += (sender, e) =>
        {
            if (_baseInterval != TimeSpan.Zero && Interval != _baseInterval)
            {
                Console.WriteLine("Reset!");
                Interval = _baseInterval;
                Stop();
                Start();
            }
        };
    }

    public void Pause()
    {
        Stop();
        _pauseStopwatch.Start();
        if (_baseInterval == TimeSpan.Zero)
        {
            _baseInterval = Interval;
        }
    }

    public void Resume()
    {
        _pauseStopwatch.Stop();
        var pauseElapsed = _pauseStopwatch.Elapsed;
        var timeLeft = Interval.TotalMilliseconds - pauseElapsed.TotalMilliseconds;
        TimeSpan.FromMilliseconds(timeLeft).TotalSeconds.Dump("time-left");
        Interval = TimeSpan.FromMilliseconds(timeLeft);
        Start();
    }
}

The calculation is very simplistic and probably won't work when the interval is exceeded but as a proof of concept it seems to work fine so you might want to check this.

Example:

public async Task Main()
{    
    var sw = Stopwatch.StartNew();

    var it = new IntermissionTimer2
    {
        Interval = TimeSpan.FromSeconds(6)
    };

    it.Tick += (sender, e) => Console.WriteLine($"Tick! {sw.Elapsed.Seconds}s");
    it.Start();

    await Task.Delay(TimeSpan.FromSeconds(2));

    it.Pause();

    await Task.Delay(TimeSpan.FromSeconds(2));

    it.Resume();    

    await Task.Delay(TimeSpan.FromSeconds(8));
    Console.WriteLine($"End! {sw.Elapsed.Seconds}s");

}

This will print:

time-left
3,999 
Reset!
Tick! 8s
End! 12s
Tick! 14s
\$\endgroup\$
  • \$\begingroup\$ One of my main ideas was not to modify the Interval property, I believe this is a bad idea, imagine if the value is 30 minutes, what if you try to access that property during this time? It would give inaccurate value. \$\endgroup\$ – Denis Oct 31 '17 at 13:50
  • \$\begingroup\$ @Denis what do you mean by accessing it during this time? Why should it be inaccurate? You stop the timer, change its value and start it agian. \$\endgroup\$ – t3chb0t Oct 31 '17 at 13:52
  • \$\begingroup\$ Interval = TimeSpan.FromMilliseconds(timeLeft);, is not the original value the user submitted, if it's later requested while you've paused the timer there will be a discrepancy, which the user cant know about, unless he checks out my code. \$\endgroup\$ – Denis Oct 31 '17 at 13:54
  • \$\begingroup\$ @Denis oh, you mean this. Well, then create another property with the original interval if you need binding to this. I find fumbling with reflection is the last resort solution and currently I don't believe it's necessary here but I of course don't know its exact usage so it's just brainstorming ;-] \$\endgroup\$ – t3chb0t Oct 31 '17 at 13:56
  • \$\begingroup\$ I also thought of that solution as well, but I believed it would make the code messier than it is now, but your implementation seems cleaner, I might give it a try :) \$\endgroup\$ – Denis Oct 31 '17 at 14:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.