DateTime.Now based timer not tracking correctly ov

Question

this is my first proper C# application that I wrote to help me at work (I'm on helpdesk for an MSP with a passing interest in scripting and code) and I'm using UWP just to make it look pretty without much effort. Our time tracking software is a web service written in ASP.Net so generally the built in timer is fine but it won't survive a browser refresh, so I wrote my own that fits into the format that we need for our tickets.

I have taken some code from other Stack questions and my dad (A C# framework dev for a multinational) helped re-write some of the timer code so it wasn't using stopwatch. He just isn't available to fix this issue at the moment. I do understand how it works now, just not how to debug the issue I'm getting.

It supports multiple timers running at the same time and creating a new timer auto-pauses all others. It handles two time formats, minutes and decimal hours, so that will explain some of the properties you see in the code.

My issue is that when I add a new timer, it pauses all others, but then when I press start on an older timer (Returning back to an earlier ticket) the time instantly jumps up to how long the new timer was running for, with about 10% difference (It's never exactly how long it was running).

This is the class that tracks notes and the current time (Tidied up a bit for neatness):

public sealed class JobTimer:INotifyPropertyChanged
{
    private DateTime _created; // When the timer was created
    private DateTime _started; // When it was most recently started
    private TimeSpan _offset; // The saved value to offset the currently running timer
    Timer _swTimer; // The actual tick that updates the screen

    public JobTimer() : this(TimeSpan.Zero)
    { }

    public JobTimer(TimeSpan offset)
    {
        _offset = offset;
        _created = DateTime.Now;
        IsNotLocked = true;
    }

    // Time in seconds
    public string TimeMin => string.Format("{0:00}:{1:00}:{2:00}", ElapsedTime.Hours, ElapsedTime.Minutes, ElapsedTime.Seconds);

    // Time in decimal hours
    public string TimeDec => string.Format("{0}", 0.1 * Math.Ceiling(10 * ElapsedTime.TotalHours));

    public DateTime Created => _created;

    public TimeSpan ElapsedTime => GetElapsed();

    public void Start()
    {
        _started = DateTime.Now;
        _swTimer = new Timer(TimerChanged, null, 0, 1000);

        NotifyPropertyChanged("IsRunning");
    }

    public void Stop()
    {
        if (_swTimer != null)
        {
            _swTimer.Dispose();
            _swTimer = null;
        }

        _offset = _offset.Add(DateTime.Now.Subtract(_started));

        NotifyPropertyChanged("IsRunning");
    }

    private TimeSpan GetElapsed()
    {
        // This was made as part of my own debugging, the ElaspsedTime property used to just be the if return
        if (IsRunning)
        {
            return _offset.Add(DateTime.Now.Subtract(_started));
        }
        else
        {
            return _offset;
        }
    }

    // Updates the UI
    private void TimerChanged(object state)
    {
        NotifyPropertyChanged("TimeDec");
        NotifyPropertyChanged("TimeMin");
    }

    public bool IsRunning
    {
        get { return _swTimer != null; }
    }

    public void ToggleRunning()
    {
        if (IsRunning)
        {
            Stop();
        }
        else
        {
            Start();
        }
    }
}

This goes into the ViewModel:

public class JobListViewModel
{
    private readonly ObservableCollection<JobTimer> _list = new ObservableCollection<JobTimer>();

    public ObservableCollection<JobTimer> JobTimers => _list;

    public JobListViewModel()
    {
        AddTimer();
    }

    public void AddTimer()
    {
        JobTimer t = new JobTimer();
        JobTimers.Add(t);
        t.Start();
    }

    public void PauseAll()
    {
        foreach(JobTimer timer in JobTimers)
        {
            timer.Stop();
        }
    }

    // Other functions unrelated
}

And this is the UI button click that adds a new timer

    private void AddTimer_Click(object sender, RoutedEventArgs e)
    {
        // Create JobTimer
        ViewModel.PauseAll();
        ViewModel.AddTimer();

        // Scroll to newly created timer
        JobTimer lastTimer = ViewModel.JobTimers.Last();
        viewTimers.UpdateLayout();
        viewTimers.ScrollIntoView(lastTimer);
    }

I realise it's a lot of code to dump into a post but I can't pinpoint where the issue is being caused. I was able to find that something alters the offset when I hit the AddTimer button whether the existing timer is running or not, but I can't find what's altering it.

Answer 1

After building enough other code to support the code you posted, I was able to reproduce your problem.

The issue in your code is that you unconditionally call the Stop() method, whether the timer is already stopped or not. And the Stop() method unconditionally resets the _offset field, whether or not the timer is already running. So, if you add a timer when any other timer is already stopped, its _offset value is incorrectly reset.

IMHO, the right fix is for the Start() and Stop() methods to only perform their work when the timer is in the appropriate state to be started or stopped. I.e. to check the IsRunning property before actually doing the operation.

See below for an actual Minimal, Complete, and Verifiable version of the code you posted, but without the bug.

In addition to fixing the bug, I removed all of the unused elements (that is, all the code that did not appear to be used or discussed in your scenario) and refactored the code so that it is more idiomatic of a typical WPF implementation (see helper/base classes at the end). When I run the program, I am able to start and stop the timer objects without any trouble, even after adding new timers to the list.

Notable modifications:

• Use of NotifyPropertyChangedBase class as base class for model classes.
• Leverage of said base class features for property change notification, by keeping public properties as simple value-storing properties modified as needed.
• Use of ICommand implementation for user actions (i.e. "commands").
• Separation of timer-specific start/stop functionality when adding timers from view-specific scrolling-into-view behavior.
• Remove time-formatting logic from non-UI model object, and put it in the XAML instead
• Use conventional (and more readable) - and + operators for DateTime and TimeSpan math

JobTimer.cs:

class JobTimer : NotifyPropertyChangedBase
{
    private DateTime _started; // When it was most recently started
    private TimeSpan _offset; // The saved value to offset the currently running timer
    Timer _swTimer; // The actual tick that updates the screen

    private readonly DelegateCommand _startCommand;
    private readonly DelegateCommand _stopCommand;

    public ICommand StartCommand => _startCommand;
    public ICommand StopCommand => _stopCommand;

    public JobTimer() : this(TimeSpan.Zero)
    { }

    public JobTimer(TimeSpan offset)
    {
        _offset = offset;
        _startCommand = new DelegateCommand(Start, () => !IsRunning);
        _stopCommand = new DelegateCommand(Stop, () => IsRunning);
    }

    private TimeSpan _elapsedTime;
    public TimeSpan ElapsedTime
    {
        get { return _elapsedTime; }
        set { _UpdateField(ref _elapsedTime, value); }
    }

    public void Start()
    {
        _started = DateTime.UtcNow;
        _swTimer = new Timer(TimerChanged, null, 0, 1000);
        IsRunning = true;
    }

    public void Stop()
    {
        if (_swTimer != null)
        {
            _swTimer.Dispose();
            _swTimer = null;
        }

        _offset += DateTime.UtcNow - _started;
        IsRunning = false;
    }

    private TimeSpan GetElapsed()
    {
        return IsRunning ? DateTime.UtcNow - _started + _offset : _offset;
    }

    // Updates the UI
    private void TimerChanged(object state)
    {
        ElapsedTime = GetElapsed();
    }

    private bool _isRunning;
    public bool IsRunning
    {
        get { return _isRunning; }
        set { _UpdateField(ref _isRunning, value, _OnIsRunningChanged); }
    }

    private void _OnIsRunningChanged(bool obj)
    {
        _startCommand.RaiseCanExecuteChanged();
        _stopCommand.RaiseCanExecuteChanged();
    }
}

MainViewModel.cs:

class MainViewModel : NotifyPropertyChangedBase
{
    public ObservableCollection<JobTimer> JobTimers { get; } = new ObservableCollection<JobTimer>();

    public ICommand AddTimerCommand { get; }

    public MainViewModel()
    {
        AddTimerCommand = new DelegateCommand(_AddTimer);
        _AddTimer();
    }

    private void _AddTimer()
    {
        foreach (JobTimer timer in JobTimers)
        {
            timer.Stop();
        }

        JobTimer t = new JobTimer();
        JobTimers.Add(t);
        t.Start();
    }
}

MainWindow.xaml.cs:

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();

        MainViewModel model = (MainViewModel)DataContext;

        model.JobTimers.CollectionChanged += _OnJobTimersCollectionChanged;
    }

    private void _OnJobTimersCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        ObservableCollection<JobTimer> jobTimers = (ObservableCollection<JobTimer>)sender;

        // Scroll to newly created timer
        JobTimer lastTimer = jobTimers.Last();
        listBox1.ScrollIntoView(lastTimer);
    }
}

