Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DP set explicitly to default value does not raise dp-changed event #11448

Open
Xiaoy312 opened this issue Feb 23, 2023 · 9 comments
Open

DP set explicitly to default value does not raise dp-changed event #11448

Xiaoy312 opened this issue Feb 23, 2023 · 9 comments
Assignees
Labels
difficulty/starter 🚀 Categorizes an issue for which the difficulty level is reachable by newcomers kind/bug Something isn't working low-hanging-🍒 Categorizes an issue that might be a quick win with meaningful positive impact. platform/all Categorizes an issue or PR as relevant to the all platforms project/binding 🪢 Categorizes an issue or PR as relevant to the binding engine

Comments

@Xiaoy312
Copy link
Contributor

Xiaoy312 commented Feb 23, 2023

Current behavior

With a DependencyProperty whose default value is set to an enum value, trying to set this dp value to the same initial value does not trigger the PropertyChangedCallback.

Expected behavior

^ it should trigger

How to reproduce it (as minimally and precisely as possible)

relevant source
public enum Asd { A, S, D }
public static class SomeExtensions
{
	#region DependencyProperty: RandomFlag

	public static DependencyProperty RandomFlagProperty { get; } = DependencyProperty.RegisterAttached(
		"RandomFlag",
		typeof(Asd),
		typeof(SomeExtensions),
		new PropertyMetadata(Asd.A, OnRandomFlagChanged));

	public static Asd GetRandomFlag(Border obj) => (Asd)obj.GetValue(RandomFlagProperty);
	public static void SetRandomFlag(Border obj, Asd value) => obj.SetValue(RandomFlagProperty, value);

	#endregion

	private static void OnRandomFlagChanged(DependencyObject sender, DependencyPropertyChangedEventArgs e)
	{
		// uwp: e -> { OldValue: A, NewValue: A }
		// uno: *no event proc*
	}
}

SomeExtensions.SetRandomFlag(new Border(), Asd.A);
<Border local:SomeExtensions.RandomFlag="A" />

repro sample: Toolkit451RelatedIssues.zip
repro steps:

  • set a breakpoint in SomeExtensions::OnRandomFlagChanged,
  • on uwp, you should hit the breakpoint twice upon launching the app (once from xaml, once from code-behind)
    ^ this does not happen with uno

Workaround

using (Asd)0 as default value, and then set it as Asd.A.

Works on UWP/WinUI

Yes

Environment

Uno.UI / Uno.UI.WebAssembly / Uno.UI.Skia

NuGet package version(s)

Uno.UI@4.7.30

Affected platforms

Android, iOS, Skia (GTK on Linux/macOS/Windows)

IDE

Visual Studio 2022

IDE version

No response

Relevant plugins

No response

Anything else we need to know?

No response

@Xiaoy312 Xiaoy312 added kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. labels Feb 23, 2023
@jeromelaban
Copy link
Member

Very curious behavior, thanks for the issue!

@Xiaoy312 Xiaoy312 added project/binding 🪢 Categorizes an issue or PR as relevant to the binding engine platform/all Categorizes an issue or PR as relevant to the all platforms labels Feb 23, 2023
@MartinZikmund MartinZikmund added difficulty/starter 🚀 Categorizes an issue for which the difficulty level is reachable by newcomers low-hanging-🍒 Categorizes an issue that might be a quick win with meaningful positive impact. and removed triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. labels Aug 23, 2023
@Youssef1313
Copy link
Member

Is it for enums specifically? Is WinUI here comparing the boxed value by reference?

@Youssef1313 Youssef1313 self-assigned this Jun 12, 2024
@Youssef1313
Copy link
Member

public sealed partial class MyPage : Page
{
    public MyPage()
    {
        Debug.WriteLine("Changing IntegerProperty");
        IntegerProperty = 0;

        Debug.WriteLine("Changing EnumProperty");
        EnumProperty = 0;

        Debug.WriteLine("Changing EnumProperty");
        EnumProperty = 0;

        Debug.WriteLine("Changing EnumProperty");
        EnumProperty = 0;

        Debug.WriteLine("Changing EnumProperty");
        EnumProperty = 0;
    }

    public int IntegerProperty
    {
        get { return (int)GetValue(IntegerPropertyProperty); }
        set { SetValue(IntegerPropertyProperty, value); }
    }

    public static DependencyProperty IntegerPropertyProperty { get; } =
        DependencyProperty.Register(nameof(IntegerProperty), typeof(int), typeof(MyPage), new PropertyMetadata(0, propertyChangedCallback: OnIntegerChanged));

    private static void OnIntegerChanged(DependencyObject dependencyObject, DependencyPropertyChangedEventArgs args)
    {
        Debug.WriteLine(nameof(OnIntegerChanged));
    }

    public E EnumProperty
    {
        get { return (E)GetValue(EnumPropertyProperty); }
        set { SetValue(EnumPropertyProperty, value); }
    }

    public static DependencyProperty EnumPropertyProperty { get; } =
        DependencyProperty.Register(nameof(EnumProperty), typeof(int), typeof(MyPage), new PropertyMetadata((E)0, propertyChangedCallback: OnEnumChanged));

    private static void OnEnumChanged(DependencyObject dependencyObject, DependencyPropertyChangedEventArgs args)
    {
        Debug.WriteLine(nameof(OnEnumChanged));
    }
}

Produces:

Changing IntegerProperty
Changing EnumProperty
OnEnumChanged
Changing EnumProperty
OnEnumChanged
Changing EnumProperty
OnEnumChanged
Changing EnumProperty
OnEnumChanged

