microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.36k stars 678 forks source link

DependencyProperty changed event fires when value is unchanged for (non-primitive) value types #1735

Open benstevens48 opened 4 years ago

benstevens48 commented 4 years ago

Steps to reproduce the bug Create a C#/XAML UWP app. Write a custom control and register a DependencyProperty in the standard way with a changed handler. Make sure the type of the DependencyProperty is a non-primitive value type, e.g. Color, Thickness. Set the value of the property from outside the control. Keep setting the value to the same thing. The changed handler keeps on firing.

Expected behavior The changed handler should not fire if the DependencyProperty value doesn't change (i.e. if the unboxed new and old values are equal using standard equality). If this is unavoidable for technical reasons, then it should be very clear in the documentation that it is necessary to check the unboxed values for equality in the changed handler. At the moment, the documentation suggests that the new and old values are guaranteed non-equal. Without this check, it is easy to write very inefficient code, or even code that can result in infinite loops.

Windows 10 version Saw the problem?
May 2019 Update (18362) Yes
Device form factor Saw the problem?
Desktop Yes
chrisglein commented 4 years ago

If this is unavoidable for technical reasons, then it should be very clear in the documentation that it is necessary to check the unboxed values for equality in the changed handler. At the moment, the documentation suggests that the new and old values are guaranteed non-equal.

@benstevens48 can you be more specific about where in the docs you're seeing this statement being made? I poked around a bit and couldn't find this language.

The changed handler should not fire if the DependencyProperty value doesn't change (i.e. if the unboxed new and old values are equal using standard equality).

In the C# language the == operator uses reference equality for reference types, not object equality. It only uses object equality for the value types. If that's what you mean by "standard equality" then this behavior is expected if you're changing object instances. You said "keep setting the value to the same thing" - do you mean the same object, or to an object with the same value? Can you clarify your example? Because if you're assigning the same object (would have reference equality) and it's firing the changed event that definitely sounds like a bug.

For assigning types that could have value comparison (they support Equals()) then it would be good to have that check made (I've seen lots of apps with inefficiencies and/or loops from this, as you've described). At a glance it looks like there's code in the core dependency property logic to attempt this type of equality check. Maybe there's some boxing getting in the way of certain type combinations? A crisp example would help.

benstevens48 commented 4 years ago

@chrisglein - it looks like someone has already acted on my feedback and updated the docs. Hopefully this explains it better than I did. See - https://docs.microsoft.com/en-gb/windows/uwp/xaml-platform/custom-dependency-properties#property-changed-behavior-for-structures-and-enumerations.

I actually found that in C# one can use Object.Equals(e.NewValue, e.OldValue) to test for equality, which is a more general solution than casting to the specific type each time.

In C#, structs like Color are value types, therefore one expects that the changed handler will not fire if e.NewValue and e.OldValue represent the same color. If it's possible from a technical point of view to implement this behavior, then I suggest it should be done as the current situation is confusing.

chrisglein commented 4 years ago

@benstevens48 Can you provide the sample code for your repro? That'll be the best way to make sure we can dig into this. The infrastructure is there to compare built-in value types (float, bool, etc.), and for some level of comparables, but either parts of that are not functioning or there are gaps.

benstevens48 commented 4 years ago

@chrisglein - here is the C# code. There's nothing particularly special about it.

CustomControl2.cs

using Windows.UI;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;

namespace App1 {
    public sealed class CustomControl2 : Control {

        public static readonly DependencyProperty ColorProperty = DependencyProperty.Register(nameof(Color), typeof(Color), typeof(CustomControl2), new PropertyMetadata(Colors.Black, OnColorChanged));

        public Color Color {
            get {
                return (Color)GetValue(ColorProperty);
            }
            set {
                SetValue(ColorProperty, value);
            }
        }

        private static void OnColorChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) {
            System.Diagnostics.Debug.WriteLine("on color changed");
            if(object.Equals(e.NewValue, e.OldValue)) {
                System.Diagnostics.Debug.WriteLine("Values are equal.");
            }
        }

        public CustomControl2() {
        }

    }
}

Elsewhere

            var control = new CustomControl2();
            control.Color = Colors.Black;
            control.Color = Colors.Black;
            control.Color = Colors.Black;
            control.Color = Colors.Black;
            control.Color = Colors.Black;

The debug output is

on color changed
Values are equal.
on color changed
Values are equal.
on color changed
Values are equal.
on color changed
Values are equal.
on color changed
Values are equal.

Note that I'm not using WinUI, I'm targeting Windows 10 version 1903 (min and target version), running on 1909.

benstevens48 commented 1 year ago

Need to check this has been fixed in WinUI3. Would also be good if it was fixed in system XAML.

Zulu-Inuoe commented 9 months ago

This is absolutely not fixed in WinUI. I discovered this issue today when profiling excessive reloads caused by DateOnly triggering a change. My 'fix' is to switch to using int representing the DayNumber, but clearly this is a hack and I'd love an actual fix to the problem for other types like Color