-
Notifications
You must be signed in to change notification settings - Fork 473
Create [AttachedBindableProperty<T>]
#3024
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
Conversation
pictos
left a comment
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.
Good work, I've a couple of comments about it
...aui.SourceGenerators.Internal/Generators/AttachedBindablePropertyAttributeSourceGenerator.cs
Outdated
Show resolved
Hide resolved
...aui.SourceGenerators.Internal/Generators/AttachedBindablePropertyAttributeSourceGenerator.cs
Outdated
Show resolved
Hide resolved
| var attachedBindablePropertiesCount = 0; | ||
| foreach (var value in values) | ||
| { | ||
| foreach (var abp in value.BindableProperties.AsImmutableArray()) |
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.
Why the usage of AsImmutableArray?
| { | ||
| var newBuffer = System.Buffers.ArrayPool<AttachedBindablePropertyModel>.Shared.Rent(attachedBindablePropertiesBuffer.Length * 2); | ||
| Array.Copy(attachedBindablePropertiesBuffer, newBuffer, attachedBindablePropertiesBuffer.Length); | ||
| System.Buffers.ArrayPool<AttachedBindablePropertyModel>.Shared.Return(attachedBindablePropertiesBuffer); |
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.
We aren't cleaning up the array when returning to the pool, this will make this array to hold values there, which can lead in bugs if we use the full Length of the array instead of the right number of BindableProperties
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.
I'm not sure I understand what's missing. What do you recommend for the fix?
...aui.SourceGenerators.Internal/Generators/AttachedBindablePropertyAttributeSourceGenerator.cs
Outdated
Show resolved
Hide resolved
…/CommunityToolkit/Maui into Add-`-AttachedBindableProperty]`
Thanks @ne0rrmatrix! Could you give it one more review? I've just added support for |
|
Some alternate syntaxes we can consider: Option 1: Attach the new attribute to dummy properties (alternatively, consider a static or const)
public partial class TestClass
{
// Attached the new attribute to a dummy property
[AttachedBindableProperty]
public partial string? Text { get; set; } = "Hello World";
}Proposed generated source code for option 1: // Use the dummy property to extract type and name and generate the attached property
public partial class TestClass
{
public partial string? Text
{
get => field;
set => field = value;
}
/// <summary>
/// Attached BindableProperty for the Text property.
/// </summary>
public static readonly global::Microsoft.Maui.Controls.BindableProperty TextProperty = global::Microsoft.Maui.Controls.BindableProperty.CreateAttached("Text", typeof(string), typeof(TestNamespace. TestClass),(string?)"Hello World", (Microsoft.Maui.Controls.BindingMode)0, null, null, null, null, null);
/// <summary>
/// Gets Text for the <paramref = "bindable"/> child element.
/// </summary>
public static string? GetText(global::Microsoft.Maui.Controls.BindableObject bindable) => (string)bindable.GetValue(TextProperty);
/// <summary>
/// Sets Text for the <paramref = "bindable"/> child element.
/// </summary>
public static void SetText(global::Microsoft.Maui.Controls.BindableObject bindable, string? value) => bindable.SetValue(TextProperty, value);
}Option 2: Attach the new attribute to the real getter/setter, e.g.
public partial class TestClass
{
[AttachedBindableProperty]
public static string? GetText(BindableObject view)
{
return (string?)view.GetValue(TextProperty);
}
[AttachedBindableProperty]
public static void SetText(BindableObject view, string? value)
{
view.SetValue(TextProperty, value);
}
}Proposed generated source code for option 2: // Generate the attached property
public partial class TestClass
{
/// <summary>
/// Attached BindableProperty for the Text property.
/// </summary>
public static readonly global::Microsoft.Maui.Controls.BindableProperty TextProperty = global::Microsoft.Maui.Controls.BindableProperty.CreateAttached("Text", typeof(string), typeof(TestNamespace. TestClass),(string?)"Hello World", (Microsoft.Maui.Controls.BindingMode)0, null, null, null, null, null);} |
|
@stephenquan, thanks for your suggestions... But I've to say that I don't like that design.
But, looking at your suggestions for the methods gave me an idea on something that's missing. Right now, the SG doesn't provide an easy way for devs to insert behavior on the generated methods. @TheCodeTraveler I think we should add it, and we can use the MVVM toolkit as a reference, it uses // SG Code
public static void SetText(BindableObject view, string? value)
{
BeforeSet(view, ref value);
view.SetValue(TextProperty, value);
AfterSet(view, value);
}
partial void BeforeSet(BindableObject view, ref string? value);
partial void AfterSet(BindableObject view, string? value);
// User code
partial void BeforeSet(BindableObject view, string? value)
{
if (!decimal.TryParse(value, out var money))
value = string.Empty;
value = $"R${money}";
} |
|
@pictos that looks good. Re: naming should we follow the |
|
Love the idea of adding partial methods! But, does it differ enough from the existing |
|
@TheCodeTraveler oh yeah, I forgot those, yeah so I believe it's good to go |
|
I had another crack at this and came up with more syntaxes to consider: Option 3 user stubs out delegates:public partial class TestClass
{
[AttachedBindableProperty]
public static partial Func<BindableObject, string?> GetText { get; }
[AttachedBindableProperty]
public static partial Action<BindableObject, string?> SetText { get; }
}SG produces: public partial class TestClass
{
public static readonly global::Microsoft.Maui.Controls.BindableProperty TextProperty = global::Microsoft.Maui.Controls.BindableProperty.CreateAttached("Text", typeof(string), typeof(TestNamespace.TestClass), (string?)"Hello World", (Microsoft.Maui.Controls.BindingMode)0, null, null, null, null, null);
public static partial Func<BindableObject, string?> GetText { get => field; } = new((bindable) => (string)bindable.GetValue(TextProperty));
public static partial Action<BindableObject, string?> SetText { get => field; } = new((bindable, value) => bindable.SetValue(TextProperty, value));
}Option 4 (Option 3 refactored with easier-to-remember delegates)public partial class TestClass
{
[AttachedBindableProperty]
public static partial AttachedBindablePropertyGetter<string?> GetText { get; }
[AttachedBindableProperty]
public static partial AttachedBindablePropertySetter<string?> SetText { get; }
}SG produces: public partial class TestClass
{
public static readonly global::Microsoft.Maui.Controls.BindableProperty TextProperty = global::Microsoft.Maui.Controls.BindableProperty.CreateAttached("Text", typeof(string), typeof(TestClass), (string?)default);
public static partial AttachedBindablePropertyGetter<string?> GetText { get => field; } = new((BindableObject view) =>
{
return (string?)view.GetValue(TextProperty);
});
public static partial AttachedBindablePropertySetter<string?> SetText { get => field; } = new((bindable, value) =>
{
bindable.SetValue(TextProperty, value);
});
}We define the helpers as delegates, i.e. public delegate T AttachedBindablePropertyGetter<T>(BindableObject view);
public delegate void AttachedBindablePropertySetter<T>(BindableObject view, T value); |
ed177c2
Description of Change
This PR implements
[AttachedBindableProperty<T>("PropertyName")]:The attribute can be applied to a Class, like so:
Both usages will generate the following source code:
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
This PR is still in draft as some unit tests are still failing and need to be resolved.
This PR unblocks #3016 and #3000