-
Notifications
You must be signed in to change notification settings - Fork 469
BindableProperty non-static method name enhancement #3002
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
base: main
Are you sure you want to change the base?
BindableProperty non-static method name enhancement #3002
Conversation
There was a problem hiding this 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 bindableparameters from these methods and eliminated manual casting - Updated method signatures to accept only
object oldValue, object newValueorobject valueparameters - 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 |
src/CommunityToolkit.Maui/Views/RatingView/RatingView.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/RatingView/RatingView.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/RatingView/RatingView.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Views/RatingView/RatingView.shared.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
There was a problem hiding this 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.
src/CommunityToolkit.Maui/Behaviors/AnimationBehavior.shared.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
...erators.Internal.UnitTests/BindablePropertyAttributeSourceGeneratorTests/CommonUsageTests.cs
Show resolved
Hide resolved
src/CommunityToolkit.Maui.SourceGenerators.Internal.UnitTests/BindablePropertyModelTests.cs
Show resolved
Hide resolved
| { | ||
| var cameraView = (CameraView)bindable; | ||
| return new(_ => cameraView.StopCameraPreview()); | ||
| return new(_ => StopCameraPreview()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@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 |
|
@pictos there are several motivations for this PR:
CommunityToolkit MVVM ExampleIf you look at the CommunityToolkit's MVVM implementation for a simple observable property, you’ll see it generates [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(
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
|
|
@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
if 1 or 2 are |
|
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 Let's pause on adding new features like this to the I'd like us to complete the following tasks before adding new features like this:
And then, before we remove the If you have the time to create |
Description of Change
Demonstrates a change to the BindablePropertyAttribute's attributes
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:
Would generate a wrapper that includes the BindableObject bindable property:
Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information