MainWindow.xaml:

<Window x:Class="TestSO46416275DateTimeTimer.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:l="clr-namespace:TestSO46416275DateTimeTimer"
        mc:Ignorable="d"
        Title="MainWindow" Height="350" Width="525">
  <Window.DataContext>
    <l:MainViewModel/>
  </Window.DataContext>

  <Window.Resources>
    <DataTemplate DataType="{x:Type l:JobTimer}">
      <StackPanel Orientation="Horizontal">
        <TextBlock Text="{Binding ElapsedTime, StringFormat=hh\\:mm\\:ss}"/>
        <Button Content="Start" Command="{Binding StartCommand}"/>
        <Button Content="Stop" Command="{Binding StopCommand}"/>
      </StackPanel>
    </DataTemplate>
  </Window.Resources>

  <Grid>
    <Grid.RowDefinitions>
      <RowDefinition Height="Auto"/>
      <RowDefinition/>
    </Grid.RowDefinitions>

    <Button Content="Add Timer" Command="{Binding AddTimerCommand}" HorizontalAlignment="Left"/>
    <ListBox x:Name="listBox1" ItemsSource="{Binding JobTimers}" Grid.Row="1"/>
  </Grid>
</Window>

NotifyPropertyChangedBase.cs:

class NotifyPropertyChangedBase : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    protected void _UpdateField<T>(ref T field, T newValue,
        Action<T> onChangedCallback = null,
        [CallerMemberName] string propertyName = null)
    {
        if (EqualityComparer<T>.Default.Equals(field, newValue))
        {
            return;
        }

        T oldValue = field;

        field = newValue;
        onChangedCallback?.Invoke(oldValue);
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
    }
}

DelegateCommand.cs:

class DelegateCommand : ICommand
{
    private readonly Action _execute;
    private readonly Func<bool> _canExecute;

    public DelegateCommand(Action execute) : this(execute, null)
    { }

    public DelegateCommand(Action execute, Func<bool> canExecute)
    {
        _execute = execute;
        _canExecute = canExecute;
    }

    public event EventHandler CanExecuteChanged;

    public bool CanExecute(object parameter)
    {
        return _canExecute == null || _canExecute();
    }

    public void Execute(object parameter)
    {
        _execute();
    }

    public void RaiseCanExecuteChanged()
    {
        CanExecuteChanged?.Invoke(this, EventArgs.Empty);
    }
}