Skip to content

Conversation

@stephenquan
Copy link
Contributor

@stephenquan stephenquan commented Dec 14, 2025

Description of Change

Demonstrates a change to the BindablePropertyAttribute's attributes

  • ValidateValueMethodName
  • PropertyChangedMethodName
  • PropertyChangingMethodName
  • CoerceValueMethodName
  • DefaultValueCreatorMethodName

so that they now refer to simpler non-static methods because the source generator will now cast the bindable object to the user's class for them.

Example

For example:

public partial class TestClassName : View
{
    [BindableProperty(
        DefaultValue = 42,
        ValidateValueMethodName = "ValidateValue",
        PropertyChangedMethodName = "OnPropertyChanged",
        PropertyChangingMethodName = "OnPropertyChanging",
        CoerceValueMethodName = "CoerceValue",
        DefaultValueCreatorMethodName = "CreateDefaultValue")]
     public partial int Value { get; set; }

     // Simple non-static methods, omit BindableObject bindable parameter
     bool ValidateValue(object value) => true;
     void OnPropertyChanged(object oldValue, object newValue) { }
     void OnPropertyChanging(object oldValue, object newValue) { }
     object CoerceValue(object value) => value;
     object CreateDefaultValue() => 0;
}

Would generate a wrapper that includes the BindableObject bindable property:

public partial class TestClassName
{
    /// <summary>
    /// Backing BindableProperty for the <see cref = "Value"/> property.
    /// </summary>
    public static readonly global::Microsoft.Maui.Controls.BindableProperty ValueProperty = global::Microsoft.Maui.Controls.BindableProperty.Create(
       "Value",
       typeof(int),
       typeof(TestNamespace.TestClassName),
       (int)42,
       (Microsoft.Maui.Controls.BindingMode)1,
       (b,v) => ((TestClassName)b).ValidateValue(v),
       (b,o,n) => ((TestClassName)b).OnPropertyChanged(o,n),
       (b,o,n) => ((TestClassName)b).OnPropertyChanging(o,n),
       (b,v) => ((TestClassName)b).CoerceValue(v),
       (b) => ((TestClassName)b).CreateDefaultValue());
    public partial int Value {
        get => (int)GetValue(ValueProperty);
        set => SetValue(ValueProperty, value);
    }
}

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description) - N/A
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls - TBA

Additional information

Copilot AI review requested due to automatic review settings December 14, 2025 01:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors BindableProperty-related methods to be non-static instance methods, simplifying the codebase by removing the need for explicit BindableObject parameters and casting. The source generator now automatically handles the casting from BindableObject to the specific class type.

Key Changes:

  • Converted static callback methods (PropertyChanged, PropertyChanging, CoerceValue, ValidateValue, DefaultValueCreator) to non-static instance methods
  • Removed BindableObject bindable parameters from these methods and eliminated manual casting
  • Updated method signatures to accept only object oldValue, object newValue or object value parameters
  • Modified source generator to wrap instance method calls with lambda expressions that perform the type casting

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
RatingView.shared.cs Converted 13 static callback methods to instance methods, removing bindable parameter and casting
Expander.shared.cs Converted 3 static callback methods to instance methods
AvatarView.shared.cs Converted 5 static callback methods to instance methods
ProgressBarAnimationBehavior.shared.cs Converted 2 static callback methods to instance methods
TouchBehavior.methods.shared.cs Converted 8 static callback methods to instance methods
ImageTouchBehavior.shared.cs Converted 3 static callback methods to instance methods
MaskedBehavior.shared.cs Converted 3 callback methods to instance methods
EventToCommandBehavior.shared.cs Converted 1 static callback method to instance method and removed unused import
AnimationBehavior.shared.cs Converted 3 static callback methods to instance methods and removed redundant type check
Records.cs Added lambda expression generators for wrapping instance method calls with type casting
BindablePropertyAttributeSourceGenerator.cs Updated to use effective method names that include lambda wrappers
BindablePropertyModelTests.cs Updated tests to verify lambda expression generation
EdgeCaseTests.cs Updated test expectations for DefaultValueCreator lambda wrapping
CommonUsageTests.cs Updated test expectations for all callback method lambda wrapping
UriMediaSource.shared.cs Converted 2 callback methods to instance methods and removed unused import
ResourceMediaSource.shared.cs Converted 1 callback method to instance method and removed unused import
FileMediaSource.shared.cs Converted 1 callback method to instance method and removed unused import
MediaElement.shared.cs Converted 4 callback methods to instance methods
CameraView.shared.cs Converted 6 callback methods to instance methods and removed unused import

Copilot AI review requested due to automatic review settings December 14, 2025 02:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings December 14, 2025 04:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings December 17, 2025 19:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

{
var cameraView = (CameraView)bindable;
return new(_ => cameraView.StopCameraPreview());
return new(_ => StopCameraPreview());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ => will swallow exceptions. Switch to capturing the exception and maybe show a log message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _ => was already in the original code and remains unchanged. The only update here was converting the method from static to non-static, so the behavior should be identical to before.

@pictos
Copy link
Member

pictos commented Dec 17, 2025

@stephenquan what motivated you to do that change? I can't recall seeing places where an instance method is used inside a BindableProperty. Also, I would say it isn't desired, since the BindableProperty lives forever, since it's static, and now will hold a strong reference to the type which can cause a memory leak or other problems, since the bindable property which is static (so isn't bounded to any control) will be bounded to a class

