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

Visual Studio 2022 v17.12 does not update Binding to [ObservableProperty] #10250

Open
bjorn-malmo opened this issue Dec 20, 2024 · 5 comments
Open
Labels
bug Something isn't working team-Markup Issue for the Markup team

Comments

@bjorn-malmo
Copy link

Describe the bug

A WinUI custom control decorated with [ObservableObject] attribute will not update Bindings in the control's template for properties on property changed.

This does work in Visual Studio 2022 v17.10!
This does NOT work in Visual Studio 2022 v17.12!

Roslyn gang directed me here (dotnet/roslyn#76470)
Issue reported in Community Toolkit can reproduce, but closed as external (CommunityToolkit/dotnet#1011)

Steps to reproduce the bug

  1. Create a WinUI 3 Custom Control
  2. Using CommunityToolkit, attribute the class [ObservableObject]
  3. Create a field and attribute it [ObservableProperty]
[ObservableObject]
public sealed partial class CustomControl1 : Control
{
    public CustomControl1()
    {
        this.DefaultStyleKey = typeof(CustomControl1);
    }

    [ObservableProperty] string _myValue = "Initial value";

    [RelayCommand]
    void UpdateValue()
    {
        MyValue = "Updated value";
    }
}
  1. In the control's template, bind to the property
    <Style TargetType="local:CustomControl1" >
        <Setter Property="Template">
            <Setter.Value>
                <ControlTemplate TargetType="local:CustomControl1">
                    <StackPanel Spacing="16"
                        Background="{TemplateBinding Background}"
                        BorderBrush="{TemplateBinding BorderBrush}"
                        BorderThickness="{TemplateBinding BorderThickness}">
                        
                        <TextBlock Text="{Binding MyValue, RelativeSource={RelativeSource TemplatedParent}}"/>

                        <Button Content="Update value!" 
                                Command="{Binding UpdateValueCommand, RelativeSource={RelativeSource TemplatedParent}}"/>
                        
                    </StackPanel>
                </ControlTemplate>
            </Setter.Value>
        </Setter>
    </Style>
  1. Update the value of the MyValue property.

Sample application found here: Project PropertyBindingTest.sln in repo https://github.com/bjorn-malmo/bug-free-happiness/

Run the application using Visual Studio 2022 v17.10 and click the button:

Image

Run the same application using Visual Studio 2022 v17.12 and click the button:

Image

Expected behavior

The Binding to update as a result of the change notification

Screenshots

No response

NuGet package version

WinUI 3 - Windows App SDK 1.6.3: 1.6.241114003

Windows version

Windows 11 (23H2): Build 22631

Additional context

No response

@bjorn-malmo bjorn-malmo added the bug Something isn't working label Dec 20, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the needs-triage Issue needs to be triaged by the area owners label Dec 20, 2024
@karkarl karkarl added team-Markup Issue for the Markup team and removed needs-triage Issue needs to be triaged by the area owners labels Dec 26, 2024
@evelynwu-msft
Copy link
Contributor

@bjorn-malmo Thanks for the report! Unfortunately, I think you're going to be bounced around some more before this gets tracked down. :(

When I tried reproducing this on my own machine (with both VS 17.10 and 17.12 installed) I saw the same broken behavior in both. I have no idea why this is but I nevertheless do have a theory (if you're willing to share a copy of PropertyBindingTest.dll from a build where the scenario works or if you're able to use ILSpy to decompile the same assembly and share with us the body of the method WinRT.PropertyBindingTestVtableClasses.PropertyBindingTest_CustomControl1WinRTTypeDetails.GetExposedInterfaces() then that would help verify my theory).

@Sergio0694 @manodasanW I suspect this is resulting from improper sequencing of Community Toolkit and C#/WinRT source generation during compilation. Given that the repro only changes VS version with no corresponding change to WinAppSDK or Community Toolkit versions I'm inclined to believe that some change in .NET (Roslyn?) resulted in the order in which the source generators run changing but I have no idea how to dig deeper into this.

Decorating a class with the [ObservableObject] attribute from Community Toolkit results in a generated partial class that implements the INotifyPropertyChanged and INotifyPropertyChanging interfaces.

In the build that results in a non-functional binary, WinRT.PropertyBindingTestVtableClasses.PropertyBindingTest_CustomControl1WinRTTypeDetails.GetExposedInterfaces() looks like this:

public ComWrappers.ComInterfaceEntry[] GetExposedInterfaces()
{
	return new ComWrappers.ComInterfaceEntry[6]
	{
		new ComWrappers.ComInterfaceEntry
		{
			IID = IUIElementOverridesMethods.IID,
			Vtable = IUIElementOverridesMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IAnimationObjectMethods.IID,
			Vtable = IAnimationObjectMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IVisualElementMethods.IID,
			Vtable = IVisualElementMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IVisualElement2Methods.IID,
			Vtable = IVisualElement2Methods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IFrameworkElementOverridesMethods.IID,
			Vtable = IFrameworkElementOverridesMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IControlOverridesMethods.IID,
			Vtable = IControlOverridesMethods.AbiToProjectionVftablePtr
		}
	};
}

Note that neither INotifyPropertyChanged nor INotifyPropertyChanging are listed as exposed interfaces; Binding will not work without the presence of the former.

If I modify the repro project (since for whatever reason both VS 17.10 and 17.12 repro the bug on my machine) to (a) remove the [ObservableObject] attribute, and (b) manually implement INotifyPropertyChanged, then the disassembly looks like this:

public ComWrappers.ComInterfaceEntry[] GetExposedInterfaces()
{
	return new ComWrappers.ComInterfaceEntry[7]
	{
		new ComWrappers.ComInterfaceEntry
		{
			IID = IUIElementOverridesMethods.IID,
			Vtable = IUIElementOverridesMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IAnimationObjectMethods.IID,
			Vtable = IAnimationObjectMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IVisualElementMethods.IID,
			Vtable = IVisualElementMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IVisualElement2Methods.IID,
			Vtable = IVisualElement2Methods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IFrameworkElementOverridesMethods.IID,
			Vtable = IFrameworkElementOverridesMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = IControlOverridesMethods.IID,
			Vtable = IControlOverridesMethods.AbiToProjectionVftablePtr
		},
		new ComWrappers.ComInterfaceEntry
		{
			IID = INotifyPropertyChangedMethods.IID,
			Vtable = INotifyPropertyChangedMethods.AbiToProjectionVftablePtr
		}
	};
}

Note that INotifyPropertyChanged is now exposed as an interface, and as a result the scenario works. Presumably a build of the repro solution that does produce a functioning binary would look similar when disassembled.

@Sergio0694
Copy link
Member

@evelynwu-msft is right, but also there's another issue at play here: you're just misusing [ObservableObject]. That attribute is only meant to be used (1) in advanced scenarios (and you should just derive from ObservableObject instead almost always), and (2) the INotifyPropertyChanged support is only meant to be used in viewmodels. You should not be implementing it on controls. Instead, controls should declare dependency properties, which do support binding out of the box.

Can you try switching to dependency properties to see if that solves the issue?

I can sync with @manodasanW to see if I can also add some diagnostics to the MVVM Toolkit to detect this scenario 🙂

@bjorn-malmo
Copy link
Author

Yes, I can confirm that everything is working as expected if I use dependency properties or if I implement INotifyPropertyChanged manually. Decorating a class NOT deriving from Control (i.e. some custom base class) will also work. Using x:Bind instead of Binding also works (with [ObservableObjectAttribute]).

This is the devil of the bug, that it is only affecting Binding.

We have been using [ObservableObjectAttribute] quite extensively in our application for convenience, which I suppose is the reason that this attribute exists at all. It has been working perfectly for several years up until now, and I'm a bit baffled that more people isn't affected by this problem...

Anyway, I tried to find the method mentioned above in the working Dll, but I couldn't find it. Please see the attached zip file containing the working version of the Dll.

PropertyBindingTest.zip

@kmgallahan
Copy link
Contributor

I'm a bit baffled that more people isn't affected by this problem...

@bjorn-malmo I use Fody/PropertyChanged to handle this. I imagine many others that use code gen to solve this problem still use it since it has been around much longer.

@Sergio0694
Copy link
Member

"We have been using [ObservableObjectAttribute] quite extensively in our application for convenience"

@bjorn-malmo you shouldn't be using that attribute unless you absolutely have to. In fact, I added a bunch of diagnostics for it. And you definitely shouldn't use it on controls, I'll add more diagnostics for that scenario. If you have a control, you should not implement INotifyPropertyChanged on it. That's meant for viewmodels. For controls, declare dependency properties instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team-Markup Issue for the Markup team
Projects
None yet
Development

No branches or pull requests

5 participants