Infinite recursion error using DependencyProperty (C#)

Question

Here's the C# code directly from the website (http://jobijoy.blogspot.com/2007/10/time-picker-user-control.html) that everyone refers to when someone asks about TimePicker for WPF, although I moved it around a little bit to be more organized. (Please note, if you're trying to run this code to work with it: you must change the XAML code on this site from KeyDown to PreviewKeyDown on the 3 Grids where Hours, Minutes, and Seconds displays live, and change the TextBlocks with each Grid to TextBoxes)

public partial class TimeControl : UserControl
{
    public TimeControl()
    {
        InitializeComponent();
    }

    public TimeSpan Value
    {
        get { return (TimeSpan)GetValue(ValueProperty); }
        set { SetValue(ValueProperty, value); }
    }
    public static readonly DependencyProperty ValueProperty =
    DependencyProperty.Register("Value", typeof(TimeSpan), typeof(TimeControl),
    new UIPropertyMetadata(DateTime.Now.TimeOfDay, new PropertyChangedCallback(OnValueChanged)));

    public int Hours
    {
        get { return (int)GetValue(HoursProperty); }
        set { SetValue(HoursProperty, value); }
    }
    public static readonly DependencyProperty HoursProperty =
    DependencyProperty.Register("Hours", typeof(int), typeof(TimeControl),
    new UIPropertyMetadata(0, new PropertyChangedCallback(OnTimeChanged)));

    public int Minutes
    {
        get { return (int)GetValue(MinutesProperty); }
        set { SetValue(MinutesProperty, value); }
    }
    public static readonly DependencyProperty MinutesProperty =
    DependencyProperty.Register("Minutes", typeof(int), typeof(TimeControl),
    new UIPropertyMetadata(0, new PropertyChangedCallback(OnTimeChanged)));

    public int Seconds
    {
        get { return (int)GetValue(SecondsProperty); }
        set { SetValue(SecondsProperty, value); }
    }
    public static readonly DependencyProperty SecondsProperty =
    DependencyProperty.Register("Seconds", typeof(int), typeof(TimeControl),
    new UIPropertyMetadata(0, new PropertyChangedCallback(OnTimeChanged)));

    private static void OnValueChanged(DependencyObject obj, DependencyPropertyChangedEventArgs e)
    {
        TimeControl control = obj as TimeControl;
        control.Hours = ((TimeSpan)e.NewValue).Hours;
        control.Minutes = ((TimeSpan)e.NewValue).Minutes;
        control.Seconds = ((TimeSpan)e.NewValue).Seconds;
    }

    private static void OnTimeChanged(DependencyObject obj, DependencyPropertyChangedEventArgs e)
    {
            TimeControl control = obj as TimeControl;
            control.Value = new TimeSpan(control.Hours, control.Minutes, control.Seconds);
    }

    private void Down(object sender, KeyEventArgs args)
    {
        switch (((Grid)sender).Name)
        {
            case "sec":
                if (args.Key == Key.Up)
                    this.Seconds++;
                if (args.Key == Key.Down)
                    this.Seconds--;
                break;

            case "min":
                if (args.Key == Key.Up)
                    this.Minutes++;
                if (args.Key == Key.Down)
                    this.Minutes--;
                break;

            case "hour":
                if (args.Key == Key.Up)
                    this.Hours++;
                if (args.Key == Key.Down)
                    this.Hours--;
                break;
        }
    }
}

I'm not very good with Dependency or Binding yet, I'm just learning it, that's why I can't figure it out. But here's the problem: When the Minutes or Seconds are taken beyond 59/-59 there is an infinite loop. I'll explain the flow of it (at least I'm learning that much here!):

Let's say the TimeControl object is at 0:59:00 and we press the up key while focused on the minute TextBox. So, as we follow the logic, it goes to the PreviewKeyDown event, and the switch statement takes us to this.Minutes++ which gets Minutes and sees 59, so sets minutes to 60.

This triggers OnTimeChanged for Minutes, which gets Hours (0) Minutes (60) Seconds (0) and sets Value to that. Since Value is a TimeSpan, it interprets this as 1:00:00, which is great.

So, once that is set, it tiggers OnValueChanged, which sets Hours to 1, and this immediately calls back to OnTimeChanged for Hours. At this point it gets Hours (1) Minutes (60) Seconds (0) and sets Value to that (which is interpreted as 2:00:00).

Now we have an infinite loop until Hours becomes too large and throws an exception. This is a little over my head to understand how to fix it. What would be the 'proper' fix? I know it could be fixed with if statements in the switch statement, or even the OnTimeChanged/OnValueChanged methods, but I'm sure there's a better way to do it with the dependencies.

Answer 1

Simple Fix: change it so it resets the minutes first, then update the hour.

// Disclaimer: Haven't read the code yet so i might be wrong

Answer 2

No need to set properties if they aren't different, try something like this:

private static void OnValueChanged(DependencyObject obj, DependencyPropertyChangedEventArgs e)
{
    TimeControl control = obj as TimeControl;
    var ts =  (TimeSpan)e.NewValue;
    if(ts.Hours != control.Hours) control.Hours = ts.Hours;
    if(ts.Minutes != control.Minutes) control.Minutes = ts.Minutes;
    if(ts.Seconds != control.Seconds) control.Seconds = ts.Seconds;
}

Normally I'd put this logic in the setters, something you see common with data access layers...but I think your dependency calls would still happen there, so best to do it in this event handler in your code.