However,

        Debug.WriteLine("Changing IntegerProperty");
        IntegerProperty = 0;

        object boxed = (E)0;

        Debug.WriteLine("Changing EnumProperty");
        SetValue(EnumPropertyProperty, boxed);

        Debug.WriteLine("Changing EnumProperty");
        SetValue(EnumPropertyProperty, boxed);

        Debug.WriteLine("Changing EnumProperty");
        SetValue(EnumPropertyProperty, boxed);

        Debug.WriteLine("Changing EnumProperty");
        SetValue(EnumPropertyProperty, boxed);

produces:

Changing IntegerProperty
Changing EnumProperty
OnEnumChanged
Changing EnumProperty
Changing EnumProperty
Changing EnumProperty

So, for enums, we should be using reference equality to match WinUI. But I don't see why are enums so special here. So need to understand this more. It also feels like a WinUI bug actually 😕

@Youssef1313
Copy link
Member

@Xiaoy312 I'm curious why this caused a trouble for you? Code shouldn't be relying on this weird behavior I think.

@Youssef1313
Copy link
Member

Youssef1313 commented Jun 12, 2024

CValue comparison in WinUI will try to unbox the value through TryUnboxPropertyValue which doesn't handle enums.

https://github.com/microsoft/microsoft-ui-xaml/blob/539e4de5bb6bf5973cc74110aa926b450b8aa53c/dxaml/xcp/components/CValue/CValueConvert.cpp#L74-L187

    template<typename CValueType>
    _Check_return_ HRESULT TryUnboxPropertyValue(
        _In_ wf::IPropertyValue* inValue,
        _Out_ CValueType& outValue,
        _Out_ bool& success)
    {
        // Quick and dirty way to convert the most common types to a CValue.

        ASSERT(inValue);

        success = false;

        wf::PropertyType ePropertyType = wf::PropertyType::PropertyType_Empty;
        IFC_RETURN(inValue->get_Type(&ePropertyType));

        switch (ePropertyType)
        {
            case wf::PropertyType_Boolean:
                {
                    BOOLEAN bValue = FALSE;
                    IFC_RETURN(inValue->GetBoolean(&bValue));
                    outValue.SetBool(!!bValue);
                }
                break;

            case wf::PropertyType_Int32:
                {
                    INT32 nValue = 0;
                    IFC_RETURN(inValue->GetInt32(&nValue));
                    outValue.SetSigned(nValue);
                }
                break;

            case wf::PropertyType_UInt32:
                {
                    // TODO: We need to introduce valueUnsigned for correctness.
                    UINT32 nValue = 0;
                    IFC_RETURN(inValue->GetUInt32(&nValue));
                    outValue.SetSigned((INT32)nValue);
                }
                break;

            case wf::PropertyType_Int64:
                {
                    INT64 value = 0;
                    IFC_RETURN(inValue->GetInt64(&value));
                    outValue.SetInt64(value);
                }
                break;

            case wf::PropertyType_UInt64:
                {
                    UINT64 value = 0;
                    IFC_RETURN(inValue->GetUInt64(&value));
                    outValue.SetInt64(static_cast<INT64>(value));
                }
                break;

            case wf::PropertyType_Double:
                {
                    DOUBLE nValue = 0.0;
                    IFC_RETURN(inValue->GetDouble(&nValue));
                    outValue.SetDouble(nValue);
                }
                break;

            case wf::PropertyType_Single:
                {
                    FLOAT nValue = 0.0f;
                    IFC_RETURN(inValue->GetSingle(&nValue));
                    outValue.SetFloat(nValue);
                }
                break;

            case wf::PropertyType_String:
                {
                    wrl_wrappers::HString str;
                    IFC_RETURN(inValue->GetString(str.GetAddressOf()));
                    IFC_RETURN(outValue.SetString(str.Get()));
                }
                break;

            case wf::PropertyType_TimeSpan:
                {
                    wf::TimeSpan timeSpan = {};
                    IFC_RETURN(inValue->GetTimeSpan(&timeSpan));
                    outValue.SetTimeSpan(timeSpan);
                }
                break;

            case wf::PropertyType_DateTime:
                {
                    wf::DateTime dateTime = {};
                    IFC_RETURN(inValue->GetDateTime(&dateTime));
                    outValue.SetDateTime(dateTime);
                }
                break;

            case wf::PropertyType_OtherType:
                // We don't have a good way to compare custom structs.
                // early exit without setting success to true
                return S_OK;

            default:
                // NOTE: If there are other commonly used types, consider supporting those here for a small CPU improvement
                //       when doing a check to see if a property value changed.
                // early exit without setting success to true
                return S_OK;
        }

        success = true;

        return S_OK;
    }

@Youssef1313
Copy link
Member

@jeromelaban @Xiaoy312 I'm not sure if this is something we should match or not.

@Xiaoy312
Copy link
Contributor Author

It is not something of priority, since I found the (Enum)0 workaround.
@Youssef1313 This can cause problem if you are expecting the relevant dp-changed to proc to initialize the control, but it doesnt happen, leaving the control uninitialized. Sure there is many ways around that, but it is just annoyance and you need to code around this issue.

@Youssef1313
Copy link
Member

@Xiaoy312 To me, it feels like a WinUI bug actually. Initialization code shouldn't be relying on "incorrect" DP changed callback IMO.

@Xiaoy312
Copy link
Contributor Author

fair enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty/starter 🚀 Categorizes an issue for which the difficulty level is reachable by newcomers kind/bug Something isn't working low-hanging-🍒 Categorizes an issue that might be a quick win with meaningful positive impact. platform/all Categorizes an issue or PR as relevant to the all platforms project/binding 🪢 Categorizes an issue or PR as relevant to the binding engine
Projects
None yet
Development

No branches or pull requests

4 participants