@stephenquan
Copy link
Contributor Author

stephenquan commented Dec 17, 2025

@pictos there are several motivations for this PR:

  1. Align the usage of the new BindablePropertyAttribute more closely with CommunityToolkit.Mvvm's ObservablePropertyAttribute.
  2. In many cases, we are already casting the BindableObject parameter to the class instance in order to work with instance members.
  3. This approach follows an established pattern in the .NET MAUI source code.

CommunityToolkit MVVM Example

If you look at the CommunityToolkit's MVVM implementation for a simple observable property, you’ll see it generates On{PropertyName}Changed and On{PropertyName}Changing partial methods. For example:

[ObservableProperty]
public partial int Value { get; set; } = 42;
public partial int Value
{
    get => field;
    set
    {
        if (!global::System.Collections.Generic.EqualityComparer<int>.Default.Equals(field, value))
        {
            OnValueChanging(value);
            OnValueChanging(default, value);
            OnPropertyChanging(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangingArgs.Value);
            field = value;
            OnValueChanged(value);
            OnValueChanged(default, value);
            OnPropertyChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Value);
        }
    }
}

partial void OnValueChanging(int value);
partial void OnValueChanging(int oldValue, int newValue);
partial void OnValueChanged(int value);
partial void OnValueChanged(int oldValue, int newValue);

The proposed changes to the BindableProperty attribute suggest that developers use consistent propertyChanged method signatures.

[BindableProperty(
    PropertyChangedMethodName = nameof(OnValueChanged),
    PropertyChangingMethodName = nameof(OnValueChanging)
)]
public partial int Value { get; set; } = 42;

void OnValueChanging(int oldvalue, int newValue) { }
void OnValueChanged(int oldValue, int newValue) { }

.NET MAUI Source Code example:

In .NET MAUI’s source code, many propertyChanged handlers begin by casting the bindable parameter to the appropriate instance so they can call instance methods. For example:

public partial class View : VisualElement, IViewController, IGestureController, IGestureRecognizers, IView, IPropertyMapperView, IHotReloadableView, IControlsView
{
    /// <summary>Bindable property for <see cref="VerticalOptions"/>.</summary>
    public static readonly BindableProperty VerticalOptionsProperty =
        BindableProperty.Create(nameof(VerticalOptions), typeof(LayoutOptions), typeof(View), LayoutOptions.Fill,
                                propertyChanged: (bindable, oldvalue, newvalue) =>
                                ((View)bindable).InvalidateMeasureInternal(InvalidationTrigger.VerticalOptionsChanged));
}

Here is a search of inlined (bindable, oldValue, newValue) => in the .NET MAUI source code, many of which map to instance methods.

References

@TheCodeTraveler TheCodeTraveler added do not merge Do not merge this PR needs discussion Discuss it on the next Monthly standup labels Dec 18, 2025
@TheCodeTraveler TheCodeTraveler marked this pull request as draft December 18, 2025 02:39
@stephenquan stephenquan marked this pull request as ready for review December 18, 2025 04:01
@pictos
Copy link
Member

pictos commented Dec 18, 2025

@stephenquan I see now and it does make sense. For the .net Maui implementation, I would say to not focus too much on MVVM SG, because it generates other kinds of bindable properties.

Also, I misread the generated code. It doesn't have issues with memory leaks because there's no closure capture. At first glance, I thought it would.

But I would say this is more of an advanced feature. It uses the instance method because the method InvalidateMeasureInternal is virtual. Since each layout, view, etc., may have its own way to measure itself, so for this, I would add some diagnostics to guide devs in the correct use, and that should be:

  1. The class should NOT be sealed.
  2. The method passed should be virtual

if 1 or 2 are false, we should add a warning or a message telling something like "hey, you may want to use a static member instead".

@TheCodeTraveler
Copy link
Collaborator

TheCodeTraveler commented Dec 20, 2025

Hey @stephenquan! I like the idea because it allows for type-safe methods, albeit it likely introduces a performance hit by using instance methods instead of static methods.

Just FYI - I've added the do not merge label to this PR for now.

Let's pause on adding new features like this to the BindablePropertySourceGenerator until we're confident that we've patched all of the yet-to-be discovered edge cases.

I'd like us to complete the following tasks before adding new features like this:

  1. Implement [AttachedBindableProperty] to support BindableProperty.CreateAttached(...)
  2. Implement [BindableProperty] for every BindableProperty in CommunityToolkit.Maui
    • Patch bugs discovered in [BindableProperty] as these new edge-cases unfold
  3. Migrate BindablePropertySourceGenerator and AttachedBindablePropertySourceGenerator to CommunityToolkit.Maui.SourceGenerators
  4. Publish CommunityToolkit.Maui v14.0.0 that includes public availability of [BindableProperty] and [AttachedBindableProperty], both with the [Experimental] attribute

And then, before we remove the [Experimental] attribute, we'll need to implement an Analyzer + Code Fix, similar to the MVVM Toolkit's Analyzers, that checks for things like missing the partial keyword, using incorrect method signatures for PropertyChanged etc, etc.

If you have the time to create [AttachedBindableProperty] that'd be a HUGE help! I plan on implementing it when I return home from holiday early January, but would more than happy to review a PR if you submit one before the end of the year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Do not merge this PR needs discussion Discuss it on the next Monthly standup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants